Skip to content

Commit

Permalink
fs: fix fs.rm support for loop symlinks
Browse files Browse the repository at this point in the history 
Fixes: nodejs#45404
PR-URL: nodejs#45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
  • Loading branch information
@nathanael-ruf @marco-ippolito
nathanael-ruf authored and marco-ippolito committed Nov 23, 2022
1 parent 4178121 commit 5c7503f
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 7 deletions.
4 changes: 2 additions & 2 deletions lib/internal/fs/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ const validateRmOptions = hideStackFrames((path, options, expectDir, cb) => {
options = validateRmdirOptions(options, defaultRmOptions);
validateBoolean(options.force, 'options.force');

lazyLoadFs().stat(path, (err, stats) => {
lazyLoadFs().lstat(path, (err, stats) => {
if (err) {
if (options.force && err.code === 'ENOENT') {
return cb(null, options);
Expand Down Expand Up @@ -800,7 +800,7 @@ const validateRmOptionsSync = hideStackFrames((path, options, expectDir) => {

if (!options.force || expectDir || !options.recursive) {
const isDirectory = lazyLoadFs()
.statSync(path, { throwIfNoEntry: !options.force })?.isDirectory();
.lstatSync(path, { throwIfNoEntry: !options.force })?.isDirectory();

if (expectDir && !isDirectory) {
return false;
Expand Down
139 changes: 134 additions & 5 deletions test/parallel/test-fs-rm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ function makeNonEmptyDirectory(depth, files, folders, dirname, createSymLinks) {
path.join(dirname, `link-${depth}-bad`),
'file'
);

// Symlinks that form a loop
[['a', 'b'], ['b', 'a']].forEach(([x, y]) => {
fs.symlinkSync(
`link-${depth}-loop-${x}`,
path.join(dirname, `link-${depth}-loop-${y}`),
'file'
);
});
}

// File with a name that looks like a glob
Expand Down Expand Up @@ -88,7 +97,7 @@ function removeAsync(dir) {

// Attempted removal should fail now because the directory is gone.
fs.rm(dir, common.mustCall((err) => {
assert.strictEqual(err.syscall, 'stat');
assert.strictEqual(err.syscall, 'lstat');
}));
}));
}));
Expand Down Expand Up @@ -137,6 +146,48 @@ function removeAsync(dir) {
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
}
}));

// Should delete a valid symlink
const linkTarget = path.join(tmpdir.path, 'link-target-async.txt');
fs.writeFileSync(linkTarget, '');
const validLink = path.join(tmpdir.path, 'valid-link-async');
fs.symlinkSync(linkTarget, validLink);
fs.rm(validLink, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
try {
assert.strictEqual(err, null);
assert.strictEqual(fs.existsSync(validLink), false);
} finally {
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
}
}));

// Should delete an invalid symlink
const invalidLink = path.join(tmpdir.path, 'invalid-link-async');
fs.symlinkSync('definitely-does-not-exist-async', invalidLink);
fs.rm(invalidLink, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
try {
assert.strictEqual(err, null);
assert.strictEqual(fs.existsSync(invalidLink), false);
} finally {
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
}
}));

// Should delete a symlink that is part of a loop
const loopLinkA = path.join(tmpdir.path, 'loop-link-async-a');
const loopLinkB = path.join(tmpdir.path, 'loop-link-async-b');
fs.symlinkSync(loopLinkA, loopLinkB);
fs.symlinkSync(loopLinkB, loopLinkA);
fs.rm(loopLinkA, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
try {
assert.strictEqual(err, null);
assert.strictEqual(fs.existsSync(loopLinkA), false);
} finally {
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
}
}));
}

// Removing a .git directory should not throw an EPERM.
Expand Down Expand Up @@ -168,7 +219,7 @@ if (isGitPresent) {
}, {
code: 'ENOENT',
name: 'Error',
message: /^ENOENT: no such file or directory, stat/
message: /^ENOENT: no such file or directory, lstat/
});

