-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed an issue that caused a segmentation fault when resolving a module in certain environments #56657
base: main
Are you sure you want to change the base?
Conversation
Enforce that it is u8 because it was crashing when loading files containing Japanese in Windows, cp392 environment. Fixes: nodejs#56650
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
I guess there is no easy way to add a test, right? |
Codecov ReportAttention: Patch coverage is
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
|
Yes. |
I have succeeded in reproducing the Japanese environment Windows on CI. 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... |
Can you please fix the linting errors? |
@lpinca I ran make test but forgot to run make lint, sorry. |
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...