src: fix empty-named env var assertion failure by bl-ue · Pull Request #32921 · nodejs/node · GitHub

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

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.

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

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

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.

@addaleax Okay. I've reverted the changes.

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

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

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

LGTM

@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.)

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

@addaleax Okay, I fixed that.

PR-URL: #32921 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>