Skip to content

Commit d56a7e6

Browse files
addaleaxaddaleax
committed
src: do proper StringBytes error handling
- Return `MaybeLocal`s from `StringBytes::Encode` - Add an `error` out parameter to pass JS exceptions to the callers (instead of directly throwing) - Simplify some of the string generation methods in `string_bytes.cc` by unifying the `EXTERN_APEX` logic - Reduce usage of deprecated V8 APIs. - Remove error handling logic from JS, the `buffer.*Slice()` methods now throw errors themselves. - Left TODO comments for future semver-major error message improvements. This paves the way for better error messages coming out of the StringBytes methods. Ref: #3175 PR-URL: #12765 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
1 parent 6c2daf0 commit d56a7e6

File tree

9 files changed

+323
-186
lines changed

9 files changed

+323
-186
lines changed

‎lib/buffer.js

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -567,34 +567,30 @@ Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
567567
// This behaves neither like String nor Uint8Array in that we set start/end
568568
// to their upper/lower bounds if the value passed is out of range.
569569
Buffer.prototype.toString = function(encoding, start, end) {
570-
var result;
571570
if (arguments.length === 0) {
572-
result = this.utf8Slice(0, this.length);
573-
} else {
574-
const len = this.length;
575-
if (len === 0)
576-
return '';
571+
return this.utf8Slice(0, this.length);
572+
}
577573

578-
if (!start || start < 0)
579-
start = 0;
580-
else if (start >= len)
581-
return '';
574+
const len = this.length;
575+
if (len === 0)
576+
return '';
582577

583-
if (end === undefined || end > len)
584-
end = len;
585-
else if (end <= 0)
586-
return '';
578+
if (!start || start < 0)
579+
start = 0;
580+
else if (start >= len)
581+
return '';
587582

588-
start |= 0;
589-
end |= 0;
583+
if (end === undefined || end > len)
584+
end = len;
585+
else if (end <= 0)
586+
return '';
590587

591-
if (end <= start)
592-
return '';
593-
result = stringSlice(this, encoding, start, end);
594-
}
595-
if (result === undefined)
596-
throw new Error('"toString()" failed');
597-
return result;
588+
start |= 0;
589+
end |= 0;
590+
591+
if (end <= start)
592+
return '';
593+
return stringSlice(this, encoding, start, end);
598594
};
599595

600596

‎src/fs_event_wrap.cc

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ using v8::FunctionTemplate;
3939
using v8::HandleScope;
4040
using v8::Integer;
4141
using v8::Local;
42+
using v8::MaybeLocal;
4243
using v8::Object;
4344
using v8::String;
4445
using v8::Value;
@@ -187,17 +188,20 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
187188
};
188189

189190
if (filename != nullptr) {
190-
Local<Value> fn = StringBytes::Encode(env->isolate(),
191-
filename,
192-
wrap->encoding_);
191+
Local<Value> error;
192+
MaybeLocal<Value> fn = StringBytes::Encode(env->isolate(),
193+
filename,
194+
wrap->encoding_,
195+
&error);
193196
if (fn.IsEmpty()) {
194197
argv[0] = Integer::New(env->isolate(), UV_EINVAL);
195198
argv[2] = StringBytes::Encode(env->isolate(),
196199
filename,
197200
strlen(filename),
198-
BUFFER);
201+
BUFFER,
202+
&error).ToLocalChecked();
199203
} else {
200-
argv[2] = fn;
204+
argv[2] = fn.ToLocalChecked();
201205
}
202206
}
203207

‎src/node.cc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,11 +1570,15 @@ Local<Value> Encode(Isolate* isolate,
15701570
size_t len,
15711571
enum encoding encoding) {
15721572
CHECK_NE(encoding, UCS2);
1573-
return StringBytes::Encode(isolate, buf, len, encoding);
1573+
Local<Value> error;
1574+
return StringBytes::Encode(isolate, buf, len, encoding, &error)
1575+
.ToLocalChecked();
15741576
}
15751577

15761578
Local<Value> Encode(Isolate* isolate, const uint16_t* buf, size_t len) {
1577-
return StringBytes::Encode(isolate, buf, len);
1579+
Local<Value> error;
1580+
return StringBytes::Encode(isolate, buf, len, &error)
1581+
.ToLocalChecked();
15781582
}
15791583

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

