Skip to content

Commit 2b760c3

Browse files
nathanael-rufruyadorno
nathanael-ruf
authored and
ruyadorno
committed
fs: fix fs.rm support for loop symlinks
Fixes: #45404 PR-URL: #45439 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
1 parent 208ea1a commit 2b760c3

File tree

2 files changed

+136
-7
lines changed

2 files changed

+136
-7
lines changed

‎lib/internal/fs/utils.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -769,7 +769,7 @@ const validateRmOptions = hideStackFrames((path, options, expectDir, cb) => {
769769
options = validateRmdirOptions(options, defaultRmOptions);
770770
validateBoolean(options.force, 'options.force');
771771

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

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

805805
if (expectDir && !isDirectory) {
806806
return false;

‎test/parallel/test-fs-rm.js

+134-5
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,15 @@ function makeNonEmptyDirectory(depth, files, folders, dirname, createSymLinks) {
4949
path.join(dirname, `link-${depth}-bad`),
5050
'file'
5151
);
52+
53+
// Symlinks that form a loop
54+
[['a', 'b'], ['b', 'a']].forEach(([x, y]) => {
55+
fs.symlinkSync(
56+
`link-${depth}-loop-${x}`,
57+
path.join(dirname, `link-${depth}-loop-${y}`),
58+
'file'
59+
);
60+
});
5261
}
5362

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

8998
// Attempted removal should fail now because the directory is gone.
9099
fs.rm(dir, common.mustCall((err) => {
91-
assert.strictEqual(err.syscall, 'stat');
100+
assert.strictEqual(err.syscall, 'lstat');
92101
}));
93102
}));
94103
}));
@@ -137,6 +146,48 @@ function removeAsync(dir) {
137146
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
138147
}
139148
}));
149+
150+
// Should delete a valid symlink
151+
const linkTarget = path.join(tmpdir.path, 'link-target-async.txt');
152+
fs.writeFileSync(linkTarget, '');
153+
const validLink = path.join(tmpdir.path, 'valid-link-async');
154+
fs.symlinkSync(linkTarget, validLink);
155+
fs.rm(validLink, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
156+
try {
157+
assert.strictEqual(err, null);
158+
assert.strictEqual(fs.existsSync(validLink), false);
159+
} finally {
160+
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
161+
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
162+
}
163+
}));
164+
165+
// Should delete an invalid symlink
166+
const invalidLink = path.join(tmpdir.path, 'invalid-link-async');
167+
fs.symlinkSync('definitely-does-not-exist-async', invalidLink);
168+
fs.rm(invalidLink, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
169+
try {
170+
assert.strictEqual(err, null);
171+
assert.strictEqual(fs.existsSync(invalidLink), false);
172+
} finally {
173+
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
174+
}
175+
}));
176+
177+
// Should delete a symlink that is part of a loop
178+
const loopLinkA = path.join(tmpdir.path, 'loop-link-async-a');
179+
const loopLinkB = path.join(tmpdir.path, 'loop-link-async-b');
180+
fs.symlinkSync(loopLinkA, loopLinkB);
181+
fs.symlinkSync(loopLinkB, loopLinkA);
182+
fs.rm(loopLinkA, common.mustNotMutateObjectDeep({ recursive: true }), common.mustCall((err) => {
183+
try {
184+
assert.strictEqual(err, null);
185+
assert.strictEqual(fs.existsSync(loopLinkA), false);
186+
} finally {
187+
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
188+
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
189+
}
190+
}));
140191
}
141192

142193
// Removing a .git directory should not throw an EPERM.
@@ -168,7 +219,7 @@ if (isGitPresent) {
168219
}, {
169220
code: 'ENOENT',
170221
name: 'Error',
171-
message: /^ENOENT: no such file or directory, stat/
222+
message: /^ENOENT: no such file or directory, lstat/
172223
});
173224

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

178229
try {
179230
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ recursive: true }));
231+
assert.strictEqual(fs.existsSync(filePath), false);
180232
} finally {
181233
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
182234
}
183235

