fs: fix crash when path contains %

[aduh95]

Without this patch, here's the crash:

$ tools/test.py test/parallel/test-fs-cp.mjs
=== release test-fs-cp ===                   
Path: parallel/test-fs-cp
#  out/Release/node[58279]: std::string node::SPrintFImpl(const char *) at ../../src/debug_utils-inl.h:71
  #  Assertion failed: (p[1]) == ('%')

----- Native stack trace -----

 1: 0x100476754 node::DumpNativeBacktrace(__sFILE*) [/Users/duhamean/Documents/node/out/Release/node]
 2: 0x1005d7b6c node::Assert(node::AssertionInfo const&) [/Users/duhamean/Documents/node/out/Release/node]
 3: 0x1003b0e94 node::SPrintFImpl(char const*) [/Users/duhamean/Documents/node/out/Release/node]
 4: 0x1026c940c std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> node::SPrintF<>(char const*) [/Users/duhamean/Documents/node/out/Release/node]
 5: 0x100604c50 v8::Local<v8::Object> node::ERR_FS_CP_EINVAL<>(v8::Isolate*, char const*) [/Users/duhamean/Documents/node/out/Release/node]
 6: 0x100604c00 void node::THROW_ERR_FS_CP_EINVAL<>(v8::Isolate*, char const*) [/Users/duhamean/Documents/node/out/Release/node]
 7: 0x100602798 void node::THROW_ERR_FS_CP_EINVAL<>(node::Environment*, char const*) [/Users/duhamean/Documents/node/out/Release/node]
 8: 0x1005f5680 node::fs::CpSyncCheckPaths(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/duhamean/Documents/node/out/Release/node]
 9: 0x10162a978 Builtins_CallApiCallbackGeneric [/Users/duhamean/Documents/node/out/Release/node]
10: 0x10643717c 
11: 0x106435de4 
12: 0x101628838 Builtins_InterpreterEntryTrampoline [/Users/duhamean/Documents/node/out/Release/node]
13: 0x10643baec 
14: 0x10643b7fc 
15: 0x101628838 Builtins_InterpreterEntryTrampoline [/Users/duhamean/Documents/node/out/Release/node]
16: 0x1016cb084 Builtins_AsyncModuleEvaluate [/Users/duhamean/Documents/node/out/Release/node]
17: 0x10162650c Builtins_JSEntryTrampoline [/Users/duhamean/Documents/node/out/Release/node]
18: 0x1016261b0 Builtins_JSEntry [/Users/duhamean/Documents/node/out/Release/node]
19: 0x100b853d4 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/duhamean/Documents/node/out/Release/node]
20: 0x100b85c34 v8::internal::(anonymous namespace)::InvokeWithTryCatch(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/duhamean/Documents/node/out/Release/node]
21: 0x100b85d00 v8::internal::Execution::TryCall(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Execution::MessageHandling, v8::internal::MaybeHandle<v8::internal::Object>*) [/Users/duhamean/Documents/node/out/Release/node]
22: 0x100f38544 v8::internal::SourceTextModule::ExecuteAsyncModule(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SourceTextModule>) [/Users/duhamean/Documents/node/out/Release/node]
23: 0x100f3807c v8::internal::SourceTextModule::InnerModuleEvaluation(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SourceTextModule>, v8::internal::ZoneForwardList<v8::internal::Handle<v8::internal::SourceTextModule>>*, unsigned int*) [/Users/duhamean/Documents/node/out/Release/node]
24: 0x100f37b28 v8::internal::SourceTextModule::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SourceTextModule>) [/Users/duhamean/Documents/node/out/Release/node]
25: 0x100ef38b8 v8::internal::Module::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Module>) [/Users/duhamean/Documents/node/out/Release/node]
26: 0x100a10b50 v8::Module::Evaluate(v8::Local<v8::Context>) [/Users/duhamean/Documents/node/out/Release/node]
27: 0x1005274fc node::loader::ModuleWrap::Evaluate(v8::FunctionCallbackInfo<v8::Value> const&)::$_0::operator()() const [/Users/duhamean/Documents/node/out/Release/node]
28: 0x1005272f0 node::loader::ModuleWrap::Evaluate(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/duhamean/Documents/node/out/Release/node]
29: 0x10162a978 Builtins_CallApiCallbackGeneric [/Users/duhamean/Documents/node/out/Release/node]
30: 0x101628838 Builtins_InterpreterEntryTrampoline [/Users/duhamean/Documents/node/out/Release/node]
31: 0x101665e20 Builtins_AsyncFunctionAwaitResolveClosure [/Users/duhamean/Documents/node/out/Release/node]
32: 0x101733298 Builtins_PromiseFulfillReactionJob [/Users/duhamean/Documents/node/out/Release/node]
33: 0x101655214 Builtins_RunMicrotasks [/Users/duhamean/Documents/node/out/Release/node]
34: 0x1016263f0 Builtins_JSRunMicrotasksEntry [/Users/duhamean/Documents/node/out/Release/node]
35: 0x100b85394 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/duhamean/Documents/node/out/Release/node]
36: 0x100b85c34 v8::internal::(anonymous namespace)::InvokeWithTryCatch(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/duhamean/Documents/node/out/Release/node]
37: 0x100b85d6c v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) [/Users/duhamean/Documents/node/out/Release/node]
38: 0x100bb37c0 v8::internal::MicrotaskQueue::RunMicrotasks(v8::internal::Isolate*) [/Users/duhamean/Documents/node/out/Release/node]
39: 0x100bb4064 v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) [/Users/duhamean/Documents/node/out/Release/node]
40: 0x1003a9b88 node::InternalCallbackScope::Close() [/Users/duhamean/Documents/node/out/Release/node]
41: 0x1003a9964 node::InternalCallbackScope::~InternalCallbackScope() [/Users/duhamean/Documents/node/out/Release/node]
42: 0x1003a9300 node::InternalCallbackScope::~InternalCallbackScope() [/Users/duhamean/Documents/node/out/Release/node]
43: 0x1005e05b8 node::fs::FileHandle::CloseReq::Resolve() [/Users/duhamean/Documents/node/out/Release/node]
44: 0x1005fc280 node::fs::FileHandle::ClosePromise()::$_0::operator()(uv_fs_s*) const [/Users/duhamean/Documents/node/out/Release/node]
45: 0x1005fc03c node::fs::FileHandle::ClosePromise()::$_0::__invoke(uv_fs_s*) [/Users/duhamean/Documents/node/out/Release/node]
46: 0x1005c974c node::MakeLibuvRequestCallback<uv_fs_s, void (*)(uv_fs_s*)>::Wrapper(uv_fs_s*) [/Users/duhamean/Documents/node/out/Release/node]
47: 0x101602628 uv__work_done [/Users/duhamean/Documents/node/out/Release/node]
48: 0x1016060d8 uv__async_io [/Users/duhamean/Documents/node/out/Release/node]
49: 0x10161908c uv__io_poll [/Users/duhamean/Documents/node/out/Release/node]
50: 0x101606640 uv_run [/Users/duhamean/Documents/node/out/Release/node]
51: 0x1003acd3c node::SpinEventLoopInternal(node::Environment*) [/Users/duhamean/Documents/node/out/Release/node]
52: 0x100662380 node::NodeMainInstance::Run(node::ExitCode*, node::Environment*) [/Users/duhamean/Documents/node/out/Release/node]
53: 0x100661fe8 node::NodeMainInstance::Run() [/Users/duhamean/Documents/node/out/Release/node]
54: 0x10053ab44 node::StartInternal(int, char**) [/Users/duhamean/Documents/node/out/Release/node]
55: 0x10053a760 node::Start(int, char**) [/Users/duhamean/Documents/node/out/Release/node]
56: 0x101a3a1c4 main [/Users/duhamean/Documents/node/out/Release/node]
57: 0x187af0274 start [/usr/lib/dyld]

