From 0c33ceae9c327c5f886c5d9c391b723f8ffecd21 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. Signed-off-by: Yeaseen --- 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 871fe78de951..cfe5fb0ae7a0 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 871d8f16bd34..4c6d41204d28 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1828,7 +1828,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; @@ -1837,14 +1837,14 @@ 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); #ifdef _AIX } else if (error == std::errc::permission_denied || @@ -1855,7 +1855,7 @@ static void RmSync(const FunctionCallbackInfo& args) { #else } else if (error == std::errc::permission_denied) { #endif - 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 000000000000..2d292d1213ae --- /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 0d8ad5f63b25ff46cf28c04f1f2658b0b08c35cd Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Wed, 18 Feb 2026 04:19:43 -0700 Subject: [PATCH 2/3] test: add rmSync path duplication coverage Signed-off-by: Yeaseen --- ...fs-rmSync-special-char-additional-error.js | 60 ++++++++++++------- 1 file changed, 39 insertions(+), 21 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 2d292d1213ae..9b4de3c84d62 100644 --- a/test/parallel/test-fs-rmSync-special-char-additional-error.js +++ b/test/parallel/test-fs-rmSync-special-char-additional-error.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); const tmpdir = require('../common/tmpdir'); const assert = require('node:assert'); const fs = require('node:fs'); @@ -8,7 +8,19 @@ const { execSync } = require('child_process'); tmpdir.refresh(); // Prepare a clean temporary directory -const dirPath = path.join(tmpdir.path, '速速速_dir'); +function countOccurrences(haystack, needle) { + let count = 0; + let offset = 0; + + while ((offset = haystack.indexOf(needle, offset)) !== -1) { + count++; + offset += needle.length; + } + + return count; +} + +const dirPath = path.join(tmpdir.path, '速_dir'); const filePath = path.join(dirPath, 'test_file.txt'); // Create a directory and a file within it @@ -24,28 +36,34 @@ if (process.platform === 'win32') { 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) { + // Attempt to delete the directory which should now fail + assert.throws(() => { + fs.rmSync(dirPath, { recursive: true }); + }, (err) => { + // Verify that the error is due to permission restrictions + let expectedCode = 'EACCES'; + if (common.isMacOS) { + expectedCode = 'ENOTEMPTY'; + } else if (common.isWindows) { + expectedCode = 'EPERM'; + } + assert.strictEqual(err.code, expectedCode); + assert.strictEqual(err.syscall, 'rm'); + assert.strictEqual(err.path, dirPath); + assert.strictEqual(countOccurrences(err.message, dirPath), 1); + return true; + }); +} finally { + // Cleanup - resetting permissions and removing the directory safely if (process.platform === 'win32') { - assert.strictEqual(err.code, 'EPERM'); + // Remove the explicit permissions before attempting to delete + execSync(`icacls "${filePath}" /remove:d Everyone`); } else { - // POSIX: rmSync may surface different errors - assert(['EACCES', 'EPERM', 'ENOTEMPTY'].includes(err.code), err.code); + // Reset permissions to allow deletion + fs.chmodSync(dirPath, 0o755); // Restore full permissions to the directory } - 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 } - -// Attempt to clean up -fs.rmSync(dirPath, { recursive: true }); // This should now succeed From de31cdde71be7104167228c63ffd81313ff5082f 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 Signed-off-by: Yeaseen --- src/api/exceptions.cc | 45 +++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc index cfe5fb0ae7a0..4396dc19d842 100644 --- a/src/api/exceptions.cc +++ b/src/api/exceptions.cc @@ -8,6 +8,8 @@ #include "v8.h" #include +#include +#include namespace node { @@ -20,19 +22,38 @@ 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) { #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)) { + std::string s; + s.reserve(2 + (path.size() - kUncPrefix.size())); + s.append("\\\\"); + s.append(path.substr(kUncPrefix.size())); + return String::NewFromUtf8(isolate, + s.data(), + v8::NewStringType::kNormal, + static_cast(s.size())) + .ToLocalChecked(); + } + + if (path.starts_with(kLongPrefix)) { + auto rest = path.substr(kLongPrefix.size()); + return String::NewFromUtf8(isolate, + rest.data(), + v8::NewStringType::kNormal, + static_cast(rest.size())) + .ToLocalChecked(); } #endif - return String::NewFromUtf8(isolate, path).ToLocalChecked(); + return String::NewFromUtf8(isolate, + path.data(), + v8::NewStringType::kNormal, + static_cast(path.size())) + .ToLocalChecked(); } Local ErrnoException(Isolate* isolate, @@ -57,7 +78,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 +134,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 +144,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, " -> '"));