src: throw ERR_INVALID_ARG_TYPE in C++ argument checks

- Moves THROW_AND_RETURN_IF_NOT_BUFFER and
  THROW_AND_RETURN_IF_NOT_STRING from node_crypto.cc to
  node_errors.h so it can be reused.
- Move THROW_AND_RETURN_UNLESS_BUFFER in util.h to
  node_buffer.cc and call THROW_AND_RETURN_IF_NOT_BUFFER
  there. The only other reference to THROW_AND_RETURN_UNLESS_BUFFER in
  node_i18n.cc can be safely replaced by an assertion since
  the argument will be checked in JS land.
- Migrate ERR_INVALID_ARG_TYPE errors in C++. We can move the
  checks to JS land if possible later without having to
  go semver-major.

PR-URL: #20121
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>

Author: joyeecheung

src/module_wrap.cc

@@ -4,6 +4,7 @@
 #include "module_wrap.h"
 
 #include "env.h"
+#include "node_errors.h"
 #include "node_url.h"
 #include "util-inl.h"
 #include "node_internals.h"
@@ -677,8 +678,8 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
   URL url(*url_utf8, url_utf8.length());
 
   if (url.flags() & URL_FLAGS_FAILED) {
-    env->ThrowError("second argument is not a URL string");
-    return;
+    return node::THROW_ERR_INVALID_ARG_TYPE(
+        env, "second argument is not a URL string");
   }
 
   Maybe<URL> result = node::loader::Resolve(env, specifier_std, url);

src/node_buffer.cc

@@ -37,6 +37,9 @@
 
 #define MIN(a, b) ((a) < (b) ? (a) : (b))
 
+#define THROW_AND_RETURN_UNLESS_BUFFER(env, obj)                            \
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, obj, "argument")
+
 #define THROW_AND_RETURN_IF_OOB(r)                                          \
   do {                                                                      \
     if (!(r)) return node::THROW_ERR_INDEX_OUT_OF_RANGE(env);               \
@@ -657,8 +660,7 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
   THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
   SPREAD_BUFFER_ARG(args.This(), ts_obj);
 
-  if (!args[0]->IsString())
-    return env->ThrowTypeError("Argument must be a string");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "argument");
 
   Local<String> str = args[0]->ToString(env->context()).ToLocalChecked();
 

src/node_crypto.cc

@@ -21,6 +21,7 @@
 
 #include "node.h"
 #include "node_buffer.h"
+#include "node_errors.h"
 #include "node_constants.h"
 #include "node_crypto.h"
 #include "node_crypto_bio.h"
@@ -45,20 +46,6 @@
 #include <memory>
 #include <vector>
 
-#define THROW_AND_RETURN_IF_NOT_BUFFER(val, prefix)           \
-  do {                                                        \
-    if (!Buffer::HasInstance(val)) {                          \
-      return env->ThrowTypeError(prefix " must be a buffer"); \
-    }                                                         \
-  } while (0)
-
-#define THROW_AND_RETURN_IF_NOT_STRING(val, prefix)           \
-  do {                                                        \
-    if (!val->IsString()) {                                   \
-      return env->ThrowTypeError(prefix " must be a string"); \
-    }                                                         \
-  } while (0)
-
 static const char PUBLIC_KEY_PFX[] =  "-----BEGIN PUBLIC KEY-----";
 static const int PUBLIC_KEY_PFX_LEN = sizeof(PUBLIC_KEY_PFX) - 1;
 static const char PUBRSA_KEY_PFX[] =  "-----BEGIN RSA PUBLIC KEY-----";
@@ -518,7 +505,7 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
     if (args[1]->IsUndefined() || args[1]->IsNull())
       len = 1;
     else
-      THROW_AND_RETURN_IF_NOT_STRING(args[1], "Pass phrase");
+      THROW_AND_RETURN_IF_NOT_STRING(env, args[1], "Pass phrase");
   }
 
   BIO *bio = LoadBIO(env, args[0]);
