Skip to content

Commit 2c23e31

Browse files
joyeecheungjoyeecheung
committed
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: nodejs#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>
1 parent 7e269f5 commit 2c23e31

12 files changed

Lines changed: 143 additions & 93 deletions

‎src/module_wrap.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include "module_wrap.h"
55

66
#include "env.h"
7+
#include "node_errors.h"
78
#include "node_url.h"
89
#include "util-inl.h"
910
#include "node_internals.h"
@@ -677,8 +678,8 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
677678
URL url(*url_utf8, url_utf8.length());
678679

679680
if (url.flags() & URL_FLAGS_FAILED) {
680-
env->ThrowError("second argument is not a URL string");
681-
return;
681+
return node::THROW_ERR_INVALID_ARG_TYPE(
682+
env, "second argument is not a URL string");
682683
}
683684

684685
Maybe<URL> result = node::loader::Resolve(env, specifier_std, url);

‎src/node_buffer.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@
3737

3838
#define MIN(a, b) ((a) < (b) ? (a) : (b))
3939

40+
#define THROW_AND_RETURN_UNLESS_BUFFER(env, obj) \
41+
THROW_AND_RETURN_IF_NOT_BUFFER(env, obj, "argument")
42+
4043
#define THROW_AND_RETURN_IF_OOB(r) \
4144
do { \
4245
if (!(r)) return node::THROW_ERR_INDEX_OUT_OF_RANGE(env); \
@@ -657,8 +660,7 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
657660
THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
658661
SPREAD_BUFFER_ARG(args.This(), ts_obj);
659662

660-
if (!args[0]->IsString())
661-
return env->ThrowTypeError("Argument must be a string");
663+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "argument");
662664

663665
Local<String> str = args[0]->ToString(env->context()).ToLocalChecked();
664666

‎src/node_crypto.cc‎

Lines changed: 22 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include "node.h"
2323
#include "node_buffer.h"
24+
#include "node_errors.h"
2425
#include "node_constants.h"
2526
#include "node_crypto.h"
2627
#include "node_crypto_bio.h"
@@ -45,20 +46,6 @@
4546
#include <memory>
4647
#include <vector>
4748

48-
#define THROW_AND_RETURN_IF_NOT_BUFFER(val, prefix) \
49-
do { \
50-
if (!Buffer::HasInstance(val)) { \
51-
return env->ThrowTypeError(prefix " must be a buffer"); \
52-
} \
53-
} while (0)
54-
55-
#define THROW_AND_RETURN_IF_NOT_STRING(val, prefix) \
56-
do { \
57-
if (!val->IsString()) { \
58-
return env->ThrowTypeError(prefix " must be a string"); \
59-
} \
60-
} while (0)
61-
6249
static const char PUBLIC_KEY_PFX[] = "-----BEGIN PUBLIC KEY-----";
6350
static const int PUBLIC_KEY_PFX_LEN = sizeof(PUBLIC_KEY_PFX) - 1;
6451
static const char PUBRSA_KEY_PFX[] = "-----BEGIN RSA PUBLIC KEY-----";
@@ -518,7 +505,7 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
518505
if (args[1]->IsUndefined() || args[1]->IsNull())
519506
len = 1;
520507
else
521-
THROW_AND_RETURN_IF_NOT_STRING(args[1], "Pass phrase");
508+
THROW_AND_RETURN_IF_NOT_STRING(env, args[1], "Pass phrase");
522509
}
523510

524511
BIO *bio = LoadBIO(env, args[0]);
@@ -916,7 +903,7 @@ void SecureContext::SetCiphers(const FunctionCallbackInfo<Value>& args) {
916903
return env->ThrowTypeError("Ciphers argument is mandatory");
917904
}
918905

919-
THROW_AND_RETURN_IF_NOT_STRING(args[0], "Ciphers");
906+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Ciphers");
920907

921908
const node::Utf8Value ciphers(args.GetIsolate(), args[0]);
922909
SSL_CTX_set_cipher_list(sc->ctx_, *ciphers);
@@ -931,7 +918,7 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {
931918
if (args.Length() != 1)
932919
return env->ThrowTypeError("ECDH curve name argument is mandatory");
933920

934-
THROW_AND_RETURN_IF_NOT_STRING(args[0], "ECDH curve name");
921+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "ECDH curve name");
935922