236+
// Should delete a valid symlink
237+
const linkTarget = path.join(tmpdir.path, 'link-target.txt');
238+
fs.writeFileSync(linkTarget, '');
239+
const validLink = path.join(tmpdir.path, 'valid-link');
240+
fs.symlinkSync(linkTarget, validLink);
241+
try {
242+
fs.rmSync(validLink);
243+
assert.strictEqual(fs.existsSync(validLink), false);
244+
} finally {
245+
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
246+
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
247+
}
248+
249+
// Should delete an invalid symlink
250+
const invalidLink = path.join(tmpdir.path, 'invalid-link');
251+
fs.symlinkSync('definitely-does-not-exist', invalidLink);
252+
try {
253+
fs.rmSync(invalidLink);
254+
assert.strictEqual(fs.existsSync(invalidLink), false);
255+
} finally {
256+
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
257+
}
258+
259+
// Should delete a symlink that is part of a loop
260+
const loopLinkA = path.join(tmpdir.path, 'loop-link-a');
261+
const loopLinkB = path.join(tmpdir.path, 'loop-link-b');
262+
fs.symlinkSync(loopLinkA, loopLinkB);
263+
fs.symlinkSync(loopLinkB, loopLinkA);
264+
try {
265+
fs.rmSync(loopLinkA);
266+
assert.strictEqual(fs.existsSync(loopLinkA), false);
267+
} finally {
268+
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
269+
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
270+
}
271+
184272
// Should accept URL
185273
const fileURL = pathToFileURL(path.join(tmpdir.path, 'rm-file.txt'));
186274
fs.writeFileSync(fileURL, '');
187275

188276
try {
189277
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ recursive: true }));
278+
assert.strictEqual(fs.existsSync(fileURL), false);
190279
} finally {
191280
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ force: true }));
192281
}
193282

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

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

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

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

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

227318
// Should fail if target does not exist
228319
await assert.rejects(fs.promises.rm(
@@ -231,7 +322,7 @@ if (isGitPresent) {
231322
), {
232323
code: 'ENOENT',
233324
name: 'Error',
234-
message: /^ENOENT: no such file or directory, stat/
325+
message: /^ENOENT: no such file or directory, lstat/
235326
});
236327

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

244335
try {
245336
await fs.promises.rm(filePath, common.mustNotMutateObjectDeep({ recursive: true }));
337+
assert.strictEqual(fs.existsSync(filePath), false);
246338
} finally {
247339
fs.rmSync(filePath, common.mustNotMutateObjectDeep({ force: true }));
248340
}
249341

342+
// Should delete a valid symlink
343+
const linkTarget = path.join(tmpdir.path, 'link-target-prom.txt');
344+
fs.writeFileSync(linkTarget, '');
345+
const validLink = path.join(tmpdir.path, 'valid-link-prom');
346+
fs.symlinkSync(linkTarget, validLink);
347+
try {
348+
await fs.promises.rm(validLink);
349+
assert.strictEqual(fs.existsSync(validLink), false);
350+
} finally {
351+
fs.rmSync(linkTarget, common.mustNotMutateObjectDeep({ force: true }));
352+
fs.rmSync(validLink, common.mustNotMutateObjectDeep({ force: true }));
353+
}
354+
355+
// Should delete an invalid symlink
356+
const invalidLink = path.join(tmpdir.path, 'invalid-link-prom');
357+
fs.symlinkSync('definitely-does-not-exist-prom', invalidLink);
358+
try {
359+
await fs.promises.rm(invalidLink);
360+
assert.strictEqual(fs.existsSync(invalidLink), false);
361+
} finally {
362+
fs.rmSync(invalidLink, common.mustNotMutateObjectDeep({ force: true }));
363+
}
364+
365+
// Should delete a symlink that is part of a loop
366+
const loopLinkA = path.join(tmpdir.path, 'loop-link-prom-a');
367+
const loopLinkB = path.join(tmpdir.path, 'loop-link-prom-b');
368+
fs.symlinkSync(loopLinkA, loopLinkB);
369+
fs.symlinkSync(loopLinkB, loopLinkA);
370+
try {
371+
await fs.promises.rm(loopLinkA);
372+
assert.strictEqual(fs.existsSync(loopLinkA), false);
373+
} finally {
374+
fs.rmSync(loopLinkA, common.mustNotMutateObjectDeep({ force: true }));
375+
fs.rmSync(loopLinkB, common.mustNotMutateObjectDeep({ force: true }));
376+
}
377+
250378
// Should accept URL
251379
const fileURL = pathToFileURL(path.join(tmpdir.path, 'rm-promises-file.txt'));
252380
fs.writeFileSync(fileURL, '');
253381

254382
try {
255383
await fs.promises.rm(fileURL, common.mustNotMutateObjectDeep({ recursive: true }));
384+
assert.strictEqual(fs.existsSync(fileURL), false);
256385
} finally {
257386
fs.rmSync(fileURL, common.mustNotMutateObjectDeep({ force: true }));
258387
}

0 commit comments

Comments
 (0)