@@ -916,7 +903,7 @@ void SecureContext::SetCiphers(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowTypeError("Ciphers argument is mandatory");
   }
 
-  THROW_AND_RETURN_IF_NOT_STRING(args[0], "Ciphers");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Ciphers");
 
   const node::Utf8Value ciphers(args.GetIsolate(), args[0]);
   SSL_CTX_set_cipher_list(sc->ctx_, *ciphers);
@@ -931,7 +918,7 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {
   if (args.Length() != 1)
     return env->ThrowTypeError("ECDH curve name argument is mandatory");
 
-  THROW_AND_RETURN_IF_NOT_STRING(args[0], "ECDH curve name");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "ECDH curve name");
 
   node::Utf8Value curve(env->isolate(), args[0]);
 
@@ -989,7 +976,8 @@ void SecureContext::SetOptions(const FunctionCallbackInfo<Value>& args) {
   ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
 
   if (args.Length() != 1 || !args[0]->IntegerValue()) {
-    return sc->env()->ThrowTypeError("Options must be an integer value");
+    return THROW_ERR_INVALID_ARG_TYPE(
+        sc->env(), "Options must be an integer value");
   }
 
   SSL_CTX_set_options(
@@ -1008,7 +996,7 @@ void SecureContext::SetSessionIdContext(
     return env->ThrowTypeError("Session ID context argument is mandatory");
   }
 
-  THROW_AND_RETURN_IF_NOT_STRING(args[0], "Session ID context");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Session ID context");
 
   const node::Utf8Value sessionIdContext(args.GetIsolate(), args[0]);
   const unsigned char* sid_ctx =
@@ -1043,8 +1031,8 @@ void SecureContext::SetSessionTimeout(const FunctionCallbackInfo<Value>& args) {
   ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
 
   if (args.Length() != 1 || !args[0]->IsInt32()) {
-    return sc->env()->ThrowTypeError(
-        "Session timeout must be a 32-bit integer");
+    return THROW_ERR_INVALID_ARG_TYPE(
+        sc->env(), "Session timeout must be a 32-bit integer");
   }
 
   int32_t sessionTimeout = args[0]->Int32Value();
@@ -1085,7 +1073,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   }
 
   if (args.Length() >= 2) {
-    THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Pass phrase");
+    THROW_AND_RETURN_IF_NOT_BUFFER(env, args[1], "Pass phrase");
     size_t passlen = Buffer::Length(args[1]);
     pass = new char[passlen + 1];
     memcpy(pass, Buffer::Data(args[1]), passlen);
@@ -1212,7 +1200,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowTypeError("Ticket keys argument is mandatory");
   }
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Ticket keys");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Ticket keys");
 
   if (Buffer::Length(args[0]) != 48) {
     return env->ThrowTypeError("Ticket keys length must be 48 bytes");
@@ -1964,7 +1952,7 @@ void SSLWrap<Base>::SetSession(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowError("Session argument is mandatory");
   }
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Session");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Session");
   size_t slen = Buffer::Length(args[0]);
   char* sbuf = new char[slen];
   memcpy(sbuf, Buffer::Data(args[0]), slen);
@@ -2088,7 +2076,7 @@ void SSLWrap<Base>::SetOCSPResponse(
   if (args.Length() < 1)
     return env->ThrowTypeError("OCSP response argument is mandatory");
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "OCSP response");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "OCSP response");
 
   w->ocsp_response_.Reset(args.GetIsolate(), args[0].As<Object>());
 #endif  // NODE__HAVE_TLSEXT_STATUS_CB
@@ -3937,11 +3925,11 @@ template <PublicKeyCipher::Operation operation,
 void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Key");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Key");
   char* kbuf = Buffer::Data(args[0]);
   ssize_t klen = Buffer::Length(args[0]);
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Data");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[1], "Data");
   char* buf = Buffer::Data(args[1]);
   ssize_t len = Buffer::Length(args[1]);
 
@@ -4097,7 +4085,7 @@ void DiffieHellman::DiffieHellmanGroup(
     return env->ThrowError("Group name argument is mandatory");
   }
 
