src: improve StringBytes error handling

src/api/encoding.cc

@@ -8,6 +8,8 @@ namespace node {
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
+using v8::MaybeLocal;
+using v8::TryCatch;
 using v8::Value;
 
 enum encoding ParseEncoding(const char* encoding,
@@ -133,20 +135,30 @@ enum encoding ParseEncoding(Isolate* isolate,
   return ParseEncoding(*encoding, default_encoding);
 }
 
+MaybeLocal<Value> TryEncode(Isolate* isolate,
+                            const char* buf,
+                            size_t len,
+                            enum encoding encoding) {
+  CHECK_NE(encoding, UCS2);
+  return StringBytes::Encode(isolate, buf, len, encoding);
+}
+
+MaybeLocal<Value> TryEncode(Isolate* isolate, const uint16_t* buf, size_t len) {
+  return StringBytes::Encode(isolate, buf, len).ToLocalChecked();
+}
+
 Local<Value> Encode(Isolate* isolate,
                     const char* buf,
                     size_t len,
                     enum encoding encoding) {
   CHECK_NE(encoding, UCS2);
-  Local<Value> error;
-  return StringBytes::Encode(isolate, buf, len, encoding, &error)
-      .ToLocalChecked();
+  TryCatch try_catch(isolate);
+  return StringBytes::Encode(isolate, buf, len, encoding).ToLocalChecked();
 }
 
 Local<Value> Encode(Isolate* isolate, const uint16_t* buf, size_t len) {
-  Local<Value> error;
-  return StringBytes::Encode(isolate, buf, len, &error)
-      .ToLocalChecked();
+  TryCatch try_catch(isolate);
+  return StringBytes::Encode(isolate, buf, len).ToLocalChecked();
 }
 
 // Returns -1 if the handle was not valid for decoding

src/crypto/crypto_ec.cc

@@ -800,17 +800,11 @@ bool ExportJWKEdKey(Environment* env,
                                        Local<Object> target,
                                        Local<String> key) {
     Local<Value> encoded;
-    Local<Value> error;
     if (!data) return false;
     const ncrypto::Buffer<const char> out = data;
-    if (!StringBytes::Encode(
-             env->isolate(), out.data, out.len, BASE64URL, &error)
-             .ToLocal(&encoded) ||
-        target->Set(env->context(), key, encoded).IsNothing()) {
-      if (!error.IsEmpty()) env->isolate()->ThrowException(error);
-      return false;
-    }
-    return true;
+    return StringBytes::Encode(env->isolate(), out.data, out.len, BASE64URL)
+               .ToLocal(&encoded) &&
+           target->Set(env->context(), key, encoded).IsJust();
   };
 
   return !(

src/crypto/crypto_hash.cc

@@ -252,19 +252,14 @@ void Hash::OneShotDigest(const FunctionCallbackInfo<Value>& args) {
     return ThrowCryptoError(env, ERR_get_error());
   }
 
-  Local<Value> error;
-  MaybeLocal<Value> rc =
-      StringBytes::Encode(env->isolate(),
+  Local<Value> ret;
+  if (StringBytes::Encode(env->isolate(),
                           static_cast<const char*>(output.get()),
                           output.size(),
-                          output_enc,
-                          &error);
-  if (rc.IsEmpty()) [[unlikely]] {
-    CHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return;
+                          output_enc)
+          .ToLocal(&ret)) {
+    args.GetReturnValue().Set(ret);
   }
-  args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
 }
 
 void Hash::Initialize(Environment* env, Local<Object> target) {
@@ -410,15 +405,12 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
     hash->digest_ = ByteSource::Allocated(data.release());
   }
 
-  Local<Value> error;
-  MaybeLocal<Value> rc = StringBytes::Encode(
-      env->isolate(), hash->digest_.data<char>(), len, encoding, &error);
-  if (rc.IsEmpty()) [[unlikely]] {
-    CHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return;
+  Local<Value> ret;
+  if (StringBytes::Encode(
+          env->isolate(), hash->digest_.data<char>(), len, encoding)
+          .ToLocal(&ret)) {
+    args.GetReturnValue().Set(ret);
   }
-  args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
 }
 
 HashConfig::HashConfig(HashConfig&& other) noexcept
@@ -541,19 +533,14 @@ void InternalVerifyIntegrity(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   if (digest_size != expected.size() ||
       CRYPTO_memcmp(digest, expected.data(), digest_size) != 0) {
-    Local<Value> error;
-    MaybeLocal<Value> rc =
-        StringBytes::Encode(env->isolate(),
+    Local<Value> ret;
+    if (StringBytes::Encode(env->isolate(),
                             reinterpret_cast<const char*>(digest),
                             digest_size,
-                            BASE64,
-                            &error);
-    if (rc.IsEmpty()) [[unlikely]] {
-      CHECK(!error.IsEmpty());
-      env->isolate()->ThrowException(error);
-      return;
+                            BASE64)
+            .ToLocal(&ret)) {
+      args.GetReturnValue().Set(ret);
     }
-    args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
   }
 }
 }  // namespace crypto

src/crypto/crypto_hmac.cc

@@ -145,19 +145,14 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
     hmac->ctx_.reset();
   }
 
-  Local<Value> error;
-  MaybeLocal<Value> rc =
-      StringBytes::Encode(env->isolate(),
+  Local<Value> ret;
+  if (StringBytes::Encode(env->isolate(),
                           reinterpret_cast<const char*>(md_value),
                           buf.len,
-                          encoding,
-                          &error);
-  if (rc.IsEmpty()) [[unlikely]] {
-    CHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return;
+                          encoding)
+          .ToLocal(&ret)) {
+    args.GetReturnValue().Set(ret);
   }
-  args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
 }
 
 HmacConfig::HmacConfig(HmacConfig&& other) noexcept

src/crypto/crypto_keys.cc

@@ -130,20 +130,13 @@ bool ExportJWKSecretKey(Environment* env,
                         Local<Object> target) {
   CHECK_EQ(key.GetKeyType(), kKeyTypeSecret);
 
-  Local<Value> error;
   Local<Value> raw;
-  if (!StringBytes::Encode(env->isolate(),
-                           key.GetSymmetricKey(),
-                           key.GetSymmetricKeySize(),
-                           BASE64URL,
-                           &error)
-           .ToLocal(&raw)) {
-    DCHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return false;
-  }
-
-  return target
+  return StringBytes::Encode(env->isolate(),
+                             key.GetSymmetricKey(),
+                             key.GetSymmetricKeySize(),
+                             BASE64URL)
+             .ToLocal(&raw) &&
+         target
              ->Set(env->context(), env->jwk_kty_string(), env->jwk_oct_string())
              .IsJust() &&
          target->Set(env->context(), env->jwk_k_string(), raw).IsJust();

src/crypto/crypto_spkac.cc

@@ -5,6 +5,7 @@
 #include "memory_tracker-inl.h"
 #include "ncrypto.h"
 #include "node.h"
+#include "string_bytes.h"
 #include "v8.h"
 
 namespace node {
@@ -52,18 +53,22 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
   ArrayBufferOrViewContents<char> input(args[0]);
   if (input.empty()) return args.GetReturnValue().SetEmptyString();
 
-  if (!input.CheckSizeInt32()) [[unlikely]]
+  if (!input.CheckSizeInt32()) [[unlikely]] {
     return THROW_ERR_OUT_OF_RANGE(env, "spkac is too large");
+  }
 
   auto cert = ByteSource::Allocated(
       ncrypto::ExportChallenge(input.data(), input.size()));
-  if (!cert)
+  if (!cert) {
     return args.GetReturnValue().SetEmptyString();
-
-  Local<Value> outString =
-      Encode(env->isolate(), cert.data<char>(), cert.size(), BUFFER);
-
-  args.GetReturnValue().Set(outString);
+  }
+
+  Local<Value> outString;
+  if (StringBytes::Encode(
+          env->isolate(), cert.data<char>(), cert.size(), BUFFER)
+          .ToLocal(&outString)) {
+    args.GetReturnValue().Set(outString);
+  }
 }
 
 void Initialize(Environment* env, Local<Object> target) {

src/crypto/crypto_util.cc

@@ -36,6 +36,7 @@ using v8::ArrayBuffer;
 using v8::BackingStore;
 using v8::BigInt;
 using v8::Context;
+using v8::EscapableHandleScope;
 using v8::Exception;
 using v8::FunctionCallbackInfo;
 using v8::HandleScope;
@@ -605,21 +606,18 @@ void SetEngine(const FunctionCallbackInfo<Value>& args) {
 #endif  // !OPENSSL_NO_ENGINE
 
 MaybeLocal<Value> EncodeBignum(Environment* env, const BIGNUM* bn, int size) {
+  EscapableHandleScope scope(env->isolate());
   auto buf = BignumPointer::EncodePadded(bn, size);
   CHECK_EQ(buf.size(), static_cast<size_t>(size));
   Local<Value> ret;
-  Local<Value> error;
   if (!StringBytes::Encode(env->isolate(),
                            reinterpret_cast<const char*>(buf.get()),
                            buf.size(),
-                           BASE64URL,
-                           &error)
+                           BASE64URL)
            .ToLocal(&ret)) {
-    CHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return MaybeLocal<Value>();
+    return {};
   }
-  return ret;
+  return scope.Escape(ret);
 }
 
 Maybe<void> SetEncodedValue(Environment* env,

src/encoding_binding.cc

@@ -183,17 +183,10 @@ void BindingData::DecodeUTF8(const FunctionCallbackInfo<Value>& args) {
 
   if (length == 0) return args.GetReturnValue().SetEmptyString();
 
-  Local<Value> error;
   Local<Value> ret;
-
-  if (!StringBytes::Encode(env->isolate(), data, length, UTF8, &error)
-           .ToLocal(&ret)) {
-    CHECK(!error.IsEmpty());
-    env->isolate()->ThrowException(error);
-    return;
+  if (StringBytes::Encode(env->isolate(), data, length, UTF8).ToLocal(&ret)) {
+    args.GetReturnValue().Set(ret);
   }
-
-  args.GetReturnValue().Set(ret);
 }
 
 void BindingData::ToASCII(const FunctionCallbackInfo<Value>& args) {

src/fs_event_wrap.cc

@@ -44,6 +44,7 @@ using v8::PropertyAttribute;
 using v8::ReadOnly;
 using v8::Signature;
 using v8::String;
+using v8::TryCatch;
 using v8::Value;
 
 namespace {
@@ -217,18 +218,19 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
   };
 
   if (filename != nullptr) {
-    Local<Value> error;
-    MaybeLocal<Value> fn = StringBytes::Encode(env->isolate(),
-                                               filename,
-                                               wrap->encoding_,
-                                               &error);
+    // TODO(@jasnell): Historically, this code has failed to correctly
+    // propagate any error returned by the StringBytes::Encode method,
+    // and would instead just crash the process. That behavior is preserved
+    // here but should be looked at. Preferrably errors would be handled
+    // correctly here.
+    TryCatch try_catch(env->isolate());
+    MaybeLocal<Value> fn =
+        StringBytes::Encode(env->isolate(), filename, wrap->encoding_);
     if (fn.IsEmpty()) {
       argv[0] = Integer::New(env->isolate(), UV_EINVAL);
-      argv[2] = StringBytes::Encode(env->isolate(),
-                                    filename,
-                                    strlen(filename),
-                                    BUFFER,
-                                    &error).ToLocalChecked();
+      argv[2] = StringBytes::Encode(
+                    env->isolate(), filename, strlen(filename), BUFFER)
+                    .ToLocalChecked();
     } else {
       argv[2] = fn.ToLocalChecked();
     }

src/node.h

@@ -1123,16 +1123,37 @@ NODE_EXTERN enum encoding ParseEncoding(
 NODE_EXTERN void FatalException(v8::Isolate* isolate,
                                 const v8::TryCatch& try_catch);
 
-NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
-                                        const char* buf,
-                                        size_t len,
-                                        enum encoding encoding = LATIN1);
+NODE_EXTERN v8::MaybeLocal<v8::Value> TryEncode(
+    v8::Isolate* isolate,
+    const char* buf,
+    size_t len,
+    enum encoding encoding = LATIN1);
+
+// Warning: This reverses endianness on Big Endian platforms, even though the
+// signature using uint16_t implies that it should not.
+NODE_EXTERN v8::MaybeLocal<v8::Value> TryEncode(v8::Isolate* isolate,
+                                                const uint16_t* buf,
+                                                size_t len);
+
+// The original Encode(...) functions are deprecated because they do not
+// appropriately propagate exceptions and instead rely on ToLocalChecked()
+// which crashes the process if an exception occurs. We cannot just remove
+// these as it would break ABI compatibility, so we keep them around but
+// deprecate them in favor of the TryEncode(...) variations which return
+// a MaybeLocal<> and do not crash the process if an exception occurs.
+NODE_DEPRECATED(
+    "Use TryEncode(...) instead",
+    NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
+                                            const char* buf,
+                                            size_t len,
+                                            enum encoding encoding = LATIN1));
 
 // Warning: This reverses endianness on Big Endian platforms, even though the
 // signature using uint16_t implies that it should not.
-NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
-                                        const uint16_t* buf,
-                                        size_t len);
+NODE_DEPRECATED("Use TryEncode(...) instead",
+                NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
+                                                        const uint16_t* buf,
+                                                        size_t len));
 
 // Returns -1 if the handle was not valid for decoding
 NODE_EXTERN ssize_t DecodeBytes(v8::Isolate* isolate,

src/node_buffer.cc

@@ -544,20 +544,11 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {
   THROW_AND_RETURN_IF_OOB(Just(end <= buffer.length()));
   size_t length = end - start;
 
-  Local<Value> error;
-  MaybeLocal<Value> maybe_ret =
-      StringBytes::Encode(isolate,
-                          buffer.data() + start,
-                          length,
-                          encoding,
-                          &error);
   Local<Value> ret;
-  if (!maybe_ret.ToLocal(&ret)) {
-    CHECK(!error.IsEmpty());
-    isolate->ThrowException(error);
-    return;
+  if (StringBytes::Encode(isolate, buffer.data() + start, length, encoding)
+          .ToLocal(&ret)) {
+    args.GetReturnValue().Set(ret);
   }
-  args.GetReturnValue().Set(ret);
 }
 
 // Assume caller has properly validated args.