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 resolved Hide 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bl-ue commented Apr 18, 2020 •

edited

Loading

bnoordhuis left a comment

nodejs-github-bot commented Apr 20, 2020

addaleax commented Apr 20, 2020 •

edited

Loading

bl-ue commented Apr 20, 2020

nodejs-github-bot commented Apr 23, 2020

nodejs-github-bot commented Apr 23, 2020

nodejs-github-bot commented Apr 25, 2020

juanarbol left a comment

addaleax commented Apr 26, 2020

nodejs-github-bot commented Apr 26, 2020

bl-ue commented Apr 26, 2020

richardlau commented Jul 2, 2020 •

edited

Loading

bl-ue commented Feb 5, 2021

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

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

Conversation

bl-ue commented Apr 18, 2020 • edited Loading

Checklist

bnoordhuis left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 20, 2020

addaleax commented Apr 20, 2020 • edited Loading

bl-ue commented Apr 20, 2020

nodejs-github-bot commented Apr 23, 2020

nodejs-github-bot commented Apr 23, 2020

nodejs-github-bot commented Apr 25, 2020

juanarbol left a comment

Choose a reason for hiding this comment

addaleax commented Apr 26, 2020

nodejs-github-bot commented Apr 26, 2020

bl-ue commented Apr 26, 2020

richardlau commented Jul 2, 2020 • edited Loading

bl-ue commented Feb 5, 2021

bl-ue commented Apr 18, 2020 •

edited

Loading

addaleax commented Apr 20, 2020 •

edited

Loading

richardlau commented Jul 2, 2020 •

edited

Loading