src: fix generation of path objects in Windows #56696

yamachu · 2025-01-22T11:36:15Z

This PR fixes a problem that caused a segmentation fault in module resolution when creating a require object with multibyte characters in a non-English environment.

Since the issue occurred when generating a std::filesystem::path object, some patches were applied to the changed areas in the following PR.

a7dad43

Since the issue was reproduced only in different locales, such as ja-JP on Windows, I rewrote the locale in the CI of coverage-windows to run the test.
https://github.com/yamachu/node/pull/2/files#diff-29094741d50149aa772b3e577ad509116bad722ad2de85689b6cb2c01e806a46

.github/workflows/coverage-windows.yml

+      - name: Change locale ja-JP for testing on SJIS environment
+        run: Set-WinSystemLocale -SystemLocale "ja-JP"
+      # to avoid configure, nobuild and noprojgen is needed
+      - name: Test on SJIS environment
+        run: ./vcbuild.bat nobuild noprojgen test-ci-js; node -e 'process.exit(0)'
+        env:
+          NODE_V8_COVERAGE: ./coverage/tmp

The previous PR did not solve the problem when using unicode, so the process was separated for windows and the path object was generated.

The part where std::filesystem::path is used can be fixed by changing this PR.

The results of the test conducted in a Japanese environment can be seen below.
https://github.com/yamachu/node/actions/runs/12903928231/job/35980111577#step:12:16844

codecov · 2025-01-22T13:32:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.22%. Comparing base (d978610) to head (24ac7b9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56696   +/-   ##
=======================================
  Coverage   89.21%   89.22%           
=======================================
  Files         662      662           
  Lines      191945   191950    +5     
  Branches    36948    36950    +2     
=======================================
+ Hits       171238   171258   +20     
+ Misses      13549    13542    -7     
+ Partials     7158     7150    -8

Files with missing lines	Coverage Δ
src/node_modules.cc	`78.91% <100.00%> (+0.13%)`	⬆️

... and 25 files with indirect coverage changes

anonrig

A similar already landed last August: #54653

We should use ToU8StringView() rather than introducing a new API.

yamachu · 2025-01-23T01:07:41Z

@anonrig

Thanks for the review!
Does this mean in short that I should use std::u8string?

However, there is a problem with the u8string approach.
As I show in the description, the test-require-unicode.js test fails when I run code using u8string in SJIS and en-US（en-US...? e.g. GHA Windows Runner default locale）environment on Windows (so affected ALL WINDOWS).

Therefore, I provided and used an API to handle wstrings without using ToU8StringView.

anonrig · 2025-01-23T01:10:35Z

I recommend improving existing solution to fit both cases rather than implementing a new solution.

yamachu · 2025-01-23T01:37:15Z

What are "both cases" presented here?

I am sure that ToU8StringView is not used in current codebase.
Since it is not being used, it does not currently solve the problem.
It is not a solution because I know that using it again will cause problems.

I don't think that trying hard to use u8string in path is a better way to go....

yamachu · 2025-01-23T11:05:22Z

The base branch is still bf59539 , so the associated test seems to be fail. (experimental flag)
Should I rebase the branch and force push?

lpinca · 2025-01-23T11:51:06Z

Should I rebase the branch and force push?

Yes, thank you.

nodejs-github-bot · 2025-01-23T13:26:41Z

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

yamachu · 2025-01-23T13:57:37Z

I had to rush to write my comments before work.
I apologize if any of my comments offended you.

I have rebased and force pushed the file, so please check with CI.

I would have liked to have been able to do this beforehand, but I looked over the entire code.
There I found the following helper functions.

node/src/node_file.cc

Lines 3149 to 3160 in d978610

    
           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; 
        
           }

This was exactly what I was looking for.
I rewrote what I wrote in this PR and added the CP settings and the test passed(on SJIS Windows environment).

yamachu · 2025-01-23T14:39:24Z

src/node_modules.cc

@@ -326,6 +330,22 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
  return nullptr;
 }

