From 15a7f4cbc0bc9eba041cd3a9b7fe62dfeb714847 Mon Sep 17 00:00:00 2001 From: Christopher Beeson Date: Sat, 18 Apr 2020 16:49:25 -0400 Subject: [PATCH 1/5] src: fix empty-named env var assertion failure 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: https://github.com/nodejs/node/issues/32920 Refs: https://github.com/nodejs/node/pull/27310 --- src/node_env_var.cc | 2 +- test/parallel/test-process-env.js | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 5f543a5e76a1f9..b4358e99c2c3f1 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -121,7 +121,7 @@ void RealEnvStore::Set(Isolate* isolate, node::Utf8Value val(isolate, value); #ifdef _WIN32 - if (key[0] == L'=') return; + if (key.length() > 0 && key[0] == L'=') return; #endif uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); diff --git a/test/parallel/test-process-env.js b/test/parallel/test-process-env.js index 0e06306634c3e2..4ece826e8b938f 100644 --- a/test/parallel/test-process-env.js +++ b/test/parallel/test-process-env.js @@ -106,3 +106,11 @@ if (common.isWindows) { const keys = Object.keys(process.env); assert.ok(keys.length > 0); } + +// Setting environment variables on Windows with empty names should not cause +// an assertion failure. +// https://github.com/nodejs/node/issues/32920 +{ + process.env[''] = ''; + assert.strictEqual(process.env[''], undefined); +} From afd02b2579235dd743946f00028c3bf17c189e84 Mon Sep 17 00:00:00 2001 From: Christopher Beeson Date: Sun, 19 Apr 2020 12:48:12 -0400 Subject: [PATCH 2/5] src: relax assertion in operator[] of MaybeStackBuffer 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/node_env_var.cc | 2 +- src/util.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index b4358e99c2c3f1..5f543a5e76a1f9 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -121,7 +121,7 @@ void RealEnvStore::Set(Isolate* isolate, node::Utf8Value val(isolate, value); #ifdef _WIN32 - if (key.length() > 0 && key[0] == L'=') return; + if (key[0] == L'=') return; #endif uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); diff --git a/src/util.h b/src/util.h index 5eaa20b760168b..02c99bd648f10b 100644 --- a/src/util.h +++ b/src/util.h @@ -351,12 +351,12 @@ class MaybeStackBuffer { } T& operator[](size_t index) { - CHECK_LT(index, length()); + CHECK_LT(index, length() + 1); return buf_[index]; } const T& operator[](size_t index) const { - CHECK_LT(index, length()); + CHECK_LT(index, length() + 1); return buf_[index]; } From 101780635cb02bcc5a0256bc09e02b781ce14b8b Mon Sep 17 00:00:00 2001 From: Christopher Beeson Date: Sun, 19 Apr 2020 14:31:01 -0400 Subject: [PATCH 3/5] src: do not compare against wide characters --- src/node_env_var.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 5f543a5e76a1f9..b6ddf760fbcdbf 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -121,7 +121,7 @@ void RealEnvStore::Set(Isolate* isolate, node::Utf8Value val(isolate, value); #ifdef _WIN32 - if (key[0] == L'=') return; + if (key[0] == '=') return; #endif uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); @@ -139,7 +139,7 @@ int32_t RealEnvStore::Query(const char* key) const { } #ifdef _WIN32 - if (key[0] == L'=') { + if (key[0] == '=') { return static_cast(v8::ReadOnly) | static_cast(v8::DontDelete) | static_cast(v8::DontEnum); From f181bc8f85b469bc487c6c50ecd3ee3101a712f5 Mon Sep 17 00:00:00 2001 From: Christopher Beeson Date: Mon, 20 Apr 2020 11:24:47 -0400 Subject: [PATCH 4/5] Revert "src: relax assertion in operator[] of MaybeStackBuffer" This reverts commit afd02b2579235dd743946f00028c3bf17c189e84. --- src/node_env_var.cc | 2 +- src/util.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index b6ddf760fbcdbf..22b25e9efff6e9 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -121,7 +121,7 @@ void RealEnvStore::Set(Isolate* isolate, node::Utf8Value val(isolate, value); #ifdef _WIN32 - if (key[0] == '=') return; + if (key.length() > 0 && key[0] == '=') return; #endif uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); diff --git a/src/util.h b/src/util.h index 02c99bd648f10b..5eaa20b760168b 100644 --- a/src/util.h +++ b/src/util.h @@ -351,12 +351,12 @@ class MaybeStackBuffer { } T& operator[](size_t index) { - CHECK_LT(index, length() + 1); + CHECK_LT(index, length()); return buf_[index]; } const T& operator[](size_t index) const { - CHECK_LT(index, length() + 1); + CHECK_LT(index, length()); return buf_[index]; } From 446a07a3dd2492e9482b4648efeec3f8801bf91a Mon Sep 17 00:00:00 2001 From: Christopher Beeson Date: Sun, 26 Apr 2020 18:28:27 -0400 Subject: [PATCH 5/5] src: fix env var empty key access in Worker thread --- src/node_env_var.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 22b25e9efff6e9..8eaf79538a26e7 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -227,7 +227,7 @@ void MapKVStore::Set(Isolate* isolate, Local key, Local 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()); }