Skip to content

[v8.x] src: fix fs.write() externalized string handling #22731

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
Closed
wants to merge 2 commits into from
Closed

[v8.x] src: fix fs.write() externalized string handling #22731

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 6, 2018

Backport #18216

The merge conflicts in node_file.cc were non-trivial, so the changes there deserve to be treated like original content during review.

@nodejs-github-bot 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. v8.x labels Sep 6, 2018
@addaleax addaleax mentioned this pull request Sep 6, 2018
Merged
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/817/pipeline

@addaleax addaleax mentioned this pull request Sep 6, 2018
Closed
@addaleax
Copy link
Member Author

addaleax commented Sep 6, 2018

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

@MylesBorins
Copy link
Contributor

@addaleax should this be included in the 8.12.0 release or is it ok to wait until 8.12.1?

@jasnell
Copy link
Member

jasnell commented Sep 6, 2018

@addaleax ... would it make sense to add a new test similar to the example case in #22728? Or are the test modifications made here already sufficient?

@addaleax
Copy link
Member Author

addaleax commented Sep 6, 2018

@MylesBorins It’s hard to tell – I’d be in favour of including it. It’s been in master for long enough to be sure that this doesn’t break anything, I’d say?

@jasnell The tests added here do check the broken code path in string_bytes.cc that #22728 also triggers. How about adding a test similar to that in a separate PR against master?

@jasnell
Copy link
Member

jasnell commented Sep 6, 2018

How about adding a test similar to that in a separate PR against master?

As long as there is appropriate coverage for this particular piece then it should be fine.

@trivikr
Copy link
Member

trivikr commented Sep 15, 2018

Resume build: https://ci.nodejs.org/job/node-test-pull-request/17206/

@MylesBorins
Copy link
Contributor

one more ci: https://ci.nodejs.org/job/node-test-pull-request/17439/

@MylesBorins
Copy link
Contributor

This PR seems to be making windows completely fail, any idea what's up?

@richardlau
Copy link
Member

It's some sort of compilation failure,

e.g. https://ci.nodejs.org/job/node-compile-windows/21107/label=win-vs2015/console

17:21:30 src\string_bytes.cc(341): error C2589: '(': illegal token on right side of '::' [c:\workspace\node-compile-windows\node_lib.vcxproj]
17:21:30 src\string_bytes.cc(341): error C2059: syntax error: '::' [c:\workspace\node-compile-windows\node_lib.vcxproj]

@richardlau
Copy link
Member

src\string_bytes.cc(341):
https://github.com/nodejs/node/blob/ed7b009709bf0156ab2cde9c1ce98c777df72250/src/string_bytes.cc#L341

Among the search results for C2589 is https://stackoverflow.com/questions/5004858/stdmin-gives-error
which suggests we need to define NOMINMAX, which looks like the original PR (#18216) did in 0b1841d.

@joyeecheung joyeecheung mentioned this pull request Sep 26, 2018
Closed
@BethGriggs
Copy link
Member

@addaleax are you able to take a look? Thanks

@bnoordhuis @addaleax
build: define NOMINMAX on windows
89d092e 
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@bnoordhuis @addaleax
src: fix fs.write() externalized string handling
590aa1f 
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
Fixes: nodejs#22728
PR-URL: nodejs#18216
@addaleax
Copy link
Member Author

addaleax commented Oct 2, 2018

@BethGriggs Thanks for the ping!

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

BethGriggs pushed a commit that referenced this pull request Oct 16, 2018
@bnoordhuis @BethGriggs
build: define NOMINMAX on windows
ca8d4e3 
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

Backport-PR-URL: #22731
PR-URL: #18216
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: #22731
BethGriggs pushed a commit that referenced this pull request Oct 16, 2018
@bnoordhuis @BethGriggs
src: fix fs.write() externalized string handling
b67bf38 
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: #18146
Fixes: #22728
Backport-PR-URL: #22731
PR-URL: #18216

Reviewed-By: James M Snell <[email protected]>
@BethGriggs
Copy link
Member

Landed in v8.x-staging 48f31bd...b67bf38

@BethGriggs BethGriggs closed this Oct 16, 2018
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@mihalskiy mihalskiy mihalskiy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@addaleax @nodejs-github-bot @MylesBorins @jasnell @trivikr @richardlau @BethGriggs @mihalskiy @bnoordhuis