fs: fix rmSync to handle non-ASCII characters by Yeaseen · Pull Request #56934 · nodejs/node · GitHub

This is a fresh version of PR #56117

Update fs.rmSync to properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names.

Add two tests in test/parallel/ to ensure that files with non-ASCII characters can be deleted without issues. This covers cases that previously caused unexpected behavior or crashes on certain file systems.

Fixes: #56049

@lemire I had to close the previous PR due to some conflicts. Please review this. Thank you!

FYI you don't need to open a new PR to address the conflicts - usually it's fine to just rebase against the latest main branch, squash the commits if that's easier, and force push to your PR branch.

@joyeecheung, thanks for your suggestion. I resolved the conflict with the latest main branch. But, due to the changes in the helper, in CI the conflict was coming from one of my own commits in that PR. Then, I tried to do cherry pick for that commit hash, but I did the reverse of what I wanted to do. And it became messy. I will recreate this scenario locally to learn how to handle this.

CI: https://ci.nodejs.org/job/node-test-pull-request/65037/

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.17%. Comparing base (1671921) to head (81955c8).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	37.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56934      +/-   ##
==========================================
+ Coverage   89.15%   89.17%   +0.01%     
==========================================
  Files         665      665              
  Lines      192798   192800       +2     
  Branches    37130    37121       -9     
==========================================
+ Hits       171886   171921      +35     
+ Misses      13673    13670       -3     
+ Partials     7239     7209      -30

Files with missing lines	Coverage Δ
src/api/exceptions.cc	94.18% <100.00%> (ø)
src/node_file.cc	77.21% <37.50%> (-0.03%)	⬇️

... and 26 files with indirect coverage changes

@joyeecheung It looks like all checks passed except the first git commit message one, where I must explicitly put the GitHub link. Should I just amend the first commit message and push it here?

Yes, you'll need to update it to pull a full GitHub link

@joyeecheung updated.

Hmm, I think you'll need to rebase against the latest main branch and drop the other commits that you've pulled in somehow from the main branch. You can use use something like git fetch origin main then git rebase origin/main -i and remove the unrelated commits.

Update fs.rmSync to properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names. Add two tests in test/parallel to ensure that files with non-ASCII characters can be deleted without issues. This covers cases that previously caused unexpected behavior or crashes on certain file systems. Fixes: #56049

@joyeecheung Thanks for the suggestion! It worked. Learned a lof things!

Why did you remove the paths from the error messages?

This was giving erroneous path if the path contains any non-ASCII character, both in Linux and Windows. The file path actually is passed as path_c_str and added to the error messages in env->ThrowErrnoException( permission_denied_error, "rm", message.c_str(), path_c_str);

In the test file, I explicitly checked whether the err.path is correct and the path is in the err.message.

@anonrig please review this when you are free.

Yes, the path here seems redundant, ThrowErrnoException would format it into something like ${message} , '${path}'. The original message is actually showing the path in the error message twice.

This FIXME should not be removed, isn't it?

I think It was needed because of String::NewFromUtf8 right after here. Now that I am using StringFromPath, it's no longer needed here.

If you want me to put it back, I can do this.

I think this dates waaay back and it's possible that some other path is passing a wide string from Windows to this that's not originated from JS but from some Windows API. So this should probably still be left as-is.

Also it doesn't look like a good idea to update ErrnoException in this PR, this is shared by too many things. Please submit a separate PR for this.

@joyeecheung It looks like somebody is working on the error codes. The original goal was to handle non-ASCII characters by using the macro to fix bug #56049, and causing conflicts here. The main problem is still there. I think I should I should first use the macro BufferValueToPath to fix the conversion. Then I should work on the error messages later! Otherwise, it will always be conflict! Should I open a new PR and solve the bug first?

@joyeecheung It looks like somebody is working on the error codes. The original goal was to handle non-ASCII characters by using the macro to fix bug #56049, and causing conflicts here. The main problem is still there. I think I should I should first use the macro BufferValueToPath to fix the conversion. Then I should work on the error messages later! Otherwise, it will always be conflict! Should I open a new PR and solve the bug first?

Hi @Yeaseen, following up on the status here.
This bug is currently blocking my project when upgrading Node from 18 to 24.
If the fix is still planned, would you prefer to continue working in this PR, or should we move forward with a new one?
Thanks!

@Yuchen-Dai, I will create a new PR next week to fix the bug #56049

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 Feb 6, 2025

joyeecheung reviewed Feb 6, 2025

View reviewed changes

Comment thread src/node_file.cc Outdated

joyeecheung approved these changes Feb 6, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2025

jasnell approved these changes Feb 7, 2025

View reviewed changes

Yeaseen added 2 commits February 7, 2025 09:32

adressred reviewer suggestions

50accca

anonrig requested changes Feb 7, 2025

View reviewed changes

Yeaseen requested a review from anonrig February 22, 2025 10:37

anonrig approved these changes Dec 14, 2025

View reviewed changes

Yeaseen closed this by deleting the head repository Dec 18, 2025

Yeaseen mentioned this pull request Dec 18, 2025

fs: fix rmSync to handle non-ASCII characters #61108

Merged

                   return env->ThrowErrnoException(ENOTDIR, "rm", message.c_str(), path_c_str);
                 } else if (error == std::errc::permission_denied) {
-                  std::string message = "Permission denied: " + file_path_str;
+                  std::string message = "Permission denied: ";

                 if (path != nullptr) {
-                  // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8.
-                  path_string = String::NewFromUtf8(isolate, path).ToLocalChecked();
+                  path_string = StringFromPath(isolate, path);

fs: fix rmSync to handle non-ASCII characters by Yeaseen · Pull Request #56934 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: fix rmSync to handle non-ASCII characters - #56934

fs: fix rmSync to handle non-ASCII characters#56934Yeaseen wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom Yeaseen:fix-non-ascii-filepath-issueCopy head branch name to clipboard

Conversation

Yeaseen commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yeaseen commented Feb 6, 2025

Uh oh!

nodejs-github-bot commented Feb 6, 2025

Uh oh!

codecov Bot commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Yeaseen commented Feb 6, 2025

Uh oh!

joyeecheung commented Feb 6, 2025

Uh oh!

Yeaseen commented Feb 6, 2025

Uh oh!

joyeecheung commented Feb 7, 2025

Uh oh!

Yeaseen commented Feb 7, 2025

Uh oh!

anonrig Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

Yeaseen Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

Yeaseen Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

Yeaseen Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung May 28, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yeaseen commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yuchen-Dai commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yeaseen commented Dec 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fs: fix rmSync to handle non-ASCII characters#56934
Yeaseen wants to merge 2 commits into
nodejs:mainfrom
Yeaseen:fix-non-ascii-filepath-issue

Yeaseen commented Feb 6, 2025 •
edited
Loading

codecov Bot commented Feb 6, 2025 •
edited
Loading

joyeecheung May 28, 2025 •
edited
Loading

joyeecheung May 28, 2025 •
edited
Loading

Yeaseen commented Feb 25, 2025 •
edited
Loading

Yuchen-Dai commented Dec 12, 2025 •
edited
Loading