1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

src: improve error handling in process.env handling

Browse Source 
PR-URL: https://github.com/nodejs/node/pull/57707
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This commit is contained in:
James M Snell 2025-04-01 07:19:44 -07:00
parent f31c88021b
commit 214e3d4aff
3 changed files with 52 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

72
src/node_env_var.cc
View File

@ -26,7 +26,6 @@ using v8::Maybe;
using v8::MaybeLocal; using v8::MaybeLocal;
using v8::Name; using v8::Name;
using v8::NamedPropertyHandlerConfiguration; using v8::NamedPropertyHandlerConfiguration;
using v8::NewStringType;
using v8::Nothing; using v8::Nothing;
using v8::Object; using v8::Object;
using v8::ObjectTemplate; using v8::ObjectTemplate;
@ -45,7 +44,7 @@ class RealEnvStore final : public KVStore {
int32_t Query(Isolate* isolate, Local<String> key) const override; int32_t Query(Isolate* isolate, Local<String> key) const override;
int32_t Query(const char* key) const override; int32_t Query(const char* key) const override;
void Delete(Isolate* isolate, Local<String> key) override; void Delete(Isolate* isolate, Local<String> key) override;
Local<Array> Enumerate(Isolate* isolate) const override; MaybeLocal<Array> Enumerate(Isolate* isolate) const override;
}; };
class MapKVStore final : public KVStore { class MapKVStore final : public KVStore {
@ -56,7 +55,7 @@ class MapKVStore final : public KVStore {
int32_t Query(Isolate* isolate, Local<String> key) const override; int32_t Query(Isolate* isolate, Local<String> key) const override;
int32_t Query(const char* key) const override; int32_t Query(const char* key) const override;
void Delete(Isolate* isolate, Local<String> key) override; void Delete(Isolate* isolate, Local<String> key) override;
Local<Array> Enumerate(Isolate* isolate) const override; MaybeLocal<Array> Enumerate(Isolate* isolate) const override;
std::shared_ptr<KVStore> Clone(Isolate* isolate) const override; std::shared_ptr<KVStore> Clone(Isolate* isolate) const override;
@ -131,8 +130,12 @@ MaybeLocal<String> RealEnvStore::Get(Isolate* isolate,
if (value.has_value()) { if (value.has_value()) {
std::string val = value.value(); std::string val = value.value();
return String::NewFromUtf8( Local<Value> ret;
isolate, val.data(), NewStringType::kNormal, val.size()); if (!ToV8Value(isolate->GetCurrentContext(), val).ToLocal(&ret)) {
return {};
}
DCHECK(ret->IsString());
return ret.As<String>();
} }
return MaybeLocal<String>(); return MaybeLocal<String>();
@ -188,7 +191,7 @@ void RealEnvStore::Delete(Isolate* isolate, Local<String> property) {
DateTimeConfigurationChangeNotification(isolate, key); DateTimeConfigurationChangeNotification(isolate, key);
} }
Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const { MaybeLocal<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
Mutex::ScopedLock lock(per_process::env_var_mutex); Mutex::ScopedLock lock(per_process::env_var_mutex);
uv_env_item_t* items; uv_env_item_t* items;
int count; int count;
@ -203,12 +206,12 @@ Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
// If the key starts with '=' it is a hidden environment variable. // If the key starts with '=' it is a hidden environment variable.
if (items[i].name[0] == '=') continue; if (items[i].name[0] == '=') continue;
#endif #endif
MaybeLocal<String> str = String::NewFromUtf8(isolate, items[i].name); Local<Value> str;
if (str.IsEmpty()) { if (!String::NewFromUtf8(isolate, items[i].name).ToLocal(&str)) {
isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); isolate->ThrowException(ERR_STRING_TOO_LONG(isolate));
return Local<Array>(); return {};
} }
env_v[env_v_index++] = str.ToLocalChecked(); env_v[env_v_index++] = str;
} }
return Array::New(isolate, env_v.out(), env_v_index); return Array::New(isolate, env_v.out(), env_v_index);
@ -219,14 +222,22 @@ std::shared_ptr<KVStore> KVStore::Clone(Isolate* isolate) const {
Local<Context> context = isolate->GetCurrentContext(); Local<Context> context = isolate->GetCurrentContext();
std::shared_ptr<KVStore> copy = KVStore::CreateMapKVStore(); std::shared_ptr<KVStore> copy = KVStore::CreateMapKVStore();
Local<Array> keys = Enumerate(isolate); Local<Array> keys;
if (!Enumerate(isolate).ToLocal(&keys)) {
return nullptr;
}
uint32_t keys_length = keys->Length(); uint32_t keys_length = keys->Length();
for (uint32_t i = 0; i < keys_length; i++) { for (uint32_t i = 0; i < keys_length; i++) {
Local<Value> key = keys->Get(context, i).ToLocalChecked(); Local<Value> key;
Local<Value> value;
if (!keys->Get(context, i).ToLocal(&key)) {
return nullptr;
}
CHECK(key->IsString()); CHECK(key->IsString());
copy->Set(isolate, if (!Get(isolate, key.As<String>()).ToLocal(&value)) {
key.As<String>(), return nullptr;
Get(isolate, key.As<String>()).ToLocalChecked()); }
copy->Set(isolate, key.As<String>(), value.As<String>());
} }
return copy; return copy;
} }
@ -242,8 +253,12 @@ MaybeLocal<String> MapKVStore::Get(Isolate* isolate, Local<String> key) const {
std::optional<std::string> value = Get(*str); std::optional<std::string> value = Get(*str);
if (!value.has_value()) return MaybeLocal<String>(); if (!value.has_value()) return MaybeLocal<String>();
std::string val = value.value(); std::string val = value.value();
return String::NewFromUtf8( Local<Value> ret;
isolate, val.data(), NewStringType::kNormal, val.size()); if (!ToV8Value(isolate->GetCurrentContext(), val).ToLocal(&ret)) {
return {};
}
DCHECK(ret->IsString());
return ret.As<String>();
} }
void MapKVStore::Set(Isolate* isolate, Local<String> key, Local<String> value) { void MapKVStore::Set(Isolate* isolate, Local<String> key, Local<String> value) {
@ -272,15 +287,16 @@ void MapKVStore::Delete(Isolate* isolate, Local<String> key) {
map_.erase(std::string(*str, str.length())); map_.erase(std::string(*str, str.length()));
} }
Local<Array> MapKVStore::Enumerate(Isolate* isolate) const { MaybeLocal<Array> MapKVStore::Enumerate(Isolate* isolate) const {
Mutex::ScopedLock lock(mutex_); Mutex::ScopedLock lock(mutex_);
LocalVector<Value> values(isolate); LocalVector<Value> values(isolate);
values.reserve(map_.size()); values.reserve(map_.size());
for (const auto& pair : map_) { for (const auto& pair : map_) {
values.emplace_back( Local<Value> val;
String::NewFromUtf8(isolate, pair.first.data(), if (!ToV8Value(isolate->GetCurrentContext(), pair.first).ToLocal(&val)) {
NewStringType::kNormal, pair.first.size()) return {};
.ToLocalChecked()); }
values.emplace_back(val);
} }
return Array::New(isolate, values.data(), values.size()); return Array::New(isolate, values.data(), values.size());
} }
@ -324,7 +340,10 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
v8::Local<v8::Context> context, v8::Local<v8::Context> context,
v8::Local<v8::Object> object) { v8::Local<v8::Object> object) {
HandleScope scope(isolate); HandleScope scope(isolate);
Local<Array> keys = Enumerate(isolate); Local<Array> keys;
if (!Enumerate(isolate).ToLocal(&keys)) {
return Nothing<void>();
}
uint32_t keys_length = keys->Length(); uint32_t keys_length = keys->Length();
for (uint32_t i = 0; i < keys_length; i++) { for (uint32_t i = 0; i < keys_length; i++) {
Local<Value> key; Local<Value> key;
@ -421,6 +440,7 @@ static Intercepted EnvGetter(Local<Name> property,
TraceEnvVar(env, "get", property.As<String>()); TraceEnvVar(env, "get", property.As<String>());
if (has_env) { if (has_env) {
// ToLocalChecked here is ok since we check IsEmpty above.
info.GetReturnValue().Set(value_string.ToLocalChecked()); info.GetReturnValue().Set(value_string.ToLocalChecked());
return Intercepted::kYes; return Intercepted::kYes;
} }
@ -502,8 +522,10 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
TraceEnvVar(env, "enumerate environment variables"); TraceEnvVar(env, "enumerate environment variables");
info.GetReturnValue().Set( Local<Array> ret;
env->env_vars()->Enumerate(env->isolate())); if (env->env_vars()->Enumerate(env->isolate()).ToLocal(&ret)) {
info.GetReturnValue().Set(ret);
}
} }
static Intercepted EnvDefiner(Local<Name> property, static Intercepted EnvDefiner(Local<Name> property,

4
src/node_worker.cc
View File

@ -545,6 +545,10 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
env_vars = env->env_vars(); env_vars = env->env_vars();
} }
if (!env_vars) {
THROW_ERR_OPERATION_FAILED(env, "Failed to copy environment variables");
}
if (args[1]->IsObject() || args[2]->IsArray()) { if (args[1]->IsObject() || args[2]->IsArray()) {
per_isolate_opts.reset(new PerIsolateOptions()); per_isolate_opts.reset(new PerIsolateOptions());

2
src/util.h
View File

@ -316,7 +316,7 @@ class KVStore {
v8::Local<v8::String> key) const = 0; v8::Local<v8::String> key) const = 0;
virtual int32_t Query(const char* 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 void Delete(v8::Isolate* isolate, v8::Local<v8::String> key) = 0;
virtual v8::Local<v8::Array> Enumerate(v8::Isolate* isolate) const = 0; virtual v8::MaybeLocal<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const; virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const;
virtual v8::Maybe<void> AssignFromObject(v8::Local<v8::Context> context, virtual v8::Maybe<void> AssignFromObject(v8::Local<v8::Context> context,