From 76dbd827cf68f8b7c4a8a9d5983155a3fb54d280 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 3 Jul 2022 19:00:23 +0530 Subject: [PATCH] src: pass only Isolate* and env_vars to EnabledDebugList::Parse() The function doesn't require access to the entire Environment object, so this change just passes what it needs. Addresses this TODO - https://github.com/nodejs/node/blob/71691e53d601a4cf3eab3b33cebe4cc9f50c8470/src/env.cc#L372-L374 Signed-off-by: Darshan Sen --- src/debug_utils.cc | 6 ++++-- src/debug_utils.h | 10 ++++++---- src/env.cc | 5 +---- src/env.h | 28 ---------------------------- src/node.cc | 2 +- src/node_credentials.cc | 21 ++++++++++++--------- src/node_internals.h | 5 ++++- src/util.h | 28 ++++++++++++++++++++++++++++ 8 files changed, 56 insertions(+), 49 deletions(-) diff --git a/src/debug_utils.cc b/src/debug_utils.cc index c4c476942eee77..f721a672f10e67 100644 --- a/src/debug_utils.cc +++ b/src/debug_utils.cc @@ -1,6 +1,7 @@ #include "debug_utils-inl.h" // NOLINT(build/include) #include "env-inl.h" #include "node_internals.h" +#include "util.h" #ifdef __POSIX__ #if defined(__linux__) @@ -58,9 +59,10 @@ namespace per_process { EnabledDebugList enabled_debug_list; } -void EnabledDebugList::Parse(Environment* env) { +void EnabledDebugList::Parse(std::shared_ptr env_vars, + v8::Isolate* isolate) { std::string cats; - credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env); + credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env_vars, isolate); Parse(cats, true); } diff --git a/src/debug_utils.h b/src/debug_utils.h index bd1fa5207f9520..19847e46549263 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "async_wrap.h" +#include "util.h" #include #include @@ -66,10 +67,11 @@ class NODE_EXTERN_PRIVATE EnabledDebugList { return enabled_[static_cast(category)]; } - // Uses NODE_DEBUG_NATIVE to initialize the categories. When env is not a - // nullptr, the environment variables set in the Environment are used. - // Otherwise the system environment variables are used. - void Parse(Environment* env); + // 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); private: // Set all categories matching cats to the value of enabled. diff --git a/src/env.cc b/src/env.cc index 65467587ffcdd8..ea223ce4b5c376 100644 --- a/src/env.cc +++ b/src/env.cc @@ -369,10 +369,7 @@ Environment::Environment(IsolateData* isolate_data, } set_env_vars(per_process::system_environment); - // TODO(joyeecheung): pass Isolate* and env_vars to it instead of the entire - // env, when the recursive dependency inclusion in "debug-utils.h" is - // resolved. - enabled_debug_list_.Parse(this); + enabled_debug_list_.Parse(env_vars(), isolate); // 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/env.h b/src/env.h index 921b6758ac5b6c..288ee6b7ca0add 100644 --- a/src/env.h +++ b/src/env.h @@ -656,34 +656,6 @@ struct ContextInfo { class EnabledDebugList; -class KVStore { - public: - KVStore() = default; - virtual ~KVStore() = default; - KVStore(const KVStore&) = delete; - KVStore& operator=(const KVStore&) = delete; - KVStore(KVStore&&) = delete; - KVStore& operator=(KVStore&&) = delete; - - virtual v8::MaybeLocal Get(v8::Isolate* isolate, - v8::Local key) const = 0; - virtual v8::Maybe Get(const char* key) const = 0; - virtual void Set(v8::Isolate* isolate, - v8::Local key, - v8::Local value) = 0; - virtual int32_t Query(v8::Isolate* isolate, - v8::Local key) const = 0; - virtual int32_t Query(const char* key) const = 0; - virtual void Delete(v8::Isolate* isolate, v8::Local key) = 0; - virtual v8::Local Enumerate(v8::Isolate* isolate) const = 0; - - virtual std::shared_ptr Clone(v8::Isolate* isolate) const; - virtual v8::Maybe AssignFromObject(v8::Local context, - v8::Local entries); - - static std::shared_ptr CreateMapKVStore(); -}; - namespace per_process { extern std::shared_ptr system_environment; } diff --git a/src/node.cc b/src/node.cc index 78e93c74d3c3c4..b822127fc97f04 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1013,7 +1013,7 @@ InitializationResult InitializeOncePerProcess( // Initialized the enabled list for Debug() calls with system // environment variables. - per_process::enabled_debug_list.Parse(nullptr); + per_process::enabled_debug_list.Parse(); atexit(ResetStdio); diff --git a/src/node_credentials.cc b/src/node_credentials.cc index de0ba3c8be2b1e..d7471eb0d4eda9 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -64,7 +64,10 @@ bool HasOnly(int capability) { // process only has the capability CAP_NET_BIND_SERVICE set. If the current // process does not have any capabilities set and the process is running as // setuid root then lookup will not be allowed. -bool SafeGetenv(const char* key, std::string* text, Environment* env) { +bool SafeGetenv(const char* key, + std::string* text, + std::shared_ptr env_vars, + v8::Isolate* isolate) { #if !defined(__CloudABI__) && !defined(_WIN32) #if defined(__linux__) if ((!HasOnly(CAP_NET_BIND_SERVICE) && per_process::linux_at_secure) || @@ -76,15 +79,15 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) { goto fail; #endif - if (env != nullptr) { - HandleScope handle_scope(env->isolate()); - TryCatch ignore_errors(env->isolate()); - MaybeLocal maybe_value = env->env_vars()->Get( - env->isolate(), - String::NewFromUtf8(env->isolate(), key).ToLocalChecked()); + 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(env->isolate(), value); + String::Utf8Value utf8_value(isolate, value); if (*utf8_value == nullptr) goto fail; *text = std::string(*utf8_value, utf8_value.length()); return true; @@ -121,7 +124,7 @@ static void SafeGetenv(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); Utf8Value strenvtag(isolate, args[0]); std::string text; - if (!SafeGetenv(*strenvtag, &text, env)) return; + if (!SafeGetenv(*strenvtag, &text, env->env_vars(), isolate)) 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 f8c31b386d7b62..aad4d89cecb147 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -286,7 +286,10 @@ class ThreadPoolWork { #endif // defined(__POSIX__) && !defined(__ANDROID__) && !defined(__CloudABI__) namespace credentials { -bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr); +bool SafeGetenv(const char* key, + std::string* text, + std::shared_ptr env_vars = nullptr, + v8::Isolate* isolate = nullptr); } // namespace credentials void DefineZlibConstants(v8::Local target); diff --git a/src/util.h b/src/util.h index 84977567fb1a53..a48071b093db97 100644 --- a/src/util.h +++ b/src/util.h @@ -273,6 +273,34 @@ template constexpr ContainerOfHelper ContainerOf(Inner Outer::*field, Inner* pointer); +class KVStore { + public: + KVStore() = default; + virtual ~KVStore() = default; + KVStore(const KVStore&) = delete; + KVStore& operator=(const KVStore&) = delete; + KVStore(KVStore&&) = delete; + KVStore& operator=(KVStore&&) = delete; + + virtual v8::MaybeLocal Get(v8::Isolate* isolate, + v8::Local key) const = 0; + virtual v8::Maybe Get(const char* key) const = 0; + virtual void Set(v8::Isolate* isolate, + v8::Local key, + v8::Local value) = 0; + virtual int32_t Query(v8::Isolate* isolate, + v8::Local key) const = 0; + virtual int32_t Query(const char* key) const = 0; + virtual void Delete(v8::Isolate* isolate, v8::Local key) = 0; + virtual v8::Local Enumerate(v8::Isolate* isolate) const = 0; + + virtual std::shared_ptr Clone(v8::Isolate* isolate) const; + virtual v8::Maybe AssignFromObject(v8::Local context, + v8::Local entries); + + static std::shared_ptr CreateMapKVStore(); +}; + // Convenience wrapper around v8::String::NewFromOneByte(). inline v8::Local OneByteString(v8::Isolate* isolate, const char* data,