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 <raisinten@gmail.com> PR-URL: #43668 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs · Jul 26, 2022 · 5db0c8f · 5db0c8f
1 parent 2493655
commit 5db0c8f
Showing 8 changed files with 56 additions and 49 deletions.
diff --git 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<KVStore> 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
@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "async_wrap.h"
+#include "util.h"
 
 #include <algorithm>
 #include <sstream>
@@ -66,10 +67,11 @@ class NODE_EXTERN_PRIVATE EnabledDebugList {
     return enabled_[static_cast<int>(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<KVStore> 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
@@ -760,10 +760,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
@@ -657,34 +657,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<v8::String> Get(v8::Isolate* isolate,
-                                         v8::Local<v8::String> key) const = 0;
-  virtual v8::Maybe<std::string> Get(const char* key) const = 0;
-  virtual void Set(v8::Isolate* isolate,
-                   v8::Local<v8::String> key,
-                   v8::Local<v8::String> value) = 0;
-  virtual int32_t Query(v8::Isolate* isolate,
-                        v8::Local<v8::String> key) const = 0;
-  virtual int32_t Query(const char* key) const = 0;
-  virtual void Delete(v8::Isolate* isolate, v8::Local<v8::String> key) = 0;
-  virtual v8::Local<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
-
-  virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const;
-  virtual v8::Maybe<bool> AssignFromObject(v8::Local<v8::Context> context,
-                                           v8::Local<v8::Object> entries);
-
-  static std::shared_ptr<KVStore> CreateMapKVStore();
-};
-
 namespace per_process {
 extern std::shared_ptr<KVStore> system_environment;
 }

diff --git a/src/node.cc b/src/node.cc
@@ -1008,7 +1008,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
@@ -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<KVStore> 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<String> 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<String> maybe_value = env_vars->Get(
+        isolate, String::NewFromUtf8(isolate, key).ToLocalChecked());
     Local<String> 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<Value>& 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<Value> result =
       ToV8Value(isolate->GetCurrentContext(), text).ToLocalChecked();
   args.GetReturnValue().Set(result);

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -290,7 +290,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<KVStore> env_vars = nullptr,
+                v8::Isolate* isolate = nullptr);
 }  // namespace credentials
 
 void DefineZlibConstants(v8::Local<v8::Object> target);

diff --git a/src/util.h b/src/util.h
@@ -273,6 +273,34 @@ template <typename Inner, typename Outer>
 constexpr ContainerOfHelper<Inner, Outer> 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<v8::String> Get(v8::Isolate* isolate,
+                                         v8::Local<v8::String> key) const = 0;
+  virtual v8::Maybe<std::string> Get(const char* key) const = 0;
+  virtual void Set(v8::Isolate* isolate,
+                   v8::Local<v8::String> key,
+                   v8::Local<v8::String> value) = 0;
+  virtual int32_t Query(v8::Isolate* isolate,
+                        v8::Local<v8::String> key) const = 0;
+  virtual int32_t Query(const char* key) const = 0;
+  virtual void Delete(v8::Isolate* isolate, v8::Local<v8::String> key) = 0;
+  virtual v8::Local<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
+
+  virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const;
+  virtual v8::Maybe<bool> AssignFromObject(v8::Local<v8::Context> context,
+                                           v8::Local<v8::Object> entries);
+
+  static std::shared_ptr<KVStore> CreateMapKVStore();
+};
+
 // Convenience wrapper around v8::String::NewFromOneByte().
 inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
                                            const char* data,