‎src/node_buffer.cc

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,14 +465,26 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {
465465

466466
SLICE_START_END(args[0], args[1], ts_obj_length)
467467

468-
args.GetReturnValue().Set(
469-
StringBytes::Encode(isolate, ts_obj_data + start, length, encoding));
468+
Local<Value> error;
469+
MaybeLocal<Value> ret =
470+
StringBytes::Encode(isolate,
471+
ts_obj_data + start,
472+
length,
473+
encoding,
474+
&error);
475+
if (ret.IsEmpty()) {
476+
CHECK(!error.IsEmpty());
477+
isolate->ThrowException(error);
478+
return;
479+
}
480+
args.GetReturnValue().Set(ret.ToLocalChecked());
470481
}
471482

472483

473484
template <>
474485
void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
475-
Environment* env = Environment::GetCurrent(args);
486+
Isolate* isolate = args.GetIsolate();
487+
Environment* env = Environment::GetCurrent(isolate);
476488

477489
THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
478490
SPREAD_BUFFER_ARG(args.This(), ts_obj);
@@ -509,10 +521,22 @@ void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
509521
buf = reinterpret_cast<const uint16_t*>(data);
510522
}
511523

512-
args.GetReturnValue().Set(StringBytes::Encode(env->isolate(), buf, length));
524+
Local<Value> error;
525+
MaybeLocal<Value> ret =
526+
StringBytes::Encode(isolate,
527+
buf,
528+
length,
529+
&error);
513530

514531
if (release)
515532
delete[] buf;
533+
534+
if (ret.IsEmpty()) {
535+
CHECK(!error.IsEmpty());
536+
isolate->ThrowException(error);
537+
return;
538+
}
539+
args.GetReturnValue().Set(ret.ToLocalChecked());
516540
}
517541

518542

‎src/node_crypto.cc

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ using v8::Integer;
104104
using v8::Isolate;
105105
using v8::Local;
106106
using v8::Maybe;
107+
using v8::MaybeLocal;
107108
using v8::Null;
108109
using v8::Object;
109110
using v8::Persistent;
@@ -3811,12 +3812,20 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
38113812
md_len = 0;
38123813
}
38133814

3814-
Local<Value> rc = StringBytes::Encode(env->isolate(),
3815-
reinterpret_cast<const char*>(md_value),
3816-
md_len,
3817-
encoding);
3815+
Local<Value> error;
3816+
MaybeLocal<Value> rc =
3817+
StringBytes::Encode(env->isolate(),
3818+
reinterpret_cast<const char*>(md_value),
3819+
md_len,
3820+
encoding,
3821+
&error);
38183822
delete[] md_value;
3819-
args.GetReturnValue().Set(rc);
3823+
if (rc.IsEmpty()) {
3824+
CHECK(!error.IsEmpty());
3825+
env->isolate()->ThrowException(error);
3826+
return;
3827+
}
3828+
args.GetReturnValue().Set(rc.ToLocalChecked());
38203829
}
38213830

38223831

@@ -3936,11 +3945,19 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
39363945
EVP_MD_CTX_cleanup(&hash->mdctx_);
39373946
hash->finalized_ = true;
39383947

3939-
Local<Value> rc = StringBytes::Encode(env->isolate(),
3940-
reinterpret_cast<const char*>(md_value),
3941-
md_len,
3942-
encoding);
3943-
args.GetReturnValue().Set(rc);
3948+
Local<Value> error;
3949+
MaybeLocal<Value> rc =
3950+
StringBytes::Encode(env->isolate(),
3951+
reinterpret_cast<const char*>(md_value),
3952+
md_len,
3953+
encoding,
3954+
&error);
3955+
if (rc.IsEmpty()) {
3956+
CHECK(!error.IsEmpty());
3957+
env->isolate()->ThrowException(error);
3958+
return;
3959+
}
3960+
args.GetReturnValue().Set(rc.ToLocalChecked());
39443961
}
39453962

39463963

0 commit comments

Comments
 (0)