[v8.x] src: fix fs.write() externalized string handling by addaleax · Pull Request #22731 · nodejs/node

addaleax · 2018-09-06T16:30:22Z

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 · 2018-09-06T16:31:08Z

@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 · 2018-09-06T16:42:20Z

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

MylesBorins · 2018-09-06T16:44:58Z

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

jasnell · 2018-09-06T16:47:07Z

@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 · 2018-09-06T16:49:21Z

@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 · 2018-09-06T16:51:22Z

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 · 2018-09-15T17:37:00Z

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

MylesBorins · 2018-09-25T21:02:29Z

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

MylesBorins · 2018-09-26T02:20:21Z

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

richardlau · 2018-09-26T02:58:27Z

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 · 2018-09-26T03:16:53Z

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.

BethGriggs · 2018-10-02T15:29:03Z

@addaleax are you able to take a look? Thanks

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>

* 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#18146Fixes: nodejs#22728 PR-URL: nodejs#18216

addaleax · 2018-10-02T15:40:36Z

@BethGriggs Thanks for the ping!

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

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

* 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: #18146Fixes: #22728 Backport-PR-URL: #22731 PR-URL: #18216 Reviewed-By: James M Snell <jasnell@gmail.com>

BethGriggs · 2018-10-16T19:39:34Z

Landed in v8.x-staging48f31bd...b67bf38

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 mentioned this pull request Sep 6, 2018
src: fix fs.write() externalized string handling #18216
Merged

addaleax mentioned this pull request Sep 6, 2018
Large strings decoded from latin1 and then encoded to utf8 has wrong size #22728
Closed

jasnell approved these changes Sep 6, 2018
View reviewed changes

mihalskiy approved these changes Sep 6, 2018
View reviewed changes

joyeecheung mentioned this pull request Sep 26, 2018
Investigate flaky addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary on FreeBSD 11 #22317
Closed

bnoordhuis added 2 commits October 2, 2018 08:37

addaleax force-pushed the backport-18216 branch from ed7b009 to 590aa1fCompare October 2, 2018 15:39

BethGriggs closed this Oct 16, 2018

BethGriggs mentioned this pull request Oct 30, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

Uh oh!

Uh oh!

Conversation

addaleax commented Sep 6, 2018

Uh oh!

nodejs-github-bot commented Sep 6, 2018

Uh oh!

addaleax commented Sep 6, 2018

Uh oh!

MylesBorins commented Sep 6, 2018

Uh oh!

jasnell commented Sep 6, 2018

Uh oh!

addaleax commented Sep 6, 2018

Uh oh!

jasnell commented Sep 6, 2018

Uh oh!

trivikr commented Sep 15, 2018

Uh oh!

MylesBorins commented Sep 25, 2018

Uh oh!

MylesBorins commented Sep 26, 2018

Uh oh!

richardlau commented Sep 26, 2018

Uh oh!

richardlau commented Sep 26, 2018

Uh oh!

BethGriggs commented Oct 2, 2018

Uh oh!

addaleax commented Oct 2, 2018

Uh oh!

BethGriggs commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants