From 6780f7b5b29c97e01eec97dc9cee660eee904613 Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Wed, 31 Dec 2025 18:59:32 -0700 Subject: [PATCH 1/3] fs: fix rmSync error messages for non-ASCII paths 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. --- src/api/exceptions.cc | 33 ++++++------ src/node_file.cc | 10 ++-- ...fs-rmSync-special-char-additional-error.js | 51 +++++++++++++++++++ 3 files changed, 72 insertions(+), 22 deletions(-) create mode 100644 test/parallel/test-fs-rmSync-special-char-additional-error.js diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc index 871fe78de95154..cfe5fb0ae7a0df 100644 --- a/src/api/exceptions.cc +++ b/src/api/exceptions.cc @@ -20,6 +20,21 @@ using v8::Object; using v8::String; using v8::Value; +static Local 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 ErrnoException(Isolate* isolate, int errorno, const char* syscall, @@ -42,7 +57,7 @@ Local ErrnoException(Isolate* isolate, Local 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, path); } if (path_string.IsEmpty() == false) { @@ -72,22 +87,6 @@ Local ErrnoException(Isolate* isolate, return e; } -static Local 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 UVException(Isolate* isolate, int errorno, const char* syscall, diff --git a/src/node_file.cc b/src/node_file.cc index d7926d9a07a3a6..9b0885af909003 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1728,7 +1728,7 @@ static void RmSync(const FunctionCallbackInfo& 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; @@ -1737,17 +1737,17 @@ static void RmSync(const FunctionCallbackInfo& 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); } diff --git a/test/parallel/test-fs-rmSync-special-char-additional-error.js b/test/parallel/test-fs-rmSync-special-char-additional-error.js new file mode 100644 index 00000000000000..2d292d1213aea0 --- /dev/null +++ b/test/parallel/test-fs-rmSync-special-char-additional-error.js @@ -0,0 +1,51 @@ +'use strict'; +require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execSync } = require('child_process'); + +tmpdir.refresh(); // Prepare a clean temporary directory + +const dirPath = path.join(tmpdir.path, '速速速_dir'); +const filePath = path.join(dirPath, 'test_file.txt'); + +// Create a directory and a file within it +fs.mkdirSync(dirPath, { recursive: true }); +fs.writeFileSync(filePath, 'This is a test file.'); + +// Set permissions to simulate a permission denied scenario +if (process.platform === 'win32') { + // Windows: Deny delete permissions + execSync(`icacls "${filePath}" /deny Everyone:(D)`); +} else { + // Unix/Linux: Remove write permissions from the directory + fs.chmodSync(dirPath, 0o555); // Read and execute permissions only +} + +// Attempt to delete the directory which should now fail +try { + fs.rmSync(dirPath, { recursive: true }); +} catch (err) { + if (process.platform === 'win32') { + assert.strictEqual(err.code, 'EPERM'); + } else { + // POSIX: rmSync may surface different errors + assert(['EACCES', 'EPERM', 'ENOTEMPTY'].includes(err.code), err.code); + } + assert.strictEqual(err.path, dirPath); + assert(err.message.includes(dirPath), 'Error message should include the path treated as a directory'); +} + +// Cleanup - resetting permissions and removing the directory safely +if (process.platform === 'win32') { + // Remove the explicit permissions before attempting to delete + execSync(`icacls "${filePath}" /remove:d Everyone`); +} else { + // Reset permissions to allow deletion + fs.chmodSync(dirPath, 0o755); // Restore full permissions to the directory +} + +// Attempt to clean up +fs.rmSync(dirPath, { recursive: true }); // This should now succeed From 7aff8e1cff36d2cc1ba8ca2e97ce6c3c15e87e1c Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Wed, 18 Feb 2026 04:19:43 -0700 Subject: [PATCH 2/3] test: add ENOTDIR additional error coverage for rmSync --- ...fs-rmSync-special-char-additional-error.js | 48 +++++-------------- 1 file changed, 11 insertions(+), 37 deletions(-) diff --git a/test/parallel/test-fs-rmSync-special-char-additional-error.js b/test/parallel/test-fs-rmSync-special-char-additional-error.js index 2d292d1213aea0..f362d1e79ee727 100644 --- a/test/parallel/test-fs-rmSync-special-char-additional-error.js +++ b/test/parallel/test-fs-rmSync-special-char-additional-error.js @@ -4,48 +4,22 @@ const tmpdir = require('../common/tmpdir'); const assert = require('node:assert'); const fs = require('node:fs'); const path = require('node:path'); -const { execSync } = require('child_process'); -tmpdir.refresh(); // Prepare a clean temporary directory +tmpdir.refresh(); -const dirPath = path.join(tmpdir.path, '速速速_dir'); -const filePath = path.join(dirPath, 'test_file.txt'); +const file = path.join(tmpdir.path, '速_file'); +fs.writeFileSync(file, 'x'); -// Create a directory and a file within it -fs.mkdirSync(dirPath, { recursive: true }); -fs.writeFileSync(filePath, 'This is a test file.'); - -// Set permissions to simulate a permission denied scenario -if (process.platform === 'win32') { - // Windows: Deny delete permissions - execSync(`icacls "${filePath}" /deny Everyone:(D)`); -} else { - // Unix/Linux: Remove write permissions from the directory - fs.chmodSync(dirPath, 0o555); // Read and execute permissions only -} +// 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(dirPath, { recursive: true }); + fs.rmSync(badPath, { recursive: true }); } catch (err) { - if (process.platform === 'win32') { - assert.strictEqual(err.code, 'EPERM'); - } else { - // POSIX: rmSync may surface different errors - assert(['EACCES', 'EPERM', 'ENOTEMPTY'].includes(err.code), err.code); - } - assert.strictEqual(err.path, dirPath); - assert(err.message.includes(dirPath), 'Error message should include the path treated as a directory'); + // 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'); } - -// Cleanup - resetting permissions and removing the directory safely -if (process.platform === 'win32') { - // Remove the explicit permissions before attempting to delete - execSync(`icacls "${filePath}" /remove:d Everyone`); -} else { - // Reset permissions to allow deletion - fs.chmodSync(dirPath, 0o755); // Restore full permissions to the directory -} - -// Attempt to clean up -fs.rmSync(dirPath, { recursive: true }); // This should now succeed From d1368b0a0c992fce0d306ae00aa4ef2d591ca49d Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Wed, 18 Feb 2026 13:57:57 -0700 Subject: [PATCH 3/3] src/api: use std::string_view in exceptions StringFromPath --- src/api/exceptions.cc | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc index cfe5fb0ae7a0df..088bbaff86a2a6 100644 --- a/src/api/exceptions.cc +++ b/src/api/exceptions.cc @@ -8,6 +8,7 @@ #include "v8.h" #include +#include namespace node { @@ -20,19 +21,31 @@ using v8::Object; using v8::String; using v8::Value; -static Local StringFromPath(Isolate* isolate, const char* path) { +static Local StringFromPath(Isolate* isolate, std::string_view path) { + auto to_v8 = [&](std::string_view s) { + return String::NewFromUtf8(isolate, + s.data(), + v8::NewStringType::kNormal, + static_cast(s.size())) + .ToLocalChecked(); + }; + #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(); + 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()))); + } + + if (path.starts_with(kLongPrefix)) { + return to_v8(path.substr(kLongPrefix.size())); } #endif - return String::NewFromUtf8(isolate, path).ToLocalChecked(); + return to_v8(path); } Local ErrnoException(Isolate* isolate, @@ -57,7 +70,7 @@ Local ErrnoException(Isolate* isolate, Local path_string; if (path != nullptr) { // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8. - path_string = StringFromPath(isolate, path); + path_string = StringFromPath(isolate, std::string_view(path)); } if (path_string.IsEmpty() == false) { @@ -113,7 +126,7 @@ Local 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, " '")); @@ -123,7 +136,7 @@ Local 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, " -> '"));