Skip to content

Commit 83b37bd

Browse files
committed
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.
1 parent 7b7f693 commit 83b37bd

File tree

3 files changed

+69
-22
lines changed

3 files changed

+69
-22
lines changed

src/api/exceptions.cc

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,21 @@ using v8::Object;
2020
using v8::String;
2121
using v8::Value;
2222

23+
static Local<String> StringFromPath(Isolate* isolate, const char* path) {
24+
#ifdef _WIN32
25+
if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
26+
return String::Concat(
27+
isolate,
28+
FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
29+
String::NewFromUtf8(isolate, path + 8).ToLocalChecked());
30+
} else if (strncmp(path, "\\\\?\\", 4) == 0) {
31+
return String::NewFromUtf8(isolate, path + 4).ToLocalChecked();
32+
}
33+
#endif
34+
35+
return String::NewFromUtf8(isolate, path).ToLocalChecked();
36+
}
37+
2338
Local<Value> ErrnoException(Isolate* isolate,
2439
int errorno,
2540
const char* syscall,
@@ -42,7 +57,7 @@ Local<Value> ErrnoException(Isolate* isolate,
4257
Local<String> path_string;
4358
if (path != nullptr) {
4459
// FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8.
45-
path_string = String::NewFromUtf8(isolate, path).ToLocalChecked();
60+
path_string = StringFromPath(isolate, path);
4661
}
4762

4863
if (path_string.IsEmpty() == false) {
@@ -72,22 +87,6 @@ Local<Value> ErrnoException(Isolate* isolate,
7287
return e;
7388
}
7489

75-
static Local<String> StringFromPath(Isolate* isolate, const char* path) {
76-
#ifdef _WIN32
77-
if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
78-
return String::Concat(
79-
isolate,
80-
FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
81-
String::NewFromUtf8(isolate, path + 8).ToLocalChecked());
82-
} else if (strncmp(path, "\\\\?\\", 4) == 0) {
83-
return String::NewFromUtf8(isolate, path + 4).ToLocalChecked();
84-
}
85-
#endif
86-
87-
return String::NewFromUtf8(isolate, path).ToLocalChecked();
88-
}
89-
90-
9190
Local<Value> UVException(Isolate* isolate,
9291
int errorno,
9392
const char* syscall,

src/node_file.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1693,7 +1693,7 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) {
16931693
}
16941694

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

17041704
if (error == std::errc::operation_not_permitted) {
1705-
std::string message = "Operation not permitted: " + file_path_str;
1705+
std::string message = "Operation not permitted: ";
17061706
return env->ThrowErrnoException(EPERM, "rm", message.c_str(), path_c_str);
17071707
} else if (error == std::errc::directory_not_empty) {
1708-
std::string message = "Directory not empty: " + file_path_str;
1708+
std::string message = "Directory not empty: ";
17091709
return env->ThrowErrnoException(
17101710
ENOTEMPTY, "rm", message.c_str(), path_c_str);
17111711
} else if (error == std::errc::not_a_directory) {
1712-
std::string message = "Not a directory: " + file_path_str;
1712+
std::string message = "Not a directory: ";
17131713
return env->ThrowErrnoException(ENOTDIR, "rm", message.c_str(), path_c_str);
17141714
} else if (error == std::errc::permission_denied) {
1715-
std::string message = "Permission denied: " + file_path_str;
1715+
std::string message = "Permission denied: ";
17161716
return env->ThrowErrnoException(
17171717
permission_denied_error, "rm", message.c_str(), path_c_str);
17181718
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
require('../common');
3+
const tmpdir = require('../common/tmpdir');
4+
const assert = require('node:assert');
5+
const fs = require('node:fs');
6+
const path = require('node:path');
7+
const { execSync } = require('child_process');
8+
9+
tmpdir.refresh(); // Prepare a clean temporary directory
10+
11+
const dirPath = path.join(tmpdir.path, '速速速_dir');
12+
const filePath = path.join(dirPath, 'test_file.txt');
13+
14+
// Create a directory and a file within it
15+
fs.mkdirSync(dirPath, { recursive: true });
16+
fs.writeFileSync(filePath, 'This is a test file.');
17+
18+
// Set permissions to simulate a permission denied scenario
19+
if (process.platform === 'win32') {
20+
// Windows: Deny delete permissions
21+
execSync(`icacls "${filePath}" /deny Everyone:(D)`);
22+
} else {
23+
// Unix/Linux: Remove write permissions from the directory
24+
fs.chmodSync(dirPath, 0o555); // Read and execute permissions only
25+
}
26+
27+
// Attempt to delete the directory which should now fail
28+
try {
29+
fs.rmSync(dirPath, { recursive: true });
30+
} catch (err) {
31+
// Verify that the error is due to permission restrictions
32+
const expectedCode = process.platform === 'win32' ? 'EPERM' : 'EACCES';
33+
assert.strictEqual(err.code, expectedCode);
34+
assert.strictEqual(err.path, dirPath);
35+
assert(err.message.includes(dirPath), 'Error message should include the path treated as a directory');
36+
}
37+
38+
// Cleanup - resetting permissions and removing the directory safely
39+
if (process.platform === 'win32') {
40+
// Remove the explicit permissions before attempting to delete
41+
execSync(`icacls "${filePath}" /remove:d Everyone`);
42+
} else {
43+
// Reset permissions to allow deletion
44+
fs.chmodSync(dirPath, 0o755); // Restore full permissions to the directory
45+
}
46+
47+
// Attempt to clean up
48+
fs.rmSync(dirPath, { recursive: true }); // This should now succeed

0 commit comments

Comments
 (0)