src: rework (mostly internal) functions to use Maybes

Rework all affected functions to use Maybes, thus improving error
handling substantially in internal functions, API functions as well as
utilities.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: https://github.com/nodejs/node/pull/21935
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

1 files changed, 71 insertions, 54 deletions

diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
index ba29aaeef03..77449b9569d 100644
--- a/src/spawn_sync.cc
+++ b/src/spawn_sync.cc
@@ -37,7 +37,11 @@ using v8::FunctionCallbackInfo;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
+using v8::Maybe;
+using v8::MaybeLocal;
+using v8::Nothing;
 using v8::Null;
 using v8::Number;
 using v8::Object;
@@ -372,7 +376,8 @@ void SyncProcessRunner::Spawn(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   env->PrintSyncTrace();
   SyncProcessRunner p(env);
-  Local<Value> result = p.Run(args[0]);
+  Local<Value> result;
+  if (!p.Run(args[0]).ToLocal(&result)) return;
   args.GetReturnValue().Set(result);
 }
 
@@ -430,22 +435,21 @@ Environment* SyncProcessRunner::env() const {
   return env_;
 }
 
-
-Local<Object> SyncProcessRunner::Run(Local<Value> options) {
+MaybeLocal<Object> SyncProcessRunner::Run(Local<Value> options) {
   EscapableHandleScope scope(env()->isolate());
 
   CHECK_EQ(lifecycle_, kUninitialized);
 
-  TryInitializeAndRunLoop(options);
+  Maybe<bool> r = TryInitializeAndRunLoop(options);
   CloseHandlesAndDeleteLoop();
+  if (r.IsNothing()) return MaybeLocal<Object>();
 
   Local<Object> result = BuildResultObject();
 
   return scope.Escape(result);
 }
 
-
-void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
+Maybe<bool> SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
   int r;
 
   // There is no recovery from failure inside TryInitializeAndRunLoop - the
@@ -454,18 +458,24 @@ void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
   lifecycle_ = kInitialized;
 
   uv_loop_ = new uv_loop_t;
-  if (uv_loop_ == nullptr)
-    return SetError(UV_ENOMEM);
+  if (uv_loop_ == nullptr) {
+    SetError(UV_ENOMEM);
+    return Just(false);
+  }
   CHECK_EQ(uv_loop_init(uv_loop_), 0);
 
-  r = ParseOptions(options);
-  if (r < 0)
-    return SetError(r);
+  if (!ParseOptions(options).To(&r)) return Nothing<bool>();
+  if (r < 0) {
+    SetError(r);
+    return Just(false);
+  }
 
   if (timeout_ > 0) {
     r = uv_timer_init(uv_loop_, &uv_timer_);
-    if (r < 0)
-      return SetError(r);
+    if (r < 0) {
+      SetError(r);
+      return Just(false);
+    }
 
     uv_unref(reinterpret_cast<uv_handle_t*>(&uv_timer_));
 
@@ -477,22 +487,28 @@ void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
     // which implicitly stops it, so there is no risk that the timeout callback
     // runs when the process didn't start.
     r = uv_timer_start(&uv_timer_, KillTimerCallback, timeout_, 0);
-    if (r < 0)
-      return SetError(r);
+    if (r < 0) {
+      SetError(r);
+      return Just(false);
+    }
   }
 
   uv_process_options_.exit_cb = ExitCallback;
   r = uv_spawn(uv_loop_, &uv_process_, &uv_process_options_);
-  if (r < 0)
-    return SetError(r);
+  if (r < 0) {
+    SetError(r);
+    return Just(false);
+  }
   uv_process_.data = this;
 
   for (uint32_t i = 0; i < stdio_count_; i++) {
     SyncProcessStdioPipe* h = stdio_pipes_[i].get();
     if (h != nullptr) {
       r = h->Start();
-      if (r < 0)
-        return SetPipeError(r);
+      if (r < 0) {
+        SetPipeError(r);
+        return Just(false);
+      }
     }
   }
 
@@ -503,6 +519,7 @@ void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
 
   // If we get here the process should have exited.
   CHECK_GE(exit_status_, 0);
+  return Just(true);
 }
 
 
@@ -724,46 +741,41 @@ Local<Array> SyncProcessRunner::BuildOutputArray() {
   return scope.Escape(js_output);
 }
 
