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.
nodejs · ryzokuken · Aug 29, 2018 · Aug 29, 2018 · Aug 29, 2018 · Aug 29, 2018
commit 96fd305be4e638414edd4fe4711dca0c55e62e1d
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -236,7 +236,9 @@ MaybeLocal<Object> New(Isolate* isolate,
                        enum encoding enc) {
   EscapableHandleScope scope(isolate);
 
-  const size_t length = StringBytes::Size(isolate, string, enc);
+  size_t length;
+  if (!StringBytes::Size(isolate, string, enc).To(&length))
+    return Local<Object>();
   size_t actual = 0;
   char* data = nullptr;
 
@@ -828,7 +830,8 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
   const size_t haystack_length = (enc == UCS2) ?
       ts_obj_length &~ 1 : ts_obj_length;  // NOLINT(whitespace/operators)
 
-  const size_t needle_length = StringBytes::Size(isolate, needle, enc);
+  size_t needle_length;
+  if (!StringBytes::Size(isolate, needle, enc).To(&needle_length)) return;
 
   int64_t opt_offset = IndexOfOffset(haystack_length,
                                      offset_i64,
@@ -868,7 +871,7 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
 
     if (IsBigEndian()) {
       StringBytes::InlineDecoder decoder;
-      decoder.Decode(env, needle, args[3], UCS2);
+      if (decoder.Decode(env, needle, args[3], UCS2).IsNothing()) return;
       const uint16_t* decoded_string =
           reinterpret_cast<const uint16_t*>(decoder.out());
 

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3055,7 +3055,8 @@ void CipherBase::Update(const FunctionCallbackInfo<Value>& args) {
   // Only copy the data if we have to, because it's a string
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8))
+    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
+             .FromMaybe(false))
       return;
     r = cipher->Update(decoder.out(), decoder.size(), &out, &out_len);
   } else {
@@ -3241,7 +3242,8 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo<Value>& args) {
   bool r = false;
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (decoder.Decode(env, args[0].As<String>(), args[1], UTF8)) {
+    if (decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
+            .FromMaybe(false)) {
       r = hmac->HmacUpdate(decoder.out(), decoder.size());
     }
   } else {
@@ -3348,7 +3350,8 @@ void Hash::HashUpdate(const FunctionCallbackInfo<Value>& args) {
   bool r = true;
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)) {
+    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
+             .FromMaybe(false)) {
       args.GetReturnValue().Set(false);
       return;
     }

diff --git a/src/node_encoding.cc b/src/node_encoding.cc
@@ -121,7 +121,7 @@ ssize_t DecodeBytes(Isolate* isolate,
                     enum encoding encoding) {
   HandleScope scope(isolate);
 
-  return StringBytes::Size(isolate, val, encoding);
+  return StringBytes::Size(isolate, val, encoding).FromMaybe(-1);
 }
 
 // Returns number of bytes written.

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1819,7 +1819,7 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
 
   if (is_async) {  // write(fd, string, pos, enc, req)
     CHECK_NOT_NULL(req_wrap_async);
-    len = StringBytes::StorageSize(env->isolate(), value, enc);
+    if (!StringBytes::StorageSize(env->isolate(), value, enc).To(&len)) return;
     FSReqBase::FSReqBuffer& stack_buffer =
         req_wrap_async->Init("write", len, enc);
     // StorageSize may return too large a char, so correct the actual length
@@ -1847,7 +1847,8 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
     FSReqWrapSync req_wrap_sync;
     FSReqBase::FSReqBuffer stack_buffer;
     if (buf == nullptr) {
-      len = StringBytes::StorageSize(env->isolate(), value, enc);
+      if (!StringBytes::StorageSize(env->isolate(), value, enc).To(&len))
+        return;
       stack_buffer.AllocateSufficientStorage(len + 1);
       // StorageSize may return too large a char, so correct the actual length
       // by the write size

diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
@@ -372,7 +372,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 +431,22 @@ Environment* SyncProcessRunner::env() const {
   return env_;
 }
 
-
-Local<Object> SyncProcessRunner::Run(Local<Value> options) {
+v8::MaybeLocal<Object> SyncProcessRunner::Run(Local<Value> options) {
   EscapableHandleScope scope(env()->isolate());
 
   CHECK_EQ(lifecycle_, kUninitialized);
 
-  TryInitializeAndRunLoop(options);
+  v8::Maybe<bool> r = TryInitializeAndRunLoop(options);
   CloseHandlesAndDeleteLoop();
+  if (r.IsNothing()) return v8::MaybeLocal<Object>();
 
   Local<Object> result = BuildResultObject();
 
   return scope.Escape(result);
 }
 
-
-void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
+v8::Maybe<bool> SyncProcessRunner::TryInitializeAndRunLoop(
+    Local<Value> options) {
   int r;
 
   // There is no recovery from failure inside TryInitializeAndRunLoop - the
@@ -454,18 +455,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 v8::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 v8::Nothing<bool>();
+  if (r < 0) {
+    SetError(r);
+    return v8::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 v8::Just(false);
+    }
 
     uv_unref(reinterpret_cast<uv_handle_t*>(&uv_timer_));
 
@@ -477,22 +484,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 v8::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 v8::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 v8::Just(false);
+      }
     }
   }
 
@@ -503,6 +516,7 @@ void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
 
   // If we get here the process should have exited.
   CHECK_GE(exit_status_, 0);
+  return v8::Just(true);
 }
 
 
@@ -724,46 +738,42 @@ Local<Array> SyncProcessRunner::BuildOutputArray() {
   return scope.Escape(js_output);
 }
 
-
-int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
+v8::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 v8::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 v8::Nothing<int>();
+  if (r < 0) return v8::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 v8::Nothing<int>();
+  if (r < 0) return v8::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 v8::Nothing<int>();
+    if (r < 0) return v8::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 v8::Nothing<int>();
+    if (r < 0) return v8::Just(r);
 
     uv_process_options_.env = reinterpret_cast<char**>(env_buffer_);
   }
