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

[Bug] [copy] filtered-out items get statted, leading to unrecoverable exception #965

Closed
c-vetter opened this issue Sep 14, 2022 · 6 comments · Fixed by #971
Closed

[Bug] [copy] filtered-out items get statted, leading to unrecoverable exception #965

c-vetter opened this issue Sep 14, 2022 · 6 comments · Fixed by #971

Comments

@c-vetter
Copy link

c-vetter commented Sep 14, 2022

  • Operating System: Windows 10 Pro
  • Node.js version: 16.15.0
  • fs-extra version: 10.1.0

Reproduction

Try to copy(src, dest, { filter }, with:

  • src being a valid and accessible source directory, in my case a secondary drive's root
  • src having a child directory that is not accessible, in my case the System Volume Information directory that is owned by the OS and inaccessible to users
  • dest being a valid and accessible destination directory, in my case an empty directory on another drive
  • filter being a valid filter function that excludes the inaccessible directory, e.g. const filter = (fp)=>!fp.includes("System Volume Information")

Observed behavior

Error: EPERM: operation not permitted, lstat 'B:\System Volume Information'

Expected behavior

The copy operation proceeds without trying to access the filtered-out directory.

Additional information

During debugging, I tracked the issue to this line:

stat.checkPaths(srcItem, destItem, 'copy', opts, (err, stats) => {

The way I understand the code, src and dest are stated before running the filter. I think that the filter should run before any attempt is made to stat any files.

If there are more important reasons why that should not happen, e.g. to enable #844, there should at least be a mechanism to recover from errors on a per-file basis in order to allow the copy operation to continue.

As it stands, I will have to work around this by giving a filtered set of files to copy 😢

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 14, 2022

How does fs.cp behave here?

@c-vetter
Copy link
Author

How does fs.cp behave here?

Catastrophically, see nodejs/node#44653

I don't have a Unix machine ready, so I cannot test there.

I tried docker, but was unable to reproduce a similar situation on account of having only root to play with.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 15, 2022

At this point, we would like to maintain compatibility with fs.cp moving forward, so we'll generally follow however Node does it.

@c-vetter
Copy link
Author

The initial issue seems to not be a bug, strictly speaking. Could be considered a docs issue.

Now that that's cleared up for the moment, I tried further and found fs.cp to behave the same way: nodejs/node#44720

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 20, 2022

Fixed by Node in nodejs/node#44922; we should backport.

@RyanZim RyanZim added this to the 11.0.0 milestone Oct 20, 2022
@RyanZim
Copy link
Collaborator

RyanZim commented Oct 20, 2022

I'm not sure the Node patch has tests that actually properly test this case. Any ideas how to construct a cross-platform test-case for this bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants