From 2400cbcf7c6b3c35c96b199676cb8a577f6e701a Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Sat, 13 Apr 2019 02:29:01 +0100 Subject: [PATCH 1/2] fs: fix infinite loop with async recursive mkdir on Windows 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: https://github.com/nodejs/node/pull/27207 Fixes: https://github.com/nodejs/node/issues/27198 Reviewed-By: Colin Ihrig Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Ben Coe --- src/node_file.cc | 48 ++++++++++++++++++++----------- test/parallel/test-fs-mkdir.js | 36 +++++++++++++++++++++-- test/parallel/test-fs-promises.js | 16 +++++++++++ 3 files changed, 82 insertions(+), 18 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index e1acf82408fcf0..5d3b48cfa4d781 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1270,8 +1270,15 @@ int MKDirpSync(uv_loop_t* loop, } 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; } @@ -1338,23 +1345,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(static_cast(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(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; diff --git a/test/parallel/test-fs-mkdir.js b/test/parallel/test-fs-mkdir.js index 9bd2df180112f0..82f126e4ed1a7d 100644 --- a/test/parallel/test-fs-mkdir.js +++ b/test/parallel/test-fs-mkdir.js @@ -132,6 +132,24 @@ function nextdir() { } } +// 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()); @@ -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 diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index b333801a8a03ee..3fc88f7d8033bc 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -309,6 +309,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()}`); From c6c37e9e850fa2a30b12a0d0fd0dfe144eda0959 Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Tue, 16 Apr 2019 03:26:28 +0100 Subject: [PATCH 2/2] test: use assert.rejects() and assert.throws() Replace try-catch blocks in tests with `assert.rejects()` and `assert.throws()`. PR-URL: https://github.com/nodejs/node/pull/27207 Fixes: https://github.com/nodejs/node/issues/27198 Reviewed-By: Colin Ihrig Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Ben Coe --- test/parallel/test-fs-mkdir.js | 51 ++++++++++++++++--------------- test/parallel/test-fs-promises.js | 34 +++++++++++---------- 2 files changed, 45 insertions(+), 40 deletions(-) diff --git a/test/parallel/test-fs-mkdir.js b/test/parallel/test-fs-mkdir.js index 82f126e4ed1a7d..2cd42f65669e90 100644 --- a/test/parallel/test-fs-mkdir.js +++ b/test/parallel/test-fs-mkdir.js @@ -122,14 +122,15 @@ function nextdir() { fs.mkdirSync(path.dirname(pathname)); fs.writeFileSync(pathname, '', 'utf8'); - try { - fs.mkdirSync(pathname, { recursive: true }); - throw new Error('unreachable'); - } catch (err) { - assert.notStrictEqual(err.message, 'unreachable'); - assert.strictEqual(err.code, 'EEXIST'); - assert.strictEqual(err.syscall, 'mkdir'); - } + assert.throws( + () => { fs.mkdirSync(pathname, { recursive: true }); }, + { + code: 'EEXIST', + message: /EEXIST: .*mkdir/, + name: 'Error', + syscall: 'mkdir', + } + ); } // mkdirpSync when part of the path is a file. @@ -140,14 +141,15 @@ function 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'); - } + assert.throws( + () => { fs.mkdirSync(pathname, { recursive: true }); }, + { + code: 'ENOTDIR', + message: /ENOTDIR: .*mkdir/, + name: 'Error', + syscall: 'mkdir', + } + ); } // `mkdirp` when folder does not yet exist. @@ -195,14 +197,15 @@ if (common.isMainThread && (common.isLinux || common.isOSX)) { fs.mkdirSync(pathname); process.chdir(pathname); fs.rmdirSync(pathname); - try { - fs.mkdirSync('X', { recursive: true }); - throw new Error('unreachable'); - } catch (err) { - assert.notStrictEqual(err.message, 'unreachable'); - assert.strictEqual(err.code, 'ENOENT'); - assert.strictEqual(err.syscall, 'mkdir'); - } + assert.throws( + () => { fs.mkdirSync('X', { recursive: true }); }, + { + code: 'ENOENT', + message: /ENOENT: .*mkdir/, + name: 'Error', + syscall: 'mkdir', + } + ); fs.mkdir('X', { recursive: true }, (err) => { assert.strictEqual(err.code, 'ENOENT'); assert.strictEqual(err.syscall, 'mkdir'); diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 3fc88f7d8033bc..28c047e3055a8c 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -299,14 +299,15 @@ async function getHandle(dest) { const dir = path.join(tmpDir, nextdir(), nextdir()); await mkdir(path.dirname(dir)); await writeFile(dir); - try { - await mkdir(dir, { recursive: true }); - throw new Error('unreachable'); - } catch (err) { - assert.notStrictEqual(err.message, 'unreachable'); - assert.strictEqual(err.code, 'EEXIST'); - assert.strictEqual(err.syscall, 'mkdir'); - } + assert.rejects( + mkdir(dir, { recursive: true }), + { + code: 'EEXIST', + message: /EEXIST: .*mkdir/, + name: 'Error', + syscall: 'mkdir', + } + ); } // `mkdirp` when part of the path is a file. @@ -315,14 +316,15 @@ async function getHandle(dest) { 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'); - } + assert.rejects( + mkdir(dir, { recursive: true }), + { + code: 'ENOTDIR', + message: /ENOTDIR: .*mkdir/, + name: 'Error', + syscall: 'mkdir', + } + ); } // mkdirp ./