From 92ca21c180e3a8f11ea2354ca7bd061cbf9d307a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 21 Feb 2024 18:59:31 +0100 Subject: [PATCH 1/2] src: simplify direct queries of env vars in C++ land In many cases we can query the environment variable directly instead of encoding it into UTF8 first and then decoding it again from UTF8 via an intermediate V8 string. Drive-by: pass per_process::system_environment explicitly when the real environment variables are supposed to be used instead of relying on fallback with defaults. --- src/debug_utils.cc | 5 ++--- src/debug_utils.h | 3 +-- src/env.cc | 2 +- src/inspector_profiler.cc | 9 +++----- src/node.cc | 2 +- src/node_credentials.cc | 46 +++++++-------------------------------- src/node_internals.h | 3 +-- src/node_worker.cc | 7 ++---- 8 files changed, 19 insertions(+), 58 deletions(-) diff --git a/src/debug_utils.cc b/src/debug_utils.cc index d3436c62079e6a..82e6587016ed3a 100644 --- a/src/debug_utils.cc +++ b/src/debug_utils.cc @@ -62,10 +62,9 @@ EnabledDebugList enabled_debug_list; using v8::Local; using v8::StackTrace; -void EnabledDebugList::Parse(std::shared_ptr env_vars, - v8::Isolate* isolate) { +void EnabledDebugList::Parse(std::shared_ptr env_vars) { std::string cats; - credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env_vars, isolate); + credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env_vars); Parse(cats); } diff --git a/src/debug_utils.h b/src/debug_utils.h index 072545c648236f..8cfb686e435fd5 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -75,8 +75,7 @@ class NODE_EXTERN_PRIVATE EnabledDebugList { // Uses NODE_DEBUG_NATIVE to initialize the categories. The env_vars variable // is parsed if it is not a nullptr, otherwise the system environment // variables are parsed. - void Parse(std::shared_ptr env_vars = nullptr, - v8::Isolate* isolate = nullptr); + void Parse(std::shared_ptr env_vars); private: // Enable all categories matching cats. diff --git a/src/env.cc b/src/env.cc index 17a1a1130c8eac..efc3dd7067bf03 100644 --- a/src/env.cc +++ b/src/env.cc @@ -848,7 +848,7 @@ Environment::Environment(IsolateData* isolate_data, } set_env_vars(per_process::system_environment); - enabled_debug_list_.Parse(env_vars(), isolate); + enabled_debug_list_.Parse(env_vars()); // We create new copies of the per-Environment option sets, so that it is // easier to modify them after Environment creation. The defaults are diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index 307049bf7c83c5..aeebc1f18a026d 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -418,12 +418,9 @@ void StartProfilers(Environment* env) { EndStartedProfilers(static_cast(env)); }, env); - Isolate* isolate = env->isolate(); - Local coverage_str = env->env_vars()->Get( - isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE")) - .FromMaybe(Local()); - if ((!coverage_str.IsEmpty() && coverage_str->Length() > 0) || - env->options()->test_runner_coverage) { + std::string coverage_str = + env->env_vars()->Get("NODE_V8_COVERAGE").FromMaybe(std::string()); + if (!coverage_str.empty() || env->options()->test_runner_coverage) { CHECK_NULL(env->coverage_connection()); env->set_coverage_connection(std::make_unique(env)); env->coverage_connection()->Start(); diff --git a/src/node.cc b/src/node.cc index f053474decc238..c351a40156928b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -992,7 +992,7 @@ InitializeOncePerProcessInternal(const std::vector& args, if (!(flags & ProcessInitializationFlags::kNoParseGlobalDebugVariables)) { // Initialized the enabled list for Debug() calls with system // environment variables. - per_process::enabled_debug_list.Parse(); + per_process::enabled_debug_list.Parse(per_process::system_environment); } PlatformInit(flags); diff --git a/src/node_credentials.cc b/src/node_credentials.cc index 97cb5745a142bb..14135d844d35da 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -77,8 +77,7 @@ static bool HasOnly(int capability) { // setuid root then lookup will not be allowed. bool SafeGetenv(const char* key, std::string* text, - std::shared_ptr env_vars, - v8::Isolate* isolate) { + std::shared_ptr env_vars) { #if !defined(__CloudABI__) && !defined(_WIN32) #if defined(__linux__) if ((!HasOnly(CAP_NET_BIND_SERVICE) && linux_at_secure()) || @@ -86,45 +85,16 @@ bool SafeGetenv(const char* key, #else if (linux_at_secure() || getuid() != geteuid() || getgid() != getegid()) #endif - goto fail; + return false; #endif - if (env_vars != nullptr) { - DCHECK_NOT_NULL(isolate); - HandleScope handle_scope(isolate); - TryCatch ignore_errors(isolate); - MaybeLocal maybe_value = env_vars->Get( - isolate, String::NewFromUtf8(isolate, key).ToLocalChecked()); - Local value; - if (!maybe_value.ToLocal(&value)) goto fail; - String::Utf8Value utf8_value(isolate, value); - if (*utf8_value == nullptr) goto fail; - *text = std::string(*utf8_value, utf8_value.length()); - return true; - } - - { - Mutex::ScopedLock lock(per_process::env_var_mutex); - - size_t init_sz = 256; - MaybeStackBuffer val; - int ret = uv_os_getenv(key, *val, &init_sz); - - if (ret == UV_ENOBUFS) { - // Buffer is not large enough, reallocate to the updated init_sz - // and fetch env value again. - val.AllocateSufficientStorage(init_sz); - ret = uv_os_getenv(key, *val, &init_sz); - } - - if (ret == 0) { // Env key value fetch success. - *text = *val; - return true; - } + // Fallback to system environment which reads the real environment variable + // through uv_os_getenv. + if (env_vars == nullptr) { + env_vars = per_process::system_environment; } -fail: - return false; + return env_vars->Get(key).To(text); } static void SafeGetenv(const FunctionCallbackInfo& args) { @@ -133,7 +103,7 @@ static void SafeGetenv(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); Utf8Value strenvtag(isolate, args[0]); std::string text; - if (!SafeGetenv(*strenvtag, &text, env->env_vars(), isolate)) return; + if (!SafeGetenv(*strenvtag, &text, env->env_vars())) return; Local result = ToV8Value(isolate->GetCurrentContext(), text).ToLocalChecked(); args.GetReturnValue().Set(result); diff --git a/src/node_internals.h b/src/node_internals.h index 1fa1f72fba9bdc..9e1d60ef27e98d 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -308,8 +308,7 @@ class ThreadPoolWork { namespace credentials { bool SafeGetenv(const char* key, std::string* text, - std::shared_ptr env_vars = nullptr, - v8::Isolate* isolate = nullptr); + std::shared_ptr env_vars = nullptr); } // namespace credentials void DefineZlibConstants(v8::Local target); diff --git a/src/node_worker.cc b/src/node_worker.cc index 552fdc438a0895..4b7fab8238a4fe 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -537,11 +537,8 @@ void Worker::New(const FunctionCallbackInfo& args) { }); #ifndef NODE_WITHOUT_NODE_OPTIONS - MaybeLocal maybe_node_opts = - env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS")); - Local node_opts; - if (maybe_node_opts.ToLocal(&node_opts)) { - std::string node_options(*String::Utf8Value(isolate, node_opts)); + std::string node_options; + if (env_vars->Get("NODE_OPTIONS").To(&node_options)) { std::vector errors{}; std::vector env_argv = ParseNodeOptionsEnvVar(node_options, &errors); From 0811e97b66bf70584ac07dae5d85c6d141a22000 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 26 Feb 2024 15:01:06 +0100 Subject: [PATCH 2/2] fixup! src: simplify direct queries of env vars in C++ land --- src/node_credentials.cc | 3 --- src/node_worker.cc | 1 - 2 files changed, 4 deletions(-) diff --git a/src/node_credentials.cc b/src/node_credentials.cc index 14135d844d35da..80605c92012c31 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -24,13 +24,10 @@ namespace node { using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; -using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Object; -using v8::String; -using v8::TryCatch; using v8::Uint32; using v8::Value; diff --git a/src/node_worker.cc b/src/node_worker.cc index 4b7fab8238a4fe..8b45c0265befaf 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -32,7 +32,6 @@ using v8::Isolate; using v8::Local; using v8::Locker; using v8::Maybe; -using v8::MaybeLocal; using v8::Null; using v8::Number; using v8::Object;