Repository: nodejs/node
Pull request: fs: fix crash when path contains `%`

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        
````
<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: src/node_file.cc
patch: @@ -3127,6 +3127,55 @@ static void GetFormatOfExtensionlessFile(
   return args.GetReturnValue().Set(EXTENSIONLESS_FORMAT_JAVASCRIPT);
 }
 
+#ifdef _WIN32
+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();
@@ -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();
 
@@ -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();
@@ -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
@@ -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;

filename: test/parallel/test-fs-cp.mjs
patch: @@ -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.