Skip to content

Commit

Permalink
fs: fix rmSync to handle non-ASCII characters\n\nUpdate fs.rmSync to …
Browse files Browse the repository at this point in the history
…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.\n\nAdd a test 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.\n\nFixes: nodejs#56049
  • Loading branch information
Yeaseen committed Feb 6, 2025
1 parent 16dc29d commit 72e760a
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 117 deletions.
34 changes: 16 additions & 18 deletions src/api/exceptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ using v8::Object;
using v8::String;
using v8::Value;

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> ErrnoException(Isolate* isolate,
int errorno,
const char* syscall,
Expand All @@ -41,8 +56,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, path);
}

if (path_string.IsEmpty() == false) {
Expand Down Expand Up @@ -72,22 +86,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 Down
199 changes: 100 additions & 99 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1614,105 +1614,6 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
}
}

static void RmSync(const FunctionCallbackInfo<Value>& 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<Int32>()->Value();
int recursive = args[2]->IsTrue();
int retryDelay = args[3].As<Int32>()->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,
Expand Down Expand Up @@ -3181,6 +3082,106 @@ std::string ConvertWideToUTF8(const std::wstring& wstr) {

#endif // _WIN32

static void RmSync(const FunctionCallbackInfo<Value>& 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<Int32>()->Value();
int recursive = args[2]->IsTrue();
int retryDelay = args[3].As<Int32>()->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<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();
Expand Down
48 changes: 48 additions & 0 deletions test/parallel/test-fs-rmSync-special-char-additional-error.js
Original file line number Diff line number Diff line change
@@ -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
32 changes: 32 additions & 0 deletions test/parallel/test-fs-rmSync-special-char.js
Original file line number Diff line number Diff line change
@@ -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;
});

0 comments on commit 72e760a

Please sign in to comment.