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 and copySync can fail with 'Source and destination must not be the same' #657

Closed
bvachon opened this issue Jan 15, 2019 · 28 comments

Comments

@bvachon
Copy link

@bvachon bvachon commented Jan 15, 2019

  • Operating System: Windows 10
  • Node.js version: 8.11.3
  • fs-extra version:7.0.0

copy and copySync will fail with 'Source and destination must not be the same' when node occasionally returns the same ino for different files due to a bigint rounding issue (nodejs/node#12115). The solution is probably to set options parameter to { bigint: true } when calling fstat (https://nodejs.org/api/fs.html#fs_fs_fstat_fd_options_callback) from the checkPaths function of copy.js and copy-sync.js.

@bvachon bvachon changed the title https://github.com/nodejs/node/issues/12115 copy and copySync can fail with 'Source and destination must not be the same' Jan 17, 2019
@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Feb 1, 2019

yeah but we should note that bigint option was added in v10.5.0 and we are still supporting node >=6.

@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 5, 2019

This is pretty bad and makes tools that depend on this library very unreliable :( As a consumer, is there anything that can be done to work around this issue?

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Mar 6, 2019

Yeah I agree we need to fix this!

@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 6, 2019

Can you elaborate on the intention of the check?

My thoughts were, if it was intentional to not just check source against destination filename, the intention was to not copy files where the source and destination are some sort of link from one to the other. In which case a copy of a link onto its target or vice versa should be prevented.

I'm not sure what the outcome would be of such a copy operation, but I guess it's undesirable or will fail.

Sadly, in my test case, the entire fs.Stats object seems to be identical for both files, as they are actually copies of the same file. So none of the values in that object could be utilized to determine if the files are actually the same or not.

In case the check is in place to prevent failures, I think the only solution is to just let the process fail, catch the error and handle it appropriately.

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Mar 7, 2019

@oliversalzburg, we mainly check paths to ensure copy doesn't end up with unintended results! We used to check just paths as strings but as you mentioned, obviously that is not enough to handle cases like symlinks.
Basically, implementing cross-platform copy that handles all types of files, dirs, and links in one shot is not easy. There are a lot of edge cases when it comes to copy. We've been trying really hard to abstract all of these away to provide an easy to use api so that the end users don't need to deal with any of that!
So in terms of solution, we can do couple of things here. @jprichardson @RyanZim @JPeer264 I'd like to know your thoughts on this too please!

  1. add a new option to copy, something like checkPaths: true|false, and let users decide if they want us to check the paths before copying or not.
  2. as @oliversalzburg mentioned, perform the copy and handles it appropriately if it fails. This might get a bit tricky because of inconsistencies in OS error codes.
  3. use bigint option if is available and fallback to old function if not available.
  4. refactor check paths to not use inodes.
@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 7, 2019

I know you are not a fan of native dependencies, but I'd also like to mention the option I also offered in #626, a small piece of native code to work around the specific problem on Windows when bigint is not available.

@urugator

This comment has been minimized.

Copy link

@urugator urugator commented Mar 11, 2019

as @oliversalzburg mentioned, perform the copy and handles it appropriately if it fails. This might get a bit tricky because of inconsistencies in OS error codes.

From the end user perspective the situation is the same as if the destination is different, but already exists. The fact that src === dest is irrelevant. There are 2 ways to handle the situation:

  • catch the error, inform user that operation cannot be done, because the dest already exist
  • catch the error, generate unique name, try again until success

So in both cases we have to catch and identify the error, so it should provide an error code.
As pointed out the meaning of error is that the destination already exists, therefore the code imo should be EEXIST (similary to: #661).

For illustration:

async function tryCopy() {
  try {
    await copy(src, dest, {
      overwrite: false,
      errorOnExist: true,
    });
  } catch (error) {
    if (error.code === 'EEXIST' 
      // These are now additionally required:
      // || error.message === 'Source and destination must not be the same'
      // || error.message === `'${dest}' already exists`      
    ) {
      iter++;
      dest = `${originalDest} (${iter})`
      return await tryCopy();      
    }
    throw error;
  }
}

EDIT: hm .. how do I handle this with directories? I suppose I would need the ability to provide onError callback which could throw / try again with new src,dest / skip ...

EDIT2: On win:

copyFile(dir, dir) // EPERM
copyFile(file, file) // EBUSY
copyFile(file, file, Fs.constants.COPYFILE_EXCL) // EEXIST
@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 18, 2019

For the time being, would you accept a patch that allows bigint to be used in Node versions that have it? So that the end user at least has the ability to fix this issue by upgrading Node? Right now, this defect has enormous impact on downstream functionality and users are left with no option at all.

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 18, 2019

@manidlou If I understood this correctly, NodeJS FS returns or for ino (inode), right? So handle as string is not an options btw. (I think aloud.) And the problem of the error "Source and destination must not be the same." is the 53bit-safe-integer limit in JavaScript, interrupt me if I'm wrong. FS-Extra could check if bigint is supported (e.g. by typeof BigInt === 'function' and / or NodeJS version / feature check or whatever. Maybe Node handle it automatically ...) and use it. Easy, isn't? This could be an optional parameter which is set to auto with options for number or bigint (force). Additional option ignore the checking path function and let the system throw errors. All variants could exist together.
So I'm for your 3rd suggestion. Bigint with fallback to number. Because Node has provided this. So the solution of this issue is simple and for the consumer easy: Just update.

NodeJS FS Docs inode

And / or just let the system decide when a copy operation has to be failed. Remove the checkPath-feature. Or disable it by option. ...

PS: We could add the ino value to the verbose log. Espessally the check failed. So we can see what happend.

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 19, 2019

@jprichardson @manidlou I recommend to increase the priority of this issue. It's a critical bug. As I suggested: Enable bigint if supported. And / or remove the checkPaths method (as option or always).
If this fixes the bug. (Validate the value of state.ino and what's about symlinks.)
What you think? 🙂

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented Mar 21, 2019

I will work on this and try to have a PR ASAP! However, I am not a fan of not checking the paths by default because that means we clearly ship the buggy code because of issues like #83, #565 and alike! But I am open to have the checkPaths as an option and let users decide how copy should be handled.

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 21, 2019

@manidlou Thanks. 🙂
I do not understand why is it possible to copy a file / dir to an existing file / dir. This should be handled by OS or Node fs. Only a overwrite flag should do this. In my Option it should not necessary to check on higher level (fs-extra). The lower level fs / os should throw an error if I want to copy a file to an existing file without overwrite flag. (What's about rename file/dir? ...)

Btw. we could also enable the bigint feature. Yes, I know that this is a Node 10.4 feature. And not supported for older Node versions. But this library could only enable it, if it is supported. And if people has problems with the checkPaths cuz state.ino, they can just update Node and all's fine.

Finally, I don't know the inode (state.ino) value on a failue. We should log this to the error. (just add the value to the error text. (both values from destStat and srcStat). To see, is this a higher value than 53-bit or maybe 0 or undefined whatever. Maybe this feature does not work correctly.

Files with checkPaths (state.ino):
https://github.com/jprichardson/node-fs-extra/blob/7.0.1/lib/copy/copy.js#L236
https://github.com/jprichardson/node-fs-extra/blob/7.0.1/lib/copy-sync/copy-sync.js#L184

@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 21, 2019

To see, is this a higher value than 53-bit or maybe 0 or undefined whatever.

In all my tests, the inode was always a valid integer. The source is the issue nodejs/node#12115 as described in the original post.

I might have another suggestion too maybe. Given that this is a Windows-specific issue, how about this:

  • If the platform is non-Windows, processed as usual.
  • If it is Windows, with Node >=10, use bigint
  • If it is Windows, with Node <10, resolve src and dest with fs.realpath() and see if they resolve to the same path

The motivation being to leave as much code as possible as-is, and just focus on the problematic scenario, which could also allow for easy removal once older Node version support is dropped.

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 21, 2019

I'm wondering why is this a Windows-only issue. Maybe the other systems uses smaller integers?
Btw. here's a demonstration of max safe integer.

9007199254740991 (max_int of 53-bit)
9007199254740992 (max_int + 1)
9007199254740992 (max_int + 2)
9007199254740994 (max_int + 3)

And here's a demonstration of big integer.

9007199254740991 (max_int of 53-bit)
9007199254740992 (max_int + 1)
9007199254740993 (max_int + 2)
9007199254740994 (max_int + 3)

If the value of inode were a string, then we would not have any problems. But bigint will also solves it.
Versus Comparison

@oliversalzburg

This comment has been minimized.

Copy link

@oliversalzburg oliversalzburg commented Mar 21, 2019

I'm wondering why is this a Windows-only issue.

You know there's a very long issue with all the details that has been linked here several times now, right? 😉

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 21, 2019

@oliversalzburg Hehe, sorry. I'm lazy in reading. 😄
But you could post it again or explain it in short sentences the difference of Inode between Linux and Windows.

@Domvel

This comment has been minimized.

Copy link

@Domvel Domvel commented Mar 21, 2019

NOTE 🚩
The inode number is an integer unique to the volume upon which it is stored.
That means, the method checkPaths() disallows me to copy a file from one to another volume.
In other words: Remove this. It will randomly fail. Collisions / same values are possible! It's not only a safe integer issue.

@manidlou

This comment has been minimized.

Copy link
Collaborator

@manidlou manidlou commented May 13, 2019

Should be fixed in v8.0.1

@manidlou manidlou closed this May 13, 2019
@mbargiel

This comment has been minimized.

Copy link
Contributor

@mbargiel mbargiel commented May 31, 2019

This should be re-opened. It's only fixed for Node 10.5+. I'm proposing a fix that would also work (or at least provide a much smaller chance of this happening) in #667 (comment), and I could open a PR for it. Thoughts?

@brodybits

This comment has been minimized.

Copy link

@brodybits brodybits commented May 31, 2019

This should be re-opened. It's only fixed for Node 10.5+.

Yes, assuming that there is sufficient desire to support the older Node.js versions. Note that Node.js version 8 EOL is coming this December and older versions are already past EOL.

@mbargiel

This comment has been minimized.

Copy link
Contributor

@mbargiel mbargiel commented May 31, 2019

A lot of effort has been put into this to both fix it, and yet maintain backward-compatibility with older Node versions. December is still far, and this issue exists now. My proposed fix is simple, straightforward, local, and practically solves this issue for those who would use it for the next 6 months. Or, for anyone using an older Node 10 version that can't (for whatever reason) update to 10.5+. Besides, once a Node versions goes in EOL, it doesn't necessarily mean that it stops working - it just won't get fixes and updates anymore, and some devs might be perfectly fine with that.

@brodybits

This comment has been minimized.

Copy link

@brodybits brodybits commented May 31, 2019

A lot of effort has been put into this to both fix it, and yet maintain backward-compatibility with older Node versions.

@mbargiel I do not see a proposal from you to resolve the issue on older Node.js versions. Did I miss anything here?

@mbargiel

This comment has been minimized.

Copy link
Contributor

@mbargiel mbargiel commented May 31, 2019

Sorry - I probably should have quoted myself.

#667 (comment)

... relying also on other file stats could help detect different files with incorrectly identical inode values (due to the Number representation). E.g. in legacy form, if the inodes match, but either one of the timestamps or the size don't, then allow the copy because we know it's not the same file, otherwise reject it because it may be the same file.

My reasoning is that since stats.ino is not reliable on Windows with those Node.js versions, it follows that copy works a heuristic check rather than a complete check - when it works, it does the right thing, but sometimes it won't work when it should have. The window for failure is pretty slim, of course. But in automated scenarios, it happens sufficiently often to introduce frequent instabilities. (We currently hit this as an instability in a couple automated tests practically every day.)

By checking other stats that should be the same for hardlinks, we can reduce this window - that's practically free since included in the stats we already have, and it provides additional chances of making the right choice.

In addition, it provides an easy way for users still hitting this problem in some automated scenarios to avoid it: using different owners, modes, timestamps, contents, etc. just to make sure the source and dest paths differ in some way.

@mbargiel

This comment has been minimized.

Copy link
Contributor

@mbargiel mbargiel commented May 31, 2019

I'll take a shot at a PR, and you can comment on it. I'll post the link here.

@DarthRevanXX

This comment has been minimized.

Copy link

@DarthRevanXX DarthRevanXX commented Jun 26, 2019

So any solutions?

@jvkxhola

This comment has been minimized.

Copy link

@jvkxhola jvkxhola commented Jun 28, 2019

Any updates on that? I'm still having this problem

@RyanZim

This comment has been minimized.

Copy link
Collaborator

@RyanZim RyanZim commented Jun 28, 2019

@manidlou Your call on what to do here.

@mbargiel

This comment has been minimized.

Copy link
Contributor

@mbargiel mbargiel commented Jun 28, 2019

See #694 for a proposed "fix".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.