From 2d0e9c876a1592b8be986b4e221e58ad71c47743 Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Wed, 5 Feb 2025 18:51:57 -0700 Subject: [PATCH 1/2] fs: fix rmSync to handle non-ASCII characters 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: https://github.com/nodejs/node/issues/56049 --- src/api/exceptions.cc | 34 ++- src/node_file.cc | 199 +++++++++--------- ...fs-rmSync-special-char-additional-error.js | 48 +++++ test/parallel/test-fs-rmSync-special-char.js | 32 +++ 4 files changed, 196 insertions(+), 117 deletions(-) create mode 100644 test/parallel/test-fs-rmSync-special-char-additional-error.js create mode 100644 test/parallel/test-fs-rmSync-special-char.js diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc index 871fe78de95154..656b6656dca32b 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, @@ -41,8 +56,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 +86,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 8e29bb39887625..a3be134e5c252f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1614,105 +1614,6 @@ static void RMDir(const FunctionCallbackInfo& args) { } } -static void RmSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); - - CHECK_EQ(args.Length(), 4); // path, maxRetries, recursive, retryDelay - - BufferValue path(isolate, args[0]); - CHECK_NOT_NULL(*path); - ToNamespacedPath(env, &path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - auto file_path = std::filesystem::path(path.ToStringView()); - std::error_code error; - auto file_status = std::filesystem::status(file_path, error); - - if (file_status.type() == std::filesystem::file_type::not_found) { - return; - } - - int maxRetries = args[1].As()->Value(); - int recursive = args[2]->IsTrue(); - int retryDelay = args[3].As()->Value(); - - // File is a directory and recursive is false - if (file_status.type() == std::filesystem::file_type::directory && - !recursive) { - return THROW_ERR_FS_EISDIR( - isolate, "Path is a directory: %s", file_path.c_str()); - } - - // Allowed errors are: - // - EBUSY: std::errc::device_or_resource_busy - // - EMFILE: std::errc::too_many_files_open - // - ENFILE: std::errc::too_many_files_open_in_system - // - ENOTEMPTY: std::errc::directory_not_empty - // - EPERM: std::errc::operation_not_permitted - auto can_omit_error = [](std::error_code error) -> bool { - return (error == std::errc::device_or_resource_busy || - error == std::errc::too_many_files_open || - error == std::errc::too_many_files_open_in_system || - error == std::errc::directory_not_empty || - error == std::errc::operation_not_permitted); - }; - - int i = 1; - - while (maxRetries >= 0) { - if (recursive) { - std::filesystem::remove_all(file_path, error); - } else { - std::filesystem::remove(file_path, error); - } - - if (!error || error == std::errc::no_such_file_or_directory) { - return; - } else if (!can_omit_error(error)) { - break; - } - - if (retryDelay > 0) { -#ifdef _WIN32 - Sleep(i * retryDelay / 1000); -#else - sleep(i * retryDelay / 1000); -#endif - } - maxRetries--; - i++; - } - - // On Windows path::c_str() returns wide char, convert to std::string first. - std::string file_path_str = file_path.string(); - const char* path_c_str = file_path_str.c_str(); -#ifdef _WIN32 - int permission_denied_error = EPERM; -#else - int permission_denied_error = EACCES; -#endif // !_WIN32 - - if (error == std::errc::operation_not_permitted) { - std::string message = "Operation not permitted: " + file_path_str; - 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; - return env->ThrowErrnoException(EACCES, "rm", message.c_str(), path_c_str); - } else if (error == std::errc::not_a_directory) { - std::string message = "Not a directory: " + file_path_str; - 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; - return env->ThrowErrnoException( - permission_denied_error, "rm", message.c_str(), path_c_str); - } - - std::string message = "Unknown error: " + error.message(); - return env->ThrowErrnoException( - UV_UNKNOWN, "rm", message.c_str(), path_c_str); -} - int MKDirpSync(uv_loop_t* loop, uv_fs_t* req, const std::string& path, @@ -3181,6 +3082,106 @@ std::string ConvertWideToUTF8(const std::wstring& wstr) { #endif // _WIN32 +static void RmSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + CHECK_EQ(args.Length(), 4); // path, maxRetries, recursive, retryDelay + + BufferValue path(isolate, args[0]); + CHECK_NOT_NULL(*path); + ToNamespacedPath(env, &path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); + auto file_path = BufferValueToPath(path); + std::error_code error; + auto file_status = std::filesystem::status(file_path, error); + + if (file_status.type() == std::filesystem::file_type::not_found) { + return; + } + + int maxRetries = args[1].As()->Value(); + int recursive = args[2]->IsTrue(); + int retryDelay = args[3].As()->Value(); + + // File is a directory and recursive is false + if (file_status.type() == std::filesystem::file_type::directory && + !recursive) { + auto file_path_as_str = PathToString(file_path); + return THROW_ERR_FS_EISDIR( + isolate, "Path is a directory: %s", file_path_as_str); + } + + // Allowed errors are: + // - EBUSY: std::errc::device_or_resource_busy + // - EMFILE: std::errc::too_many_files_open + // - ENFILE: std::errc::too_many_files_open_in_system + // - ENOTEMPTY: std::errc::directory_not_empty + // - EPERM: std::errc::operation_not_permitted + auto can_omit_error = [](std::error_code error) -> bool { + return (error == std::errc::device_or_resource_busy || + error == std::errc::too_many_files_open || + error == std::errc::too_many_files_open_in_system || + error == std::errc::directory_not_empty || + error == std::errc::operation_not_permitted); + }; + + int i = 1; + + while (maxRetries >= 0) { + if (recursive) { + std::filesystem::remove_all(file_path, error); + } else { + std::filesystem::remove(file_path, error); + } + + if (!error || error == std::errc::no_such_file_or_directory) { + return; + } else if (!can_omit_error(error)) { + break; + } + + if (retryDelay > 0) { +#ifdef _WIN32 + Sleep(i * retryDelay / 1000); +#else + sleep(i * retryDelay / 1000); +#endif + } + maxRetries--; + i++; + } + + // On Windows path::c_str() returns wide char, convert to std::string first. + std::string file_path_str = PathToString(file_path); + const char* path_c_str = file_path_str.c_str(); +#ifdef _WIN32 + int permission_denied_error = EPERM; +#else + int permission_denied_error = EACCES; +#endif // !_WIN32 + + if (error == std::errc::operation_not_permitted) { + 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: "; + return env->ThrowErrnoException(EACCES, "rm", message.c_str(), path_c_str); + } else if (error == std::errc::not_a_directory) { + 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: "; + return env->ThrowErrnoException( + permission_denied_error, "rm", message.c_str(), path_c_str); + } + + std::string message = "Unknown error: " + error.message(); + return env->ThrowErrnoException( + UV_UNKNOWN, "rm", message.c_str(), path_c_str); +} + static void CpSyncCheckPaths(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); 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..606eed6764ac95 --- /dev/null +++ b/test/parallel/test-fs-rmSync-special-char-additional-error.js @@ -0,0 +1,48 @@ +'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) { + // Verify that the error is due to permission restrictions + const expectedCode = process.platform === 'win32' ? 'EPERM' : 'EACCES'; + assert.strictEqual(err.code, expectedCode); + 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 diff --git a/test/parallel/test-fs-rmSync-special-char.js b/test/parallel/test-fs-rmSync-special-char.js new file mode 100644 index 00000000000000..00d7062d8b1077 --- /dev/null +++ b/test/parallel/test-fs-rmSync-special-char.js @@ -0,0 +1,32 @@ +'use strict'; +require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +// This test ensures that fs.rmSync handles non-ASCII characters in file paths, +// and that errors contain correctly encoded paths and err.path values. + +tmpdir.refresh(); // Prepare a clean temporary directory + +// Define paths with non-ASCII characters +const dirPath = path.join(tmpdir.path, '速_dir'); +const filePath = path.join(tmpdir.path, '速.txt'); + +// Create a directory and a file with non-ASCII characters +fs.mkdirSync(dirPath); +fs.writeFileSync(filePath, 'This is a test file with special characters.'); +fs.rmSync(filePath); +assert.strictEqual(fs.existsSync(filePath), false); + +// Ensure rmSync throws an error when trying to remove a directory without recursive +assert.throws(() => { + fs.rmSync(dirPath, { recursive: false }); +}, (err) => { + // Assert the error code and check that the error message includes the correct non-ASCII path + assert.strictEqual(err.code, 'ERR_FS_EISDIR'); + assert(err.message.includes(dirPath), 'Error message should include the directory path'); + assert.strictEqual(err.path, dirPath); + return true; +}); From 50acccabb495c23f9f83421dc0af613024be4e09 Mon Sep 17 00:00:00 2001 From: Yeaseen Date: Thu, 6 Feb 2025 07:50:11 -0700 Subject: [PATCH 2/2] adressred reviewer suggestions --- src/node_file.cc | 272 +++++++++++++++++++++++------------------------ 1 file changed, 136 insertions(+), 136 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index a3be134e5c252f..f671c2cc1c79b8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1614,6 +1614,142 @@ static void RMDir(const FunctionCallbackInfo& args) { } } +#ifdef _WIN32 +#define BufferValueToPath(str) \ + std::filesystem::path(ConvertToWideString(str.ToString(), CP_UTF8)) + +std::string ConvertWideToUTF8(const std::wstring& wstr) { + if (wstr.empty()) return std::string(); + + int size_needed = WideCharToMultiByte(CP_UTF8, + 0, + &wstr[0], + static_cast(wstr.size()), + nullptr, + 0, + nullptr, + nullptr); + std::string strTo(size_needed, 0); + WideCharToMultiByte(CP_UTF8, + 0, + &wstr[0], + static_cast(wstr.size()), + &strTo[0], + size_needed, + nullptr, + nullptr); + return strTo; +} + +#define PathToString(path) ConvertWideToUTF8(path.wstring()); + +#else // _WIN32 + +#define BufferValueToPath(str) std::filesystem::path(str.ToStringView()); +#define PathToString(path) path.native(); + +#endif // _WIN32 + +static void RmSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + CHECK_EQ(args.Length(), 4); // path, maxRetries, recursive, retryDelay + + BufferValue path(isolate, args[0]); + CHECK_NOT_NULL(*path); + ToNamespacedPath(env, &path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); + auto file_path = BufferValueToPath(path); + std::error_code error; + auto file_status = std::filesystem::status(file_path, error); + + if (file_status.type() == std::filesystem::file_type::not_found) { + return; + } + + int maxRetries = args[1].As()->Value(); + int recursive = args[2]->IsTrue(); + int retryDelay = args[3].As()->Value(); + + // File is a directory and recursive is false + if (file_status.type() == std::filesystem::file_type::directory && + !recursive) { + auto file_path_as_str = PathToString(file_path); + return THROW_ERR_FS_EISDIR( + isolate, "Path is a directory: %s", file_path_as_str); + } + + // Allowed errors are: + // - EBUSY: std::errc::device_or_resource_busy + // - EMFILE: std::errc::too_many_files_open + // - ENFILE: std::errc::too_many_files_open_in_system + // - ENOTEMPTY: std::errc::directory_not_empty + // - EPERM: std::errc::operation_not_permitted + auto can_omit_error = [](std::error_code error) -> bool { + return (error == std::errc::device_or_resource_busy || + error == std::errc::too_many_files_open || + error == std::errc::too_many_files_open_in_system || + error == std::errc::directory_not_empty || + error == std::errc::operation_not_permitted); + }; + + int i = 1; + + while (maxRetries >= 0) { + if (recursive) { + std::filesystem::remove_all(file_path, error); + } else { + std::filesystem::remove(file_path, error); + } + + if (!error || error == std::errc::no_such_file_or_directory) { + return; + } else if (!can_omit_error(error)) { + break; + } + + if (retryDelay > 0) { +#ifdef _WIN32 + Sleep(i * retryDelay / 1000); +#else + sleep(i * retryDelay / 1000); +#endif + } + maxRetries--; + i++; + } + + // On Windows path::c_str() returns wide char, convert to std::string first. + std::string file_path_str = PathToString(file_path); + const char* path_c_str = file_path_str.c_str(); +#ifdef _WIN32 + int permission_denied_error = EPERM; +#else + int permission_denied_error = EACCES; +#endif // !_WIN32 + + if (error == std::errc::operation_not_permitted) { + 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: "; + return env->ThrowErrnoException(EACCES, "rm", message.c_str(), path_c_str); + } else if (error == std::errc::not_a_directory) { + 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: "; + return env->ThrowErrnoException( + permission_denied_error, "rm", message.c_str(), path_c_str); + } + + std::string message = "Unknown error: " + error.message(); + return env->ThrowErrnoException( + UV_UNKNOWN, "rm", message.c_str(), path_c_str); +} + int MKDirpSync(uv_loop_t* loop, uv_fs_t* req, const std::string& path, @@ -3046,142 +3182,6 @@ static void GetFormatOfExtensionlessFile( return args.GetReturnValue().Set(EXTENSIONLESS_FORMAT_JAVASCRIPT); } -#ifdef _WIN32 -#define BufferValueToPath(str) \ - std::filesystem::path(ConvertToWideString(str.ToString(), CP_UTF8)) - -std::string ConvertWideToUTF8(const std::wstring& wstr) { - if (wstr.empty()) return std::string(); - - int size_needed = WideCharToMultiByte(CP_UTF8, - 0, - &wstr[0], - static_cast(wstr.size()), - nullptr, - 0, - nullptr, - nullptr); - std::string strTo(size_needed, 0); - WideCharToMultiByte(CP_UTF8, - 0, - &wstr[0], - static_cast(wstr.size()), - &strTo[0], - size_needed, - nullptr, - nullptr); - return strTo; -} - -#define PathToString(path) ConvertWideToUTF8(path.wstring()); - -#else // _WIN32 - -#define BufferValueToPath(str) std::filesystem::path(str.ToStringView()); -#define PathToString(path) path.native(); - -#endif // _WIN32 - -static void RmSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); - - CHECK_EQ(args.Length(), 4); // path, maxRetries, recursive, retryDelay - - BufferValue path(isolate, args[0]); - CHECK_NOT_NULL(*path); - ToNamespacedPath(env, &path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - auto file_path = BufferValueToPath(path); - std::error_code error; - auto file_status = std::filesystem::status(file_path, error); - - if (file_status.type() == std::filesystem::file_type::not_found) { - return; - } - - int maxRetries = args[1].As()->Value(); - int recursive = args[2]->IsTrue(); - int retryDelay = args[3].As()->Value(); - - // File is a directory and recursive is false - if (file_status.type() == std::filesystem::file_type::directory && - !recursive) { - auto file_path_as_str = PathToString(file_path); - return THROW_ERR_FS_EISDIR( - isolate, "Path is a directory: %s", file_path_as_str); - } - - // Allowed errors are: - // - EBUSY: std::errc::device_or_resource_busy - // - EMFILE: std::errc::too_many_files_open - // - ENFILE: std::errc::too_many_files_open_in_system - // - ENOTEMPTY: std::errc::directory_not_empty - // - EPERM: std::errc::operation_not_permitted - auto can_omit_error = [](std::error_code error) -> bool { - return (error == std::errc::device_or_resource_busy || - error == std::errc::too_many_files_open || - error == std::errc::too_many_files_open_in_system || - error == std::errc::directory_not_empty || - error == std::errc::operation_not_permitted); - }; - - int i = 1; - - while (maxRetries >= 0) { - if (recursive) { - std::filesystem::remove_all(file_path, error); - } else { - std::filesystem::remove(file_path, error); - } - - if (!error || error == std::errc::no_such_file_or_directory) { - return; - } else if (!can_omit_error(error)) { - break; - } - - if (retryDelay > 0) { -#ifdef _WIN32 - Sleep(i * retryDelay / 1000); -#else - sleep(i * retryDelay / 1000); -#endif - } - maxRetries--; - i++; - } - - // On Windows path::c_str() returns wide char, convert to std::string first. - std::string file_path_str = PathToString(file_path); - const char* path_c_str = file_path_str.c_str(); -#ifdef _WIN32 - int permission_denied_error = EPERM; -#else - int permission_denied_error = EACCES; -#endif // !_WIN32 - - if (error == std::errc::operation_not_permitted) { - 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: "; - return env->ThrowErrnoException(EACCES, "rm", message.c_str(), path_c_str); - } else if (error == std::errc::not_a_directory) { - 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: "; - return env->ThrowErrnoException( - permission_denied_error, "rm", message.c_str(), path_c_str); - } - - std::string message = "Unknown error: " + error.message(); - return env->ThrowErrnoException( - UV_UNKNOWN, "rm", message.c_str(), path_c_str); -} - static void CpSyncCheckPaths(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate();