Skip to content

Commit 56380df

Browse files
jasnellRafaelGSS
jasnell
authored and
RafaelGSS
committed
src: improve error handling in process.env handling
PR-URL: #57707 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
1 parent db8b29d commit 56380df

File tree

3 files changed

+52
-26
lines changed

3 files changed

+52
-26
lines changed

‎src/node_env_var.cc

Lines changed: 47 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ using v8::Maybe;
2626
using v8::MaybeLocal;
2727
using v8::Name;
2828
using v8::NamedPropertyHandlerConfiguration;
29-
using v8::NewStringType;
3029
using v8::Nothing;
3130
using v8::Object;
3231
using v8::ObjectTemplate;
@@ -45,7 +44,7 @@ class RealEnvStore final : public KVStore {
4544
int32_t Query(Isolate* isolate, Local<String> key) const override;
4645
int32_t Query(const char* key) const override;
4746
void Delete(Isolate* isolate, Local<String> key) override;
48-
Local<Array> Enumerate(Isolate* isolate) const override;
47+
MaybeLocal<Array> Enumerate(Isolate* isolate) const override;
4948
};
5049

5150
class MapKVStore final : public KVStore {
@@ -56,7 +55,7 @@ class MapKVStore final : public KVStore {
5655
int32_t Query(Isolate* isolate, Local<String> key) const override;
5756
int32_t Query(const char* key) const override;
5857
void Delete(Isolate* isolate, Local<String> key) override;
59-
Local<Array> Enumerate(Isolate* isolate) const override;
58+
MaybeLocal<Array> Enumerate(Isolate* isolate) const override;
6059

6160
std::shared_ptr<KVStore> Clone(Isolate* isolate) const override;
6261

@@ -131,8 +130,12 @@ MaybeLocal<String> RealEnvStore::Get(Isolate* isolate,
131130

132131
if (value.has_value()) {
133132
std::string val = value.value();
134-
return String::NewFromUtf8(
135-
isolate, val.data(), NewStringType::kNormal, val.size());
133+
Local<Value> ret;
134+
if (!ToV8Value(isolate->GetCurrentContext(), val).ToLocal(&ret)) {
135+
return {};
136+
}
137+
DCHECK(ret->IsString());
138+
return ret.As<String>();
136139
}
137140

138141
return MaybeLocal<String>();
@@ -188,7 +191,7 @@ void RealEnvStore::Delete(Isolate* isolate, Local<String> property) {
188191
DateTimeConfigurationChangeNotification(isolate, key);
189192
}
190193

191-
Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
194+
MaybeLocal<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
192195
Mutex::ScopedLock lock(per_process::env_var_mutex);
193196
uv_env_item_t* items;
194197
int count;
@@ -203,12 +206,12 @@ Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
203206
// If the key starts with '=' it is a hidden environment variable.
204207
if (items[i].name[0] == '=') continue;
205208
#endif
206-
MaybeLocal<String> str = String::NewFromUtf8(isolate, items[i].name);
207-
if (str.IsEmpty()) {
209+
Local<Value> str;
210+
if (!String::NewFromUtf8(isolate, items[i].name).ToLocal(&str)) {
208211
isolate->ThrowException(ERR_STRING_TOO_LONG(isolate));
209-
return Local<Array>();
212+
return {};
210213
}
211-
env_v[env_v_index++] = str.ToLocalChecked();
214+
env_v[env_v_index++] = str;
212215
}
213216

214217
return Array::New(isolate, env_v.out(), env_v_index);
@@ -219,14 +222,22 @@ std::shared_ptr<KVStore> KVStore::Clone(Isolate* isolate) const {
219222
Local<Context> context = isolate->GetCurrentContext();
220223

221224
std::shared_ptr<KVStore> copy = KVStore::CreateMapKVStore();
222-
Local<Array> keys = Enumerate(isolate);
225+
Local<Array> keys;
226+
if (!Enumerate(isolate).ToLocal(&keys)) {
227+
return nullptr;
228+
}
223229
uint32_t keys_length = keys->Length();
224230
for (uint32_t i = 0; i < keys_length; i++) {
225-
Local<Value> key = keys->Get(context, i).ToLocalChecked();
231+
Local<Value> key;
232+
Local<Value> value;
233+
if (!keys->Get(context, i).ToLocal(&key)) {
234+
return nullptr;
235+
}
226236
CHECK(key->IsString());
227-
copy->Set(isolate,
228-
key.As<String>(),
229-
Get(isolate, key.As<String>()).ToLocalChecked());
237+
if (!Get(isolate, key.As<String>()).ToLocal(&value)) {
238+
return nullptr;
239+
}
240+
copy->Set(isolate, key.As<String>(), value.As<String>());
230241
}
231242
return copy;
232243
}
@@ -242,8 +253,12 @@ MaybeLocal<String> MapKVStore::Get(Isolate* isolate, Local<String> key) const {
242253
std::optional<std::string> value = Get(*str);
243254
if (!value.has_value()) return MaybeLocal<String>();
244255
std::string val = value.value();
245-
return String::NewFromUtf8(
246-
isolate, val.data(), NewStringType::kNormal, val.size());
256+
Local<Value> ret;
257+
if (!ToV8Value(isolate->GetCurrentContext(), val).ToLocal(&ret)) {
258+
return {};
259+
}
260+
DCHECK(ret->IsString());
261+
return ret.As<String>();
247262
}
248263

249264
void MapKVStore::Set(Isolate* isolate, Local<String> key, Local<String> value) {
@@ -272,15 +287,16 @@ void MapKVStore::Delete(Isolate* isolate, Local<String> key) {
272287
map_.erase(std::string(*str, str.length()));
273288
}
274289

275-
Local<Array> MapKVStore::Enumerate(Isolate* isolate) const {
290+
MaybeLocal<Array> MapKVStore::Enumerate(Isolate* isolate) const {
276291
Mutex::ScopedLock lock(mutex_);
277292
LocalVector<Value> values(isolate);
278293
values.reserve(map_.size());
279294
for (const auto& pair : map_) {
280-
values.emplace_back(
281-
String::NewFromUtf8(isolate, pair.first.data(),
282-
NewStringType::kNormal, pair.first.size())
283-
.ToLocalChecked());
295+
Local<Value> val;
296+
if (!ToV8Value(isolate->GetCurrentContext(), pair.first).ToLocal(&val)) {
297+
return {};
298+
}
299+
values.emplace_back(val);
284300
}
285301
return Array::New(isolate, values.data(), values.size());
286302
}
@@ -324,7 +340,10 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
324340
v8::Local<v8::Context> context,
325341
v8::Local<v8::Object> object) {
326342
HandleScope scope(isolate);
327-
Local<Array> keys = Enumerate(isolate);
343+
Local<Array> keys;
344+
if (!Enumerate(isolate).ToLocal(&keys)) {
345+
return Nothing<void>();
346+
}
328347
uint32_t keys_length = keys->Length();
329348
for (uint32_t i = 0; i < keys_length; i++) {
330349
Local<Value> key;
@@ -421,6 +440,7 @@ static Intercepted EnvGetter(Local<Name> property,
421440
TraceEnvVar(env, "get", property.As<String>());
422441

423442
if (has_env) {
443+
// ToLocalChecked here is ok since we check IsEmpty above.
424444
info.GetReturnValue().Set(value_string.ToLocalChecked());
425445
return Intercepted::kYes;
426446
}
@@ -502,8 +522,10 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
502522

503523
TraceEnvVar(env, "enumerate environment variables");
504524

505-
info.GetReturnValue().Set(
506-
env->env_vars()->Enumerate(env->isolate()));
525+
Local<Array> ret;
526+
if (env->env_vars()->Enumerate(env->isolate()).ToLocal(&ret)) {
527+
info.GetReturnValue().Set(ret);
528+
}
507529
}
508530

509531
static Intercepted EnvDefiner(Local<Name> property,

‎src/node_worker.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,10 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
545545
env_vars = env->env_vars();
546546
}
547547

548+
if (!env_vars) {
549+
THROW_ERR_OPERATION_FAILED(env, "Failed to copy environment variables");
550+
}
551+
548552
if (args[1]->IsObject() || args[2]->IsArray()) {
549553
per_isolate_opts.reset(new PerIsolateOptions());
550554

‎src/util.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ class KVStore {
316316
v8::Local<v8::String> key) const = 0;
317317
virtual int32_t Query(const char* key) const = 0;
318318
virtual void Delete(v8::Isolate* isolate, v8::Local<v8::String> key) = 0;
319-
virtual v8::Local<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
319+
virtual v8::MaybeLocal<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
320320

321321
virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const;
322322
virtual v8::Maybe<void> AssignFromObject(v8::Local<v8::Context> context,

0 commit comments

Comments
 (0)