src: fix empty-named env var assertion failure#32921

bl-ue · 2020-04-18T21:33:54Z

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0.

Fixes: #32920
Refs: #27310

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

src/node_env_var.cc

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310

Setting environment variables on Windows was causing an abort because the assertion did not account for the null-terminator at the end of the buffer.

bnoordhuis

LGTM, thanks, but I'd like @addaleax to take a look as well.

nodejs-github-bot · 2020-04-20T11:47:03Z

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

addaleax · 2020-04-20T14:12:28Z

Yeah, I would strongly prefer the original variant of this PR (i.e. 15a7f4c and
1017806 only):

Checking whether i < length when accessing an array at index i is a standard thing to do
The zero-termination happens for the string utility subclasses of MaybeStackBuffer (i.e. Utf8Value, TwoByteValue, BufferValue), but not for the class itself, so adding CHECK_LT(index, length() + 1); to MaybeStackBuffer is definitely incorrect.

You could compare this to std::string::at() – std::string also zero-terminates so that using str.c_str() works, but str.at(str.size()) still fails with an OOB exception (as it should).

This reverts commit afd02b2.

bl-ue · 2020-04-20T15:36:32Z

@addaleax Okay. I've reverted the changes.

nodejs-github-bot · 2020-04-23T05:12:12Z

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

nodejs-github-bot · 2020-04-23T16:15:39Z

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

nodejs-github-bot · 2020-04-25T00:48:14Z

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

juanarbol

LGTM

addaleax · 2020-04-26T19:52:10Z

@bl-ue Looks like tools/test.py --worker parallel/test-process-env fails because the env implementation for Worker threads does allow using the empty string as a key, unlike the “real” environment. I think this fix should work:

diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 22b25e9efff6..8eaf79538a26 100644 --- a/src/node_env_var.cc+++ b/src/node_env_var.cc@@ -227,7 +227,7 @@ void MapKVStore::Set(Isolate* isolate, Local<String> key, Local<String> value){Mutex::ScopedLock lock(mutex_); Utf8Value key_str(isolate, key); Utf8Value value_str(isolate, value); - if (*key_str != nullptr && *value_str != nullptr){+ if (*key_str != nullptr && key_str.length() > 0 && *value_str != nullptr){ map_[std::string(*key_str, key_str.length())] = std::string(*value_str, value_str.length())}

(I can push that change too, if you like, but it’s your PR after all.)

nodejs-github-bot · 2020-04-26T22:35:23Z

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

bl-ue · 2020-04-26T22:38:44Z

@addaleax Okay, I fixed that.

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310 PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

PR-URL: #32921 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

richardlau · 2020-07-02T14:06:01Z

Does not land cleanly on v10.x-staging and will need a manual backport. Looks dependent on #25067.

bl-ue · 2021-02-05T12:15:06Z

@richardlau sorry, this is so late, but this issue doesn't even happen on node versions >=12.11.0, so I'm not sure if it needs to be backported...

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 18, 2020

bnoordhuis approved these changes Apr 18, 2020
View reviewed changes

src/node_env_var.cc Outdated Show resolvedHide resolved

devnexen approved these changes Apr 19, 2020
View reviewed changes

bl-ue added 3 commits April 19, 2020 14:31

src: fix empty-named env var assertion failure
15a7f4c
Setting an environment variable with an empty name on Windows resulted in an assertion failure, because it was checked for an '=' sign at the beginning without verifying the length was greater than 0. Fixes: #32920 Refs: #27310

src: relax assertion in operator[] of MaybeStackBuffer
afd02b2
Setting environment variables on Windows was causing an abort because the assertion did not account for the null-terminator at the end of the buffer.

src: do not compare against wide characters
1017806

jasnell approved these changes Apr 19, 2020
View reviewed changes

bnoordhuis approved these changes Apr 20, 2020
View reviewed changes

bnoordhuis requested a review from addaleax April 20, 2020 11:46

Revert "src: relax assertion in operator[] of MaybeStackBuffer"
f181bc8
This reverts commit afd02b2.

fhinkel approved these changes Apr 21, 2020
View reviewed changes

himself65 approved these changes Apr 23, 2020
View reviewed changes

addaleax approved these changes Apr 25, 2020
View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v10.x labels Apr 25, 2020

juanarbol approved these changes Apr 26, 2020
View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2020

src: fix env var empty key access in Worker thread
446a07a

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2020

bl-ue requested a review from addaleax April 27, 2020 02:21

BethGriggs mentioned this pull request Apr 27, 2020
v14.1.0 proposal #33103
Merged

BridgeAR mentioned this pull request Apr 28, 2020
v13.14.0 proposal #33122
Merged

targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged

richardlau added backported-to-v12.x and removed lts-watch-v12.x labels Jul 1, 2020

richardlau added the backport-requested-v10.x label Jul 2, 2020

richardlau removed the lts-watch-v10.x label Feb 5, 2021

Uh oh!

src: fix empty-named env var assertion failure#32921

src: fix empty-named env var assertion failure #32921

Uh oh!

Conversation

bl-ue commented Apr 18, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 20, 2020

Uh oh!

addaleax commented Apr 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bl-ue commented Apr 20, 2020

Uh oh!

nodejs-github-bot commented Apr 23, 2020

Uh oh!

nodejs-github-bot commented Apr 23, 2020

Uh oh!

nodejs-github-bot commented Apr 25, 2020

Uh oh!

juanarbol left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 26, 2020

Uh oh!

nodejs-github-bot commented Apr 26, 2020

Uh oh!

bl-ue commented Apr 26, 2020

Uh oh!

richardlau commented Jul 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bl-ue commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bl-ue commented Apr 18, 2020•
edited
Loading

addaleax commented Apr 20, 2020•
edited
Loading

richardlau commented Jul 2, 2020•
edited
Loading