fs: fix crash when path contains `%` #55171

aduh95 · 2024-09-30T09:17:27Z

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

src/node_file.cc

codecov · 2024-10-04T16:06:13Z

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 · 2024-10-07T22:37:43Z

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

nodejs-github-bot · 2024-10-09T17:24:31Z

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

aduh95 · 2024-10-11T16:34:32Z

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

cjihrig · 2024-10-11T16:46:21Z

src/node_file.cc

@@ -3127,6 +3127,44 @@ static void GetFormatOfExtensionlessFile(
  return args.GetReturnValue().Set(EXTENSIONLESS_FORMAT_JAVASCRIPT);
 }

+#ifdef _WIN32


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

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

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.

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.

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.

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 · 2024-10-17T10:40:48Z

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

nodejs-github-bot · 2024-10-17T23:11:25Z

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

jazelly · 2024-10-18T00:24:54Z

Thanks for pushing this through 👍 Looks much nicer.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

aduh95 · 2024-10-18T07:10:50Z

Landed in 56e5bd8

aduh95 · 2024-10-18T07:10:54Z

Landed in 56e5bd8

ShenHongFei · 2024-10-18T08:51:11Z

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 · 2024-10-18T13:03:08Z

@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.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 30, 2024

jazelly reviewed Sep 30, 2024

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

aduh95 force-pushed the fix-fs-percent-crash branch from 1f24267 to 6c5b1c2 Compare October 4, 2024 14:51

aduh95 force-pushed the fix-fs-percent-crash branch from 6c5b1c2 to de93001 Compare October 7, 2024 08:49

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024

aduh95 requested a review from targos October 8, 2024 09:51

aduh95 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 11, 2024

cjihrig reviewed Oct 11, 2024

View reviewed changes

aduh95 added review wanted PRs that need reviews. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 17, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024

RafaelGSS approved these changes Oct 17, 2024

View reviewed changes

fs: use wstring on Windows paths

56e5bd8

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

aduh95 force-pushed the fix-fs-percent-crash branch from 3068487 to 56e5bd8 Compare October 18, 2024 07:08

aduh95 merged commit 56e5bd8 into nodejs:main Oct 18, 2024
20 checks passed

aduh95 deleted the fix-fs-percent-crash branch October 18, 2024 07:10

This comment was marked as outdated.

Sign in to view

aduh95 added a commit that referenced this pull request Oct 19, 2024

fs: use wstring on Windows paths

d9494a7

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

aduh95 mentioned this pull request Oct 24, 2024

V23.1.0 proposal #55513

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

fs: use wstring on Windows paths

3cc9a1f

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#55171 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix crash when path contains `%` #55171

fs: fix crash when path contains `%` #55171

aduh95 commented Sep 30, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading

nodejs-github-bot commented Oct 7, 2024

nodejs-github-bot commented Oct 9, 2024

aduh95 commented Oct 11, 2024

cjihrig Oct 11, 2024

aduh95 Oct 11, 2024

jazelly Oct 12, 2024

jazelly Oct 13, 2024

aduh95 Oct 14, 2024

jazelly Oct 14, 2024

nodejs-github-bot commented Oct 17, 2024

nodejs-github-bot commented Oct 17, 2024

jazelly commented Oct 18, 2024

aduh95 commented Oct 18, 2024

aduh95 commented Oct 18, 2024

ShenHongFei commented Oct 18, 2024

This comment was marked as outdated.

jazelly commented Oct 18, 2024

fs: fix crash when path contains % #55171

fs: fix crash when path contains % #55171

Conversation

aduh95 commented Sep 30, 2024

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Oct 7, 2024

nodejs-github-bot commented Oct 9, 2024

aduh95 commented Oct 11, 2024

cjihrig Oct 11, 2024

Choose a reason for hiding this comment

aduh95 Oct 11, 2024

Choose a reason for hiding this comment

jazelly Oct 12, 2024

Choose a reason for hiding this comment

jazelly Oct 13, 2024

Choose a reason for hiding this comment

aduh95 Oct 14, 2024

Choose a reason for hiding this comment

jazelly Oct 14, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 17, 2024

nodejs-github-bot commented Oct 17, 2024

jazelly commented Oct 18, 2024

aduh95 commented Oct 18, 2024

aduh95 commented Oct 18, 2024

ShenHongFei commented Oct 18, 2024

This comment was marked as outdated.

jazelly commented Oct 18, 2024

fs: fix crash when path contains `%` #55171

fs: fix crash when path contains `%` #55171

codecov bot commented Oct 4, 2024 •

edited

Loading