Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Yeaseen wants to merge 3 commits into
base: main
Choose a base branch
from
+61 −24
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6780f7b
fs: fix rmSync error messages for non-ASCII paths
Yeaseen Jan 1, 2026
7aff8e1
test: add ENOTDIR additional error coverage for rmSync
Yeaseen Feb 18, 2026
d1368b0
src/api: use std::string_view in exceptions StringFromPath
Yeaseen Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 31 additions & 19 deletions src/api/exceptions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "v8.h"

#include <cstring>
#include <string_view>

namespace node {

Expand All @@ -20,6 +21,33 @@ using v8::Object;
using v8::String;
using v8::Value;

static Local<String> StringFromPath(Isolate* isolate, std::string_view path) {
auto to_v8 = [&](std::string_view s) {
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.

We already have this method:

node/src/util-inl.h

Line 344 in 94b1f66

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

Copy link
Contributor Author

@Yeaseen Yeaseen Feb 18, 2026

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

return String::NewFromUtf8(isolate,
s.data(),
v8::NewStringType::kNormal,
static_cast<int>(s.size()))
.ToLocalChecked();
};

#ifdef _WIN32
constexpr std::string_view kUncPrefix = "\\\\?\\UNC\\";
constexpr std::string_view kLongPrefix = "\\\\?\\";

if (path.starts_with(kUncPrefix)) {
return String::Concat(isolate,
FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
to_v8(path.substr(kUncPrefix.size())));
Comment on lines +38 to +40
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.

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

}

if (path.starts_with(kLongPrefix)) {
return to_v8(path.substr(kLongPrefix.size()));
}
#endif

return to_v8(path);
}

Local<Value> ErrnoException(Isolate* isolate,
int errorno,
const char* syscall,
Expand All @@ -42,7 +70,7 @@ Local<Value> ErrnoException(Isolate* isolate,
Local<String> path_string;
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, std::string_view(path));
}

if (path_string.IsEmpty() == false) {
Expand Down Expand Up @@ -72,22 +100,6 @@ Local<Value> ErrnoException(Isolate* isolate,
return e;
}

static Local<String> StringFromPath(Isolate* isolate, const char* path) {
#ifdef _WIN32
if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
return String::Concat(
isolate,
FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
String::NewFromUtf8(isolate, path + 8).ToLocalChecked());
} else if (strncmp(path, "\\\\?\\", 4) == 0) {
return String::NewFromUtf8(isolate, path + 4).ToLocalChecked();
}
#endif

return String::NewFromUtf8(isolate, path).ToLocalChecked();
}


Local<Value> UVException(Isolate* isolate,
int errorno,
const char* syscall,
Expand All @@ -114,7 +126,7 @@ Local<Value> UVException(Isolate* isolate,
js_msg = String::Concat(isolate, js_msg, js_syscall);

if (path != nullptr) {
js_path = StringFromPath(isolate, path);
js_path = StringFromPath(isolate, std::string_view(path));

js_msg =
String::Concat(isolate, js_msg, FIXED_ONE_BYTE_STRING(isolate, " '"));
Expand All @@ -124,7 +136,7 @@ Local<Value> UVException(Isolate* isolate,
}

if (dest != nullptr) {
js_dest = StringFromPath(isolate, dest);
js_dest = StringFromPath(isolate, std::string_view(dest));

js_msg = String::Concat(
isolate, js_msg, FIXED_ONE_BYTE_STRING(isolate, " -> '"));
Expand Down
10 changes: 5 additions & 5 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1728,7 +1728,7 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) {
}

// On Windows path::c_str() returns wide char, convert to std::string first.
std::string file_path_str = file_path.string();
std::string file_path_str = ConvertPathToUTF8(file_path);
const char* path_c_str = file_path_str.c_str();
#ifdef _WIN32
int permission_denied_error = EPERM;
Expand All @@ -1737,17 +1737,17 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) {
#endif // !_WIN32

if (error == std::errc::operation_not_permitted) {
std::string message = "Operation not permitted: " + file_path_str;
std::string message = "Operation not permitted: ";
return env->ThrowErrnoException(EPERM, "rm", message.c_str(), path_c_str);
} else if (error == std::errc::directory_not_empty) {
std::string message = "Directory not empty: " + file_path_str;
std::string message = "Directory not empty: ";
return env->ThrowErrnoException(
ENOTEMPTY, "rm", message.c_str(), path_c_str);
} else if (error == std::errc::not_a_directory) {
std::string message = "Not a directory: " + file_path_str;
std::string message = "Not a directory: ";
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: ";
return env->ThrowErrnoException(
permission_denied_error, "rm", message.c_str(), path_c_str);
}
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-fs-rmSync-special-char-additional-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
require('../common');
const tmpdir = require('../common/tmpdir');
const assert = require('node:assert');
const fs = require('node:fs');
const path = require('node:path');

tmpdir.refresh();

const file = path.join(tmpdir.path, '速_file');
fs.writeFileSync(file, 'x');

// Treat that file as if it were a directory so the error message
// includes the path treated as a directory, not the file.
const badPath = path.join(file, 'child');

// Attempt to delete the directory which should now fail
try {
fs.rmSync(badPath, { recursive: true });
} catch (err) {
// Verify that the error is due to the path being treated as a directory
assert.strictEqual(err.code, 'ENOTDIR');
assert.strictEqual(err.path, badPath);
assert(err.message.includes(badPath), 'Error message should include the path treated as a directory');
}
Loading