lib: normalize . and .. in path before rm/rmSync#61968
Open
RajeshKumar11 wants to merge 1 commit intonodejs:mainfrom
Open
lib: normalize . and .. in path before rm/rmSync#61968RajeshKumar11 wants to merge 1 commit intonodejs:mainfrom
RajeshKumar11 wants to merge 1 commit intonodejs:mainfrom
Conversation
When fs.rm, fs.rmSync, or fs.promises.rm receive a path containing '.' or '..' components (e.g. 'a/b/../.'), the sync and async implementations behaved differently: - rmSync (C++ binding): std::filesystem::remove_all deleted the directory's children, but when trying to remove the top-level entry the path became invalid (the '..' referred to a now-deleted directory), causing the parent directory to be left behind. - promises.rm / rm (JS rimraf): rmdir(2) on a path ending in '.' returns EINVAL on POSIX, so the operation failed immediately without removing anything. Fix by calling path.normalize() on string paths immediately after getValidatedPath(), before the path is passed to either rimraf or the C++ binding. This resolves '.' and '..' lexically (e.g. 'a/b/../.' becomes 'a'), giving both code paths a clean, unambiguous path to operate on. Fixes: nodejs#61958
cd665e4 to
887b2c8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61968 +/- ##
==========================================
+ Coverage 88.84% 89.76% +0.92%
==========================================
Files 674 674
Lines 204957 205609 +652
Branches 39309 39419 +110
==========================================
+ Hits 182087 184563 +2476
+ Misses 15088 13268 -1820
+ Partials 7782 7778 -4
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fs.rmSync,fs.rm, andfs.promises.rmnow callpath.normalize()on string paths before passing them to either the C++ binding (rmSync) or the JS rimraf implementation (rm/promises.rm).or..componentsRoot cause
Two different code paths handle
rmin Node.js:rmSync(C++ —src/node_file.cc:1661): callsstd::filesystem::remove_all(file_path).With
file_path = "a/b/../.", C++ deletes the contents ofa(by resolving the path through the kernel), but when it then tries tormdir("a/b/../.")to remove the top-level entry, thebcomponent was already deleted, so the path can't be resolved → ENOENT → treated as success →a/is left behind.rm/promises.rm(JS rimraf —lib/internal/fs/rimraf.js): callsrmdir(2)on the original path first.On POSIX,
rmdir("a/b/../.")returnsEINVAL(trailing.is an invalid argument for rmdir). EINVAL is not handled by rimraf's retry logic → the operation fails immediately without removing anything.Neither behaviour is correct, and they disagree with each other.
Fix
Apply
path.normalize()to string paths right aftergetValidatedPath()in all three entry points:lib/fs.jsrm()if (typeof path === 'string') path = pathModule.normalize(path)lib/fs.jsrmSync()lib/internal/fs/promises.jsrm()path.normalize('a/b/../.')→'a', which both rimraf and the C++ binding handle without issue. Buffer paths are left unchanged (they are an edge case andpath.normalizeonly accepts strings).Test
Three new test cases added to
test/parallel/test-fs-rm.js:fs.rmSyncwith a../.path removes the target directory completelyfs.rm(callback) with the same pathfs.promises.rmwith the same pathEach case creates
<base>/a/b/c/d, applies rm to<base>/a/b/../.(which should resolve to<base>/a), and asserts that<base>/ano longer exists.Related
Fixes: #61958