@@ -827,10 +837,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 v8::Just(r);
 
-  return 0;
+  return v8::Just(0);
 }
 
 
@@ -970,44 +979,43 @@ bool SyncProcessRunner::IsSet(Local<Value> value) {
   return !value->IsUndefined() && !value->IsNull();
 }
 
-
-int SyncProcessRunner::CopyJsString(Local<Value> js_value,
-                                    const char** target) {
+v8::Maybe<int> SyncProcessRunner::CopyJsString(Local<Value> js_value,
+                                               const char** target) {
   Isolate* isolate = env()->isolate();
   Local<String> js_string;
   size_t size, written;
   char* buffer;
 
   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 v8::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 v8::Nothing<int>();
+  size += 1;
 
   buffer = new char[size];
 
   written = StringBytes::Write(isolate, buffer, -1, js_string, UTF8);
   buffer[written] = '\0';
 
   *target = buffer;
-  return 0;
+  return v8::Just(0);
 }
 
-
-int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
-                                         char** target) {
+v8::Maybe<int> SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
+                                                    char** target) {
   Isolate* isolate = env()->isolate();
   Local<Array> js_array;
   uint32_t length;
   size_t list_size, data_size, data_offset;
   char** list;
   char* buffer;
 
-  if (!js_value->IsArray())
-    return UV_EINVAL;
+  if (!js_value->IsArray()) return v8::Just<int>(UV_EINVAL);
 
   Local<Context> context = env()->context();
   js_array = js_value.As<Array>()->Clone().As<Array>();
@@ -1025,15 +1033,23 @@ 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()) {
+      v8::Local<String> string;
+      if (!value->ToString(env()->isolate()->GetCurrentContext())
+               .ToLocal(&string))
+        return v8::Nothing<int>();
       js_array
           ->Set(context,
                 i,
                 value->ToString(env()->isolate()->GetCurrentContext())
                     .ToLocalChecked())
           .FromJust();
+    }
 
-    data_size += StringBytes::StorageSize(isolate, value, UTF8) + 1;
+    v8::Maybe<size_t> maybe_size =
+        StringBytes::StorageSize(isolate, value, UTF8);
+    if (maybe_size.IsNothing()) return v8::Nothing<int>();
+    data_size += maybe_size.FromJust() + 1;
     data_size = ROUND_UP(data_size, sizeof(void*));
   }
 
@@ -1057,7 +1073,7 @@ int SyncProcessRunner::CopyJsStringArray(Local<Value> js_value,
   list[length] = nullptr;
 
   *target = buffer;
-  return 0;
+  return v8::Just(0);
 }
 
 

diff --git a/src/spawn_sync.h b/src/spawn_sync.h
@@ -152,8 +152,8 @@ class SyncProcessRunner {
 
   inline Environment* env() const;
 
-  v8::Local<v8::Object> Run(v8::Local<v8::Value> options);
-  void TryInitializeAndRunLoop(v8::Local<v8::Value> options);
+  v8::MaybeLocal<v8::Object> Run(v8::Local<v8::Value> options);
+  v8::Maybe<bool> TryInitializeAndRunLoop(v8::Local<v8::Value> options);
   void CloseHandlesAndDeleteLoop();
 
   void CloseStdioPipes();
@@ -172,7 +172,7 @@ class SyncProcessRunner {
   v8::Local<v8::Object> BuildResultObject();
   v8::Local<v8::Array> BuildOutputArray();
 
-  int ParseOptions(v8::Local<v8::Value> js_value);
+  v8::Maybe<int> ParseOptions(v8::Local<v8::Value> js_value);
   int ParseStdioOptions(v8::Local<v8::Value> js_value);
   int ParseStdioOption(int child_fd, v8::Local<v8::Object> js_stdio_option);
 
@@ -184,8 +184,10 @@ class SyncProcessRunner {
   inline int AddStdioInheritFD(uint32_t child_fd, int inherit_fd);
 
   static bool IsSet(v8::Local<v8::Value> value);
-  int CopyJsString(v8::Local<v8::Value> js_value, const char** target);
-  int CopyJsStringArray(v8::Local<v8::Value> js_value, char** target);
+  v8::Maybe<int> CopyJsString(v8::Local<v8::Value> js_value,
+                              const char** target);
+  v8::Maybe<int> CopyJsStringArray(v8::Local<v8::Value> js_value,
+                                   char** target);
 
   static void ExitCallback(uv_process_t* handle,
                            int64_t exit_status,