Skip to content

Make recursive rmdir more strict #35250

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.

Sign up for GitHub

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

Closed
iansu wants to merge 6 commits into from
Closed

Make recursive rmdir more strict #35250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
02cee53
fs: throw in rmdir recursive if path does not exist or is not a direc…
iansu Sep 13, 2020
43c785e
Fix tests
iansu Sep 18, 2020
e26dbba
Improve test cleanup
iansu Sep 18, 2020
0eb9255
Fix tests
iansu Sep 23, 2020
7fb4613
Make sure stats exists before checking it
iansu Sep 23, 2020
523ec9b
Replace invalid argument error with custom error when target is not a…
iansu Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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`.
Expand Down
48 changes: 43 additions & 5 deletions lib/internal/fs/rimraf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -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.
Expand All @@ -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) => {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -174,6 +194,24 @@ function rimrafPromises(path, options) {
function rimrafSync(path, options) {
let stats;

try {
stats = statSync(path);
} catch (err) {
if (err.code === 'ENOENT')
Copy link
Member

@boneskull boneskull Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is swallowed here if not ENOENT?

Copy link
Contributor Author

@iansu iansu Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rimraf swallows all kinds of errors and throwing all errors from stat caused lots of failures.

Copy link
Member

@boneskull boneskull Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you'd get into trouble here if L198 threw, because stats would then be undefined, and the call to isDirectory() on L204 would fail... it doesn't seem recoverable?

throw err;
}

if (!stats.isDirectory()) {
throw new ERR_INVALID_ARG_VALUE('path', path, 'is not a directory');
Copy link
Member

@boneskull boneskull Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the correct error to throw? unsure of the intent of ERR_INVALID_ARG_VALUE

Copy link
Contributor Author

@iansu iansu Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that it is. I wanted to throw an ENOTDIR error but I'm not sure how to do that. I don't see it happening anywhere else in the code. I tried making a regular Error and setting the code to ENOTDIR but the linter didn't like that.

Copy link
Member

@boneskull boneskull Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be a convenience function for this error; maybe we should create one.

Copy link
Contributor

@bcoe bcoe Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iansu in internal/errors.js, I think we want to add:

E('ENOTDIR',
  'no such file or directory',
  SystemError);

You'd then instantiate it something like this:

new SystemError('ENOTDIR', {syscall: 'rmdir', code: 'ENOTDIR', path: './', message: 'whatever error message rimraf throws'', errno: whatever error code rmdir has})

Basically the ideal is that someone would be able to look at err.code both in the recursive and non recursive rmdir, and use the same logic.

Edit: I mean ENOTDIR.

Copy link
Contributor

@bcoe bcoe Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, sorry I mean ENOTDIR, but I think it should be similar logic.

}

_rimrafSync(path, options);
}


function _rimrafSync(path, options) {
let stats;

try {
stats = lstatSync(path);
} catch (err) {
Expand Down Expand Up @@ -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;
Expand Down
80 changes: 67 additions & 13 deletions test/parallel/test-fs-rmdir-recursive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}));
}));
}));
Expand All @@ -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);
Copy link
Member

@boneskull boneskull Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this should be guaranteed to be called--not sure if it is. also: don't know offhand if other tests try to guarantee this or just leave crap all over the fs

Copy link
Contributor Author

@iansu iansu Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use a try/finally to clean this up.

}

// Test the synchronous version.
Expand All @@ -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.
Expand All @@ -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' });
Expand Down