Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from
+8 −4

Conversation

yamachu
Copy link

@yamachu yamachu commented Jan 19, 2025 •
edited
Loading

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

@yamachu
src: use std::u8string instead of std::string to pass path constructor
ceec5cc 
Enforce that it is u8 because it was crashing
when loading files containing Japanese in Windows, cp392 environment.

Fixes: nodejs#56650
@nodejs-github-bot 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 19, 2025
lpinca
lpinca approved these changes Jan 19, 2025
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@lpinca
Copy link
Member

lpinca commented Jan 19, 2025

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
Copy link

codecov bot commented Jan 19, 2025 •
edited
Loading

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 13 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
Copy link
Author

yamachu commented Jan 20, 2025

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
Copy link
Author

yamachu commented Jan 20, 2025 •
edited
Loading

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
Copy link
Member

lpinca commented Jan 20, 2025

Can you please fix the linting errors?

@yamachu
Copy link
Author

yamachu commented Jan 20, 2025

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

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2025
@nodejs-github-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segmentation Fault When Passing Paths with Japanese Characters to createRequire in Node.js 22+
5 participants
@yamachu @lpinca @nodejs-github-bot @jasnell @StefanStojanovic