Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix operation not permitted #44786

Closed
wants to merge 3 commits into from

Conversation

thoqbk
Copy link
Contributor

@thoqbk thoqbk commented Sep 25, 2022

fix: #44720

issue:

  • copyDir() calls checkPathsSync(), which invokes lstat()
    causes error because of not checking the opts.filter

changes:

  • check opts.filter before calling checkPathsSync and copy logic
  • cleanup startCopy function

fix: nodejs#44720

issue:
- `copyDir()` calls `checkPathsSync()`, which invokes `lstat()`
which causes error because of not checking the opts.filter

changes:
- check opts.filter before calling `checkPathsSync` and copy logic
- cleanup `startCopy` function
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 25, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include a test?

@thoqbk
Copy link
Contributor Author

thoqbk commented Sep 29, 2022

Can you include a test?

test was added

filter: path => !path.includes('foo'),
recursive: true,
});
}
Copy link
Contributor Author

@thoqbk thoqbk Oct 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before fixing, when I run this test it throws the following error because checkPathsSync(srcItem, destItem, opts) gets called even though the srcItem shouldn't be copied:

    SystemError [ERR_FS_CP_DIR_TO_NON_DIR]: Cannot overwrite directory with non-directory: cp returned EISDIR (cannot overwrite directory /Users/xxx/dev/node/test/.tmp.0/copy_28/foo with non-directory /Users/xxx/dev/node/test/.tmp.0/copy_29/foo) /Users/xxx/dev/node/test/.tmp.0/copy_29/foo
        at new SystemError (node:internal/errors:244:5)
        at new NodeError (node:internal/errors:355:7)
        at checkPathsSync (node:internal/fs/cp/cp-sync:77:13)
        at copyDir (node:internal/fs/cp/cp-sync:287:28)
        at onDir (node:internal/fs/cp/cp-sync:268:10)
        at getStats (node:internal/fs/cp/cp-sync:171:12)
        at handleFilterAndCopy (node:internal/fs/cp/cp-sync:158:10)
        at cpSyncFn (node:internal/fs/cp/cp-sync:60:10)
        at cpSync (node:fs:2899:3)
        at file:///Users/xxx/dev/node/test/parallel/test-fs-cp.mjs:353:3 {
      code: 'ERR_FS_CP_DIR_TO_NON_DIR',
      info: {
        message: 'cannot overwrite directory /Users/xxx/dev/node/test/.tmp.0/copy_28/foo with non-directory /Users/xxx/dev/node/test/.tmp.0/copy_29/foo',
        path: '/Users/xxx/dev/node/test/.tmp.0/copy_29/foo',
        syscall: 'cp',
        errno: 21,
        code: 'EISDIR'
      },
      errno: [Getter/Setter],
      syscall: [Getter/Setter],
      path: [Getter/Setter]
    }

@thoqbk thoqbk force-pushed the fix-operation-not-permitted branch 2 times, most recently from 4c84e46 to aa3e4f1 Compare October 1, 2022 11:18
@thoqbk thoqbk requested a review from joyeecheung October 1, 2022 13:41
// It should not throw exception if child folder
// does not pass filter function
{
// Mimic there's a file in dest with the same name as the child folder in src
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start of this sentence is a bit awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@thoqbk thoqbk force-pushed the fix-operation-not-permitted branch from aa3e4f1 to ec30bf0 Compare October 2, 2022 14:50
@thoqbk thoqbk requested review from jasnell and joyeecheung and removed request for joyeecheung and jasnell October 2, 2022 14:51
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit to clarify the comments.

test/parallel/test-fs-cp.mjs Outdated Show resolved Hide resolved
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@thoqbk
Copy link
Contributor Author

thoqbk commented Oct 24, 2022

Rework PR #45143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fs.cp] fails with EPERM despite filter
5 participants