src: fix empty-named env var assertion failure

[bl-ue]

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

[bnoordhuis]

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

[nodejs-github-bot]

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

[addaleax]

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

[bl-ue]

@addaleax Okay. I've reverted the changes.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[juanarbol]

LGTM

[addaleax]

@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]

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

[bl-ue]

@addaleax Okay, I fixed that.

[richardlau]

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

[bl-ue]

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