From 4ca2f901e35c9cf787f7b8b38375251ab71063ca Mon Sep 17 00:00:00 2001 From: bcoe Date: Fri, 24 Jan 2020 22:12:39 -0800 Subject: [PATCH 1/7] fs: recursion should bail on permission error When creating directories recursively, the logic should bail immediately on UV_EACCES and bubble the error to the user. Fixes: #31481 --- src/node_file.cc | 7 +++ .../test-fs-mkdir-recursive-eaccess.js | 50 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 test/parallel/test-fs-mkdir-recursive-eaccess.js diff --git a/src/node_file.cc b/src/node_file.cc index fbab726afb4fa0..c3ab573b2995d4 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1246,6 +1246,9 @@ int MKDirpSync(uv_loop_t* loop, case UV_EPERM: { return err; } + case UV_EACCES: { + return err; + } default: uv_fs_req_cleanup(req); int orig_err = err; @@ -1322,6 +1325,10 @@ int MKDirpAsync(uv_loop_t* loop, req_wrap->continuation_data()->Done(err); break; } + case UV_EACCES: { + req_wrap->continuation_data()->Done(err); + break; + } default: uv_fs_req_cleanup(req); // Stash err for use in the callback. diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js new file mode 100644 index 00000000000000..cb420c18489748 --- /dev/null +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -0,0 +1,50 @@ +'use strict'; + +// Test that mkdir with recursive option returns appropriate error +// when executed on folder it does not have permission to access. +// Ref: https://github.com/nodejs/node/issues/31481 + +const common = require('../common'); + +if (!common.isWindows && process.getuid() === 0) + common.skip('as this test should not be run as `root`'); + +if (common.isIBMi) + common.skip('IBMi has a different access permission mechanism'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); + +let n = 0; + +// Synchronous API should return an EACCESS error with path populated. +{ + const dir = path.join(tmpdir.path, `mkdirp_${n++}`); + fs.mkdirSync(dir); + fs.chmodSync(dir, '444'); + let err = null; + try { + fs.mkdirSync(path.join(dir, '/foo'), { recursive: true }); + } catch (_err) { + err = _err; + } + assert(err); + assert.strictEqual(err.code, 'EACCES'); + assert(err.path); +} + +// Asynchronous API should return an EACCESS error with path populated. +{ + const dir = path.join(tmpdir.path, `mkdirp_${n++}`); + fs.mkdirSync(dir); + fs.chmodSync(dir, '444'); + fs.mkdir(path.join(dir, '/bar'), { recursive: true }, (err) => { + assert(err); + assert.strictEqual(err.code, 'EACCES'); + assert(err.path); + }); +} From 55cd7f49245582edb50282c541a56b4988e7075f Mon Sep 17 00:00:00 2001 From: bcoe Date: Sat, 25 Jan 2020 09:38:57 -0800 Subject: [PATCH 2/7] chore: address code review --- src/node_file.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index c3ab573b2995d4..d9f36fe7ff7447 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1243,10 +1243,9 @@ int MKDirpSync(uv_loop_t* loop, } break; } + case UV_EACCES: case UV_EPERM: { - return err; - } - case UV_EACCES: { + uv_fs_req_cleanup(req); return err; } default: @@ -1321,14 +1320,11 @@ int MKDirpAsync(uv_loop_t* loop, req_wrap->continuation_data()->mode(), nullptr); break; } + case UV_EACCES: case UV_EPERM: { req_wrap->continuation_data()->Done(err); break; } - case UV_EACCES: { - req_wrap->continuation_data()->Done(err); - break; - } default: uv_fs_req_cleanup(req); // Stash err for use in the callback. From 0e1be3ccc463ebedc1c920ea39c4abee684f0a94 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sat, 25 Jan 2020 10:52:56 -0800 Subject: [PATCH 3/7] chore: debugging Windows issue --- src/node_file.cc | 2 ++ test/parallel/test-fs-mkdir-recursive-eaccess.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/node_file.cc b/src/node_file.cc index d9f36fe7ff7447..2476026616a222 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1247,6 +1247,7 @@ int MKDirpSync(uv_loop_t* loop, case UV_EPERM: { uv_fs_req_cleanup(req); return err; + break; } default: uv_fs_req_cleanup(req); @@ -1323,6 +1324,7 @@ int MKDirpAsync(uv_loop_t* loop, case UV_EACCES: case UV_EPERM: { req_wrap->continuation_data()->Done(err); + uv_fs_req_cleanup(req); break; } default: diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index cb420c18489748..6554fca5cfda18 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -28,8 +28,10 @@ let n = 0; fs.chmodSync(dir, '444'); let err = null; try { + console.log('31: mkdirsync'); fs.mkdirSync(path.join(dir, '/foo'), { recursive: true }); } catch (_err) { + console.log('34: mkdirsync', err); err = _err; } assert(err); @@ -42,7 +44,9 @@ let n = 0; const dir = path.join(tmpdir.path, `mkdirp_${n++}`); fs.mkdirSync(dir); fs.chmodSync(dir, '444'); + console.log('47: mkdir'); fs.mkdir(path.join(dir, '/bar'), { recursive: true }, (err) => { + console.log('49: mkdir', err); assert(err); assert.strictEqual(err.code, 'EACCES'); assert(err.path); From 47ba6b439e87c6d0d27f437b77177a38309049ac Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 26 Jan 2020 13:15:24 -0800 Subject: [PATCH 4/7] chore: fix up logic to capture error --- src/node_file.cc | 25 ++++++++++--------- .../test-fs-mkdir-recursive-eaccess.js | 4 --- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 2476026616a222..8d43b0ef662796 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1226,11 +1226,17 @@ int MKDirpSync(uv_loop_t* loop, int err = uv_fs_mkdir(loop, req, next_path.c_str(), mode, nullptr); while (true) { switch (err) { + // Note: uv_fs_req_cleanup in terminal paths will be called by + // ~FSReqWrapSync(): case 0: if (continuation_data.paths().size() == 0) { return 0; } break; + case UV_EACCES: + case UV_EPERM: { + return err; + } case UV_ENOENT: { std::string dirname = next_path.substr(0, next_path.find_last_of(kPathSeparator)); @@ -1243,12 +1249,6 @@ int MKDirpSync(uv_loop_t* loop, } break; } - case UV_EACCES: - case UV_EPERM: { - uv_fs_req_cleanup(req); - return err; - break; - } default: uv_fs_req_cleanup(req); int orig_err = err; @@ -1296,6 +1296,8 @@ int MKDirpAsync(uv_loop_t* loop, while (true) { switch (err) { + // Note: uv_fs_req_cleanup in terminal paths will be called by + // FSReqAfterScope::~FSReqAfterScope() case 0: { if (req_wrap->continuation_data()->paths().size() == 0) { req_wrap->continuation_data()->Done(0); @@ -1306,6 +1308,11 @@ int MKDirpAsync(uv_loop_t* loop, } break; } + case UV_EACCES: + case UV_EPERM: { + req_wrap->continuation_data()->Done(err); + break; + } case UV_ENOENT: { std::string dirname = path.substr(0, path.find_last_of(kPathSeparator)); @@ -1321,12 +1328,6 @@ int MKDirpAsync(uv_loop_t* loop, req_wrap->continuation_data()->mode(), nullptr); break; } - case UV_EACCES: - case UV_EPERM: { - req_wrap->continuation_data()->Done(err); - uv_fs_req_cleanup(req); - break; - } default: uv_fs_req_cleanup(req); // Stash err for use in the callback. diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index 6554fca5cfda18..cb420c18489748 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -28,10 +28,8 @@ let n = 0; fs.chmodSync(dir, '444'); let err = null; try { - console.log('31: mkdirsync'); fs.mkdirSync(path.join(dir, '/foo'), { recursive: true }); } catch (_err) { - console.log('34: mkdirsync', err); err = _err; } assert(err); @@ -44,9 +42,7 @@ let n = 0; const dir = path.join(tmpdir.path, `mkdirp_${n++}`); fs.mkdirSync(dir); fs.chmodSync(dir, '444'); - console.log('47: mkdir'); fs.mkdir(path.join(dir, '/bar'), { recursive: true }, (err) => { - console.log('49: mkdir', err); assert(err); assert.strictEqual(err.code, 'EACCES'); assert(err.path); From 6e4ae17debd7d5e62b0d81c338a2be29ab6f729b Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 26 Jan 2020 14:08:21 -0800 Subject: [PATCH 5/7] chore: make test work on Windows --- .../test-fs-mkdir-recursive-eaccess.js | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index cb420c18489748..0c016e5ddb862b 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -16,6 +16,7 @@ const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); const assert = require('assert'); +const { execSync } = require('child_process'); const fs = require('fs'); const path = require('path'); @@ -25,7 +26,14 @@ let n = 0; { const dir = path.join(tmpdir.path, `mkdirp_${n++}`); fs.mkdirSync(dir); - fs.chmodSync(dir, '444'); + let codeExpected = 'EACCES'; + if (common.isWindows) { + codeExpected = 'EPERM'; + execSync(`icacls ${dir} /inheritance:r`); + execSync(`icacls ${dir} /deny "everyone":W`); + } else { + fs.chmodSync(dir, '444'); + } let err = null; try { fs.mkdirSync(path.join(dir, '/foo'), { recursive: true }); @@ -33,7 +41,7 @@ let n = 0; err = _err; } assert(err); - assert.strictEqual(err.code, 'EACCES'); + assert.strictEqual(err.code, codeExpected); assert(err.path); } @@ -41,10 +49,17 @@ let n = 0; { const dir = path.join(tmpdir.path, `mkdirp_${n++}`); fs.mkdirSync(dir); - fs.chmodSync(dir, '444'); + let codeExpected = 'EACCES'; + if (common.isWindows) { + codeExpected = 'EPERM'; + execSync(`icacls ${dir} /inheritance:r`); + execSync(`icacls ${dir} /deny "everyone":W`); + } else { + fs.chmodSync(dir, '444'); + } fs.mkdir(path.join(dir, '/bar'), { recursive: true }, (err) => { assert(err); - assert.strictEqual(err.code, 'EACCES'); + assert.strictEqual(err.code, codeExpected); assert(err.path); }); } From a1031617f462f81d50fb076a30e1feaa4d18964e Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 26 Jan 2020 14:20:28 -0800 Subject: [PATCH 6/7] chore: refactor windows perms logic --- .../test-fs-mkdir-recursive-eaccess.js | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index 0c016e5ddb862b..fa8602c075510b 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -22,24 +22,36 @@ const path = require('path'); let n = 0; -// Synchronous API should return an EACCESS error with path populated. -{ - const dir = path.join(tmpdir.path, `mkdirp_${n++}`); - fs.mkdirSync(dir); - let codeExpected = 'EACCES'; +function makeDirectoryReadOnly(dir) { + let accessErrorCode = 'EACCES'; if (common.isWindows) { - codeExpected = 'EPERM'; + accessErrorCode = 'EPERM'; execSync(`icacls ${dir} /inheritance:r`); execSync(`icacls ${dir} /deny "everyone":W`); } else { fs.chmodSync(dir, '444'); } + return accessErrorCode; +} + +function makeDirectoryWritable(dir) { + if (common.isWindows) { + execSync(`icacls ${dir} /inheritance:e`); + } +} + +// Synchronous API should return an EACCESS error with path populated. +{ + const dir = path.join(tmpdir.path, `mkdirp_${n++}`); + fs.mkdirSync(dir); + const codeExpected = makeDirectoryReadOnly(dir); let err = null; try { fs.mkdirSync(path.join(dir, '/foo'), { recursive: true }); } catch (_err) { err = _err; } + makeDirectoryWritable(dir); assert(err); assert.strictEqual(err.code, codeExpected); assert(err.path); @@ -49,15 +61,9 @@ let n = 0; { const dir = path.join(tmpdir.path, `mkdirp_${n++}`); fs.mkdirSync(dir); - let codeExpected = 'EACCES'; - if (common.isWindows) { - codeExpected = 'EPERM'; - execSync(`icacls ${dir} /inheritance:r`); - execSync(`icacls ${dir} /deny "everyone":W`); - } else { - fs.chmodSync(dir, '444'); - } + const codeExpected = makeDirectoryReadOnly(dir); fs.mkdir(path.join(dir, '/bar'), { recursive: true }, (err) => { + makeDirectoryWritable(dir); assert(err); assert.strictEqual(err.code, codeExpected); assert(err.path); From 903be10434c38378ecc1d2e81634d2c30c4a46a1 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 26 Jan 2020 14:26:57 -0800 Subject: [PATCH 7/7] chore: fix bug with makeDirectoryWritable --- test/parallel/test-fs-mkdir-recursive-eaccess.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index fa8602c075510b..d0b3b2b950cf7b 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -36,7 +36,7 @@ function makeDirectoryReadOnly(dir) { function makeDirectoryWritable(dir) { if (common.isWindows) { - execSync(`icacls ${dir} /inheritance:e`); + execSync(`icacls ${dir} /grant "everyone":W`); } }