// Should delete a file
Expand All @@ -177,25 +228,64 @@ if (isGitPresent) {

try {
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ recursive: true }));
assert.strictEqual(fs.existsSync(filePath), false);
} finally {
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete a valid symlink
const linkTarget = path.join(tmpdir.path, 'link-target.txt');
fs.writeFileSync(linkTarget, '');
const validLink = path.join(tmpdir.path, 'valid-link');
fs.symlinkSync(linkTarget, validLink);
try {
fs.rmSync(validLink);
assert.strictEqual(fs.existsSync(validLink), false);
} finally {
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete an invalid symlink
const invalidLink = path.join(tmpdir.path, 'invalid-link');
fs.symlinkSync('definitely-does-not-exist', invalidLink);
try {
fs.rmSync(invalidLink);
assert.strictEqual(fs.existsSync(invalidLink), false);
} finally {
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete a symlink that is part of a loop
const loopLinkA = path.join(tmpdir.path, 'loop-link-a');
const loopLinkB = path.join(tmpdir.path, 'loop-link-b');
fs.symlinkSync(loopLinkA, loopLinkB);
fs.symlinkSync(loopLinkB, loopLinkA);
try {
fs.rmSync(loopLinkA);
assert.strictEqual(fs.existsSync(loopLinkA), false);
} finally {
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
}

// Should accept URL
const fileURL = pathToFileURL(path.join(tmpdir.path, 'rm-file.txt'));
fs.writeFileSync(fileURL, '');

try {
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ recursive: true }));
assert.strictEqual(fs.existsSync(fileURL), false);
} finally {
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ force: true }));
}

// Recursive removal should succeed.
fs.rmSync(dir, { recursive: true });
assert.strictEqual(fs.existsSync(dir), false);

// Attempted removal should fail now because the directory is gone.
assert.throws(() => fs.rmSync(dir), { syscall: 'stat' });
assert.throws(() => fs.rmSync(dir), { syscall: 'lstat' });
}

// Removing a .git directory should not throw an EPERM.
Expand All @@ -220,9 +310,10 @@ if (isGitPresent) {

// Recursive removal should succeed.
await fs.promises.rm(dir, common.mustNotMutateObjectDeep({ recursive: true }));
assert.strictEqual(fs.existsSync(dir), false);

// Attempted removal should fail now because the directory is gone.
await assert.rejects(fs.promises.rm(dir), { syscall: 'stat' });
await assert.rejects(fs.promises.rm(dir), { syscall: 'lstat' });

// Should fail if target does not exist
await assert.rejects(fs.promises.rm(
Expand All @@ -231,7 +322,7 @@ if (isGitPresent) {
), {
code: 'ENOENT',
name: 'Error',
message: /^ENOENT: no such file or directory, stat/
message: /^ENOENT: no such file or directory, lstat/
});

// Should not fail if target does not exist and force option is true
Expand All @@ -243,16 +334,54 @@ if (isGitPresent) {

try {
await fs.promises.rm(filePath, common.mustNotMutateObjectDeep({ recursive: true }));
assert.strictEqual(fs.existsSync(filePath), false);
} finally {
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete a valid symlink
const linkTarget = path.join(tmpdir.path, 'link-target-prom.txt');
fs.writeFileSync(linkTarget, '');
const validLink = path.join(tmpdir.path, 'valid-link-prom');
fs.symlinkSync(linkTarget, validLink);
try {
await fs.promises.rm(validLink);
assert.strictEqual(fs.existsSync(validLink), false);
} finally {
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete an invalid symlink
const invalidLink = path.join(tmpdir.path, 'invalid-link-prom');
fs.symlinkSync('definitely-does-not-exist-prom', invalidLink);
try {
await fs.promises.rm(invalidLink);
assert.strictEqual(fs.existsSync(invalidLink), false);
} finally {
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
}

// Should delete a symlink that is part of a loop
const loopLinkA = path.join(tmpdir.path, 'loop-link-prom-a');
const loopLinkB = path.join(tmpdir.path, 'loop-link-prom-b');
fs.symlinkSync(loopLinkA, loopLinkB);
fs.symlinkSync(loopLinkB, loopLinkA);
try {
await fs.promises.rm(loopLinkA);
assert.strictEqual(fs.existsSync(loopLinkA), false);
} finally {
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
}

// Should accept URL
const fileURL = pathToFileURL(path.join(tmpdir.path, 'rm-promises-file.txt'));
fs.writeFileSync(fileURL, '');

try {
await fs.promises.rm(fileURL, common.mustNotMutateObjectDeep({ recursive: true }));
assert.strictEqual(fs.existsSync(fileURL), false);
} finally {
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ force: true }));
}
Expand Down

0 comments on commit 5c7503f

Please sign in to comment.