Skip to content

fs: fix rmSync error messages for non-ASCII paths#61233

Open
Yeaseen wants to merge 3 commits intonodejs:mainfrom
Yeaseen:fix-additional-error-message-rmsync
Open

fs: fix rmSync error messages for non-ASCII paths#61233
Yeaseen wants to merge 3 commits intonodejs:mainfrom
Yeaseen:fix-additional-error-message-rmsync

Conversation

@Yeaseen
Copy link
Contributor

@Yeaseen Yeaseen commented Jan 1, 2026

This PR fixes incorrect and inconsistent error messages produced by fs.rmSync when 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 to ThrowErrnoException.

1. Duplicate paths in error messages (Linux, ASCII paths)

fs.rmSync constructed messages like:

std::string message = "Permission denied: " + file_path_str;
env->ThrowErrnoException(err, "rm", message.c_str(), path_c_str);

Because ThrowErrnoException automatically appends the path, this resulted in duplicated paths when directory names were ASCII-only:

err.path: /tmp/_dir_0
err.message: EACCES, Permission denied: /tmp/_dir_0 '/tmp/_dir_0'

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:

err.path: /tmp/速速速_dir
err.message: EACCES, Permission denied: '/tmp/é速速_dir'

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.path could expose raw extended-length paths prefixed with \\?\\, because the platform-specific normalization logic (StringFromPath) was not applied when constructing ErrnoException.
Example:

err.path: \\?\\C:\\temp\\速速速_dir

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:

  • Removing path concatenation from custom error messages
  • Passing the path only via the path argument to ThrowErrnoException
  • Relying on Node’s standard error formatting to attach paths safely
  • Applying StringFromPath in src/api/exceptions for Windows so err.path is normalized correctly

A new test has been added: test-fs-rmSync-special-char-additional-error.js to verify that:

  • fs.rmSync reports the correct error code
  • err.path preserves non-ASCII directory names
  • err.message includes the correct path
  • No path corruption or duplication occurs

@joyeecheung @anonrig Please review this when you are available.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 1, 2026
@codecov
Copy link

codecov bot commented Jan 1, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (94b1f66) to head (d1368b0).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 40.00% 3 Missing ⚠️
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     
Files with missing lines Coverage Δ
src/api/exceptions.cc 94.50% <100.00%> (ø)
src/node_file.cc 75.31% <40.00%> (ø)

... and 455 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Yeaseen Yeaseen force-pushed the fix-additional-error-message-rmsync branch 2 times, most recently from a836c7f to a68d666 Compare January 1, 2026 11:21
@LucasmDjuiw
Copy link

LucasmDjuiw commented Jan 27, 2026

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.

@joyeecheung
Copy link
Member

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?

@Yeaseen Yeaseen force-pushed the fix-additional-error-message-rmsync branch from a68d666 to 4bc9b95 Compare February 18, 2026 11:20
@Yeaseen
Copy link
Contributor Author

Yeaseen commented Feb 18, 2026

@joyeecheung thanks for the feedback. I agree. I updated the test case for hitting NOT A DIRECTORY error. Let's see if it passes CI.

@Yeaseen Yeaseen force-pushed the fix-additional-error-message-rmsync branch from 4bc9b95 to 91a7aad Compare February 18, 2026 11:34
@Yeaseen
Copy link
Contributor Author

Yeaseen commented Feb 18, 2026

@joyeecheung looks good so far. The failing test on mac is unrelated to my changes. I appreciate a rerun. Thanks.

@Yeaseen
Copy link
Contributor Author

Yeaseen commented Feb 18, 2026

@joyeecheung CI is green now — would appreciate your review.

Copy link
Member

@anonrig anonrig Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though you moved this method, please use std::string_view. newfromutf8 takes additional parameters for the size of the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
@Yeaseen Yeaseen force-pushed the fix-additional-error-message-rmsync branch from 91a7aad to d1368b0 Compare February 18, 2026 20:58
Comment on lines +38 to +40
return String::Concat(isolate,
FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
to_v8(path.substr(kUncPrefix.size())));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this method:

v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments