From d7c0823d1a5bd9253475dabdd130767bbd2569dc Mon Sep 17 00:00:00 2001 From: Nathanael Ruf <104262550+nathanael-ruf@users.noreply.github.com> Date: Fri, 18 Nov 2022 04:59:45 +0100 Subject: [PATCH] fs: fix fs.rm support for loop symlinks Fixes: https://github.com/nodejs/node/issues/45404 PR-URL: https://github.com/nodejs/node/pull/45439 Reviewed-By: Antoine du Hamel Reviewed-By: LiviaMedeiros Reviewed-By: Minwoo Jung --- lib/internal/fs/utils.js | 4 +- test/parallel/test-fs-rm.js | 139 ++++++++++++++++++++++++++++++++++-- 2 files changed, 136 insertions(+), 7 deletions(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 9ff9d46b1bc75b..25fd1cc28a8ce9 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -770,7 +770,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); @@ -801,7 +801,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..e6bc47038b8d92 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 @@ -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. @@ -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( @@ -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 @@ -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 })); }