-
Notifications
You must be signed in to change notification settings - Fork 31.3k
fs: fix rmSync to handle non-ASCII characters #56934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1614,6 +1614,42 @@ static void RMDir(const FunctionCallbackInfo<Value>& 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<int>(wstr.size()), | ||
nullptr, | ||
0, | ||
nullptr, | ||
nullptr); | ||
std::string strTo(size_needed, 0); | ||
WideCharToMultiByte(CP_UTF8, | ||
0, | ||
&wstr[0], | ||
static_cast<int>(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<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
Isolate* isolate = env->isolate(); | ||
|
@@ -1625,7 +1661,7 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) { | |
ToNamespacedPath(env, &path); | ||
THROW_IF_INSUFFICIENT_PERMISSIONS( | ||
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); | ||
auto file_path = std::filesystem::path(path.ToStringView()); | ||
auto file_path = BufferValueToPath(path); | ||
std::error_code error; | ||
auto file_status = std::filesystem::status(file_path, error); | ||
|
||
|
@@ -1640,8 +1676,9 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) { | |
// 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.c_str()); | ||
isolate, "Path is a directory: %s", file_path_as_str); | ||
} | ||
|
||
// Allowed errors are: | ||
|
@@ -1685,7 +1722,7 @@ static void RmSync(const FunctionCallbackInfo<Value>& 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 = PathToString(file_path); | ||
const char* path_c_str = file_path_str.c_str(); | ||
#ifdef _WIN32 | ||
int permission_denied_error = EPERM; | ||
|
@@ -1694,16 +1731,16 @@ static void RmSync(const FunctionCallbackInfo<Value>& 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(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; | ||
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: "; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove the paths from the error messages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was giving erroneous path if the path contains any non-ASCII character, both in Linux and Windows. The file path actually is passed as In the test file, I explicitly checked whether the err.path is correct and the path is in the err.message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anonrig please review this when you are free. |
||
return env->ThrowErrnoException( | ||
permission_denied_error, "rm", message.c_str(), path_c_str); | ||
} | ||
|
@@ -3145,42 +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<int>(wstr.size()), | ||
nullptr, | ||
0, | ||
nullptr, | ||
nullptr); | ||
std::string strTo(size_needed, 0); | ||
WideCharToMultiByte(CP_UTF8, | ||
0, | ||
&wstr[0], | ||
static_cast<int>(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 CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
Isolate* isolate = env->isolate(); | ||
|
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 |
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; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This FIXME should not be removed, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think It was needed because of
String::NewFromUtf8
right after here. Now that I am usingStringFromPath
, it's no longer needed here.If you want me to put it back, I can do this.