src: use CP_UTF8 for wide file names on win32 #60575

indutny · 2025-11-04T20:57:09Z

src/node_modules.cc needs to be consistent with src/node_file.cc in how it translates the utf8 strings to std::wstring otherwise we might end up in situation where we can read the source code of imported package from disk, but fail to recognize that it is an ESM (or CJS) and cause runtime errors. This type of error is possible on Windows when the path contains unicode characters and "Language for non-Unicode programs" is set to "Chinese (Traditional, Taiwan)".

See: #58768

indutny · 2025-11-04T21:00:05Z

cc @joyeecheung

indutny · 2025-11-04T21:00:16Z

cc @nodejs/cpp-reviewers

`src/node_modules.cc` needs to be consistent with `src/node_file.cc` in how it translates the utf8 strings to `std::wstring` otherwise we might end up in situation where we can read the source code of imported package from disk, but fail to recognize that it is an ESM (or CJS) and cause runtime errors. This type of error is possible on Windows when the path contains unicode characters and "Language for non-Unicode programs" is set to "Chinese (Traditional, Taiwan)". See: nodejs#58768

RafaelGSS

Can we have test for this?

indutny · 2025-11-04T21:24:31Z

@RafaelGSS it is a good question, and I'd like to answer yes, but I'm afraid this is complicated to reproduce as it requires changing "Language for non-Unicode programs" to non-english locale so that GetACP() returns some other value. It is pretty straightforward to test manually on Windows, though:

Create a folder named ”炭治郎” (yay, Demon Slayer reference) and cd into it
npm init -y
npm install pino
Run node -p "require('internal/test/binding').internalBinding('modules').getNearestParentPackageJSON(require('path').join(process.cwd(), 'node_modules', 'pino', 'bin.js'))"
Verify that it doesn't print undefined
Change "Language for non-Unicode programs" to "Chinese (Traditional, Taiwan)"
Run node -p ... again and verify that the result is the same

indutny · 2025-11-04T21:25:19Z

I feel like this should be the safe change, though, since it moves to match src/node_file.cc.

indutny · 2025-11-04T21:31:23Z

Unrelated to above a backport to 22-x-y would be appreciated! (Although, the patch can't be a plain cherry-pick since there are two conversion callsites in 22-x-y)

codecov · 2025-11-04T22:04:21Z

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (71da1a8) to head (297b2b9).
⚠️ Report is 11 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60575      +/-   ##
==========================================
+ Coverage   88.54%   88.56%   +0.01%     
==========================================
  Files         704      704              
  Lines      208072   208092      +20     
  Branches    40076    40084       +8     
==========================================
+ Hits       184241   184293      +52     
+ Misses      15884    15831      -53     
- Partials     7947     7968      +21

Files with missing lines	Coverage Δ
src/compile_cache.cc	`80.78% <ø> (ø)`
src/node_file.cc	`75.60% <100.00%> (ø)`
src/util-inl.h	`81.57% <100.00%> (+0.37%)`	⬆️
src/util.h	`89.71% <ø> (ø)`
src/node_modules.cc	`76.16% <88.88%> (+0.17%)`	⬆️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

joyeecheung · 2025-11-05T10:55:30Z

src/node_modules.cc

-      return nullptr;
+    if (is_permissions_enabled) {
+      std::string generic_path;
+#ifdef _WIN32


I feel that the amount of ifdefs sprinkled everywhere is a bit cluttering; it looks like all new usages of these are on a std::filesystem::path, so technically they are just repeating what PathToString() does, I think we can just put PathToString to util.h and use that instead?

I agree. I think usage of generic_string vs string complicates it a bit, but let me see what I can do.

How about db29a1d? I tried to unify things as much as I could.

joyeecheung · 2025-11-05T10:58:28Z

src/node_modules.cc


 #ifdef _WIN32
-  std::wstring wide_path = ConvertToWideString(path_value_str, GetACP());
+  std::wstring wide_path = ConvertToWideString(path_value_str, CP_UTF8);


Ditto here - I think we can just create StringToPath and let BufferValueToPath use it, and here it can just use StringToPath

Addressed by db29a1d. I also dropped the code_point argument from the util method to make sure we don't use GetACP() by accident.

joyeecheung · 2025-11-05T11:04:49Z

FWIW it doesn't look like GHA supports locale setting https://github.com/orgs/community/discussions/68929 and I doubt that we can afford one more Windows setup on Jenkins. I don't have a Windows machine with me right now but I can confirm #60575 (comment) is generally how you reproduce this types of issues on Windows (it's relatively common to see programs misbehave on CJK locale, but crashing is unusual).

joyeecheung

LGTM with a suggestion

src/util.h

Co-authored-by: Joyee Cheung <[email protected]>

indutny · 2025-11-06T16:05:04Z

Let me know if anything else is needed before the merge! (It sounds like all LGTMs that this PR got a stale now because of addressing PR feedback so I can't merge it).

cc @joyeecheung

nodejs-github-bot · 2025-11-06T18:44:13Z

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

indutny · 2025-11-06T20:02:58Z

Looks like CI is green, whew 🥲

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 4, 2025

indutny force-pushed the fix/esm-cp-utf8 branch 2 times, most recently from 13d23c9 to 440a6be Compare November 4, 2025 20:59

indutny force-pushed the fix/esm-cp-utf8 branch from 440a6be to ec1fc03 Compare November 4, 2025 21:03

RafaelGSS reviewed Nov 4, 2025

View reviewed changes

addaleax approved these changes Nov 4, 2025

View reviewed changes

RaisinTen approved these changes Nov 5, 2025

View reviewed changes

joyeecheung reviewed Nov 5, 2025

View reviewed changes

StefanStojanovic approved these changes Nov 5, 2025

View reviewed changes

indutny added 3 commits November 5, 2025 09:28

address feedback

db29a1d

fix windows

8d686c3

fix one more windows error

f620bb4

juanarbol approved these changes Nov 5, 2025

View reviewed changes

joyeecheung approved these changes Nov 5, 2025

View reviewed changes

src/util.h Show resolved Hide resolved

Update src/util.h

297b2b9

Co-authored-by: Joyee Cheung <[email protected]>

indutny-signal mentioned this pull request Nov 6, 2025

Signal Desktop fails to start due to "Cannot use import statement outside a module" on Windows 11 25H2 signalapp/Signal-Desktop#7547

Open

2 tasks

joyeecheung approved these changes Nov 6, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2025

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

RafaelGSS approved these changes Nov 6, 2025

View reviewed changes

Uh oh!

src: use CP_UTF8 for wide file names on win32 #60575

Are you sure you want to change the base?

src: use CP_UTF8 for wide file names on win32 #60575

Conversation

indutny commented Nov 4, 2025

Uh oh!

indutny commented Nov 4, 2025

Uh oh!

indutny commented Nov 4, 2025

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

indutny commented Nov 4, 2025

Uh oh!

indutny commented Nov 4, 2025

Uh oh!

indutny commented Nov 4, 2025

Uh oh!

codecov bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joyeecheung Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

indutny Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

indutny Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

indutny Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 5, 2025

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

indutny commented Nov 6, 2025

Uh oh!

nodejs-github-bot commented Nov 6, 2025

Uh oh!

indutny commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Nov 4, 2025 •

edited

Loading