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

[yamachu]

Fix: #56650

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

Caution

It cannot be reproduced by using chcp command in a Windows environment, and it is necessary to change the locale in the OS settings...

[lpinca]

RSLGTM

[lpinca]

It cannot be reproduced by using chcp command in a Windows environment, and it is necessary to change the locale in the OS settings...

I guess there is no easy way to add a test, right?

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (009d53e) to head (bc1a91e).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_modules.cc	83.33%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #56657 +/- ## ========================================== - Coverage 89.21% 89.21% -0.01%  ========================================== Files 662 662 Lines 191893 191895 +2 Branches 36931 36928 -3 ========================================== - Hits 171196 171191 -5 - Misses 13541 13546 +5 - Partials 7156 7158 +2 

Files with missing lines	Coverage Δ
src/node_modules.cc	78.91% <83.33%> (-0.19%)	⬇️

... and 17 files with indirect coverage changes

[yamachu]

Yes.
Maybe I'm just not familiar with Windows commands, but I think the only way is through GUI operation.
This makes it very difficult to run test.

[yamachu]

I have succeeded in reproducing the Japanese environment Windows on CI.
I also ran it on default Windows and Japanese environment Windows and confirmed that only the Japanese environment failed.

https://github.com/yamachu/node-require-japanese-repro/actions/runs/12861537170

To prepare a test using this setup in the node repository, I also need a node build workflow for Windows, which is difficult for me...

[lpinca]

Can you please fix the linting errors?

[yamachu]

@lpinca I ran make test but forgot to run make lint, sorry.
I pushed the formatted file again.

[yamachu]

Thank you for the review.
I noticed that the test regarding unicode was failing in the CI for coverage-windows, so I fixed it in another PR.
Therefore, I will close this PR and take a different approach.

Since I was able to create a ja-JP environment with CI, I performed tests using it.
The summary of the test is as follows.

Default Windows runner
Before https://github.com/yamachu/node/actions/runs/12890202110/job/35939236599#step:7:17013
After https://github.com/yamachu/node/actions/runs/12890008746/job/35938588016#step:7:17010

 Failed tests: D:\a\node\node\out\Release\node.exe --experimental-vm-modules --max-old-space-size=16 --trace-gc D:\a\node\node\test\es-module\test-vm-source-text-module-leak.js D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-child-process-exec-any-shells-windows.js D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-inspector-wait-for-connection.js + D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-require-unicode.js D:\a\node\node\out\Release\node.exe --expose-internals D:\a\node\node\test\pummel\test-heapdump-inspector.js

SJIS Windows runner
Before https://github.com/yamachu/node/actions/runs/12890202110/job/35939236599#step:12:16851
After https://github.com/yamachu/node/actions/runs/12890008746/job/35938588016#step:12:16853

 Failed tests: D:\a\node\node\out\Release\node.exe --experimental-vm-modules --max-old-space-size=16 --trace-gc D:\a\node\node\test\es-module\test-vm-source-text-module-leak.js D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-child-process-exec-any-shells-windows.js - D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-module-create-require-multibyte.js D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-require-unicode.js D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-inspector-wait-for-connection.js D:\a\node\node\out\Release\node.exe --expose-internals D:\a\node\node\test\pummel\test-heapdump-inspector.js

[nodejs-github-bot]

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

[lpinca]

@yamachu is this superseded by #56696?

[yamachu]

@lpinca Yes.
I have found that this approach did not solve this issue and I'm fixing it in #56696.

I will close this PR for that purpose.
Thank you for your review.