From 150143356a99108c6b13ae25abe92aae9dccc77a Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Fri, 19 Apr 2019 18:03:18 +0530 Subject: [PATCH 01/10] src: modified RealEnvStore::Get method to use libuv functions Modified RealEnvStore::Get - removed os switch statements and replaced logic to use libuv uv_getos_env method. Fixes: https://github.com/nodejs/node/issues/27211 Refs: http://docs.libuv.org/en/v1.x/misc.html --- src/node_env_var.cc | 53 ++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index ef2b78d664858e..27de76245cc82d 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -82,35 +82,38 @@ void DateTimeConfigurationChangeNotification(Isolate* isolate, const T& key) { Local RealEnvStore::Get(Isolate* isolate, Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); -#ifdef __POSIX__ + node::Utf8Value key(isolate, property); - const char* val = getenv(*key); - if (val) { - return String::NewFromUtf8(isolate, val, NewStringType::kNormal) - .ToLocalChecked(); - } -#else // _WIN32 - node::TwoByteValue key(isolate, property); - WCHAR buffer[32767]; // The maximum size allowed for environment variables. - SetLastError(ERROR_SUCCESS); - DWORD result = GetEnvironmentVariableW( - reinterpret_cast(*key), buffer, arraysize(buffer)); - // If result >= sizeof buffer the buffer was too small. That should never - // happen. If result == 0 and result != ERROR_SUCCESS the variable was not - // found. - if ((result > 0 || GetLastError() == ERROR_SUCCESS) && - result < arraysize(buffer)) { - const uint16_t* two_byte_buffer = reinterpret_cast(buffer); - v8::MaybeLocal rc = String::NewFromTwoByte( - isolate, two_byte_buffer, NewStringType::kNormal); - if (rc.IsEmpty()) { - isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); + char* val = nullptr; + size_t initSz = 256; + + // Allocate 256 bytes initially, if not enough reallocate. + val = static_cast(malloc(sizeof(char) * initSz)); + + int ret = uv_os_getenv(*key, val, &initSz); + + if (UV_ENOBUFS == ret) { + // Buffer is not large enough, reallocate to the updated initSz + // and fetch env value again. + val = static_cast(realloc(val, sizeof(char) * initSz)); + + ret = uv_os_getenv(*key, val, &initSz); + + // Still failed to fetch env value return emptry string. + if (UV_ENOBUFS == ret || UV_ENOENT == ret) { return Local(); } - return rc.ToLocalChecked(); + } else if (UV_ENOENT == ret) { + return Local(); } -#endif - return Local(); + + Local valueString = + String::NewFromUtf8(isolate, val, NewStringType::kNormal) + .ToLocalChecked(); + + if (nullptr != val) free(val); + + return valueString; } void RealEnvStore::Set(Isolate* isolate, From eef9b8bc5fdd4d85a2c001245d656fce0fdbe82c Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Fri, 19 Apr 2019 18:44:36 +0530 Subject: [PATCH 02/10] src: modified RealEnvStore methods to use libuv functions Modified RealEnvStore::Get, Set, Query and Delete methods to use libuv methods environment variables operations instead of using os specific logic and switches. Fixes: https://github.com/nodejs/node/issues/27211 Refs: http://docs.libuv.org/en/v1.x/misc.html --- src/node_env_var.cc | 54 ++++++++++++++------------------------------- 1 file changed, 17 insertions(+), 37 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 27de76245cc82d..e695e1a013f595 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -88,14 +88,14 @@ Local RealEnvStore::Get(Isolate* isolate, size_t initSz = 256; // Allocate 256 bytes initially, if not enough reallocate. - val = static_cast(malloc(sizeof(char) * initSz)); + val = Malloc(sizeof(char) * initSz); int ret = uv_os_getenv(*key, val, &initSz); if (UV_ENOBUFS == ret) { // Buffer is not large enough, reallocate to the updated initSz // and fetch env value again. - val = static_cast(realloc(val, sizeof(char) * initSz)); + val = Realloc(val, sizeof(char) * initSz); ret = uv_os_getenv(*key, val, &initSz); @@ -120,55 +120,35 @@ void RealEnvStore::Set(Isolate* isolate, Local property, Local value) { Mutex::ScopedLock lock(per_process::env_var_mutex); -#ifdef __POSIX__ + node::Utf8Value key(isolate, property); node::Utf8Value val(isolate, value); - setenv(*key, *val, 1); -#else // _WIN32 - node::TwoByteValue key(isolate, property); - node::TwoByteValue val(isolate, value); - WCHAR* key_ptr = reinterpret_cast(*key); - // Environment variables that start with '=' are read-only. - if (key_ptr[0] != L'=') { - SetEnvironmentVariableW(key_ptr, reinterpret_cast(*val)); - } -#endif + uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); } int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); -#ifdef __POSIX__ + node::Utf8Value key(isolate, property); - if (getenv(*key)) return 0; -#else // _WIN32 - node::TwoByteValue key(isolate, property); - WCHAR* key_ptr = reinterpret_cast(*key); - SetLastError(ERROR_SUCCESS); - if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 || - GetLastError() == ERROR_SUCCESS) { - if (key_ptr[0] == L'=') { - // Environment variables that start with '=' are hidden and read-only. - return static_cast(v8::ReadOnly) | - static_cast(v8::DontDelete) | - static_cast(v8::DontEnum); - } - return 0; + + char val[2]; + size_t init_sz = sizeof(val); + + int ret = uv_os_getenv(*key, val, &init_sz); + + if (UV_ENOENT == ret) { + return -1; } -#endif - return -1; + + return 0; } void RealEnvStore::Delete(Isolate* isolate, Local property) { Mutex::ScopedLock lock(per_process::env_var_mutex); -#ifdef __POSIX__ + node::Utf8Value key(isolate, property); - unsetenv(*key); -#else - node::TwoByteValue key(isolate, property); - WCHAR* key_ptr = reinterpret_cast(*key); - SetEnvironmentVariableW(key_ptr, nullptr); -#endif + uv_os_unsetenv(*key); DateTimeConfigurationChangeNotification(isolate, key); } From 7db9f03cf67f5475fe894492a80c1aa2a6c7279b Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Fri, 19 Apr 2019 22:43:42 +0530 Subject: [PATCH 03/10] =?UTF-8?q?src:=20Modified=20src/node=5Fenv=5Fvar.cc?= =?UTF-8?q?=20as=20per=20Joyee=20Cheung=E2=80=99s=20review=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Below review comments are taken care in this submission: 1. followed snake case naming convention for local variables 2. dropped sizeof(char) since it is obvious value 1 3. Used MaybeLocal instead of Local to check
for empty strings and throw exception if empty. Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://v8docs.nodesource.com/node-4.8/annotated.html --- src/node_env_var.cc | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index e695e1a013f595..b9d44a768821fb 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -85,19 +85,19 @@ Local RealEnvStore::Get(Isolate* isolate, node::Utf8Value key(isolate, property); char* val = nullptr; - size_t initSz = 256; + size_t init_sz = 256; // Allocate 256 bytes initially, if not enough reallocate. - val = Malloc(sizeof(char) * initSz); + val = Malloc(init_sz); - int ret = uv_os_getenv(*key, val, &initSz); + int ret = uv_os_getenv(*key, val, &init_sz); if (UV_ENOBUFS == ret) { - // Buffer is not large enough, reallocate to the updated initSz + // Buffer is not large enough, reallocate to the updated init_sz // and fetch env value again. - val = Realloc(val, sizeof(char) * initSz); + val = Realloc(val, init_sz); - ret = uv_os_getenv(*key, val, &initSz); + ret = uv_os_getenv(*key, val, &init_sz); // Still failed to fetch env value return emptry string. if (UV_ENOBUFS == ret || UV_ENOENT == ret) { @@ -107,13 +107,17 @@ Local RealEnvStore::Get(Isolate* isolate, return Local(); } - Local valueString = - String::NewFromUtf8(isolate, val, NewStringType::kNormal) - .ToLocalChecked(); + MaybeLocal value_string = + String::NewFromUtf8(isolate, val, NewStringType::kNormal); + + if (value_string.IsEmpty()) { + isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); + return Local(); + } if (nullptr != val) free(val); - return valueString; + return value_string.ToLocalChecked(); } void RealEnvStore::Set(Isolate* isolate, From 6ae79f7ce1b73dc814ef8aa690ddc0daec2a9da1 Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Sat, 20 Apr 2019 00:19:07 +0530 Subject: [PATCH 04/10] src: Modified to use 2 space for indentation. used 2 spaces instead of 4 for indentation Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://v8docs.nodesource.com/node-4.8/annotated.html --- 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 b9d44a768821fb..b36fc83e5dbe3d 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -111,8 +111,8 @@ Local RealEnvStore::Get(Isolate* isolate, String::NewFromUtf8(isolate, val, NewStringType::kNormal); if (value_string.IsEmpty()) { - isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); - return Local(); + isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); + return Local(); } if (nullptr != val) free(val); From 0271ee147b443c1bbc70c4061bd57b9bba888ebb Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Sat, 20 Apr 2019 18:39:59 +0530 Subject: [PATCH 05/10] src: refactor RealEnvStore methods - review comments fixing - 1 Below review comments by Anna Henningsen are taken care: 1. avoided Yoda style comparisons 2. used MaybeStackBuffer instead of raw char pointers 3. used MaybeLocal to inspect for empty string value and then raise exception and return empty Local handle. (Changing return type of RealEnvStore::Get method to MaybeLocal is pending, and planning to submit in next commit) Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277063377 --- src/node_env_var.cc | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index b36fc83e5dbe3d..c72be37db04047 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -84,40 +84,34 @@ Local RealEnvStore::Get(Isolate* isolate, Mutex::ScopedLock lock(per_process::env_var_mutex); node::Utf8Value key(isolate, property); - char* val = nullptr; size_t init_sz = 256; + MaybeStackBuffer val; + int ret = uv_os_getenv(*key, *val, &init_sz); - // Allocate 256 bytes initially, if not enough reallocate. - val = Malloc(init_sz); - - int ret = uv_os_getenv(*key, val, &init_sz); - - if (UV_ENOBUFS == ret) { + if (ret == UV_ENOBUFS) { // Buffer is not large enough, reallocate to the updated init_sz // and fetch env value again. - val = Realloc(val, init_sz); + val.AllocateSufficientStorage(init_sz); + ret = uv_os_getenv(*key, *val, &init_sz); + } - ret = uv_os_getenv(*key, val, &init_sz); + if (ret >= 0) { // Env key value fetch success. + MaybeLocal value_string = + String::NewFromUtf8(isolate, *val, NewStringType::kNormal, init_sz); - // Still failed to fetch env value return emptry string. - if (UV_ENOBUFS == ret || UV_ENOENT == ret) { + // If fetched value is empty, raise exception + // and return empty handle. + if (value_string.IsEmpty()) { + //Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); return Local(); } - } else if (UV_ENOENT == ret) { - return Local(); - } - MaybeLocal value_string = - String::NewFromUtf8(isolate, val, NewStringType::kNormal); - - if (value_string.IsEmpty()) { - isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); - return Local(); + return value_string.ToLocalChecked(); } - if (nullptr != val) free(val); - - return value_string.ToLocalChecked(); + // Failed to fetch env value, raise exception and return empty handle. + //Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); + return Local(); } void RealEnvStore::Set(Isolate* isolate, @@ -135,13 +129,10 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); node::Utf8Value key(isolate, property); - char val[2]; size_t init_sz = sizeof(val); - int ret = uv_os_getenv(*key, val, &init_sz); - - if (UV_ENOENT == ret) { + if (ret == UV_ENOENT) { return -1; } From 4d51c93aff1ce3ba09160b689c373fa512bfed46 Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Sat, 20 Apr 2019 19:55:46 +0530 Subject: [PATCH 06/10] src: refactor RealEnvStore methods - review comments fixing - 2 Modified KVStore::Get return type to MaybeLocal and also modified RealEnvStore::Get, MapKVStore::Get respectively. Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277063377 --- src/env.h | 4 ++-- src/inspector_profiler.cc | 3 ++- src/node_env_var.cc | 34 +++++++++++++++++++--------------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/env.h b/src/env.h index 15c2f953133089..a01a6057c5b7f6 100644 --- a/src/env.h +++ b/src/env.h @@ -607,8 +607,8 @@ class KVStore { KVStore(KVStore&&) = delete; KVStore& operator=(KVStore&&) = delete; - virtual v8::Local Get(v8::Isolate* isolate, - v8::Local key) const = 0; + virtual v8::MaybeLocal Get(v8::Isolate* isolate, + v8::Local key) const = 0; virtual void Set(v8::Isolate* isolate, v8::Local key, v8::Local value) = 0; diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index e5e01dc3c9204c..867f145ff5907d 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -340,7 +340,8 @@ std::string GetCwd(Environment* env) { void StartProfilers(Environment* env) { Isolate* isolate = env->isolate(); Local coverage_str = env->env_vars()->Get( - isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE")); + isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE")) + .FromMaybe(Local()); if (!coverage_str.IsEmpty() && coverage_str->Length() > 0) { CHECK_NULL(env->coverage_connection()); env->set_coverage_connection(std::make_unique(env)); diff --git a/src/node_env_var.cc b/src/node_env_var.cc index c72be37db04047..61c481948fe524 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -36,7 +36,7 @@ using v8::Value; class RealEnvStore final : public KVStore { public: - Local Get(Isolate* isolate, Local key) const override; + MaybeLocal Get(Isolate* isolate, Local key) const override; void Set(Isolate* isolate, Local key, Local value) override; int32_t Query(Isolate* isolate, Local key) const override; void Delete(Isolate* isolate, Local key) override; @@ -45,7 +45,7 @@ class RealEnvStore final : public KVStore { class MapKVStore final : public KVStore { public: - Local Get(Isolate* isolate, Local key) const override; + MaybeLocal Get(Isolate* isolate, Local key) const override; void Set(Isolate* isolate, Local key, Local value) override; int32_t Query(Isolate* isolate, Local key) const override; void Delete(Isolate* isolate, Local key) override; @@ -79,8 +79,8 @@ void DateTimeConfigurationChangeNotification(Isolate* isolate, const T& key) { } } -Local RealEnvStore::Get(Isolate* isolate, - Local property) const { +MaybeLocal RealEnvStore::Get(Isolate* isolate, + Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); node::Utf8Value key(isolate, property); @@ -102,16 +102,16 @@ Local RealEnvStore::Get(Isolate* isolate, // If fetched value is empty, raise exception // and return empty handle. if (value_string.IsEmpty()) { - //Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); - return Local(); + Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); + return MaybeLocal(); } - return value_string.ToLocalChecked(); + return value_string; } // Failed to fetch env value, raise exception and return empty handle. - //Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); - return Local(); + Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); + return MaybeLocal(); } void RealEnvStore::Set(Isolate* isolate, @@ -209,19 +209,19 @@ std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const { for (uint32_t i = 0; i < keys_length; i++) { Local key = keys->Get(context, i).ToLocalChecked(); CHECK(key->IsString()); - copy->Set(isolate, key.As(), Get(isolate, key.As())); + copy->Set(isolate, key.As(), Get(isolate, key.As()) + .FromMaybe(Local())); } return copy; } -Local MapKVStore::Get(Isolate* isolate, Local key) const { +MaybeLocal MapKVStore::Get(Isolate* isolate, Local key) const { Mutex::ScopedLock lock(mutex_); Utf8Value str(isolate, key); auto it = map_.find(std::string(*str, str.length())); if (it == map_.end()) return Local(); return String::NewFromUtf8(isolate, it->second.data(), - NewStringType::kNormal, it->second.size()) - .ToLocalChecked(); + NewStringType::kNormal, it->second.size()); } void MapKVStore::Set(Isolate* isolate, Local key, Local value) { @@ -301,8 +301,12 @@ static void EnvGetter(Local property, return info.GetReturnValue().SetUndefined(); } CHECK(property->IsString()); - info.GetReturnValue().Set( - env->env_vars()->Get(env->isolate(), property.As())); + MaybeLocal value_string = + env->env_vars()->Get(env->isolate(), property.As()); + if (value_string.IsEmpty()) { + return; + } + info.GetReturnValue().Set(value_string.FromMaybe(Local())); } static void EnvSetter(Local property, From 1d0aa6e1cd6f54b5beef526a59519a5b6b0a9735 Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Sat, 20 Apr 2019 23:44:04 +0530 Subject: [PATCH 07/10] src: refactor RealEnvStore methods - review comments fixing - 2a Removed exception throwing if value is empty, since the same is handled in caller and returned immediately. Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277063377 --- src/node_env_var.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 61c481948fe524..5ac44646c6603f 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -98,14 +98,6 @@ MaybeLocal RealEnvStore::Get(Isolate* isolate, if (ret >= 0) { // Env key value fetch success. MaybeLocal value_string = String::NewFromUtf8(isolate, *val, NewStringType::kNormal, init_sz); - - // If fetched value is empty, raise exception - // and return empty handle. - if (value_string.IsEmpty()) { - Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); - return MaybeLocal(); - } - return value_string; } From b40d71e38fa9a224d2b50945eb6736eb97a673b7 Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Mon, 22 Apr 2019 19:27:27 +0530 Subject: [PATCH 08/10] src: refactor RealEnvStore methods - review comments fixing - 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. All tests were failing due to the exception raising from within 
 RealEnvStore::Get method, removed the exception raising since 
 the caller are checking for empty handles and taking necessary 
