From 8455a62f65b3ced0a4f5403dbf7c0da09cde15ed Mon Sep 17 00:00:00 2001 From: Nathanael Ruf Date: Sat, 12 Nov 2022 20:36:53 +0100 Subject: [PATCH 1/4] fs: fix fs.rm support for loop symlinks Fixes: https://github.com/nodejs/node/issues/45404 --- lib/internal/fs/utils.js | 4 +- test/parallel/test-fs-rm.js | 127 ++++++++++++++++++++++++++++++++++-- 2 files changed, 124 insertions(+), 7 deletions(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 1e2bdbcd8823a6..a3ef20b708a030 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -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); @@ -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; diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 6723d2b1cabd85..218386616d83db 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -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 @@ -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'); })); })); })); @@ -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. @@ -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 @@ -181,6 +232,39 @@ if (isGitPresent) { 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); + } 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); + } 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); + } 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, ''); @@ -195,7 +279,7 @@ if (isGitPresent) { fs.rmSync(dir, { recursive: true }); // 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. @@ -222,7 +306,7 @@ if (isGitPresent) { await fs.promises.rm(dir, common.mustNotMutateObjectDeep({ recursive: true })); // 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( @@ -231,7 +315,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 @@ -247,6 +331,39 @@ if (isGitPresent) { 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); + } 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); + } 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); + } 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, ''); From 11b190d9016347922af7d323b841a2e016251f90 Mon Sep 17 00:00:00 2001 From: Nathanael Ruf Date: Mon, 14 Nov 2022 07:30:36 +0100 Subject: [PATCH 2/4] fs: ass missing assertions to test-fs-rm tests --- test/parallel/test-fs-rm.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 218386616d83db..77082c423695fa 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -228,6 +228,7 @@ if (isGitPresent) { try { fs.rmSync(filePath, common.mustNotMutateObjectDeep({ recursive: true })); + assert.strictEqual(fs.existsSync(filePath), false); } finally { fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true })); } @@ -239,6 +240,7 @@ if (isGitPresent) { 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 })); @@ -249,6 +251,7 @@ if (isGitPresent) { 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 })); } @@ -261,6 +264,8 @@ if (isGitPresent) { try { fs.rmSync(loopLinkA); } finally { + assert.strictEqual(fs.existsSync(loopLinkA), false); + assert.strictEqual(fs.existsSync(loopLinkB), true); fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true })); fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true })); } @@ -271,12 +276,14 @@ if (isGitPresent) { 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: 'lstat' }); @@ -304,6 +311,7 @@ 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: 'lstat' }); @@ -327,6 +335,7 @@ 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 })); } @@ -338,6 +347,7 @@ if (isGitPresent) { 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 })); @@ -348,6 +358,7 @@ if (isGitPresent) { 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 })); } @@ -359,6 +370,8 @@ if (isGitPresent) { fs.symlinkSync(loopLinkB, loopLinkA); try { await fs.promises.rm(loopLinkA); + assert.strictEqual(fs.existsSync(loopLinkA), false); + assert.strictEqual(fs.existsSync(loopLinkB), true); } finally { fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true })); fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true })); @@ -370,6 +383,7 @@ if (isGitPresent) { try { await fs.promises.rm(fileURL, common.mustNotMutateObjectDeep({ recursive: true })); + assert.strictEqual(fs.existsSync(fileURL), false); } finally { fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ force: true })); } From bc3ec4a9c1e0aaba6fc49578368bb6b0e94cd5a8 Mon Sep 17 00:00:00 2001 From: Nathanael Ruf Date: Mon, 14 Nov 2022 07:38:22 +0100 Subject: [PATCH 3/4] fs: remove exists check for remaining loop symlinks --- test/parallel/test-fs-rm.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 77082c423695fa..4186637d6cced1 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -265,7 +265,6 @@ if (isGitPresent) { fs.rmSync(loopLinkA); } finally { assert.strictEqual(fs.existsSync(loopLinkA), false); - assert.strictEqual(fs.existsSync(loopLinkB), true); fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true })); fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true })); } @@ -371,7 +370,6 @@ if (isGitPresent) { try { await fs.promises.rm(loopLinkA); assert.strictEqual(fs.existsSync(loopLinkA), false); - assert.strictEqual(fs.existsSync(loopLinkB), true); } finally { fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true })); fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true })); From 6992b221651d67b14c6cb2120fddc6cc14d8b357 Mon Sep 17 00:00:00 2001 From: Nathanael Ruf <104262550+nathanael-ruf@users.noreply.github.com> Date: Mon, 14 Nov 2022 09:33:43 +0100 Subject: [PATCH 4/4] fs: move assertions of sync loop test into try block Co-authored-by: Livia Medeiros --- test/parallel/test-fs-rm.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 4186637d6cced1..e6bc47038b8d92 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -263,8 +263,8 @@ if (isGitPresent) { fs.symlinkSync(loopLinkB, loopLinkA); try { fs.rmSync(loopLinkA); - } finally { assert.strictEqual(fs.existsSync(loopLinkA), false); + } finally { fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true })); fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true })); }