----- JavaScript stack trace -----

1: cpSyncFn (node:internal/fs/cp/cp-sync:56:13)
2: cpSync (node:fs:3047:3)
3: assert.throws.code (file:///Users/duhamean/Documents/node/test/parallel/test-fs-cp.mjs:276:11)
4: getActual (node:assert:498:5)
5: throws (node:assert:644:24)
6: file:///Users/duhamean/Documents/node/test/parallel/test-fs-cp.mjs:275:10
7: run (node:internal/modules/esm/module_job:268:25)
Command: out/Release/node /Users/duhamean/Documents/node/test/parallel/test-fs-cp.mjs
--- CRASHED (Signal: 6) ---


[00:00|% 100|+   0|-   1]: Done        

[codecov]

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base (9a9409f) to head (3068487).
Report is 86 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	81.81%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55171      +/-   ##
==========================================
+ Coverage   88.41%   88.42%   +0.01%     
==========================================
  Files         652      652              
  Lines      186796   186901     +105     
  Branches    36103    36072      -31     
==========================================
+ Hits       165154   165266     +112     
+ Misses      14909    14889      -20     
- Partials     6733     6746      +13     

Files with missing lines	Coverage Δ
src/node_file.cc	77.29% <81.81%> (-0.05%)	⬇️

... and 93 files with indirect coverage changes

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/62980/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63014/

[aduh95]

/cc @nodejs/fs can I get some reviews?

[cjihrig]

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

[aduh95]

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

[jazelly]

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.

[jazelly]

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.

[aduh95]

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.

[jazelly]

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.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63157/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63168/

[jazelly]

Thanks for pushing this through 👍 Looks much nicer.

[aduh95]

Landed in 56e5bd8

[aduh95]

Landed in 56e5bd8

[ShenHongFei]

Write auto src_path = std::filesystem::path(src.ToStringView()); and use string to represent it uniformly, without converting between utf-8 and utf-16

After compiling, The test (test/parallel/test-fs-cp.mjs) ran fine on my local Windows machine. I tried both of the following methods

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

and

function nextdir(dirname) {
  return tmpdir.resolve(dirname || `copy_%_asdfasdf中文测试👽${++dirc}`);
}

The following issue has also been tested and no problem
#54476

@jazelly Could you try this instead?

[jazelly]

@ShenHongFei please ignore my previous comment, I applied the diff and got segfault at std::filesystem::path(src.ToStringView()), which had been identified in my previous comment.

diff --git a/src/node_file.cc b/src/node_file.cc
index dfdc25fd1d..a1dfc2c673 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -3188,7 +3188,7 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
   THROW_IF_INSUFFICIENT_PERMISSIONS(
       env, permission::PermissionScope::kFileSystemRead, src.ToStringView());

-  auto src_path = BufferValueToPath(src);
+  auto src_path = std::filesystem::path(src.ToStringView());

   BufferValue dest(isolate, args[1]);
   CHECK_NOT_NULL(*dest);
@@ -3196,7 +3196,7 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
   THROW_IF_INSUFFICIENT_PERMISSIONS(
       env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView());

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

@@ -3225,8 +3225,8 @@ 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);
+  auto src_path_str = src_path.string();
+  auto dest_path_str = dest_path.string();

   if (!error_code) {
     // Check if src and dest are identical.