-  THROW_AND_RETURN_IF_NOT_STRING(args[0], "Group name");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Group name");
 
   bool initialized = false;
 
@@ -4246,7 +4234,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
   if (args.Length() == 0) {
     return env->ThrowError("Other party's public key argument is mandatory");
   } else {
-    THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Other party's public key");
+    THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Other party's public key");
     key = BN_bin2bn(
         reinterpret_cast<unsigned char*>(Buffer::Data(args[0])),
         Buffer::Length(args[0]),
@@ -4319,7 +4307,7 @@ void DiffieHellman::SetKey(const v8::FunctionCallbackInfo<v8::Value>& args,
 
   if (!Buffer::HasInstance(args[0])) {
     snprintf(errmsg, sizeof(errmsg), "%s must be a buffer", what);
-    return env->ThrowTypeError(errmsg);
+    return THROW_ERR_INVALID_ARG_TYPE(env, errmsg);
   }
 
   BIGNUM* num =
@@ -4397,7 +4385,7 @@ void ECDH::New(const FunctionCallbackInfo<Value>& args) {
   MarkPopErrorOnReturn mark_pop_error_on_return;
 
   // TODO(indutny): Support raw curves?
-  THROW_AND_RETURN_IF_NOT_STRING(args[0], "ECDH curve name");
+  THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "ECDH curve name");
   node::Utf8Value curve(env->isolate(), args[0]);
 
   int nid = OBJ_sn2nid(*curve);
@@ -4454,7 +4442,7 @@ EC_POINT* ECDH::BufferToPoint(Environment* env,
 void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Data");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Data");
 
   ECDH* ecdh;
   ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
@@ -4557,7 +4545,7 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
   ECDH* ecdh;
   ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Private key");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Private key");
 
   BIGNUM* priv = BN_bin2bn(
       reinterpret_cast<unsigned char*>(Buffer::Data(args[0].As<Object>())),
@@ -4611,7 +4599,7 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
   ECDH* ecdh;
   ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
 
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");
+  THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Public key");
 
   MarkPopErrorOnReturn mark_pop_error_on_return;
 

src/node_dtrace.cc

@@ -42,6 +42,7 @@
 #define NODE_GC_DONE(arg0, arg1, arg2)
 #endif
 
+#include "node_errors.h"
 #include "node_internals.h"
 
 #include <string.h>
@@ -60,7 +61,7 @@ using v8::Value;
 
 #define SLURP_STRING(obj, member, valp)                                    \
   if (!(obj)->IsObject()) {                                                \
-    return env->ThrowError(                                                \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
         "expected object for " #obj " to contain string member " #member); \
   }                                                                        \
   node::Utf8Value _##member(env->isolate(),                                \
@@ -70,23 +71,23 @@ using v8::Value;
 
 #define SLURP_INT(obj, member, valp)                                       \
   if (!(obj)->IsObject()) {                                                \
-    return env->ThrowError(                                                \
-      "expected object for " #obj " to contain integer member " #member);  \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected object for " #obj " to contain integer member " #member);\
   }                                                                        \
   *valp = obj->Get(OneByteString(env->isolate(), #member))                 \
       ->Int32Value();
 
 #define SLURP_OBJECT(obj, member, valp)                                    \
   if (!(obj)->IsObject()) {                                                \
-    return env->ThrowError(                                                \
-      "expected object for " #obj " to contain object member " #member);   \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected object for " #obj " to contain object member " #member); \
   }                                                                        \
   *valp = Local<Object>::Cast(obj->Get(OneByteString(env->isolate(), #member)));
 
 #define SLURP_CONNECTION(arg, conn)                                        \
   if (!(arg)->IsObject()) {                                                \
-    return env->ThrowError(                                                \
-      "expected argument " #arg " to be a connection object");             \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected argument " #arg " to be a connection object");           \
   }                                                                        \
   node_dtrace_connection_t conn;                                           \
   Local<Object> _##conn = Local<Object>::Cast(arg);                        \
@@ -103,8 +104,8 @@ using v8::Value;
 
 #define SLURP_CONNECTION_HTTP_CLIENT(arg, conn)                            \
   if (!(arg)->IsObject()) {                                                \
-    return env->ThrowError(                                                \
-      "expected argument " #arg " to be a connection object");             \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected argument " #arg " to be a connection object");           \
   }                                                                        \
   node_dtrace_connection_t conn;                                           \
   Local<Object> _##conn = Local<Object>::Cast(arg);                        \
@@ -115,12 +116,12 @@ using v8::Value;
 
 #define SLURP_CONNECTION_HTTP_CLIENT_RESPONSE(arg0, arg1, conn)            \
   if (!(arg0)->IsObject()) {                                               \
-    return env->ThrowError(                                                \
-      "expected argument " #arg0 " to be a connection object");            \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected argument " #arg0 " to be a connection object");          \
   }                                                                        \
   if (!(arg1)->IsObject()) {                                               \
-    return env->ThrowError(                                                \
-      "expected argument " #arg1 " to be a connection object");            \
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+        "expected argument " #arg1 " to be a connection object");          \
   }                                                                        \
   node_dtrace_connection_t conn;                                           \
   Local<Object> _##conn = Local<Object>::Cast(arg0);                       \
@@ -166,8 +167,8 @@ void DTRACE_HTTP_SERVER_REQUEST(const FunctionCallbackInfo<Value>& args) {
   SLURP_OBJECT(arg0, headers, &headers);
 
   if (!(headers)->IsObject()) {
-    return env->ThrowError(
-      "expected object for request to contain string member headers");
+    return node::THROW_ERR_INVALID_ARG_TYPE(env,
+        "expected object for request to contain string member headers");
   }
 
   Local<Value> strfwdfor = headers->Get(env->x_forwarded_string());

src/node_errors.h

@@ -18,6 +18,7 @@ namespace node {
 
 #define ERRORS_WITH_CODE(V)                                                  \
   V(ERR_INDEX_OUT_OF_RANGE, RangeError)                                      \
+  V(ERR_INVALID_ARG_TYPE, TypeError)                                         \
   V(ERR_MEMORY_ALLOCATION_FAILED, Error)                                     \
   V(ERR_STRING_TOO_LONG, Error)                                              \
   V(ERR_BUFFER_TOO_LARGE, Error)
@@ -74,6 +75,20 @@ inline v8::Local<v8::Value> ERR_STRING_TOO_LONG(v8::Isolate *isolate) {
   return ERR_STRING_TOO_LONG(isolate, message);
 }
 
+#define THROW_AND_RETURN_IF_NOT_BUFFER(env, val, prefix)                     \
+  do {                                                                       \
+    if (!Buffer::HasInstance(val))                                           \
+      return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+                                              prefix " must be a buffer");   \
+  } while (0)
+
+#define THROW_AND_RETURN_IF_NOT_STRING(env, val, prefix)                     \
+  do {                                                                       \
+    if (!val->IsString())                                                    \
+      return node::THROW_ERR_INVALID_ARG_TYPE(env,                           \
+                                              prefix " must be a string");   \
+  } while (0)
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/node_i18n.cc

@@ -46,6 +46,7 @@
 
 #include "node.h"
 #include "node_buffer.h"
+#include "node_errors.h"
 #include "env-inl.h"
 #include "util-inl.h"
 #include "base_object-inl.h"
@@ -447,7 +448,7 @@ void Transcode(const FunctionCallbackInfo<Value>&args) {
   UErrorCode status = U_ZERO_ERROR;
   MaybeLocal<Object> result;
 
-  THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
+  CHECK(Buffer::HasInstance(args[0]));
   SPREAD_BUFFER_ARG(args[0], ts_obj);
   const enum encoding fromEncoding = ParseEncoding(isolate, args[1], BUFFER);
   const enum encoding toEncoding = ParseEncoding(isolate, args[2], BUFFER);