+#ifdef _WIN32
+std::wstring ConvertToWideString(const std::string& str) {
+  auto cp = GetACP();


This section was copied from node_file.cc, but this GetACP is particularly important.
As shown in the description, when the Windows locale is changed and executed, if the original UTF-8 code is left here, the strings cannot be handled properly.

I wonder if this utility function can be moved to util-inl.h? so that the same implementation (taking into account GetACP) is used both here and in node_file.cc without duplication. I think this is also what @anonrig is requesting.

If we do the same for node_fs, test-fs-cp.mjs will falil...

It is very strange, but I have found that if I run the failed path(utf/~~~) in addition to the test-require-~ , it can be handled correctly.
It may be that the pre or post-process (if there is one) regarding path generation in node_fs is having an effect.

Therefore, this time I confined the implementation within node_modules instead of applying it to the whole system.

The following is the error log when applied to the entire system.
https://github.com/yamachu/node/actions/runs/12941644792/job/36098035649?pr=3#step:7:5033

code diff
yamachu@0041793

I was thinking about making cp a function parameter and use CP_UTF8 in node_file.cc and GetACP() here. Does it make sense?

Ah, I see.
Do you mean to make these changes?

util-inl.h

#ifdef _WIN32 inline std::wstring ConvertToWideString(const std::string& str, UINT cp) { int size_needed = MultiByteToWideChar( cp, 0, &str[0], static_cast<int>(str.size()), nullptr, 0); std::wstring wstrTo(size_needed, 0); MultiByteToWideChar( cp, 0, &str[0], static_cast<int>(str.size()), &wstrTo[0], size_needed); return wstrTo; } #endif // _WIN32

node_file.cc

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

I thought that this implementation would certainly have less impact and could be implemented with less code duplication.

Yes, exactly. FWIW, I'm also ok with this as is.

apply 👍 : ef0ade1

yamachu · 2025-01-23T18:29:47Z

https://github.com/yamachu/node/actions/runs/12933259601/job/36071441238#step:12:16876

Test has passed on SJIS Windows

lpinca

RSLGTM

lpinca · 2025-01-23T20:22:48Z

Can you please squash the last 3 commits into the second or everything into one?

yamachu · 2025-01-24T04:09:17Z

I squashed and added a test case for the newly found problem.

take a similar approach to node_file and allow the creation of paths code point must be specified to convert from wchar_t to utf8.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 22, 2025

jasnell approved these changes Jan 22, 2025

View reviewed changes

yamachu mentioned this pull request Jan 22, 2025

Fixed an issue that caused a segmentation fault when resolving a module in certain environments #56657

Closed

anonrig requested changes Jan 23, 2025

View reviewed changes

yamachu force-pushed the fix-windows-path-string branch from 0697f02 to cba8830 Compare January 23, 2025 12:57

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2025

yamachu commented Jan 23, 2025

View reviewed changes

lpinca approved these changes Jan 23, 2025

View reviewed changes

yamachu force-pushed the fix-windows-path-string branch from 24ac7b9 to ea943d8 Compare January 24, 2025 04:07

yamachu force-pushed the fix-windows-path-string branch from ea943d8 to 4ddd7ae Compare January 24, 2025 09:33

yamachu added 2 commits January 24, 2025 19:05

test: added test that uses multibyte for path and resolves modules

846537f

src: fix to generate path from wchar_t via wstring.

ef0ade1

take a similar approach to node_file and allow the creation of paths code point must be specified to convert from wchar_t to utf8.

yamachu force-pushed the fix-windows-path-string branch from 4ddd7ae to ef0ade1 Compare January 24, 2025 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix generation of path objects in Windows #56696

src: fix generation of path objects in Windows #56696

yamachu commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

anonrig left a comment

yamachu commented Jan 23, 2025 •

edited

Loading

anonrig commented Jan 23, 2025

yamachu commented Jan 23, 2025 •

edited

Loading

yamachu commented Jan 23, 2025

lpinca commented Jan 23, 2025

nodejs-github-bot commented Jan 23, 2025

yamachu commented Jan 23, 2025 •

edited

Loading

yamachu Jan 23, 2025

lpinca Jan 23, 2025 •

edited

Loading

yamachu Jan 24, 2025

lpinca Jan 24, 2025 •

edited

Loading

yamachu Jan 24, 2025

lpinca Jan 24, 2025 •

edited

Loading

yamachu Jan 24, 2025 •

edited

Loading

yamachu commented Jan 23, 2025

lpinca left a comment

lpinca commented Jan 23, 2025

yamachu commented Jan 24, 2025

src: fix generation of path objects in Windows #56696

Are you sure you want to change the base?

src: fix generation of path objects in Windows #56696

Conversation

yamachu commented Jan 22, 2025 • edited Loading

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

anonrig left a comment

Choose a reason for hiding this comment

yamachu commented Jan 23, 2025 • edited Loading

anonrig commented Jan 23, 2025

yamachu commented Jan 23, 2025 • edited Loading

yamachu commented Jan 23, 2025

lpinca commented Jan 23, 2025

nodejs-github-bot commented Jan 23, 2025

yamachu commented Jan 23, 2025 • edited Loading

yamachu Jan 23, 2025

Choose a reason for hiding this comment

lpinca Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

yamachu Jan 24, 2025

Choose a reason for hiding this comment

lpinca Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

yamachu Jan 24, 2025

Choose a reason for hiding this comment

lpinca Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

yamachu Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

yamachu commented Jan 23, 2025

lpinca left a comment

Choose a reason for hiding this comment

lpinca commented Jan 23, 2025

yamachu commented Jan 24, 2025

yamachu commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

yamachu commented Jan 23, 2025 •

edited

Loading

yamachu commented Jan 23, 2025 •

edited

Loading

yamachu commented Jan 23, 2025 •

edited

Loading

lpinca Jan 23, 2025 •

edited

Loading

lpinca Jan 24, 2025 •

edited

Loading

lpinca Jan 24, 2025 •

edited

Loading

yamachu Jan 24, 2025 •

edited

Loading