Skip to content

Commit

Permalink
src: do proper StringBytes error handling
Browse files Browse the repository at this point in the history 
- 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: nodejs#3175
  • Loading branch information
@addaleax
addaleax committed May 1, 2017
1 parent 0833d31 commit 0aff46e
Show file tree
Hide file tree
Showing 9 changed files with 303 additions and 170 deletions.
8 changes: 2 additions & 6 deletions lib/buffer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,8 @@ Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
// This behaves neither like String nor Uint8Array in that we set start/end
// to their upper/lower bounds if the value passed is out of range.
Buffer.prototype.toString = function(encoding, start, end) {
var result;
if (arguments.length === 0) {
result = this.utf8Slice(0, this.length);
return this.utf8Slice(0, this.length);
} else {
const len = this.length;
if (len === 0)
Expand All @@ -590,11 +589,8 @@ Buffer.prototype.toString = function(encoding, start, end) {

if (end <= start)
return '';
result = stringSlice(this, encoding, start, end);
return stringSlice(this, encoding, start, end);
}
if (result === undefined)
throw new Error('"toString()" failed');
return result;
};


Expand Down
14 changes: 9 additions & 5 deletions src/fs_event_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ using v8::FunctionTemplate;
using v8::HandleScope;
using v8::Integer;
using v8::Local;
using v8::MaybeLocal;
using v8::Object;
using v8::String;
using v8::Value;
Expand Down Expand Up @@ -187,17 +188,20 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
};

if (filename != nullptr) {
Local<Value> fn = StringBytes::Encode(env->isolate(),
filename,
wrap->encoding_);
Local<Value> error;
MaybeLocal<Value> fn = StringBytes::Encode(env->isolate(),
filename,
wrap->encoding_,
&error);
if (fn.IsEmpty()) {
argv[0] = Integer::New(env->isolate(), UV_EINVAL);
argv[2] = StringBytes::Encode(env->isolate(),
filename,
strlen(filename),
BUFFER);
BUFFER,
&error).ToLocalChecked();
} else {
argv[2] = fn;
argv[2] = fn.ToLocalChecked();
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1570,11 +1570,15 @@ Local<Value> Encode(Isolate* isolate,
size_t len,
enum encoding encoding) {
CHECK_NE(encoding, UCS2);
return StringBytes::Encode(isolate, buf, len, encoding);
Local<Value> error;
return StringBytes::Encode(isolate, buf, len, encoding, &error)
.ToLocalChecked();
}

Local<Value> Encode(Isolate* isolate, const uint16_t* buf, size_t len) {
return StringBytes::Encode(isolate, buf, len);
Local<Value> error;
return StringBytes::Encode(isolate, buf, len, &error)
.ToLocalChecked();
}

// Returns -1 if the handle was not valid for decoding
Expand Down
32 changes: 28 additions & 4 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,14 +465,26 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {

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

args.GetReturnValue().Set(
StringBytes::Encode(isolate, ts_obj_data + start, length, encoding));
Local<Value> error;
MaybeLocal<Value> ret =
StringBytes::Encode(isolate,
ts_obj_data + start,
length,
encoding,
&error);
if (ret.IsEmpty()) {
CHECK(!error.IsEmpty());
isolate->ThrowException(error);
return;
}
args.GetReturnValue().Set(ret.ToLocalChecked());
}


template <>
void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = args.GetIsolate();
Environment* env = Environment::GetCurrent(isolate);

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

args.GetReturnValue().Set(StringBytes::Encode(env->isolate(), buf, length));
Local<Value> error;
MaybeLocal<Value> ret =
StringBytes::Encode(isolate,
buf,
length,
&error);

if (release)
delete[] buf;

if (ret.IsEmpty()) {
CHECK(!error.IsEmpty());
isolate->ThrowException(error);
return;
}
args.GetReturnValue().Set(ret.ToLocalChecked());
}


Expand Down
33 changes: 23 additions & 10 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Null;
using v8::Object;
using v8::Persistent;
Expand Down Expand Up @@ -3807,12 +3808,18 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
md_len = 0;
}

Local<Value> rc = StringBytes::Encode(env->isolate(),
reinterpret_cast<const char*>(md_value),
md_len,
encoding);
Local<Value> error;
MaybeLocal<Value> rc =
StringBytes::Encode(env->isolate(),
reinterpret_cast<const char*>(md_value),
md_len,
encoding,
&error);
delete[] md_value;
args.GetReturnValue().Set(rc);
// TODO(addaleax): Throw `error` here. This isn't so terribly important,
// hashes are very short so creating the corresponding strings is very
// unlikely to fail.
args.GetReturnValue().Set(rc.ToLocalChecked());
}


Expand Down Expand Up @@ -3928,11 +3935,17 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
EVP_MD_CTX_cleanup(&hash->mdctx_);
hash->finalized_ = true;

Local<Value> rc = StringBytes::Encode(env->isolate(),
reinterpret_cast<const char*>(md_value),
md_len,
encoding);
args.GetReturnValue().Set(rc);
Local<Value> error;
MaybeLocal<Value> rc =
StringBytes::Encode(env->isolate(),
reinterpret_cast<const char*>(md_value),
md_len,
encoding,
&error);
// TODO(addaleax): Throw `error` here. This isn't so terribly important,
// hashes are very short so creating the corresponding strings is very
// unlikely to fail.
args.GetReturnValue().Set(rc.ToLocalChecked());
}


