Skip to content

Commit 78d81b9

Browse files
jasnellRafaelGSS
jasnell
authored and
RafaelGSS
committed
src: improve StringBytes error handling
PR-URL: #57706 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
1 parent 9a90056 commit 78d81b9

18 files changed

+213
-283
lines changed

‎src/api/encoding.cc‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ namespace node {
88
using v8::HandleScope;
99
using v8::Isolate;
1010
using v8::Local;
11+
using v8::MaybeLocal;
12+
using v8::TryCatch;
1113
using v8::Value;
1214

1315
enum encoding ParseEncoding(const char* encoding,
@@ -133,20 +135,30 @@ enum encoding ParseEncoding(Isolate* isolate,
133135
return ParseEncoding(*encoding, default_encoding);
134136
}
135137

138+
MaybeLocal<Value> TryEncode(Isolate* isolate,
139+
const char* buf,
140+
size_t len,
141+
enum encoding encoding) {
142+
CHECK_NE(encoding, UCS2);
143+
return StringBytes::Encode(isolate, buf, len, encoding);
144+
}
145+
146+
MaybeLocal<Value> TryEncode(Isolate* isolate, const uint16_t* buf, size_t len) {
147+
return StringBytes::Encode(isolate, buf, len).ToLocalChecked();
148+
}
149+
136150
Local<Value> Encode(Isolate* isolate,
137151
const char* buf,
138152
size_t len,
139153
enum encoding encoding) {
140154
CHECK_NE(encoding, UCS2);
141-
Local<Value> error;
142-
return StringBytes::Encode(isolate, buf, len, encoding, &error)
143-
.ToLocalChecked();
155+
TryCatch try_catch(isolate);
156+
return StringBytes::Encode(isolate, buf, len, encoding).ToLocalChecked();
144157
}
145158

146159
Local<Value> Encode(Isolate* isolate, const uint16_t* buf, size_t len) {
147-
Local<Value> error;
148-
return StringBytes::Encode(isolate, buf, len, &error)
149-
.ToLocalChecked();
160+
TryCatch try_catch(isolate);
161+
return StringBytes::Encode(isolate, buf, len).ToLocalChecked();
150162
}
151163

152164
// Returns -1 if the handle was not valid for decoding

‎src/crypto/crypto_ec.cc‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -800,17 +800,11 @@ bool ExportJWKEdKey(Environment* env,
800800
Local<Object> target,
801801
Local<String> key) {
802802
Local<Value> encoded;
803-
Local<Value> error;
804803
if (!data) return false;
805804
const ncrypto::Buffer<const char> out = data;
806-
if (!StringBytes::Encode(
807-
env->isolate(), out.data, out.len, BASE64URL, &error)
808-
.ToLocal(&encoded) ||
809-
target->Set(env->context(), key, encoded).IsNothing()) {
810-
if (!error.IsEmpty()) env->isolate()->ThrowException(error);
811-
return false;
812-
}
813-
return true;
805+
return StringBytes::Encode(env->isolate(), out.data, out.len, BASE64URL)
806+
.ToLocal(&encoded) &&
807+
target->Set(env->context(), key, encoded).IsJust();
814808
};
815809

816810
return !(

‎src/crypto/crypto_hash.cc‎

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -252,19 +252,14 @@ void Hash::OneShotDigest(const FunctionCallbackInfo<Value>& args) {
252252
return ThrowCryptoError(env, ERR_get_error());
253253
}
254254

255-
Local<Value> error;
256-
MaybeLocal<Value> rc =
257-
StringBytes::Encode(env->isolate(),
255+
Local<Value> ret;
256+
if (StringBytes::Encode(env->isolate(),
258257
static_cast<const char*>(output.get()),
259258
output.size(),
260-
output_enc,
261-
&error);
262-
if (rc.IsEmpty()) [[unlikely]] {
263-
CHECK(!error.IsEmpty());
264-
env->isolate()->ThrowException(error);
265-
return;
259+
output_enc)
260+
.ToLocal(&ret)) {
261+
args.GetReturnValue().Set(ret);
266262
}
267-
args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
268263
}
269264

270265
void Hash::Initialize(Environment* env, Local<Object> target) {
@@ -410,15 +405,12 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
410405
hash->digest_ = ByteSource::Allocated(data.release());
411406
}
412407

413-
Local<Value> error;
414-
MaybeLocal<Value> rc = StringBytes::Encode(
415-
env->isolate(), hash->digest_.data<char>(), len, encoding, &error);
416-
if (rc.IsEmpty()) [[unlikely]] {
417-
CHECK(!error.IsEmpty());
418-
env->isolate()->ThrowException(error);
419-
return;
408+
Local<Value> ret;
409+
if (StringBytes::Encode(
410+
env->isolate(), hash->digest_.data<char>(), len, encoding)
411+
.ToLocal(&ret)) {
412+
args.GetReturnValue().Set(ret);
420413
}
421-
args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
422414
}
423415

424416
HashConfig::HashConfig(HashConfig&& other) noexcept
@@ -541,19 +533,14 @@ void InternalVerifyIntegrity(const v8::FunctionCallbackInfo<v8::Value>& args) {
541533

542534
if (digest_size != expected.size() ||
543535
CRYPTO_memcmp(digest, expected.data(), digest_size) != 0) {
544-
Local<Value> error;
545-
MaybeLocal<Value> rc =
546-
StringBytes::Encode(env->isolate(),
536+
Local<Value> ret;
537+
if (StringBytes::Encode(env->isolate(),
547538
reinterpret_cast<const char*>(digest),
548539
digest_size,
549-
BASE64,
550-
&error);
551-
if (rc.IsEmpty()) [[unlikely]] {
552-
CHECK(!error.IsEmpty());
553-
env->isolate()->ThrowException(error);
554-
return;
540+
BASE64)
541+
.ToLocal(&ret)) {
542+
args.GetReturnValue().Set(ret);
555543
}
556-
args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
557544
}
558545
}
559546
} // namespace crypto

‎src/crypto/crypto_hmac.cc‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -145,19 +145,14 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
145145
hmac->ctx_.reset();
146146
}
147147

148-
Local<Value> error;
149-
MaybeLocal<Value> rc =
150-
StringBytes::Encode(env->isolate(),
148+
Local<Value> ret;
149+
if (StringBytes::Encode(env->isolate(),
151150
reinterpret_cast<const char*>(md_value),
152151
buf.len,
153-
encoding,
154-
&error);
155-
if (rc.IsEmpty()) [[unlikely]] {
156-
CHECK(!error.IsEmpty());
157-
env->isolate()->ThrowException(error);
158-
return;
152+
encoding)
153+
.ToLocal(&ret)) {
154+
args.GetReturnValue().Set(ret);
159155
}
160-
args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
161156
}
162157

163158
HmacConfig::HmacConfig(HmacConfig&& other) noexcept

‎src/crypto/crypto_keys.cc‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -130,20 +130,13 @@ bool ExportJWKSecretKey(Environment* env,
130130
Local<Object> target) {
131131
CHECK_EQ(key.GetKeyType(), kKeyTypeSecret);
132132

133-
Local<Value> error;
134133
Local<Value> raw;
135-
if (!StringBytes::Encode(env->isolate(),
136-
key.GetSymmetricKey(),
137-
key.GetSymmetricKeySize(),
138-
BASE64URL,
139-
&error)
140-
.ToLocal(&raw)) {
141-
DCHECK(!error.IsEmpty());
142-
env->isolate()->ThrowException(error);
143-
return false;
144-
}
145-
146-
return target
134+
return StringBytes::Encode(env->isolate(),
135+
key.GetSymmetricKey(),
136+
key.GetSymmetricKeySize(),
137+
BASE64URL)
138+
.ToLocal(&raw) &&
139+
target
147140
->Set(env->context(), env->jwk_kty_string(), env->jwk_oct_string())
148141
.IsJust() &&
149142
target->Set(env->context(), env->jwk_k_string(), raw).IsJust();

‎src/crypto/crypto_spkac.cc‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "memory_tracker-inl.h"
66
#include "ncrypto.h"
77
#include "node.h"
8+
#include "string_bytes.h"
89
#include "v8.h"
910

1011
namespace node {
@@ -52,18 +53,22 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
5253
ArrayBufferOrViewContents<char> input(args[0]);
5354
if (input.empty()) return args.GetReturnValue().SetEmptyString();
5455

55-
if (!input.CheckSizeInt32()) [[unlikely]]
56+
if (!input.CheckSizeInt32()) [[unlikely]] {
5657
return THROW_ERR_OUT_OF_RANGE(env, "spkac is too large");
58+
}
5759

5860
auto cert = ByteSource::Allocated(
5961
ncrypto::ExportChallenge(input.data(), input.size()));
60-
if (!cert)
62+
if (!cert) {
6163
return args.GetReturnValue().SetEmptyString();
62-
63-
Local<Value> outString =
64-
Encode(env->isolate(), cert.data<char>(), cert.size(), BUFFER);
65-
66-
args.GetReturnValue().Set(outString);
64+
}
65+
66+
Local<Value> outString;
67+
if (StringBytes::Encode(
68+
env->isolate(), cert.data<char>(), cert.size(), BUFFER)
69+
.ToLocal(&outString)) {
70+
args.GetReturnValue().Set(outString);
71+
}
6772
}
6873

6974
void Initialize(Environment* env, Local<Object> target) {

‎src/crypto/crypto_util.cc‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ using v8::ArrayBuffer;
3636
using v8::BackingStore;
3737
using v8::BigInt;
3838
using v8::Context;
39+
using v8::EscapableHandleScope;
3940
using v8::Exception;
4041
using v8::FunctionCallbackInfo;
4142
using v8::HandleScope;
@@ -605,21 +606,18 @@ void SetEngine(const FunctionCallbackInfo<Value>& args) {
605606
#endif // !OPENSSL_NO_ENGINE
606607

607608
MaybeLocal<Value> EncodeBignum(Environment* env, const BIGNUM* bn, int size) {
609+
EscapableHandleScope scope(env->isolate());
608610
auto buf = BignumPointer::EncodePadded(bn, size);
609611
CHECK_EQ(buf.size(), static_cast<size_t>(size));
610612
Local<Value> ret;
611-
Local<Value> error;
612613
if (!StringBytes::Encode(env->isolate(),
613614
reinterpret_cast<const char*>(buf.get()),
614615
buf.size(),
615-
BASE64URL,
616-
&error)
616+
BASE64URL)
617617
.ToLocal(&ret)) {
618-
CHECK(!error.IsEmpty());
619-
env->isolate()->ThrowException(error);
620-
return MaybeLocal<Value>();
618+
return {};
621619
}
622-
return ret;
620+
return scope.Escape(ret);
623621
}
624622

625623
Maybe<void> SetEncodedValue(Environment* env,

‎src/encoding_binding.cc‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -183,17 +183,10 @@ void BindingData::DecodeUTF8(const FunctionCallbackInfo<Value>& args) {
183183

184184
if (length == 0) return args.GetReturnValue().SetEmptyString();
185185

186-
Local<Value> error;
187186
Local<Value> ret;
188-
189-
if (!StringBytes::Encode(env->isolate(), data, length, UTF8, &error)
190-
.ToLocal(&ret)) {
191-
CHECK(!error.IsEmpty());
192-
env->isolate()->ThrowException(error);
193-
return;
187+
if (StringBytes::Encode(env->isolate(), data, length, UTF8).ToLocal(&ret)) {
188+
args.GetReturnValue().Set(ret);
194189
}
195-
196-
args.GetReturnValue().Set(ret);
197190
}
198191

199192
void BindingData::ToASCII(const FunctionCallbackInfo<Value>& args) {

‎src/fs_event_wrap.cc‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ using v8::PropertyAttribute;
4444
using v8::ReadOnly;
4545
using v8::Signature;
4646
using v8::String;
47+
using v8::TryCatch;
4748
using v8::Value;
4849

4950
namespace {
@@ -217,18 +218,19 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
217218
};
218219

219220
if (filename != nullptr) {
220-
Local<Value> error;
221-
MaybeLocal<Value> fn = StringBytes::Encode(env->isolate(),
222-
filename,
223-
wrap->encoding_,
224-
&error);
221+
// TODO(@jasnell): Historically, this code has failed to correctly
222+
// propagate any error returned by the StringBytes::Encode method,
223+
// and would instead just crash the process. That behavior is preserved
224+
// here but should be looked at. Preferrably errors would be handled
225+
// correctly here.
226+
TryCatch try_catch(env->isolate());
227+
MaybeLocal<Value> fn =
228+
StringBytes::Encode(env->isolate(), filename, wrap->encoding_);
225229
if (fn.IsEmpty()) {
226230
argv[0] = Integer::New(env->isolate(), UV_EINVAL);
227-
argv[2] = StringBytes::Encode(env->isolate(),
228-
filename,
229-
strlen(filename),
230-
BUFFER,
231-
&error).ToLocalChecked();
231+
argv[2] = StringBytes::Encode(
232+
env->isolate(), filename, strlen(filename), BUFFER)
233+
.ToLocalChecked();
232234
} else {
233235
argv[2] = fn.ToLocalChecked();
234236
}

‎src/node.h‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1124,16 +1124,37 @@ NODE_EXTERN enum encoding ParseEncoding(
11241124
NODE_EXTERN void FatalException(v8::Isolate* isolate,
11251125
const v8::TryCatch& try_catch);
11261126

1127-
NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
1128-
const char* buf,
1129-
size_t len,
1130-
enum encoding encoding = LATIN1);
1127+
NODE_EXTERN v8::MaybeLocal<v8::Value> TryEncode(
1128+
v8::Isolate* isolate,
1129+
const char* buf,
1130+
size_t len,
1131+
enum encoding encoding = LATIN1);
1132+
1133+
// Warning: This reverses endianness on Big Endian platforms, even though the
1134+
// signature using uint16_t implies that it should not.
1135+
NODE_EXTERN v8::MaybeLocal<v8::Value> TryEncode(v8::Isolate* isolate,
1136+
const uint16_t* buf,
1137+
size_t len);
1138+
1139+
// The original Encode(...) functions are deprecated because they do not
1140+
// appropriately propagate exceptions and instead rely on ToLocalChecked()
1141+
// which crashes the process if an exception occurs. We cannot just remove
1142+
// these as it would break ABI compatibility, so we keep them around but
1143+
// deprecate them in favor of the TryEncode(...) variations which return
1144+
// a MaybeLocal<> and do not crash the process if an exception occurs.
1145+
NODE_DEPRECATED(
1146+
"Use TryEncode(...) instead",
1147+
NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
1148+
const char* buf,
1149+
size_t len,
1150+
enum encoding encoding = LATIN1));
11311151

11321152
// Warning: This reverses endianness on Big Endian platforms, even though the
11331153
// signature using uint16_t implies that it should not.
1134-
NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
1135-
const uint16_t* buf,
1136-
size_t len);
1154+
NODE_DEPRECATED("Use TryEncode(...) instead",
1155+
NODE_EXTERN v8::Local<v8::Value> Encode(v8::Isolate* isolate,
1156+
const uint16_t* buf,
1157+
size_t len));
11371158

11381159
// Returns -1 if the handle was not valid for decoding
11391160
NODE_EXTERN ssize_t DecodeBytes(v8::Isolate* isolate,

0 commit comments

Comments
 (0)