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

Move fails with EPERM when destination is in root of drive #819

Closed
ZudoB opened this issue Jul 3, 2020 · 11 comments
Closed

Move fails with EPERM when destination is in root of drive #819

ZudoB opened this issue Jul 3, 2020 · 11 comments
Assignees
Milestone

Comments

@ZudoB
Copy link

@ZudoB ZudoB commented Jul 3, 2020

  • Operating System: Windows 10 1909
  • Node.js version: 14.2.0
  • fs-extra version: 9.0.1

Moving a file or directory where the destination is in the root of a drive (e.g. D:\filename) causes the operation to fail, as the move function attempts to mkdir the root of the drive.

const fs = require("fs-extra");
fs.moveSync("D:\\Source.txt", "D:\\Destination\\SomeFolder"); // succeeds
fs.moveSync("D:\\Source.txt", "D:\\Destination"); // fails

Error is as follows:

Uncaught Error: EPERM: operation not permitted, mkdir 'D:\'
    at Object.mkdirSync (fs.js:940:3)
    at module.exports.makeDirSync (---\node_modules\fs-extra\lib\mkdirs\make-dir.js:101:15)
    at Object.moveSync (---\node_modules\fs-extra\lib\move-sync\move-sync.js:16:3) {
  errno: -4048,
  syscall: 'mkdir',
  code: 'EPERM',
  path: 'D:\\'
}
@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Jul 3, 2020

Huh, that's a bug; looks like we need to either check if the parent is root, or else have proper handling of an EPERM error here:

mkdirp(path.dirname(dest), err => {

@manidlou what's your preference?

@manidlou
Copy link
Collaborator

@manidlou manidlou commented Jul 5, 2020

That's actually documented (https://nodejs.org/api/fs.html#fs_fs_mkdir_path_options_callback). I would say we check the error, but if anyone thinks checking if parent is root is a better approach because of specific reasons, I am open to that!

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Jul 6, 2020

We will need to check for root either way most likely, because we can't consider all EPERM errors to be as a result of root. The question is whether we pre-emptively check for root, or check after we get EPERM. As for the choice between those two, if we need to hit the FS to check root, do it after we get the error, to keep it fast for all other cases; if we can just do a static path check, perhaps we can check before calling mkdirp.

To be perfectly clear here, I'm not in favor of changing the way mkdirp works here, I just think move should handle this better.

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Jul 6, 2020

Interestingly enough, copy doesn't have this problem, because it does an explicit pathExists call before calling mkdirs:

function checkParentDir (destStat, src, dest, opts, cb) {
const destParent = path.dirname(dest)
pathExists(destParent, (err, dirExists) => {
if (err) return cb(err)
if (dirExists) return startCopy(destStat, src, dest, opts, cb)
mkdirs(destParent, err => {
if (err) return cb(err)
return startCopy(destStat, src, dest, opts, cb)
})
})
}

Not sure if that's ideal, though. Anytime you use pathExists, you have race condition potential.

@RyanZim RyanZim added this to the 10.0.0 milestone Jul 29, 2020
@manidlou
Copy link
Collaborator

@manidlou manidlou commented Oct 30, 2020

Is it enough to use path.parse() to get the root and check if that's the same as the parent?

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Oct 30, 2020

@manidlou That's a good idea! It should work, though I'm not sure, we might need to do some slash normalization.

@manidlou manidlou self-assigned this Feb 17, 2021
@manidlou
Copy link
Collaborator

@manidlou manidlou commented Feb 17, 2021

hmm.. I actually just remembered that moving to the root would basically fail because of permission error (unless you run it with sudo) at least on unix like systems, but I guess things are different in windows! That makes writing tests for this a bit tricky!

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Apr 2, 2021

@manidlou it'd be perfectly fine to write a test that's skipped on all platforms other than Windows for this. Any progress here?

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Apr 21, 2021

@manidlou what's the status here?

@RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented May 1, 2021

I'd like to ship v10 next week, looks like this will need to be pushed back.

@manidlou
Copy link
Collaborator

@manidlou manidlou commented May 1, 2021

Sorry @RyanZim! been super busy! I plan to wrap this up this weekend. If I cannot manage to do that, we will leave it for v11.

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

Successfully merging a pull request may close this issue.

3 participants