fs: normalize trailing dot segments for rm - #65883
Open
AmarWaqar-TSKLI wants to merge 1 commit into
Open
Conversation
fs.rmSync() dispatches to std::filesystem::remove_all() while fs.rm() and fsPromises.rm() walk the tree in JavaScript, so the two forms disagree whenever the trailing path component is `.` or `..`. rmSync() removes the contents of the resolved target but leaves the target itself behind, and for a trailing `.` it throws an error carrying no code property. The promise form rejects with EINVAL for a trailing `.`, and for a trailing `..` it reports success while removing a directory below the one that was requested. The silent case happens because _rmchildren() builds child paths by concatenating onto the unresolved path. The walk removes a directory that an unresolved `..` still needs in order to resolve, so every later operation fails with ENOENT, which rimraf() treats as already deleted. Resolve a trailing dot segment at the three entry points so both forms operate on the same path. Only a trailing `.` or `..` is rewritten, since that is where the two implementations diverge; every other path is passed through unchanged, so paths that already behaved correctly keep their existing behaviour, including the resource string the permission model reports for them. Buffer paths go through latin1 rather than utf8 because filenames are arbitrary byte sequences, and a utf8 round trip rewrites invalid sequences to U+FFFD, which would remove a different path than the one requested. Fixes: nodejs#61958 Signed-off-by: AmarWaqar-TSKLI <amarwaqar15@gmail.com>
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65883 +/- ##
==========================================
- Coverage 90.18% 90.17% -0.01%
==========================================
Files 771 771
Lines 264619 265126 +507
Branches 50231 50357 +126
==========================================
+ Hits 238653 239085 +432
- Misses 16961 17005 +44
- Partials 9005 9036 +31
🚀 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
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.
fs.rmSync()dispatches tobinding.rmSync()(std::filesystem::remove_all) whilefs.rm()andfsPromises.rm()use the JSrimraf, so the two forms disagree whenever thetrailing path component is
.or... Fixture isa/b/c/d, options are{ recursive: true, force: true }:fs.rmSyncbeforefsPromises.rmbeforea/b/../.aaa/b/..aaaa/.acodeis""a/b/c/.a/b/ccodeis""a/b/c/../..aaa a/ba/b/c/d/../../..aaa a/b a/b/cThe last two rows are the ones worth attention: both forms report success and remove
different directories.
Why the async form fails silently
_rmchildren()builds child paths by concatenating onto the unresolved path. Tracing thefscalls forrm('<root>/a/b/c/../..'):The walk removes
a/b/c, which the literal path needs in order to resolve. Every later callthen fails
ENOENT, andrimraf()treatsENOENTas "already gone", so the failure isreported as success.
The change
Resolve a trailing dot segment at the three
rmentry points so both forms operate on thesame path.
Only a trailing
.or..is rewritten. That is exactly where the two implementationsdiverge, and keeping every other path byte for byte identical matters: normalizing
unconditionally also rewrote
./footofoo, which changed theresourcestring thepermission model reports on denial and broke
test-permission-fs-write. Narrowing to thetrailing component leaves paths that already behaved correctly completely untouched.
This is not a new behaviour so much as making the three documented input types agree:
URLpaths are already correct in every row above, because the WHATWG parser resolves dotsegments before the path reaches
fs.stringandBufferpaths were not.Bufferpaths are round tripped throughlatin1rather thanutf8, because filenames arearbitrary byte sequences and a
utf8round trip rewrites invalid sequences to U+FFFD, whichwould remove a different path than the caller asked for:
Test
test/parallel/test-fs-rm-dot-segments.jscovers nine path shapes acrossstring,Bufferand
URLinput, plus a directory whose name is not valid UTF-8. Each case asserts both thatthe two forms agree and that the surviving tree is the one POSIX path resolution implies, so
the test still fails if both forms are wrong in the same way. It was written before the fix
and fails on current
main.Locally: the new test passes on this branch and fails on v26.7.0; all 280
parallel/test-fs-*tests pass; allparallel/test-permission-*tests pass excepttest-permission-drop-ffi, which fails on a clean checkout too because the FFI fixturelibrary is not built.
make lint-jsis clean for the touched files.Notes
Supersedes #61968, which has been stale since February. That PR took the same general
approach and the discussion there informed this one, but the implementation here is fresh
rather than carried over, so I have not added a co-author trailer. Happy to add one if that
is preferred.
fs.rmSync()throwing an error with an emptycode(rows 3 and 4) is a separate defect inRmSync()insrc/node_file.cc, where thestd::error_codetranslation is a hardcoded listof four values and everything else becomes
UV_UNKNOWN. This change stopsa/.from reachingthat path but does not fix the mapping. Filed separately as #65884.
Disclosure: I used an AI coding assistant on this change. The behaviour matrix, the call
trace and the round trip check above are reproducible scripts run locally against v26.7.0 and
against a checkout of
main; I have read through the patch and the test and can speak toboth.