copy and copySync can fail with 'Source and destination must not be the same'

[bvachon]

• 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.

[manidlou]

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

[oliversalzburg]

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]

Yeah I agree we need to fix this!

[oliversalzburg]

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]

@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]

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]

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]

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]

@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]

@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]

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]

@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]

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]

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]

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]

@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]

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]

Should be fixed in v8.0.1

[mbargiel]

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 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]

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]

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]

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]

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

[DarthRevanXX]

So any solutions?

[jvkxhola]

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

[RyanZim]

@manidlou Your call on what to do here.

[mbargiel]

See #694 for a proposed "fix".