Skip to content

Commit

Permalink
src: throw ERR_INVALID_ARG_TYPE in C++ argument checks
Browse files Browse the repository at this point in the history 
- 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 <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
  • Loading branch information
@joyeecheung @jasnell
joyeecheung authored and jasnell committed Apr 19, 2018
1 parent 1d0ad63 commit 0fdf39a
Show file tree
Hide file tree
Showing 12 changed files with 143 additions and 93 deletions.
5 changes: 3 additions & 2 deletions src/module_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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); \
Expand Down Expand Up @@ -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();

Expand Down
56 changes: 22 additions & 34 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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-----";
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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);
Expand All @@ -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]);

Expand Down Expand Up @@ -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(
Expand All @@ -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 =
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -3938,11 +3926,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]);

Expand Down Expand Up @@ -4098,7 +4086,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;

Expand Down Expand Up @@ -4247,7 +4235,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]),
Expand Down Expand Up @@ -4320,7 +4308,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 =
Expand Down Expand Up @@ -4398,7 +4386,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);
Expand Down Expand Up @@ -4455,7 +4443,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());
Expand Down Expand Up @@ -4558,7 +4546,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>())),
Expand Down Expand Up @@ -4612,7 +4600,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;

Expand Down
31 changes: 16 additions & 15 deletions src/node_dtrace.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#define NODE_GC_DONE(arg0, arg1, arg2)
#endif

#include "node_errors.h"
#include "node_internals.h"

#include <string.h>
Expand All @@ -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(), \
Expand All @@ -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); \
Expand All @@ -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); \
Expand All @@ -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); \
Expand Down Expand Up @@ -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());
Expand Down
15 changes: 15 additions & 0 deletions src/node_errors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/node_i18n.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 0fdf39a

Please sign in to comment.