src: improve error handling in process.env handling

Author: jasnell

src/node_env_var.cc

@@ -45,7 +45,7 @@ class RealEnvStore final : public KVStore {
   int32_t Query(Isolate* isolate, Local<String> key) const override;
   int32_t Query(const char* key) const 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 {
@@ -56,7 +56,7 @@ class MapKVStore final : public KVStore {
   int32_t Query(Isolate* isolate, Local<String> key) const override;
   int32_t Query(const char* key) const 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;
 
@@ -188,7 +188,7 @@ void RealEnvStore::Delete(Isolate* isolate, Local<String> property) {
   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);
   uv_env_item_t* items;
   int count;
@@ -203,12 +203,12 @@ Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
     // If the key starts with '=' it is a hidden environment variable.
     if (items[i].name[0] == '=') continue;
 #endif
-    MaybeLocal<String> str = String::NewFromUtf8(isolate, items[i].name);
-    if (str.IsEmpty()) {
+    Local<Value> str;
+    if (!String::NewFromUtf8(isolate, items[i].name).ToLocal(&str)) {
       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);
@@ -219,14 +219,22 @@ std::shared_ptr<KVStore> KVStore::Clone(Isolate* isolate) const {
   Local<Context> context = isolate->GetCurrentContext();
 
   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();
   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());
-    copy->Set(isolate,
-              key.As<String>(),
-              Get(isolate, key.As<String>()).ToLocalChecked());
+    if (!Get(isolate, key.As<String>()).ToLocal(&value)) {
+      return nullptr;
+    }
+    copy->Set(isolate, key.As<String>(), value.As<String>());
   }
   return copy;
 }
@@ -272,15 +280,20 @@ void MapKVStore::Delete(Isolate* isolate, Local<String> key) {
   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_);
   LocalVector<Value> values(isolate);
   values.reserve(map_.size());
   for (const auto& pair : map_) {
-    values.emplace_back(
-        String::NewFromUtf8(isolate, pair.first.data(),
-                            NewStringType::kNormal, pair.first.size())
-            .ToLocalChecked());
+    Local<Value> val;
+    if (!String::NewFromUtf8(isolate,
+                             pair.first.data(),
+                             NewStringType::kNormal,
+                             pair.first.size())
+             .ToLocal(&val)) {
+      return {};
+    }
+    values.emplace_back(val);
   }
   return Array::New(isolate, values.data(), values.size());
 }
@@ -324,7 +337,10 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
                                     v8::Local<v8::Context> context,
                                     v8::Local<v8::Object> object) {
   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();
   for (uint32_t i = 0; i < keys_length; i++) {
     Local<Value> key;
@@ -421,6 +437,7 @@ static Intercepted EnvGetter(Local<Name> property,
   TraceEnvVar(env, "get", property.As<String>());
 
   if (has_env) {
+    // ToLocalChecked here is ok since we check IsEmpty above.
     info.GetReturnValue().Set(value_string.ToLocalChecked());
     return Intercepted::kYes;
   }
@@ -502,8 +519,10 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
 
   TraceEnvVar(env, "enumerate environment variables");
 
-  info.GetReturnValue().Set(
-      env->env_vars()->Enumerate(env->isolate()));
+  Local<Array> ret;
+  if (env->env_vars()->Enumerate(env->isolate()).ToLocal(&ret)) {
+    info.GetReturnValue().Set(ret);
+  }
 }
 
 static Intercepted EnvDefiner(Local<Name> property,

src/node_worker.cc

@@ -545,6 +545,10 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
     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()) {
     per_isolate_opts.reset(new PerIsolateOptions());
 

src/util.h

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