Skip to content

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

Closed
addaleax wants to merge 2 commits intonodejs:v8.x-stagingfrom
addaleax:backport-18216
Closed

[v8.x] src: fix fs.write() externalized string handling#22731
addaleax wants to merge 2 commits intonodejs:v8.x-stagingfrom
addaleax:backport-18216

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

nodejs-github-bot commented Sep 6, 2018

@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

MylesBorins commented Sep 6, 2018

@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

MylesBorins commented Sep 25, 2018

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 26, 2018

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

@richardlau
Copy link
Member

richardlau commented Sep 26, 2018

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

richardlau commented Sep 26, 2018

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

BethGriggs commented Oct 2, 2018

@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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <jasnell@gmail.com>
@BethGriggs
Copy link
Member

BethGriggs commented Oct 16, 2018

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