actions. 2. Used MayLocal’s ToLocalChecked() function instead of
FromMaybe(). Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277063377 --- src/node_env_var.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 5ac44646c6603f..28436b992cdf09 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -101,8 +101,6 @@ MaybeLocal RealEnvStore::Get(Isolate* isolate, return value_string; } - // Failed to fetch env value, raise exception and return empty handle. - Environment::GetCurrent(isolate)->ThrowUVException(ret, "getenv"); return MaybeLocal(); } @@ -202,7 +200,7 @@ std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const { Local key = keys->Get(context, i).ToLocalChecked(); CHECK(key->IsString()); copy->Set(isolate, key.As(), Get(isolate, key.As()) - .FromMaybe(Local())); + .ToLocalChecked()); } return copy; } @@ -296,9 +294,10 @@ static void EnvGetter(Local property, MaybeLocal value_string = env->env_vars()->Get(env->isolate(), property.As()); if (value_string.IsEmpty()) { + info.GetReturnValue().Set(value_string.FromMaybe(Local())); return; } - info.GetReturnValue().Set(value_string.FromMaybe(Local())); + info.GetReturnValue().Set(value_string.ToLocalChecked()); } static void EnvSetter(Local property, From 88fedfcca6a91c4073605118816a50a22c69d1bb Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Wed, 24 Apr 2019 20:07:47 +0530 Subject: [PATCH 09/10] =?UTF-8?q?src:=20refactor=20RealEnvStore=20methods?= =?UTF-8?q?=20-=20Joyee=20Cheung=E2=80=99s=20review=20remarks=20fixed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Modified an if statment and formatted the code as per Joyee Cheung's review comments Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277820347 Refs: https://github.com/nodejs/node/pull/27310#discussion_r277821566 --- src/node_env_var.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 28436b992cdf09..c025a8d1fa5f1f 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -199,8 +199,9 @@ std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const { for (uint32_t i = 0; i < keys_length; i++) { Local key = keys->Get(context, i).ToLocalChecked(); CHECK(key->IsString()); - copy->Set(isolate, key.As(), Get(isolate, key.As()) - .ToLocalChecked()); + copy->Set(isolate, + key.As(), + Get(isolate, key.As()).ToLocalChecked()); } return copy; } @@ -293,11 +294,9 @@ static void EnvGetter(Local property, CHECK(property->IsString()); MaybeLocal value_string = env->env_vars()->Get(env->isolate(), property.As()); - if (value_string.IsEmpty()) { - info.GetReturnValue().Set(value_string.FromMaybe(Local())); - return; + if (!value_string.IsEmpty()) { + info.GetReturnValue().Set(value_string.ToLocalChecked()); } - info.GetReturnValue().Set(value_string.ToLocalChecked()); } static void EnvSetter(Local property, From 614f357f4b78fd61faf3991f7512355c9b60c1e4 Mon Sep 17 00:00:00 2001 From: Devendra Satram Date: Tue, 30 Apr 2019 20:36:52 +0530 Subject: [PATCH 10/10] src: handle windows hidden/read-only env keys in set/query methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All windows tests were failing after previous commit, as per Joyee Cheung suggestion modified to inspect the first character of env variable key’s on windows machine for the character ‘=‘ and skip if the key contains. Since these keys are hidden/read-only env vars in windows machines. Fixes: https://github.com/nodejs/node/issues/27211 Refs: https://github.com/nodejs/node/pull/27310#issuecomment-487552882 --- src/node_env_var.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index c025a8d1fa5f1f..2c1af65b867b0b 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -111,6 +111,10 @@ void RealEnvStore::Set(Isolate* isolate, node::Utf8Value key(isolate, property); node::Utf8Value val(isolate, value); + +#ifdef _WIN32 + if (key[0] == L'=') return; +#endif uv_os_setenv(*key, *val); DateTimeConfigurationChangeNotification(isolate, key); } @@ -119,9 +123,17 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); node::Utf8Value key(isolate, property); +#ifdef _WIN32 + if (key[0] == L'=') + return static_cast(v8::ReadOnly) | + static_cast(v8::DontDelete) | + static_cast(v8::DontEnum); +#endif + char val[2]; size_t init_sz = sizeof(val); int ret = uv_os_getenv(*key, val, &init_sz); + if (ret == UV_ENOENT) { return -1; }