936923
node::Utf8Value curve(env->isolate(), args[0]);
937924

@@ -989,7 +976,8 @@ void SecureContext::SetOptions(const FunctionCallbackInfo<Value>& args) {
989976
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
990977

991978
if (args.Length() != 1 || !args[0]->IntegerValue()) {
992-
return sc->env()->ThrowTypeError("Options must be an integer value");
979+
return THROW_ERR_INVALID_ARG_TYPE(
980+
sc->env(), "Options must be an integer value");
993981
}
994982

995983
SSL_CTX_set_options(
@@ -1008,7 +996,7 @@ void SecureContext::SetSessionIdContext(
1008996
return env->ThrowTypeError("Session ID context argument is mandatory");
1009997
}
1010998

1011-
THROW_AND_RETURN_IF_NOT_STRING(args[0], "Session ID context");
999+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Session ID context");
10121000

10131001
const node::Utf8Value sessionIdContext(args.GetIsolate(), args[0]);
10141002
const unsigned char* sid_ctx =
@@ -1043,8 +1031,8 @@ void SecureContext::SetSessionTimeout(const FunctionCallbackInfo<Value>& args) {
10431031
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
10441032

10451033
if (args.Length() != 1 || !args[0]->IsInt32()) {
1046-
return sc->env()->ThrowTypeError(
1047-
"Session timeout must be a 32-bit integer");
1034+
return THROW_ERR_INVALID_ARG_TYPE(
1035+
sc->env(), "Session timeout must be a 32-bit integer");
10481036
}
10491037

10501038
int32_t sessionTimeout = args[0]->Int32Value();
@@ -1085,7 +1073,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
10851073
}
10861074

10871075
if (args.Length() >= 2) {
1088-
THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Pass phrase");
1076+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[1], "Pass phrase");
10891077
size_t passlen = Buffer::Length(args[1]);
10901078
pass = new char[passlen + 1];
10911079
memcpy(pass, Buffer::Data(args[1]), passlen);
@@ -1212,7 +1200,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {
12121200
return env->ThrowTypeError("Ticket keys argument is mandatory");
12131201
}
12141202

1215-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Ticket keys");
1203+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Ticket keys");
12161204

12171205
if (Buffer::Length(args[0]) != 48) {
12181206
return env->ThrowTypeError("Ticket keys length must be 48 bytes");
@@ -1964,7 +1952,7 @@ void SSLWrap<Base>::SetSession(const FunctionCallbackInfo<Value>& args) {
19641952
return env->ThrowError("Session argument is mandatory");
19651953
}
19661954

1967-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Session");
1955+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Session");
19681956
size_t slen = Buffer::Length(args[0]);
19691957
char* sbuf = new char[slen];
19701958
memcpy(sbuf, Buffer::Data(args[0]), slen);
@@ -2088,7 +2076,7 @@ void SSLWrap<Base>::SetOCSPResponse(
20882076
if (args.Length() < 1)
20892077
return env->ThrowTypeError("OCSP response argument is mandatory");
20902078

2091-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "OCSP response");
2079+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "OCSP response");
20922080

20932081
w->ocsp_response_.Reset(args.GetIsolate(), args[0].As<Object>());
20942082
#endif // NODE__HAVE_TLSEXT_STATUS_CB
@@ -3937,11 +3925,11 @@ template <PublicKeyCipher::Operation operation,
39373925
void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
39383926
Environment* env = Environment::GetCurrent(args);
39393927

3940-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Key");
3928+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Key");
39413929
char* kbuf = Buffer::Data(args[0]);
39423930
ssize_t klen = Buffer::Length(args[0]);
39433931

3944-
THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Data");
3932+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[1], "Data");
39453933
char* buf = Buffer::Data(args[1]);
39463934
ssize_t len = Buffer::Length(args[1]);
39473935

@@ -4097,7 +4085,7 @@ void DiffieHellman::DiffieHellmanGroup(
40974085
return env->ThrowError("Group name argument is mandatory");
40984086
}
40994087

4100-
THROW_AND_RETURN_IF_NOT_STRING(args[0], "Group name");
4088+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Group name");
41014089

41024090
bool initialized = false;
41034091

@@ -4246,7 +4234,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
42464234
if (args.Length() == 0) {
42474235
return env->ThrowError("Other party's public key argument is mandatory");
42484236
} else {
4249-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Other party's public key");
4237+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Other party's public key");
42504238
key = BN_bin2bn(
42514239
reinterpret_cast<unsigned char*>(Buffer::Data(args[0])),
42524240
Buffer::Length(args[0]),
@@ -4319,7 +4307,7 @@ void DiffieHellman::SetKey(const v8::FunctionCallbackInfo<v8::Value>& args,
43194307

43204308
if (!Buffer::HasInstance(args[0])) {
43214309
snprintf(errmsg, sizeof(errmsg), "%s must be a buffer", what);
4322-
return env->ThrowTypeError(errmsg);
4310+
return THROW_ERR_INVALID_ARG_TYPE(env, errmsg);
43234311
}
43244312

43254313
BIGNUM* num =
@@ -4397,7 +4385,7 @@ void ECDH::New(const FunctionCallbackInfo<Value>& args) {
43974385
MarkPopErrorOnReturn mark_pop_error_on_return;
43984386

43994387
// TODO(indutny): Support raw curves?
4400-
THROW_AND_RETURN_IF_NOT_STRING(args[0], "ECDH curve name");
4388+
THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "ECDH curve name");
44014389
node::Utf8Value curve(env->isolate(), args[0]);
44024390

44034391
int nid = OBJ_sn2nid(*curve);
@@ -4454,7 +4442,7 @@ EC_POINT* ECDH::BufferToPoint(Environment* env,
44544442
void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
44554443
Environment* env = Environment::GetCurrent(args);
44564444

4457-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Data");
4445+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Data");
44584446

44594447
ECDH* ecdh;
44604448
ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
@@ -4557,7 +4545,7 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
45574545
ECDH* ecdh;
45584546
ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
45594547

4560-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Private key");
4548+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Private key");
45614549

45624550
BIGNUM* priv = BN_bin2bn(
45634551
reinterpret_cast<unsigned char*>(Buffer::Data(args[0].As<Object>())),
@@ -4611,7 +4599,7 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
46114599
ECDH* ecdh;
46124600
ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
46134601

4614-
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");
4602+
THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Public key");
46154603

46164604
MarkPopErrorOnReturn mark_pop_error_on_return;
46174605

‎src/node_dtrace.cc‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#define NODE_GC_DONE(arg0, arg1, arg2)
4343
#endif
4444

45+
#include "node_errors.h"
4546
#include "node_internals.h"
4647

4748
#include <string.h>
@@ -60,7 +61,7 @@ using v8::Value;
6061

6162
#define SLURP_STRING(obj, member, valp) \
6263
if (!(obj)->IsObject()) { \
63-
return env->ThrowError( \
64+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
6465
"expected object for " #obj " to contain string member " #member); \
6566
} \
6667
node::Utf8Value _##member(env->isolate(), \
@@ -70,23 +71,23 @@ using v8::Value;
7071

7172
#define SLURP_INT(obj, member, valp) \
7273
if (!(obj)->IsObject()) { \
73-
return env->ThrowError( \
74-
"expected object for " #obj " to contain integer member " #member); \
74+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
75+
"expected object for " #obj " to contain integer member " #member);\
7576
} \
7677
*valp = obj->Get(OneByteString(env->isolate(), #member)) \
7778
->Int32Value();
7879

7980
#define SLURP_OBJECT(obj, member, valp) \
8081
if (!(obj)->IsObject()) { \
81-
return env->ThrowError( \
82-
"expected object for " #obj " to contain object member " #member); \
82+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
83+
"expected object for " #obj " to contain object member " #member); \
8384
} \
8485
*valp = Local<Object>::Cast(obj->Get(OneByteString(env->isolate(), #member)));
8586

8687
#define SLURP_CONNECTION(arg, conn) \
8788
if (!(arg)->IsObject()) { \
88-
return env->ThrowError( \
89-
"expected argument " #arg " to be a connection object"); \
89+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
90+
"expected argument " #arg " to be a connection object"); \
9091
} \
9192
node_dtrace_connection_t conn; \
9293
Local<Object> _##conn = Local<Object>::Cast(arg); \
@@ -103,8 +104,8 @@ using v8::Value;
103104

104105
#define SLURP_CONNECTION_HTTP_CLIENT(arg, conn) \
105106
if (!(arg)->IsObject()) { \
106-
return env->ThrowError( \
107-
"expected argument " #arg " to be a connection object"); \
107+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
108+
"expected argument " #arg " to be a connection object"); \
108109
} \
109110
node_dtrace_connection_t conn; \
110111
Local<Object> _##conn = Local<Object>::Cast(arg); \
@@ -115,12 +116,12 @@ using v8::Value;
115116

116117
#define SLURP_CONNECTION_HTTP_CLIENT_RESPONSE(arg0, arg1, conn) \
117118
if (!(arg0)->IsObject()) { \
118-
return env->ThrowError( \
119-
"expected argument " #arg0 " to be a connection object"); \
119+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
120+
"expected argument " #arg0 " to be a connection object"); \
120121
} \
121122
if (!(arg1)->IsObject()) { \
122-
return env->ThrowError( \
123-
"expected argument " #arg1 " to be a connection object"); \
123+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
124+
"expected argument " #arg1 " to be a connection object"); \
124125
} \
125126
node_dtrace_connection_t conn; \
126127
Local<Object> _##conn = Local<Object>::Cast(arg0); \
@@ -166,8 +167,8 @@ void DTRACE_HTTP_SERVER_REQUEST(const FunctionCallbackInfo<Value>& args) {
166167
SLURP_OBJECT(arg0, headers, &headers);
167168

168169
if (!(headers)->IsObject()) {
169-
return env->ThrowError(
170-
"expected object for request to contain string member headers");
170+
return node::THROW_ERR_INVALID_ARG_TYPE(env,
171+
"expected object for request to contain string member headers");
171172
}
172173

173174
Local<Value> strfwdfor = headers->Get(env->x_forwarded_string());

‎src/node_errors.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ namespace node {
1818

1919
#define ERRORS_WITH_CODE(V) \
2020
V(ERR_INDEX_OUT_OF_RANGE, RangeError) \
21+
V(ERR_INVALID_ARG_TYPE, TypeError) \
2122
V(ERR_MEMORY_ALLOCATION_FAILED, Error) \
2223
V(ERR_STRING_TOO_LONG, Error) \
2324
V(ERR_BUFFER_TOO_LARGE, Error)
@@ -74,6 +75,20 @@ inline v8::Local<v8::Value> ERR_STRING_TOO_LONG(v8::Isolate *isolate) {
7475
return ERR_STRING_TOO_LONG(isolate, message);
7576
}
7677

78+
#define THROW_AND_RETURN_IF_NOT_BUFFER(env, val, prefix) \
79+
do { \
80+
if (!Buffer::HasInstance(val)) \
81+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
82+
prefix " must be a buffer"); \
83+
} while (0)
84+
85+
#define THROW_AND_RETURN_IF_NOT_STRING(env, val, prefix) \
86+
do { \
87+
if (!val->IsString()) \
88+
return node::THROW_ERR_INVALID_ARG_TYPE(env, \
89+
prefix " must be a string"); \
90+
} while (0)
91+
7792
} // namespace node
7893

7994
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

‎src/node_i18n.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646

4747
#include "node.h"
4848
#include "node_buffer.h"
49+
#include "node_errors.h"
4950
#include "env-inl.h"
5051
#include "util-inl.h"
5152
#include "base_object-inl.h"
@@ -447,7 +448,7 @@ void Transcode(const FunctionCallbackInfo<Value>&args) {
447448
UErrorCode status = U_ZERO_ERROR;
448449
MaybeLocal<Object> result;
449450

450-
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
451+
CHECK(Buffer::HasInstance(args[0]));
451452
SPREAD_BUFFER_ARG(args[0], ts_obj);
452453
const enum encoding fromEncoding = ParseEncoding(isolate, args[1], BUFFER);
453454
const enum encoding toEncoding = ParseEncoding(isolate, args[2], BUFFER);

0 commit comments

Comments
 (0)