Skip to content
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.

Sign up for GitHub

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

fs: fix crash when path contains % #55171

Merged
merged 1 commit into from
Oct 18, 2024
Merged

fs: fix crash when path contains % #55171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 75 additions & 40 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3127,6 +3127,55 @@ static void GetFormatOfExtensionlessFile(
return args.GetReturnValue().Set(EXTENSIONLESS_FORMAT_JAVASCRIPT);
}

#ifdef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libuv exports a number of similar functions. Before adding this to Node, do any of these work for your use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know, and I don’t have a Windows machine to try them out :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help verify. I didn't find any unicode utils in node, so was a bit surprised actually. Now it makes sense since it's from libuv.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with it. There are some boilerplates for that to work. One example how it can be used by node is this libuv's util function, which is, unfortunately, not exposed. Node would have to implement that or something similar, and pass in c_str pointers when calling it.

What we have here, IMHO, is actually more suitable for node as it's operates directly on std::string

We can certainly take this out to a util file, e.g. util.cc, to make it more proper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually make sense to move it to util.cc? Given that it's path-only, node_file.cc doesn't seem out of place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I was thinking about like libuv to have a windows specific file to place these, namely util_win.cc but that would be a different PR. I think it's fine to have these here for now.

std::wstring ConvertToWideString(const std::string& str) {
int size_needed = MultiByteToWideChar(
CP_UTF8, 0, &str[0], static_cast<int>(str.size()), nullptr, 0);
std::wstring wstrTo(size_needed, 0);
MultiByteToWideChar(CP_UTF8,
0,
&str[0],
static_cast<int>(str.size()),
&wstrTo[0],
size_needed);
return wstrTo;
}

#define BufferValueToPath(str) \
std::filesystem::path(ConvertToWideString(str.ToString()))

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();
Expand All @@ -3139,15 +3188,15 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kFileSystemRead, src.ToStringView());

auto src_path = std::filesystem::path(src.ToU8StringView());
auto src_path = BufferValueToPath(src);

BufferValue dest(isolate, args[1]);
CHECK_NOT_NULL(*dest);
ToNamespacedPath(env, &dest);
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView());

auto dest_path = std::filesystem::path(dest.ToU8StringView());
auto dest_path = BufferValueToPath(dest);
bool dereference = args[2]->IsTrue();
bool recursive = args[3]->IsTrue();

Expand Down Expand Up @@ -3176,47 +3225,41 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
(src_status.type() == std::filesystem::file_type::directory) ||
(dereference && src_status.type() == std::filesystem::file_type::symlink);

auto src_path_str = PathToString(src_path);
auto dest_path_str = PathToString(dest_path);

if (!error_code) {
// Check if src and dest are identical.
if (std::filesystem::equivalent(src_path, dest_path)) {
std::u8string message =
u8"src and dest cannot be the same " + dest_path.u8string();
return THROW_ERR_FS_CP_EINVAL(
env, reinterpret_cast<const char*>(message.c_str()));
std::string message = "src and dest cannot be the same %s";
return THROW_ERR_FS_CP_EINVAL(env, message.c_str(), dest_path_str);
}

const bool dest_is_dir =
dest_status.type() == std::filesystem::file_type::directory;

if (src_is_dir && !dest_is_dir) {
std::u8string message = u8"Cannot overwrite non-directory " +
src_path.u8string() + u8" with directory " +
dest_path.u8string();
std::string message =
"Cannot overwrite non-directory %s with directory %s";
return THROW_ERR_FS_CP_DIR_TO_NON_DIR(
env, reinterpret_cast<const char*>(message.c_str()));
env, message.c_str(), src_path_str, dest_path_str);
}

if (!src_is_dir && dest_is_dir) {
std::u8string message = u8"Cannot overwrite directory " +
dest_path.u8string() + u8" with non-directory " +
src_path.u8string();
std::string message =
"Cannot overwrite directory %s with non-directory %s";
return THROW_ERR_FS_CP_NON_DIR_TO_DIR(
env, reinterpret_cast<const char*>(message.c_str()));
env, message.c_str(), dest_path_str, src_path_str);
}
}

std::u8string dest_path_str = dest_path.u8string();
std::u8string src_path_str = src_path.u8string();
if (!src_path_str.ends_with(std::filesystem::path::preferred_separator)) {
src_path_str += std::filesystem::path::preferred_separator;
}
// Check if dest_path is a subdirectory of src_path.
if (src_is_dir && dest_path_str.starts_with(src_path_str)) {
std::u8string message = u8"Cannot copy " + src_path.u8string() +
u8" to a subdirectory of self " +
dest_path.u8string();
std::string message = "Cannot copy %s to a subdirectory of self %s";
return THROW_ERR_FS_CP_EINVAL(
env, reinterpret_cast<const char*>(message.c_str()));
env, message.c_str(), src_path_str, dest_path_str);
}

auto dest_parent = dest_path.parent_path();
Expand All @@ -3227,11 +3270,9 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
dest_parent.parent_path() != dest_parent) {
if (std::filesystem::equivalent(
src_path, dest_path.parent_path(), error_code)) {
std::u8string message = u8"Cannot copy " + src_path.u8string() +
u8" to a subdirectory of self " +
dest_path.u8string();
std::string message = "Cannot copy %s to a subdirectory of self %s";
return THROW_ERR_FS_CP_EINVAL(
env, reinterpret_cast<const char*>(message.c_str()));
env, message.c_str(), src_path_str, dest_path_str);
}

// If equivalent fails, it's highly likely that dest_parent does not exist
Expand All @@ -3243,29 +3284,23 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
}

if (src_is_dir && !recursive) {
std::u8string message =
u8"Recursive option not enabled, cannot copy a directory: " +
src_path.u8string();
return THROW_ERR_FS_EISDIR(env,
reinterpret_cast<const char*>(message.c_str()));
std::string message =
"Recursive option not enabled, cannot copy a directory: %s";
return THROW_ERR_FS_EISDIR(env, message.c_str(), src_path_str);
}

switch (src_status.type()) {
case std::filesystem::file_type::socket: {
std::u8string message = u8"Cannot copy a socket file: " + dest_path_str;
return THROW_ERR_FS_CP_SOCKET(
env, reinterpret_cast<const char*>(message.c_str()));
std::string message = "Cannot copy a socket file: %s";
return THROW_ERR_FS_CP_SOCKET(env, message.c_str(), dest_path_str);
}
case std::filesystem::file_type::fifo: {
std::u8string message = u8"Cannot copy a FIFO pipe: " + dest_path_str;
return THROW_ERR_FS_CP_FIFO_PIPE(
env, reinterpret_cast<const char*>(message.c_str()));
std::string message = "Cannot copy a FIFO pipe: %s";
return THROW_ERR_FS_CP_FIFO_PIPE(env, message.c_str(), dest_path_str);
}
case std::filesystem::file_type::unknown: {
std::u8string message =
u8"Cannot copy an unknown file type: " + dest_path_str;
return THROW_ERR_FS_CP_UNKNOWN(
env, reinterpret_cast<const char*>(message.c_str()));
std::string message = "Cannot copy an unknown file type: %s";
return THROW_ERR_FS_CP_UNKNOWN(env, message.c_str(), dest_path_str);
}
default:
break;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-cp.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ tmpdir.refresh();

let dirc = 0;
function nextdir(dirname) {
return tmpdir.resolve(dirname || `copy_${++dirc}`);
return tmpdir.resolve(dirname || `copy_%${++dirc}`);
}

// Synchronous implementation of copy.
Expand Down
Loading