Skip to content

Commit

Permalink
src: simplify direct queries of env vars in C++ land
Browse files Browse the repository at this point in the history
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.

PR-URL: #51829
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
joyeecheung authored and marco-ippolito committed Feb 29, 2024
1 parent 2cdfe35 commit 7543e77
Show file tree
Hide file tree
Showing 8 changed files with 19 additions and 62 deletions.
5 changes: 2 additions & 3 deletions src/debug_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,9 @@ EnabledDebugList enabled_debug_list;
using v8::Local;
using v8::StackTrace;

void EnabledDebugList::Parse(std::shared_ptr<KVStore> env_vars,
v8::Isolate* isolate) {
void EnabledDebugList::Parse(std::shared_ptr<KVStore> env_vars) {
std::string cats;
credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env_vars, isolate);
credentials::SafeGetenv("NODE_DEBUG_NATIVE", &cats, env_vars);
Parse(cats);
}

Expand Down
3 changes: 1 addition & 2 deletions src/debug_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<KVStore> env_vars = nullptr,
v8::Isolate* isolate = nullptr);
void Parse(std::shared_ptr<KVStore> env_vars);

private:
// Enable all categories matching cats.
Expand Down
2 changes: 1 addition & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 3 additions & 6 deletions src/inspector_profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -418,12 +418,9 @@ void StartProfilers(Environment* env) {
EndStartedProfilers(static_cast<Environment*>(env));
}, env);

Isolate* isolate = env->isolate();
Local<String> coverage_str = env->env_vars()->Get(
isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE"))
.FromMaybe(Local<String>());
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<V8CoverageConnection>(env));
env->coverage_connection()->Start();
Expand Down
2 changes: 1 addition & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& 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);
Expand Down
49 changes: 8 additions & 41 deletions src/node_credentials.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -77,54 +74,24 @@ static bool HasOnly(int capability) {
// setuid root then lookup will not be allowed.
bool SafeGetenv(const char* key,
std::string* text,
std::shared_ptr<KVStore> env_vars,
v8::Isolate* isolate) {
std::shared_ptr<KVStore> env_vars) {
#if !defined(__CloudABI__) && !defined(_WIN32)
#if defined(__linux__)
if ((!HasOnly(CAP_NET_BIND_SERVICE) && linux_at_secure()) ||
getuid() != geteuid() || getgid() != getegid())
#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<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(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<char, 256> 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<Value>& args) {
Expand All @@ -133,7 +100,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->env_vars(), isolate)) return;
if (!SafeGetenv(*strenvtag, &text, env->env_vars())) return;
Local<Value> result =
ToV8Value(isolate->GetCurrentContext(), text).ToLocalChecked();
args.GetReturnValue().Set(result);
Expand Down
3 changes: 1 addition & 2 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,7 @@ class ThreadPoolWork {
namespace credentials {
bool SafeGetenv(const char* key,
std::string* text,
std::shared_ptr<KVStore> env_vars = nullptr,
v8::Isolate* isolate = nullptr);
std::shared_ptr<KVStore> env_vars = nullptr);
} // namespace credentials

void DefineZlibConstants(v8::Local<v8::Object> target);
Expand Down
8 changes: 2 additions & 6 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -537,11 +536,8 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
});

#ifndef NODE_WITHOUT_NODE_OPTIONS
MaybeLocal<String> maybe_node_opts =
env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS"));
Local<String> 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<std::string> errors{};
std::vector<std::string> env_argv =
ParseNodeOptionsEnvVar(node_options, &errors);
Expand Down

0 comments on commit 7543e77

Please sign in to comment.