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 <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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‎

Lines changed: 2 additions & 2 deletions
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
returncb(null,options);
@@ -800,7 +800,7 @@ const validateRmOptionsSync = hideStackFrames((path, options, expectDir) =>{
800800

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

805805
if(expectDir&&!isDirectory){
806806
returnfalse;

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

Lines changed: 134 additions & 5 deletions
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+
constlinkTarget=path.join(tmpdir.path,'link-target-async.txt');
152+
fs.writeFileSync(linkTarget,'');
153+
constvalidLink=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+
constinvalidLink=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+
constloopLinkA=path.join(tmpdir.path,'loop-link-async-a');
179+
constloopLinkB=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:nosuchfileordirectory,stat/
222+
message: /^ENOENT:nosuchfileordirectory,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+
constlinkTarget=path.join(tmpdir.path,'link-target.txt');
238+
fs.writeFileSync(linkTarget,'');
239+
constvalidLink=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+
constinvalidLink=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+
constloopLinkA=path.join(tmpdir.path,'loop-link-a');
261+
constloopLinkB=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
constfileURL=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
awaitfs.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-
awaitassert.rejects(fs.promises.rm(dir),{syscall: 'stat'});
316+
awaitassert.rejects(fs.promises.rm(dir),{syscall: 'lstat'});
226317

227318
// Should fail if target does not exist
228319
awaitassert.rejects(fs.promises.rm(
@@ -231,7 +322,7 @@ if (isGitPresent){
231322
),{
232323
code: 'ENOENT',
233324
name: 'Error',
234-
message: /^ENOENT:nosuchfileordirectory,stat/
325+
message: /^ENOENT:nosuchfileordirectory,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
awaitfs.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+
constlinkTarget=path.join(tmpdir.path,'link-target-prom.txt');
344+
fs.writeFileSync(linkTarget,'');
345+
constvalidLink=path.join(tmpdir.path,'valid-link-prom');
346+
fs.symlinkSync(linkTarget,validLink);
347+
try{
348+
awaitfs.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+
constinvalidLink=path.join(tmpdir.path,'invalid-link-prom');
357+
fs.symlinkSync('definitely-does-not-exist-prom',invalidLink);
358+
try{
359+
awaitfs.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+
constloopLinkA=path.join(tmpdir.path,'loop-link-prom-a');
367+
constloopLinkB=path.join(tmpdir.path,'loop-link-prom-b');
368+
fs.symlinkSync(loopLinkA,loopLinkB);
369+
fs.symlinkSync(loopLinkB,loopLinkA);
370+
try{
371+
awaitfs.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
constfileURL=pathToFileURL(path.join(tmpdir.path,'rm-promises-file.txt'));
252380
fs.writeFileSync(fileURL,'');
253381

254382
try{
255383
awaitfs.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)