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 d3436c62079e..82e6587016ed 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 072545c64823..8cfb686e435f 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 17a1a1130c8e..efc3dd7067bf 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 307049bf7c83..aeebc1f18a02 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 f053474decc2..c351a4015692 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 97cb5745a142..14135d844d35 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 1fa1f72fba9b..9e1d60ef27e9 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 552fdc438a08..4b7fab8238a4 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 14135d844d35..80605c92012c 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 4b7fab8238a4..8b45c0265bef 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;