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

copy*() name-based isSrcSubdir check is insufficient, perhaps needs ino #613

Closed
rossj opened this issue Aug 5, 2018 · 3 comments
Assignees
Labels
Milestone

Comments

@rossj
Copy link

@rossj rossj commented Aug 5, 2018

  • Operating System: Debian Jessie
  • Node.js version: 10.8
  • fs-extra version: 7.0.0

The work that @manidlou did in PR #582 changed a lot of sanity and edge-case checks in copy() and copySync() to use ino instead of name / paths for comparison. However, the isSrcSubdir() check still relies on checking string names, which is insufficient in the case that a single folder has multiple locations / names, e.g. a double-mounted Docker volume or a bind-mounted folder. I believe that this check should also be switched to rely on ino.

In order to exploit this name check, here is what I did:

  1. Made an empty directory on my host Windows OS
  2. Started a Docker container, double-mounting the empty dir into /mount/dir1 and /mount/dir2
  3. Using fs-extra inside the container, run fs.copySync('/mount/dir1', '/mount/dir2/sub'). The current isSrcSubdir() check will fail to abort the copy, and the operation will recurse infinitely, continuing to create nested /sub dirs until execution is stopped manually, or perhaps a stack overflow occurs.

As an additional note, I noticed this issue while working on #575. Removing the string-based checks is a pre-requisite for finishing #575.

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Aug 7, 2018

I confirm the bug tho! you can also replicate that by creating a self-linked directory containing subdirectory.

mkdir -p src/sub
ln -s src/ dest

then using

fs.copy('src', 'dest/sub', cb)

will fail with ENAMETOOLONG error!

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Aug 11, 2018

@rossj another way of solving this is by checking the dest parent stats since in cases like this src and dest parent have the same inode. I already tested out the idea and implemented the function that it works fine.

@RyanZim

This comment has been minimized.

Copy link
Collaborator

@RyanZim RyanZim commented May 11, 2019

Fixed in #618 & #667

@RyanZim RyanZim closed this May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.