Expand Down
78 changes: 53 additions & 25 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ using v8::FunctionTemplate;
using v8::HandleScope;
using v8::Integer;
using v8::Local;
using v8::MaybeLocal;
using v8::Number;
using v8::Object;
using v8::String;
Expand Down Expand Up @@ -172,7 +173,8 @@ void After(uv_fs_t *req) {
// Allocate space for two args. We may only use one depending on the case.
// (Feel free to increase this if you need more)
Local<Value> argv[2];
Local<Value> link;
MaybeLocal<Value> link;
Local<Value> error;

if (req->result < 0) {
// An error happened.
Expand Down Expand Up @@ -232,50 +234,58 @@ void After(uv_fs_t *req) {
break;

case UV_FS_MKDTEMP:
{
link = StringBytes::Encode(env->isolate(),
static_cast<const char*>(req->path),
req_wrap->encoding_);
req_wrap->encoding_,
&error);
if (link.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
argv[0] = UVException(env->isolate(),
UV_EINVAL,
req_wrap->syscall(),
"Invalid character encoding for filename",
req->path,
req_wrap->data());
} else {
argv[1] = link;
argv[1] = link.ToLocalChecked();
}
break;
}

case UV_FS_READLINK:
link = StringBytes::Encode(env->isolate(),
static_cast<const char*>(req->ptr),
req_wrap->encoding_);
req_wrap->encoding_,
&error);
if (link.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
argv[0] = UVException(env->isolate(),
UV_EINVAL,
req_wrap->syscall(),
"Invalid character encoding for link",
req->path,
req_wrap->data());
} else {
argv[1] = link;
argv[1] = link.ToLocalChecked();
}
break;

case UV_FS_REALPATH:
link = StringBytes::Encode(env->isolate(),
static_cast<const char*>(req->ptr),
req_wrap->encoding_);
req_wrap->encoding_,
&error);
if (link.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
argv[0] = UVException(env->isolate(),
UV_EINVAL,
req_wrap->syscall(),
"Invalid character encoding for link",
req->path,
req_wrap->data());
} else {
argv[1] = link;
argv[1] = link.ToLocalChecked();
}
break;

Expand Down Expand Up @@ -306,10 +316,13 @@ void After(uv_fs_t *req) {
break;
}

Local<Value> filename = StringBytes::Encode(env->isolate(),
ent.name,
req_wrap->encoding_);
MaybeLocal<Value> filename =
StringBytes::Encode(env->isolate(),
ent.name,
req_wrap->encoding_,
&error);
if (filename.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
argv[0] = UVException(env->isolate(),
UV_EINVAL,
req_wrap->syscall(),
Expand All @@ -318,7 +331,7 @@ void After(uv_fs_t *req) {
req_wrap->data());
break;
}
name_argv[name_idx++] = filename;
name_argv[name_idx++] = filename.ToLocalChecked();

if (name_idx >= arraysize(name_argv)) {
fn->Call(env->context(), names, name_idx, name_argv)
Expand Down Expand Up @@ -681,16 +694,20 @@ static void ReadLink(const FunctionCallbackInfo<Value>& args) {
} else {
SYNC_CALL(readlink, *path, *path)
const char* link_path = static_cast<const char*>(SYNC_REQ.ptr);
Local<Value> rc = StringBytes::Encode(env->isolate(),
link_path,
encoding);

Local<Value> error;
MaybeLocal<Value> rc = StringBytes::Encode(env->isolate(),
link_path,
encoding,
&error);
if (rc.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
return env->ThrowUVException(UV_EINVAL,
"readlink",
"Invalid character encoding for link",
*path);
}
args.GetReturnValue().Set(rc);
args.GetReturnValue().Set(rc.ToLocalChecked());
}
}

Expand Down Expand Up @@ -852,16 +869,20 @@ static void RealPath(const FunctionCallbackInfo<Value>& args) {
} else {
SYNC_CALL(realpath, *path, *path);
const char* link_path = static_cast<const char*>(SYNC_REQ.ptr);
Local<Value> rc = StringBytes::Encode(env->isolate(),
link_path,
encoding);

Local<Value> error;
MaybeLocal<Value> rc = StringBytes::Encode(env->isolate(),
link_path,
encoding,
&error);
if (rc.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
return env->ThrowUVException(UV_EINVAL,
"realpath",
"Invalid character encoding for path",
*path);
}
args.GetReturnValue().Set(rc);
args.GetReturnValue().Set(rc.ToLocalChecked());
}
}

Expand Down Expand Up @@ -903,17 +924,20 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
if (r != 0)
return env->ThrowUVException(r, "readdir", "", *path);

Local<Value> filename = StringBytes::Encode(env->isolate(),
ent.name,
encoding);
Local<Value> error;
MaybeLocal<Value> filename = StringBytes::Encode(env->isolate(),
ent.name,
encoding,
&error);
if (filename.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
return env->ThrowUVException(UV_EINVAL,
"readdir",
"Invalid character encoding for filename",
*path);
}

name_v[name_idx++] = filename;
name_v[name_idx++] = filename.ToLocalChecked();

if (name_idx >= arraysize(name_v)) {
fn->Call(env->context(), names, name_idx, name_v)
Expand Down Expand Up @@ -1367,14 +1391,18 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
} else {
SYNC_CALL(mkdtemp, *tmpl, *tmpl);
const char* path = static_cast<const char*>(SYNC_REQ.path);
Local<Value> rc = StringBytes::Encode(env->isolate(), path, encoding);

Local<Value> error;
MaybeLocal<Value> rc =
StringBytes::Encode(env->isolate(), path, encoding, &error);
if (rc.IsEmpty()) {
// TODO(addaleax): Use `error` itself here.
return env->ThrowUVException(UV_EINVAL,
"mkdtemp",
"Invalid character encoding for filename",
*tmpl);
}
args.GetReturnValue().Set(rc);
args.GetReturnValue().Set(rc.ToLocalChecked());
}
}

Expand Down
Loading

0 comments on commit 0aff46e

Please sign in to comment.