From 02cee53c78fb8b29599437380d80fd522db0db5c Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Sun, 13 Sep 2020 14:09:06 -0600 Subject: [PATCH 1/6] fs: throw in rmdir recursive if path does not exist or is not a directory --- doc/api/fs.md | 6 +- lib/internal/fs/rimraf.js | 48 ++++++++++++-- test/parallel/test-fs-rmdir-recursive.js | 80 ++++++++++++++++++++---- 3 files changed, 112 insertions(+), 22 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 457df39bc310a0..65851be2cc0bbc 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -3518,8 +3518,7 @@ changes: the number of retries. This option is ignored if the `recursive` option is not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In - recursive mode, errors are not reported if `path` does not exist, and - operations are retried on failure. **Default:** `false`. + recursive mode operations are retried on failure. **Default:** `false`. * `retryDelay` {integer} The amount of time in milliseconds to wait between retries. This option is ignored if the `recursive` option is not `true`. **Default:** `100`. @@ -3565,8 +3564,7 @@ changes: the number of retries. This option is ignored if the `recursive` option is not `true`. **Default:** `0`. * `recursive` {boolean} If `true`, perform a recursive directory removal. In - recursive mode, errors are not reported if `path` does not exist, and - operations are retried on failure. **Default:** `false`. + recursive mode operations are retried on failure. **Default:** `false`. * `retryDelay` {integer} The amount of time in milliseconds to wait between retries. This option is ignored if the `recursive` option is not `true`. **Default:** `100`. diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index e88dd9697b1938..14a8b22ebbd7ee 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -29,6 +29,11 @@ const { const { sep } = require('path'); const { setTimeout } = require('timers'); const { sleep } = require('internal/util'); +const { + codes: { + ERR_INVALID_ARG_VALUE + }, +} = require('internal/errors'); const notEmptyErrorCodes = new Set(['ENOTEMPTY', 'EEXIST', 'EPERM']); const retryErrorCodes = new Set( ['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']); @@ -40,14 +45,29 @@ const separator = Buffer.from(sep); function rimraf(path, options, callback) { + stat(path, (err, stats) => { + if (err && err.code === 'ENOENT') { + callback(err); + } else if (stats && !stats.isDirectory()) { + callback(new ERR_INVALID_ARG_VALUE( + 'path', path, 'is not a directory' + )); + } else { + _rimraf(path, options, callback); + } + }); +} + + +function _rimraf(path, options, callback) { let retries = 0; - _rimraf(path, options, function CB(err) { + __rimraf(path, options, function CB(err) { if (err) { if (retryErrorCodes.has(err.code) && retries < options.maxRetries) { retries++; const delay = retries * options.retryDelay; - return setTimeout(_rimraf, delay, path, options, CB); + return setTimeout(__rimraf, delay, path, options, CB); } // The file is already gone. @@ -60,7 +80,7 @@ function rimraf(path, options, callback) { } -function _rimraf(path, options, callback) { +function __rimraf(path, options, callback) { // SunOS lets the root user unlink directories. Use lstat here to make sure // it's not a directory. lstat(path, (err, stats) => { @@ -141,7 +161,7 @@ function _rmchildren(path, options, callback) { files.forEach((child) => { const childPath = Buffer.concat([pathBuf, separator, child]); - rimraf(childPath, options, (err) => { + _rimraf(childPath, options, (err) => { if (done) return; @@ -174,6 +194,24 @@ function rimrafPromises(path, options) { function rimrafSync(path, options) { let stats; + try { + stats = statSync(path); + } catch (err) { + if (err.code === 'ENOENT') + throw err; + } + + if (!stats.isDirectory()) { + throw new ERR_INVALID_ARG_VALUE('path', path, 'is not a directory'); + } + + _rimrafSync(path, options); +} + + +function _rimrafSync(path, options) { + let stats; + try { stats = lstatSync(path); } catch (err) { @@ -242,7 +280,7 @@ function _rmdirSync(path, options, originalErr) { readdirSync(pathBuf, readdirEncoding).forEach((child) => { const childPath = Buffer.concat([pathBuf, separator, child]); - rimrafSync(childPath, options); + _rimrafSync(childPath, options); }); const tries = options.maxRetries + 1; diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 2793e0ee111f82..381a00285d4a19 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -75,14 +75,9 @@ function removeAsync(dir) { fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { assert.ifError(err); - // No error should occur if recursive and the directory does not exist. - fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { - assert.ifError(err); - - // Attempted removal should fail now because the directory is gone. - fs.rmdir(dir, common.mustCall((err) => { - assert.strictEqual(err.syscall, 'rmdir'); - })); + // Attempted removal should fail now because the directory is gone. + fs.rmdir(dir, common.mustCall((err) => { + assert.strictEqual(err.syscall, 'rmdir'); })); })); })); @@ -105,6 +100,25 @@ function removeAsync(dir) { dir = nextDirPath(); makeNonEmptyDirectory(1, 10, 2, dir, true); removeAsync(dir); + + // Should fail if target does not exist + fs.rmdir( + path.join(tmpdir.path, 'noexist.txt'), + { recursive: true }, + common.mustCall((err) => { + assert.strictEqual(err.code, 'ENOENT'); + }) + ); + + // Should fail if target is a file + const filePath = path.join(tmpdir.path, 'rmdir-async-file.txt'); + fs.writeFileSync(filePath, ''); + fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); + assert.strictEqual(err.name, 'TypeError'); + assert.match(err.message, /^The argument 'path' is not a directory\./); + })); + fs.unlinkSync(filePath); } // Test the synchronous version. @@ -120,10 +134,30 @@ function removeAsync(dir) { fs.rmdirSync(dir, { recursive: false }); }, { syscall: 'rmdir' }); - // Recursive removal should succeed. - fs.rmdirSync(dir, { recursive: true }); + // Should fail if target does not exist + assert.throws(() => { + fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true }); + }, { + code: 'ENOENT', + name: 'Error', + message: /^ENOENT: no such file or directory, stat/ + }); + + // Should fail if target is a file + const filePath = path.join(tmpdir.path, 'rmdir-sync-file.txt'); + fs.writeFileSync(filePath, ''); + + assert.throws(() => { + fs.rmdirSync(filePath, { recursive: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: /^The argument 'path' is not a directory\./ + }); - // No error should occur if recursive and the directory does not exist. + fs.unlinkSync(filePath); + + // Recursive removal should succeed. fs.rmdirSync(dir, { recursive: true }); // Attempted removal should fail now because the directory is gone. @@ -144,8 +178,28 @@ function removeAsync(dir) { // Recursive removal should succeed. await fs.promises.rmdir(dir, { recursive: true }); - // No error should occur if recursive and the directory does not exist. - await fs.promises.rmdir(dir, { recursive: true }); + // Should fail if target does not exist + assert.rejects(fs.promises.rmdir( + path.join(tmpdir.path, 'noexist.txt'), + { recursive: true } + ), { + code: 'ENOENT', + name: 'Error', + message: /^ENOENT: no such file or directory, stat/ + }); + + // Should fail if target is a file + const filePath = path.join(tmpdir.path, 'rmdir-sync-file.txt'); + fs.writeFileSync(filePath, ''); + + assert.rejects(fs.promises.rmdir( + filePath, + { recursive: true } + ), { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: /^The argument 'path' is not a directory\./ + }); // Attempted removal should fail now because the directory is gone. assert.rejects(fs.promises.rmdir(dir), { syscall: 'rmdir' }); From 43c785e79256eb9ae10d4d09e416e98796dc5669 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Fri, 18 Sep 2020 00:52:58 -0600 Subject: [PATCH 2/6] Fix tests --- test/common/tmpdir.js | 6 +++++- test/parallel/test-policy-parse-integrity.js | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index 5e1e21429fe06b..8522ef6dc69981 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -6,7 +6,11 @@ const path = require('path'); const { isMainThread } = require('worker_threads'); function rimrafSync(pathname) { - fs.rmdirSync(pathname, { maxRetries: 3, recursive: true }); + try { + fs.rmdirSync(pathname, { maxRetries: 3, recursive: true }); + } catch { + // do nothing + } } const testRoot = process.env.NODE_TEST_DIR ? diff --git a/test/parallel/test-policy-parse-integrity.js b/test/parallel/test-policy-parse-integrity.js index 2443d9691c2a51..295162468f1d30 100644 --- a/test/parallel/test-policy-parse-integrity.js +++ b/test/parallel/test-policy-parse-integrity.js @@ -20,7 +20,13 @@ function hash(algo, body) { } const tmpdirPath = path.join(tmpdir.path, 'test-policy-parse-integrity'); -fs.rmdirSync(tmpdirPath, { maxRetries: 3, recursive: true }); + +try { + fs.rmdirSync(tmpdirPath, { maxRetries: 3, recursive: true }); +} catch { + // do nothing +} + fs.mkdirSync(tmpdirPath, { recursive: true }); const policyFilepath = path.join(tmpdirPath, 'policy'); From e26dbbaf2e50939a67da9b3f63c4ac11e547aa40 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Fri, 18 Sep 2020 01:05:37 -0600 Subject: [PATCH 3/6] Improve test cleanup --- test/parallel/test-fs-rmdir-recursive.js | 43 ++++++++++++++---------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 381a00285d4a19..8d9467c58a829a 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -117,8 +117,8 @@ function removeAsync(dir) { assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); assert.strictEqual(err.name, 'TypeError'); assert.match(err.message, /^The argument 'path' is not a directory\./); + fs.unlinkSync(filePath); })); - fs.unlinkSync(filePath); } // Test the synchronous version. @@ -147,15 +147,18 @@ function removeAsync(dir) { const filePath = path.join(tmpdir.path, 'rmdir-sync-file.txt'); fs.writeFileSync(filePath, ''); - assert.throws(() => { - fs.rmdirSync(filePath, { recursive: true }); - }, { - code: 'ERR_INVALID_ARG_VALUE', - name: 'TypeError', - message: /^The argument 'path' is not a directory\./ - }); + try { + assert.throws(() => { + fs.rmdirSync(filePath, { recursive: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: /^The argument 'path' is not a directory\./ + }); + } finally { + fs.unlinkSync(filePath); + } - fs.unlinkSync(filePath); // Recursive removal should succeed. fs.rmdirSync(dir, { recursive: true }); @@ -189,17 +192,21 @@ function removeAsync(dir) { }); // Should fail if target is a file - const filePath = path.join(tmpdir.path, 'rmdir-sync-file.txt'); + const filePath = path.join(tmpdir.path, 'rmdir-promises-file.txt'); fs.writeFileSync(filePath, ''); - assert.rejects(fs.promises.rmdir( - filePath, - { recursive: true } - ), { - code: 'ERR_INVALID_ARG_VALUE', - name: 'TypeError', - message: /^The argument 'path' is not a directory\./ - }); + try { + assert.rejects(fs.promises.rmdir( + filePath, + { recursive: true } + ), { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: /^The argument 'path' is not a directory\./ + }); + } finally { + fs.unlinkSync(filePath); + } // Attempted removal should fail now because the directory is gone. assert.rejects(fs.promises.rmdir(dir), { syscall: 'rmdir' }); From 0eb9255057fae5ef6e5148596f7cc5d55203ac04 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Wed, 23 Sep 2020 14:39:03 -0600 Subject: [PATCH 4/6] Fix tests --- test/parallel/test-fs-rmdir-recursive.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 8d9467c58a829a..a3536df1cf0e34 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -114,10 +114,13 @@ function removeAsync(dir) { const filePath = path.join(tmpdir.path, 'rmdir-async-file.txt'); fs.writeFileSync(filePath, ''); fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); - assert.strictEqual(err.name, 'TypeError'); - assert.match(err.message, /^The argument 'path' is not a directory\./); - fs.unlinkSync(filePath); + try { + assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); + assert.strictEqual(err.name, 'TypeError'); + assert.match(err.message, /^The argument 'path' is not a directory\./); + } finally { + fs.unlinkSync(filePath); + } })); } @@ -196,7 +199,7 @@ function removeAsync(dir) { fs.writeFileSync(filePath, ''); try { - assert.rejects(fs.promises.rmdir( + await assert.rejects(fs.promises.rmdir( filePath, { recursive: true } ), { From 7fb46132276ddac5be3fd159fbee8722822ee1e1 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Wed, 23 Sep 2020 14:43:38 -0600 Subject: [PATCH 5/6] Make sure stats exists before checking it --- lib/internal/fs/rimraf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index 14a8b22ebbd7ee..aec4b4feb9cf66 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -201,7 +201,7 @@ function rimrafSync(path, options) { throw err; } - if (!stats.isDirectory()) { + if (stats && !stats.isDirectory()) { throw new ERR_INVALID_ARG_VALUE('path', path, 'is not a directory'); } From 523ec9b3996a2eb5e543b51e5052482b67f68b73 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Wed, 23 Sep 2020 16:26:41 -0600 Subject: [PATCH 6/6] Replace invalid argument error with custom error when target is not a directory --- doc/api/errors.md | 5 ++++ lib/internal/errors.js | 1 + lib/internal/fs/rimraf.js | 20 +++++++++---- test/parallel/test-fs-rmdir-recursive.js | 37 ++++++++++++++++++------ 4 files changed, 49 insertions(+), 14 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index eace3deaec76d2..1cf701f5e62e78 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -928,6 +928,11 @@ added: v14.0.0 Used when a feature that is not available to the current platform which is running Node.js is used. + +### `ERR_FS_ENOTDIR` + +Path is not a directory. + ### `ERR_FS_FILE_TOO_LARGE` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 787298288b6d90..95482e12f3c8b4 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -838,6 +838,7 @@ E('ERR_FEATURE_UNAVAILABLE_ON_PLATFORM', 'The feature %s is unavailable on the current platform' + ', which is being used to run Node.js', TypeError); +E('ERR_FS_ENOTDIR', 'not a directory', SystemError); E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than 2 GB', RangeError); E('ERR_FS_INVALID_SYMLINK_TYPE', 'Symlink type must be one of "dir", "file", or "junction". Received "%s"', diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index aec4b4feb9cf66..35ff1b3977cb82 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -31,7 +31,7 @@ const { setTimeout } = require('timers'); const { sleep } = require('internal/util'); const { codes: { - ERR_INVALID_ARG_VALUE + ERR_FS_ENOTDIR }, } = require('internal/errors'); const notEmptyErrorCodes = new Set(['ENOTEMPTY', 'EEXIST', 'EPERM']); @@ -49,9 +49,13 @@ function rimraf(path, options, callback) { if (err && err.code === 'ENOENT') { callback(err); } else if (stats && !stats.isDirectory()) { - callback(new ERR_INVALID_ARG_VALUE( - 'path', path, 'is not a directory' - )); + callback(new ERR_FS_ENOTDIR({ + code: 'ENOTDIR', + message: 'not a directory', + path, + syscall: 'rmdir', + errno: -20 + })); } else { _rimraf(path, options, callback); } @@ -202,7 +206,13 @@ function rimrafSync(path, options) { } if (stats && !stats.isDirectory()) { - throw new ERR_INVALID_ARG_VALUE('path', path, 'is not a directory'); + throw new ERR_FS_ENOTDIR({ + code: 'ENOTDIR', + message: 'not a directory', + path, + syscall: 'rmdir', + errno: -20 + }); } _rimrafSync(path, options); diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index a3536df1cf0e34..9addb45baadd9f 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -115,9 +115,14 @@ function removeAsync(dir) { fs.writeFileSync(filePath, ''); fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => { try { - assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); - assert.strictEqual(err.name, 'TypeError'); - assert.match(err.message, /^The argument 'path' is not a directory\./); + assert.strictEqual(err.code, 'ERR_FS_ENOTDIR'); + assert.strictEqual(err.name, 'SystemError'); + assert.match(err.message, /^not a directory/); + assert.strictEqual(err.info.code, 'ENOTDIR'); + assert.strictEqual(err.info.message, 'not a directory'); + assert.strictEqual(err.info.path, filePath); + assert.strictEqual(err.info.syscall, 'rmdir'); + assert.strictEqual(err.info.errno, -20); } finally { fs.unlinkSync(filePath); } @@ -154,9 +159,16 @@ function removeAsync(dir) { assert.throws(() => { fs.rmdirSync(filePath, { recursive: true }); }, { - code: 'ERR_INVALID_ARG_VALUE', - name: 'TypeError', - message: /^The argument 'path' is not a directory\./ + code: 'ERR_FS_ENOTDIR', + name: 'SystemError', + message: /^not a directory/, + info: { + code: 'ENOTDIR', + message: 'not a directory', + path: filePath, + syscall: 'rmdir', + errno: -20 + } }); } finally { fs.unlinkSync(filePath); @@ -203,9 +215,16 @@ function removeAsync(dir) { filePath, { recursive: true } ), { - code: 'ERR_INVALID_ARG_VALUE', - name: 'TypeError', - message: /^The argument 'path' is not a directory\./ + code: 'ERR_FS_ENOTDIR', + name: 'SystemError', + message: /^not a directory/, + info: { + code: 'ENOTDIR', + message: 'not a directory', + path: filePath, + syscall: 'rmdir', + errno: -20 + } }); } finally { fs.unlinkSync(filePath);