src: revert filesystem::path changes#55015

anonrig · 2024-09-19T17:12:38Z

Reverts #53063. There are still some filesystem::path references left, since they're added in multiple different PRs. Depending on whether we need them or not, we can remove them.

We're reverting because it fixes multiple different regressions due to Windows UTF-16 path requirements.

Fixes#54991
Fixes#54476

nodejs-github-bot · 2024-09-19T17:12:44Z

Review requested:

@nodejs/security-wg

anonrig · 2024-09-19T17:21:46Z

@joyeecheung Can you double check the changes on compile_cache.cc and header file? I manually had to change those lines, there was a conflict.

anonrig · 2024-09-19T17:22:44Z

src/permission/fs_permission.cc

-auto file_status = std::filesystem::status(path);
-if (file_status.type() == std::filesystem::file_type::directory){
- path /= "*";
+uv_fs_t req;


@joyeecheung Is it safe to keep this function, or do we need this revert?

Is std::filesystem::path(res) correct? From what I can tell, res contains UTF8-encoded data, and it's about to be passed into uv_fs_stat which also expects UTF8-encoded data. So std::filesystem::path(res) here is also going to crash on Windows when res contains code points that are encoded differently in UTF16 unless it's casted between u8string both ways.

avivkeller · 2024-09-19T17:31:06Z

Is it possible to add a test for those issues?

codecov · 2024-09-19T19:03:12Z

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (291d90a) to head (ba26f0b).
Report is 461 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #55015 +/- ## ======================================= Coverage 88.04% 88.04% ======================================= Files 652 652 Lines 183764 183760 -4 Branches 35862 35863 +1 ======================================= Hits 161787 161787 + Misses 15233 15230 -3 + Partials 6744 6743 -1

Files with missing lines	Coverage Δ
src/compile_cache.cc	`79.59% <100.00%> (-0.41%)`	⬇️
src/compile_cache.h	`100.00% <100.00%> (ø)`
src/inspector_profiler.cc	`78.79% <100.00%> (+0.26%)`	⬆️
src/node_file.cc	`77.03% <100.00%> (-0.02%)`	⬇️
src/node_report.cc	`93.15% <100.00%> (ø)`
src/path.cc	`67.39% <100.00%> (ø)`
src/permission/fs_permission.cc	`73.22% <100.00%> (+1.68%)`	⬆️
src/permission/fs_permission.h	`91.80% <100.00%> (ø)`
src/util.h	`89.91% <ø> (ø)`
src/env.cc	`85.58% <50.00%> (-0.02%)`	⬇️
... and 1 more

... and 29 files with indirect coverage changes

nodejs-github-bot · 2024-09-19T19:07:01Z

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

nodejs-github-bot · 2024-09-20T01:34:58Z

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

joyeecheung

LGTM % nits

joyeecheung · 2024-09-20T09:01:05Z

src/compile_cache.cc

- std::u8string cache_dir_with_tag_u8 = cache_dir_with_tag.u8string();
- std::string cache_dir_with_tag_str(cache_dir_with_tag_u8.begin(),
- cache_dir_with_tag_u8.end());
+ std::string cache_dir_with_tag =


You can minimize the diff/potential conflicts if this is named as cache_dir_with_tag_str

joyeecheung · 2024-09-20T09:09:07Z

By the way I don't think this fixes #54476 because fs.cpSync changes were done in #53614 and #54653 is fixing that with the casting, as the path is going to be consumed by std::filesystem::create_directories at the end.

nodejs-github-bot · 2024-09-21T17:22:23Z

Landed in 5a96671

PR-URL: #55015 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs/node#55015

* chore: bump node in DEPS to v22.11.0 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

nodejs/node#55015

* chore: bump node in DEPS to v22.13.0 * chore: bump node in DEPS to v22.13.1 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

anonrig requested review from joyeecheung and richardlau September 19, 2024 17:12

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 Sep 19, 2024

anonrig force-pushed the revert-filesystem-path branch 3 times, most recently from eea22f7 to 37bd2dcCompare September 19, 2024 17:17

benjamingr approved these changes Sep 19, 2024
View reviewed changes

anonrig force-pushed the revert-filesystem-path branch 2 times, most recently from be2b0b8 to b0e83d9Compare September 19, 2024 17:21

anonrig commented Sep 19, 2024
View reviewed changes

anonrig force-pushed the revert-filesystem-path branch from b0e83d9 to 67ddaa9Compare September 19, 2024 17:23

avivkeller added fs Issues and PRs related to the fs subsystem / file system. revert PRs that revert previously landed PRs. labels Sep 19, 2024

src: revert filesystem::path changes
ba26f0b

anonrig force-pushed the revert-filesystem-path branch from 67ddaa9 to ba26f0bCompare September 19, 2024 17:36

joyeecheung approved these changes Sep 20, 2024
View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2024

nodejs-github-bot merged commit 5a96671 into nodejs:mainSep 21, 2024

targos pushed a commit that referenced this pull request Oct 4, 2024
src: revert filesystem::path changes
9ae80e1
PR-URL: #55015 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

aduh95 mentioned this pull request Oct 9, 2024
V22.10.0 proposal #55343
Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
src: revert filesystem::path changes
733eec9
PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
src: revert filesystem::path changes
0200cd7
PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

joyeecheung mentioned this pull request Nov 29, 2024
fs.rmSync('速') crash without throw #56049
Closed

codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025
src: revert filesystem::path changes
c7552ad
nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
src: revert filesystem::path changes
eb130b0
nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: revert filesystem::path changes
c178633
nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: revert filesystem::path changes
2aceefd
nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
src: revert filesystem::path changes
9c2550b
nodejs/node#55015

YEONDG mentioned this pull request Jul 25, 2025
Node.js fails to run when folder path contains Korean characters #59208
Closed

Uh oh!

src: revert filesystem::path changes#55015

src: revert filesystem::path changes #55015

Uh oh!

Conversation

anonrig commented Sep 19, 2024

Uh oh!

nodejs-github-bot commented Sep 19, 2024

Uh oh!

anonrig commented Sep 19, 2024

Uh oh!

anonrigSep 19, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Sep 19, 2024

Uh oh!

codecovbot commented Sep 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 19, 2024

Uh oh!

nodejs-github-bot commented Sep 20, 2024

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 20, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Sep 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joyeecheungSep 20, 2024•
edited
Loading

codecovbot commented Sep 19, 2024•
edited
Loading

joyeecheung commented Sep 20, 2024•
edited
Loading