-
-int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
+Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> js_value) {
   HandleScope scope(env()->isolate());
   int r;
 
-  if (!js_value->IsObject())
-    return UV_EINVAL;
+  if (!js_value->IsObject()) return Just<int>(UV_EINVAL);
 
   Local<Context> context = env()->context();
   Local<Object> js_options = js_value.As<Object>();
 
   Local<Value> js_file =
       js_options->Get(context, env()->file_string()).ToLocalChecked();
-  r = CopyJsString(js_file, &file_buffer_);
-  if (r < 0)
-    return r;
+  if (!CopyJsString(js_file, &file_buffer_).To(&r)) return Nothing<int>();
+  if (r < 0) return Just(r);
   uv_process_options_.file = file_buffer_;
 
   Local<Value> js_args =
       js_options->Get(context, env()->args_string()).ToLocalChecked();
-  r = CopyJsStringArray(js_args, &args_buffer_);
-  if (r < 0)
-    return r;
+  if (!CopyJsStringArray(js_args, &args_buffer_).To(&r)) return Nothing<int>();
+  if (r < 0) return Just(r);
   uv_process_options_.args = reinterpret_cast<char**>(args_buffer_);
 
   Local<Value> js_cwd =
       js_options->Get(context, env()->cwd_string()).ToLocalChecked();
   if (IsSet(js_cwd)) {
-    r = CopyJsString(js_cwd, &cwd_buffer_);
-    if (r < 0)
-      return r;
+    if (!CopyJsString(js_cwd, &cwd_buffer_).To(&r)) return Nothing<int>();
+    if (r < 0) return Just(r);
     uv_process_options_.cwd = cwd_buffer_;
   }
 
   Local<Value> js_env_pairs =
       js_options->Get(context, env()->env_pairs_string()).ToLocalChecked();
   if (IsSet(js_env_pairs)) {
-    r = CopyJsStringArray(js_env_pairs, &env_buffer_);
-    if (r < 0)
-      return r;
+    if (!CopyJsStringArray(js_env_pairs, &env_buffer_).To(&r))
+      return Nothing<int>();
+    if (r < 0) return Just(r);
 
     uv_process_options_.env = reinterpret_cast<char**>(env_buffer_);
   }
@@ -827,10 +839,9 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
   Local<Value> js_stdio =
       js_options->Get(context, env()->stdio_string()).ToLocalChecked();
   r = ParseStdioOptions(js_stdio);
-  if (r < 0)
-    return r;
+  if (r < 0) return Just(r);
 
-  return 0;
+  return Just(0);
 }
 
 
@@ -970,9 +981,8 @@ bool SyncProcessRunner::IsSet(Local<Value> value) {
   return !value->IsUndefined() && !value->IsNull();
 }
 
-
-int SyncProcessRunner::CopyJsString(Local<Value> js_value,
-                                    const char** target) {
+Maybe<int> SyncProcessRunner::CopyJsString(Local<Value> js_value,
+                                           const char** target) {
   Isolate* isolate = env()->isolate();
   Local<String> js_string;
   size_t size, written;
@@ -980,12 +990,14 @@ int SyncProcessRunner::CopyJsString(Local<Value> js_value,
 
   if (js_value->IsString())
     js_string = js_value.As<String>();
-  else
-    js_string = js_value->ToString(env()->isolate()->GetCurrentContext())
-                    .ToLocalChecked();
+  else if (!js_value->ToString(env()->isolate()->GetCurrentContext())
+                .ToLocal(&js_string))
+    return Nothing<int>();
 
   // Include space for null terminator byte.
-  size = StringBytes::StorageSize(isolate, js_string, UTF8) + 1;
+  if (!StringBytes::StorageSize(isolate, js_string, UTF8).To(&size))
+    return Nothing<int>();
+  size += 1;
 
   buffer = new char[size];
 
@@ -993,12 +1005,11 @@ int SyncProcessRunner::CopyJsString(Local<Value> js_value,
   buffer[written] = '\0';
 
   *target = buffer;
-  return 0;
+  return Just(0);
 }
 
-
-int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
-                                         char** target) {
+Maybe<int> SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
+                                                char** target) {
   Isolate* isolate = env()->isolate();
   Local<Array> js_array;
   uint32_t length;
@@ -1006,8 +1017,7 @@ int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
   char** list;
   char* buffer;
 
-  if (!js_value->IsArray())
-    return UV_EINVAL;
+  if (!js_value->IsArray()) return Just<int>(UV_EINVAL);
 
   Local<Context> context = env()->context();
   js_array = js_value.As<Array>()->Clone().As<Array>();
@@ -1025,15 +1035,22 @@ int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
   for (uint32_t i = 0; i < length; i++) {
     auto value = js_array->Get(context, i).ToLocalChecked();
 
-    if (!value->IsString())
+    if (!value->IsString()) {
+      Local<String> string;
+      if (!value->ToString(env()->isolate()->GetCurrentContext())
+               .ToLocal(&string))
+        return Nothing<int>();
       js_array
           ->Set(context,
                 i,
                 value->ToString(env()->isolate()->GetCurrentContext())
                     .ToLocalChecked())
           .FromJust();
+    }
 
-    data_size += StringBytes::StorageSize(isolate, value, UTF8) + 1;
+    Maybe<size_t> maybe_size = StringBytes::StorageSize(isolate, value, UTF8);
+    if (maybe_size.IsNothing()) return Nothing<int>();
+    data_size += maybe_size.FromJust() + 1;
     data_size = ROUND_UP(data_size, sizeof(void*));
   }
 
@@ -1057,7 +1074,7 @@ int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
   list[length] = nullptr;
 
   *target = buffer;
-  return 0;
+  return Just(0);
 }