src: do proper StringBytes error handling

- Return `MaybeLocal`s from `StringBytes::Encode` - Add an `error` out parameter to pass JS exceptions to the callers (instead of directly throwing) - Simplify some of the string generation methods in `string_bytes.cc` by unifying the `EXTERN_APEX` logic - Reduce usage of deprecated V8 APIs. - Remove error handling logic from JS, the `buffer.*Slice()` methods now throw errors themselves. - Left TODO comments for future semver-major error message improvements. This paves the way for better error messages coming out of the StringBytes methods. Ref: nodejs#3175 PR-URL: nodejs#12765 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
anchnk · May 6, 2017 · 143c891 · 143c891
1 parent 09325ba
commit 143c891
Show file tree

Hide file tree

Showing 9 changed files with 323 additions and 186 deletions.
diff --git a/lib/buffer.js b/lib/buffer.js
@@ -567,34 +567,30 @@ Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
 // This behaves neither like String nor Uint8Array in that we set start/end
 // to their upper/lower bounds if the value passed is out of range.
 Buffer.prototype.toString = function(encoding, start, end) {
-  var result;
   if (arguments.length === 0) {
-    result = this.utf8Slice(0, this.length);
-  } else {
-    const len = this.length;
-    if (len === 0)
-      return '';
+    return this.utf8Slice(0, this.length);
+  }
 
-    if (!start || start < 0)
-      start = 0;
-    else if (start >= len)
-      return '';
+  const len = this.length;
+  if (len === 0)
+    return '';
 
-    if (end === undefined || end > len)
-      end = len;
-    else if (end <= 0)
-      return '';
+  if (!start || start < 0)
+    start = 0;
+  else if (start >= len)
+    return '';
 
-    start |= 0;
-    end |= 0;
+  if (end === undefined || end > len)
+    end = len;
+  else if (end <= 0)
+    return '';
 
-    if (end <= start)
-      return '';
-    result = stringSlice(this, encoding, start, end);
-  }
-  if (result === undefined)
-    throw new Error('"toString()" failed');
-  return result;
+  start |= 0;
+  end |= 0;
+
+  if (end <= start)
+    return '';
+  return stringSlice(this, encoding, start, end);
 };
 
 

diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc
@@ -39,6 +39,7 @@ using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Local;
+using v8::MaybeLocal;
 using v8::Object;
 using v8::String;
 using v8::Value;
@@ -187,17 +188,20 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
   };
 
   if (filename != nullptr) {
-    Local<Value> fn = StringBytes::Encode(env->isolate(),
-                                          filename,
-                                          wrap->encoding_);
+    Local<Value> error;
+    MaybeLocal<Value> fn = StringBytes::Encode(env->isolate(),
+                                               filename,
+                                               wrap->encoding_,
+                                               &error);
     if (fn.IsEmpty()) {
       argv[0] = Integer::New(env->isolate(), UV_EINVAL);
       argv[2] = StringBytes::Encode(env->isolate(),
                                     filename,
                                     strlen(filename),
-                                    BUFFER);
+                                    BUFFER,
+                                    &error).ToLocalChecked();
     } else {
-      argv[2] = fn;
+      argv[2] = fn.ToLocalChecked();
     }
   }
 

diff --git a/src/node.cc b/src/node.cc
@@ -1570,11 +1570,15 @@ Local<Value> Encode(Isolate* isolate,
                     size_t len,
                     enum encoding encoding) {
   CHECK_NE(encoding, UCS2);
-  return StringBytes::Encode(isolate, buf, len, encoding);
+  Local<Value> error;
+  return StringBytes::Encode(isolate, buf, len, encoding, &error)
+      .ToLocalChecked();
 }
 
 Local<Value> Encode(Isolate* isolate, const uint16_t* buf, size_t len) {
-  return StringBytes::Encode(isolate, buf, len);
+  Local<Value> error;
+  return StringBytes::Encode(isolate, buf, len, &error)
+      .ToLocalChecked();
 }
 
 // Returns -1 if the handle was not valid for decoding

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -465,14 +465,26 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {
 
   SLICE_START_END(args[0], args[1], ts_obj_length)
 
-  args.GetReturnValue().Set(
-      StringBytes::Encode(isolate, ts_obj_data + start, length, encoding));
+  Local<Value> error;
+  MaybeLocal<Value> ret =
+      StringBytes::Encode(isolate,
+                          ts_obj_data + start,
+                          length,
+                          encoding,
+                          &error);
+  if (ret.IsEmpty()) {
+    CHECK(!error.IsEmpty());
+    isolate->ThrowException(error);
+    return;
+  }
+  args.GetReturnValue().Set(ret.ToLocalChecked());
 }
 
 
 template <>
 void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = args.GetIsolate();
+  Environment* env = Environment::GetCurrent(isolate);
 
   THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
   SPREAD_BUFFER_ARG(args.This(), ts_obj);
@@ -509,10 +521,22 @@ void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
     buf = reinterpret_cast<const uint16_t*>(data);
   }
 
-  args.GetReturnValue().Set(StringBytes::Encode(env->isolate(), buf, length));
+  Local<Value> error;
+  MaybeLocal<Value> ret =
+      StringBytes::Encode(isolate,
+                          buf,
+                          length,
+                          &error);
 
   if (release)
     delete[] buf;
+
+  if (ret.IsEmpty()) {
+    CHECK(!error.IsEmpty());
+    isolate->ThrowException(error);
+    return;
+  }
+  args.GetReturnValue().Set(ret.ToLocalChecked());
 }
 
 

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -104,6 +104,7 @@ using v8::Integer;
 using v8::Isolate;
 using v8::Local;
 using v8::Maybe;
+using v8::MaybeLocal;
 using v8::Null;
 using v8::Object;
 using v8::Persistent;
@@ -3811,12 +3812,20 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
     md_len = 0;
   }
 
-  Local<Value> rc = StringBytes::Encode(env->isolate(),
-                                        reinterpret_cast<const char*>(md_value),
-                                        md_len,
-                                        encoding);
+  Local<Value> error;
+  MaybeLocal<Value> rc =
+      StringBytes::Encode(env->isolate(),
+                          reinterpret_cast<const char*>(md_value),
+                          md_len,
+                          encoding,
+                          &error);
   delete[] md_value;
-  args.GetReturnValue().Set(rc);
+  if (rc.IsEmpty()) {
+    CHECK(!error.IsEmpty());
+    env->isolate()->ThrowException(error);
+    return;
+  }
+  args.GetReturnValue().Set(rc.ToLocalChecked());
 }
 
 
@@ -3936,11 +3945,19 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
   EVP_MD_CTX_cleanup(&hash->mdctx_);
   hash->finalized_ = true;
 
-  Local<Value> rc = StringBytes::Encode(env->isolate(),
-                                        reinterpret_cast<const char*>(md_value),
-                                        md_len,
-                                        encoding);
-  args.GetReturnValue().Set(rc);
+  Local<Value> error;
+  MaybeLocal<Value> rc =
+      StringBytes::Encode(env->isolate(),
+                          reinterpret_cast<const char*>(md_value),
+                          md_len,
+                          encoding,
+                          &error);
+  if (rc.IsEmpty()) {
+    CHECK(!error.IsEmpty());
+    env->isolate()->ThrowException(error);
+    return;
+  }
+  args.GetReturnValue().Set(rc.ToLocalChecked());
 }