Skip to content

Commit

Permalink
fs: fix infinite loop with async recursive mkdir on Windows
Browse files Browse the repository at this point in the history
If `file` is a file then on Windows `mkdir` on `file/a` returns an
`ENOENT` error while on POSIX the equivalent returns `ENOTDIR`. On the
POSIX systems `ENOTDIR` would break out of the loop but on Windows the
`ENOENT` would strip off the `a` and attempt to make `file` as a
directory. This would return `EEXIST` but the code wasn't detecting
that the existing path was a file and attempted to make `file/a` again.

PR-URL: #27207
Fixes: #27198
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
  • Loading branch information
richardlau authored and MylesBorins committed May 16, 2019
1 parent aceac05 commit af301b2
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 19 deletions.
48 changes: 32 additions & 16 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1289,8 +1289,15 @@ int MKDirpSync(uv_loop_t* loop, uv_fs_t* req, const std::string& path, int mode,
}
default:
uv_fs_req_cleanup(req);
int orig_err = err;
err = uv_fs_stat(loop, req, next_path.c_str(), nullptr);
if (err == 0 && !S_ISDIR(req->statbuf.st_mode)) return UV_EEXIST;
if (err == 0 && !S_ISDIR(req->statbuf.st_mode)) {
uv_fs_req_cleanup(req);
if (orig_err == UV_EEXIST && continuation_data.paths.size() > 0) {
return UV_ENOTDIR;
}
return UV_EEXIST;
}
if (err < 0) return err;
break;
}
Expand Down Expand Up @@ -1357,23 +1364,32 @@ int MKDirpAsync(uv_loop_t* loop,
break;
}
default:
if (err == UV_EEXIST &&
req_wrap->continuation_data->paths.size() > 0) {
uv_fs_req_cleanup(req);
MKDirpAsync(loop, req, path.c_str(),
req_wrap->continuation_data->mode, nullptr);
} else {
uv_fs_req_cleanup(req);
// Stash err for use in the callback.
req->data = reinterpret_cast<void*>(static_cast<intptr_t>(err));
int err = uv_fs_stat(loop, req, path.c_str(),
uv_fs_callback_t{[](uv_fs_t* req) {
FSReqBase* req_wrap = FSReqBase::from_req(req);
int err = req->result;
if (reinterpret_cast<intptr_t>(req->data) == UV_EEXIST &&
req_wrap->continuation_data->paths.size() > 0) {
if (err == 0 && S_ISDIR(req->statbuf.st_mode)) {
Environment* env = req_wrap->env();
uv_loop_t* loop = env->event_loop();
std::string path = req->path;
uv_fs_req_cleanup(req);
MKDirpAsync(loop, req, path.c_str(),
req_wrap->continuation_data->mode, nullptr);
return;
}
err = UV_ENOTDIR;
}
// verify that the path pointed to is actually a directory.
if (err == 0 && !S_ISDIR(req->statbuf.st_mode)) err = UV_EEXIST;
uv_fs_req_cleanup(req);
int err = uv_fs_stat(loop, req, path.c_str(),
uv_fs_callback_t{[](uv_fs_t* req) {
FSReqBase* req_wrap = FSReqBase::from_req(req);
int err = req->result;
if (err == 0 && !S_ISDIR(req->statbuf.st_mode)) err = UV_EEXIST;
req_wrap->continuation_data->Done(err);
}});
if (err < 0) req_wrap->continuation_data->Done(err);
}
req_wrap->continuation_data->Done(err);
}});
if (err < 0) req_wrap->continuation_data->Done(err);
break;
}
break;
Expand Down
38 changes: 35 additions & 3 deletions test/parallel/test-fs-mkdir.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,25 @@ function nextdir() {
}
}

// mkdirp when folder does not yet exist.
// mkdirpSync when part of the path is a file.
{
const filename = path.join(tmpdir.path, nextdir(), nextdir());
const pathname = path.join(filename, nextdir(), nextdir());

fs.mkdirSync(path.dirname(filename));
fs.writeFileSync(filename, '', 'utf8');

try {
fs.mkdirSync(pathname, { recursive: true });
throw new Error('unreachable');
} catch (err) {
assert.notStrictEqual(err.message, 'unreachable');
assert.strictEqual(err.code, 'ENOTDIR');
assert.strictEqual(err.syscall, 'mkdir');
}
}

// `mkdirp` when folder does not yet exist.
{
const pathname = path.join(tmpdir.path, nextdir(), nextdir());

Expand All @@ -149,11 +167,25 @@ function nextdir() {

fs.mkdirSync(path.dirname(pathname));
fs.writeFileSync(pathname, '', 'utf8');
fs.mkdir(pathname, { recursive: true }, (err) => {
fs.mkdir(pathname, { recursive: true }, common.mustCall((err) => {
assert.strictEqual(err.code, 'EEXIST');
assert.strictEqual(err.syscall, 'mkdir');
assert.strictEqual(fs.statSync(pathname).isDirectory(), false);
});
}));
}

// `mkdirp` when part of the path is a file.
{
const filename = path.join(tmpdir.path, nextdir(), nextdir());
const pathname = path.join(filename, nextdir(), nextdir());

fs.mkdirSync(path.dirname(filename));
fs.writeFileSync(filename, '', 'utf8');
fs.mkdir(pathname, { recursive: true }, common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTDIR');
assert.strictEqual(err.syscall, 'mkdir');
assert.strictEqual(fs.existsSync(pathname), false);
}));
}

// mkdirpSync dirname loop
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-fs-promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,22 @@ async function getHandle(dest) {
}
}

// `mkdirp` when part of the path is a file.
{
const file = path.join(tmpDir, nextdir(), nextdir());
const dir = path.join(file, nextdir(), nextdir());
await mkdir(path.dirname(file));
await writeFile(file);
try {
await mkdir(dir, { recursive: true });
throw new Error('unreachable');
} catch (err) {
assert.notStrictEqual(err.message, 'unreachable');
assert.strictEqual(err.code, 'ENOTDIR');
assert.strictEqual(err.syscall, 'mkdir');
}
}

// mkdirp ./
{
const dir = path.resolve(tmpDir, `${nextdir()}/./${nextdir()}`);
Expand Down

0 comments on commit af301b2

Please sign in to comment.