FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: fix cpSync crash on utf characters by jazelly · Pull Request #54653 · nodejs/node · GitHub

/ node Public

fs: fix cpSync crash on utf characters - #54653

Closed
jazelly wants to merge 1 commit into
nodejs:mainfrom
jazelly:fix-cpsync-utf-fail
ConversationCommits (1)ChecksFiles changed
Closed

fs: fix cpSync crash on utf characters#54653
jazelly wants to merge 1 commit into
nodejs:mainfrom
jazelly:fix-cpsync-utf-fail

Conversation

jazelly commented Aug 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #54476
Refs: #54653 (comment)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 30, 2024
anonrig requested a review from joyeecheung August 30, 2024 02:26

codecov Bot commented Aug 30, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (59c7c55) to head (32faca8).
Report is 481 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 85.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54653      +/-   ##
==========================================
+ Coverage   88.03%   88.04%   +0.01%     
==========================================
  Files         652      652              
  Lines      183763   183779      +16     
  Branches    35860    35865       +5     
==========================================
+ Hits       161774   161817      +43     
+ Misses      15237    15228       -9     
+ Partials     6752     6734      -18
Files with missing lines Coverage Δ
src/util.h 90.16% <100.00%> (+0.24%) ⬆️
src/node_file.cc 77.04% <85.00%> (-0.06%) ⬇️

... and 37 files with indirect coverage changes

jazelly force-pushed the fix-cpsync-utf-fail branch 3 times, most recently from 1ba6b97 to d78ea43 Compare August 30, 2024 08:29

aduh95 commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

It looks like it's consistently failing to build on our macOS GHA CI

jakecastelli commented Aug 30, 2024

Copy link
Copy Markdown
Member

Possibly related to this

ld: write() failed errno=28

which indicates no space left on the device, I assume that is why it fails

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024

This comment was marked as outdated.

Sign in to view
ShenHongFei reviewed Aug 30, 2024
View reviewed changes
Comment thread src/util.h Outdated

This comment was marked as outdated.

Sign in to view
jazelly force-pushed the fix-cpsync-utf-fail branch 2 times, most recently from 8362525 to 52d0c3a Compare August 31, 2024 13:06
jazelly force-pushed the fix-cpsync-utf-fail branch 7 times, most recently from fe5f947 to a09e59f Compare August 31, 2024 14:04
jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2024
jazelly force-pushed the fix-cpsync-utf-fail branch 2 times, most recently from 1669d2a to 7cd8eb1 Compare September 12, 2024 06:08
joyeecheung reviewed Sep 18, 2024
View reviewed changes
Comment thread src/node_file.cc Outdated
Comment thread src/node_file.cc Outdated
jazelly force-pushed the fix-cpsync-utf-fail branch from 7cd8eb1 to f2f749f Compare September 20, 2024 04:47
jazelly marked this pull request as ready for review September 20, 2024 04:50
jazelly force-pushed the fix-cpsync-utf-fail branch from f2f749f to 32faca8 Compare September 20, 2024 05:08

jazelly commented Sep 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

With the requested CI env, I can finally reproduce the segfaults.

The original reported issue
Chinese are UTF-8 encoded and each char is 3-byte encoded. 5 characters are 15 bytes in the reported case. Windows std::filesystem::path treats it as UTF-16 encoded, which is 2 bytes per char, and hence the segfault happened when constructing the std::filesystem::path for src or dest.

Simply constructing from u8string still has segfault (this was the one I couldn't reproduce locally, very strange)
The segfault comes from the later call of std::filesystem::path.string() on the path constructed from u8string. I don't particularly know why? But using u8string for all the error messages fixed it. Thanks @jakecastelli for requesting the CI environment. (sorry for dragging this too long).

cc @joyeecheung @anonrig

joyeecheung mentioned this pull request Sep 20, 2024
Merged

joyeecheung commented Sep 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

The segfault comes from the later call of std::filesystem::path.string() on the path constructed from u8string. I don't particularly know why? But using u8string for all the error messages fixed it.

This is because the error throwing functions convert the messages into JS strings - technically, it is expecting Latin1

node/src/node_errors.h

Line 121 in eab97b5

OneByteString(isolate, message.c_str(), message.length()); \
which is also incorrect, but Latin1 has an overlapping set with UTF-8, and if you pass UTF8 data as Latin1 you are likely to see gibberish for non-ASCII characters but it won't crash, whereas UTF16 has fixed 2 char widths, so it would fare worse if you pass it as Latin1.

joyeecheung approved these changes Sep 20, 2024
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM - I opened a separate PR to fix the decoding in error throwing utils #55024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2024

nodejs-github-bot commented Sep 20, 2024 •
edited by jakecastelli
Loading

Copy link
Copy Markdown
Collaborator

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

jakecastelli added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 20, 2024
jazelly mentioned this pull request Sep 20, 2024
Merged
jasnell pushed a commit that referenced this pull request Sep 21, 2024
fs: convert to u8 string for filesystem path
77ca5ca 
PR-URL: #54653
Fixes: #54476
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Sep 21, 2024

Copy link
Copy Markdown
Member

Landed in 77ca5ca

jasnell closed this Sep 21, 2024
jazelly mentioned this pull request Sep 30, 2024
Merged
targos pushed a commit that referenced this pull request Oct 4, 2024
fs: convert to u8 string for filesystem path
79ffefa 
PR-URL: #54653
Fixes: #54476
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 mentioned this pull request Oct 9, 2024
Merged
jazelly deleted the fix-cpsync-utf-fail branch November 25, 2024 22:06
This was referenced Jan 23, 2025
Closed
Closed
joyeecheung mentioned this pull request Jul 27, 2025
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
joyeecheung joyeecheung approved these changes
anonrig Awaiting requested review from anonrig
ShenHongFei Awaiting requested review from ShenHongFei
aduh95 Awaiting requested review from aduh95

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. 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.

v22.6.0 fs.cpSync crash

9 participants

Back | FazBrowse Home | New Git URL