fs: fix rmSync error messages for non-ASCII paths#61233
fs: fix rmSync error messages for non-ASCII paths#61233Yeaseen wants to merge 3 commits intonodejs:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61233 +/- ##
==========================================
- Coverage 91.77% 89.73% -2.04%
==========================================
Files 338 675 +337
Lines 140064 204811 +64747
Branches 22045 39352 +17307
==========================================
+ Hits 128543 183789 +55246
- Misses 11298 13294 +1996
- Partials 223 7728 +7505
🚀 New features to boost your workflow:
|
a836c7f to
a68d666
Compare
|
8,5/10. Ce document est clair, techniquement précis et facile à comprendre. Sa structure est fluide et explique le problème et sa solution sans détails superflus. Pour atteindre 9 ou 10, il faudrait simplifier légèrement quelques Casino Frumzi phrases ou ajouter une courte phrase de conclusion soulignant l'impact pour les utilisateurs ou les responsables de la maintenance. |
|
Code LGTM, though I am a bit skeptical about the tests - it may not fare well on Windows in the CI or in people's local machines to change the permission this way. Can we use other safer error paths to check it instead? Perhaps the directory not empty one? |
a68d666 to
4bc9b95
Compare
|
@joyeecheung thanks for the feedback. I agree. I updated the test case for hitting |
4bc9b95 to
91a7aad
Compare
|
@joyeecheung looks good so far. The failing test on mac is unrelated to my changes. I appreciate a rerun. Thanks. |
|
@joyeecheung CI is green now — would appreciate your review. |
src/api/exceptions.cc
Outdated
There was a problem hiding this comment.
even though you moved this method, please use std::string_view. newfromutf8 takes additional parameters for the size of the input.
There was a problem hiding this comment.
@anonrig thanks for the suggestions. I've updated this function. let's see what CI says.
fs.rmSync previously embedded paths directly into custom error messages while also passing the path to ThrowErrnoException. This caused duplicated paths for ASCII names and corrupted paths for non-ASCII directory names on Linux, and inconsistent path formatting on Windows. Remove path concatenation from custom messages and rely on ThrowErrnoException to attach the path safely. Add a test to cover non-ASCII directory names.
91a7aad to
d1368b0
Compare
| return String::Concat(isolate, | ||
| FIXED_ONE_BYTE_STRING(isolate, "\\\\"), | ||
| to_v8(path.substr(kUncPrefix.size()))); |
There was a problem hiding this comment.
you don't need to call concat. you can just concat them on C++ by copying the string using std::string(valueToCopy) + appendedValue and later call String::NewFromUtf8
| using v8::Value; | ||
|
|
||
| static Local<String> StringFromPath(Isolate* isolate, std::string_view path) { | ||
| auto to_v8 = [&](std::string_view s) { |
There was a problem hiding this comment.
@anonrig one thing to confirm: do you want this to use ToV8Value instead of calling String::NewFromUtf8 directly? just confirming the preferred approach.
Like, doing these steps?
getting the context first,
Local<Context> context = isolate->GetCurrentContext();
... then after c++ concatenation using std::string s; and append,
at the time of return this way
Local<Value> v;
if (!ToV8Value(context, s, isolate).ToLocal(&v)) return Local<String>();
return v.As<String>();
This PR fixes incorrect and inconsistent error messages produced by
fs.rmSyncwhen deletion fails, especially for directories containing non-ASCII characters. The issue affected Linux and Windows differently but shared the same root cause: paths were embedded into custom error messages while also being passed separately toThrowErrnoException.1. Duplicate paths in error messages (Linux, ASCII paths)
fs.rmSyncconstructed messages like:Because
ThrowErrnoExceptionautomatically appends the path, this resulted in duplicated paths when directory names were ASCII-only:2. Corrupted paths for non-ASCII directory names (Linux)
When the directory name contained non-ASCII characters, embedding the path into the message caused UTF-8 bytes to be misinterpreted.
Example:
Here, the first byte of the UTF-8 sequence was interpreted as a Latin-1 character (
é), corrupting the path shown in the error message.3. Inconsistent Windows paths (?\ prefix)
On Windows,
err.pathcould expose raw extended-length paths prefixed with\\?\\, because the platform-specific normalization logic (StringFromPath) was not applied when constructingErrnoException.Example:
This is inconsistent with other fs APIs used in
src/api/exceptions, which strip the prefix before exposing paths to JavaScript.This PR fixes the issue by:
pathargument toThrowErrnoExceptionStringFromPathinsrc/api/exceptionsfor Windows soerr.pathis normalized correctlyA new test has been added:
test-fs-rmSync-special-char-additional-error.jsto verify that:fs.rmSyncreports the correct error codeerr.pathpreserves non-ASCII directory nameserr.messageincludes the correct path@joyeecheung @anonrig Please review this when you are available.