Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

os,contextify: fix segfaults and CHECK failure #12371

Closed
wants to merge 3 commits into from
Closed

os,contextify: fix segfaults and CHECK failure #12371

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
225fd64
os,contextify: fix segfaults and CHECK failure
tniessen Apr 12, 2017
6379d86
Add regression test
tniessen Apr 12, 2017
bb5ed7b
style: maybeValue -> maybe_value
tniessen Apr 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 113 additions & 47 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@ using v8::FunctionCallbackInfo;
using v8::FunctionTemplate;
using v8::HandleScope;
using v8::Integer;
using v8::Just;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Name;
using v8::NamedPropertyHandlerConfiguration;
using v8::Nothing;
using v8::Object;
using v8::ObjectTemplate;
using v8::Persistent;
Expand Down Expand Up @@ -546,17 +548,20 @@ class ContextifyScript : public BaseObject {
Local<String> code = args[0]->ToString(env->isolate());

Local<Value> options = args[1];
Local<String> filename = GetFilenameArg(env, options);
Local<Integer> lineOffset = GetLineOffsetArg(env, options);
Local<Integer> columnOffset = GetColumnOffsetArg(env, options);
bool display_errors = GetDisplayErrorsArg(env, options);
MaybeLocal<String> filename = GetFilenameArg(env, options);
MaybeLocal<Integer> lineOffset = GetLineOffsetArg(env, options);
MaybeLocal<Integer> columnOffset = GetColumnOffsetArg(env, options);
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, options);
MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, options);
bool produce_cached_data = GetProduceCachedData(env, options);
Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
if (try_catch.HasCaught()) {
try_catch.ReThrow();
return;
}

bool display_errors = maybe_display_errors.ToChecked();
bool produce_cached_data = maybe_produce_cached_data.ToChecked();

ScriptCompiler::CachedData* cached_data = nullptr;
Local<Uint8Array> ui8;
if (cached_data_buf.ToLocal(&ui8)) {
Expand All @@ -566,7 +571,8 @@ class ContextifyScript : public BaseObject {
ui8->ByteLength());
}

ScriptOrigin origin(filename, lineOffset, columnOffset);
ScriptOrigin origin(filename.ToLocalChecked(), lineOffset.ToLocalChecked(),
columnOffset.ToLocalChecked());
ScriptCompiler::Source source(code, origin, cached_data);
ScriptCompiler::CompileOptions compile_options =
ScriptCompiler::kNoCompileOptions;
Expand Down Expand Up @@ -624,14 +630,18 @@ class ContextifyScript : public BaseObject {

// Assemble arguments
TryCatch try_catch(args.GetIsolate());
uint64_t timeout = GetTimeoutArg(env, args[0]);
bool display_errors = GetDisplayErrorsArg(env, args[0]);
bool break_on_sigint = GetBreakOnSigintArg(env, args[0]);
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[0]);
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[0]);
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]);
if (try_catch.HasCaught()) {
try_catch.ReThrow();
return;
}

int64_t timeout = maybe_timeout.ToChecked();
bool display_errors = maybe_display_errors.ToChecked();
bool break_on_sigint = maybe_break_on_sigint.ToChecked();

// Do the eval within this context
EvalMachine(env, timeout, display_errors, break_on_sigint, args,
&try_catch);
Expand All @@ -654,13 +664,17 @@ class ContextifyScript : public BaseObject {
Local<Object> sandbox = args[0].As<Object>();
{
TryCatch try_catch(env->isolate());
timeout = GetTimeoutArg(env, args[1]);
display_errors = GetDisplayErrorsArg(env, args[1]);
break_on_sigint = GetBreakOnSigintArg(env, args[1]);
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[1]);
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[1]);
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]);
if (try_catch.HasCaught()) {
try_catch.ReThrow();
return;
}

timeout = maybe_timeout.ToChecked();
display_errors = maybe_display_errors.ToChecked();
break_on_sigint = maybe_break_on_sigint.ToChecked();
}

// Get the context from the sandbox
Expand Down Expand Up @@ -730,60 +744,82 @@ class ContextifyScript : public BaseObject {
True(env->isolate()));
}

static bool GetBreakOnSigintArg(Environment* env, Local<Value> options) {
static Maybe<bool> GetBreakOnSigintArg(Environment* env,
Local<Value> options) {
if (options->IsUndefined() || options->IsString()) {
return false;
return Just(false);
}
if (!options->IsObject()) {
env->ThrowTypeError("options must be an object");
return false;
return Nothing<bool>();
}

Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint");
Local<Value> value = options.As<Object>()->Get(key);
return value->IsTrue();
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe maybe_value? ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node_contextify.cc is pretty inconsistent about this, even within functions 😞 By the way, why don't we have linting for variable names?

If you prefer, I will change it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, why don't we have linting for variable names?

I guess it’s mostly because there are only a handful of people contributing to the C++ sources on a regular basis, so adding linter rules comes with a disproportionate overhead when compared to e.g. the JS sources…

If you prefer, I will change it :)

I don’t care much personally, but I’d bet @bnoordhuis is going to request it. 😄

if (maybeValue.IsEmpty())
return Nothing<bool>();

Local<Value> value = maybeValue.ToLocalChecked();
return Just(value->IsTrue());
}

static int64_t GetTimeoutArg(Environment* env, Local<Value> options) {
static Maybe<int64_t> GetTimeoutArg(Environment* env, Local<Value> options) {
if (options->IsUndefined() || options->IsString()) {
return -1;
return Just<int64_t>(-1);
}
if (!options->IsObject()) {
env->ThrowTypeError("options must be an object");
return -1;
return Nothing<int64_t>();
}

Local<Value> value = options.As<Object>()->Get(env->timeout_string());
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), env->timeout_string());
if (maybeValue.IsEmpty())
return Nothing<int64_t>();

Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined()) {
return -1;
return Just<int64_t>(-1);
}
int64_t timeout = value->IntegerValue();

if (timeout <= 0) {
Maybe<int64_t> timeout = value->IntegerValue(env->context());

if (timeout.IsJust() && timeout.ToChecked() <= 0) {
env->ThrowRangeError("timeout must be a positive number");
return -1;
return Nothing<int64_t>();
}

return timeout;
}


static bool GetDisplayErrorsArg(Environment* env, Local<Value> options) {
static Maybe<bool> GetDisplayErrorsArg(Environment* env,
Local<Value> options) {
if (options->IsUndefined() || options->IsString()) {
return true;
return Just(true);
}
if (!options->IsObject()) {
env->ThrowTypeError("options must be an object");
return false;
return Nothing<bool>();
}

Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors");
Local<Value> value = options.As<Object>()->Get(key);
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
if (maybeValue.IsEmpty())
return Nothing<bool>();

return value->IsUndefined() ? true : value->BooleanValue();
Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined())
return Just(true);

return value->BooleanValue(env->context());
}


static Local<String> GetFilenameArg(Environment* env, Local<Value> options) {
static MaybeLocal<String> GetFilenameArg(Environment* env,
Local<Value> options) {
Local<String> defaultFilename =
FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine.<anonymous>");

Expand All @@ -799,11 +835,15 @@ class ContextifyScript : public BaseObject {
}

Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename");
Local<Value> value = options.As<Object>()->Get(key);
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
if (maybeValue.IsEmpty())
return MaybeLocal<String>();

Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined())
return defaultFilename;
return value->ToString(env->isolate());
return value->ToString(env->context());
}


Expand All @@ -812,7 +852,13 @@ class ContextifyScript : public BaseObject {
if (!options->IsObject()) {
return MaybeLocal<Uint8Array>();
}
Local<Value> value = options.As<Object>()->Get(env->cached_data_string());

MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), env->cached_data_string());
if (maybeValue.IsEmpty())
return MaybeLocal<Uint8Array>();

Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined()) {
return MaybeLocal<Uint8Array>();
}
Expand All @@ -826,44 +872,64 @@ class ContextifyScript : public BaseObject {
}


static bool GetProduceCachedData(Environment* env, Local<Value> options) {
static Maybe<bool> GetProduceCachedData(Environment* env,
Local<Value> options) {
if (!options->IsObject()) {
return false;
return Just(false);
}
Local<Value> value =
options.As<Object>()->Get(env->produce_cached_data_string());

return value->IsTrue();
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(),
env->produce_cached_data_string());
if (maybeValue.IsEmpty())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ditto everywhere, you may want to search+replace for maybeValue)

return Nothing<bool>();

Local<Value> value = maybeValue.ToLocalChecked();
return Just(value->IsTrue());
}


static Local<Integer> GetLineOffsetArg(Environment* env,
Local<Value> options) {
static MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
Local<Value> options) {
Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);

if (!options->IsObject()) {
return defaultLineOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset");
Local<Value> value = options.As<Object>()->Get(key);
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
if (maybeValue.IsEmpty())
return MaybeLocal<Integer>();

return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined())
return defaultLineOffset;

return value->ToInteger(env->context());
}


static Local<Integer> GetColumnOffsetArg(Environment* env,
Local<Value> options) {
static MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
Local<Value> options) {
Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);

if (!options->IsObject()) {
return defaultColumnOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset");
Local<Value> value = options.As<Object>()->Get(key);
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
if (maybeValue.IsEmpty())
return MaybeLocal<Integer>();

Local<Value> value = maybeValue.ToLocalChecked();
if (value->IsUndefined())
return defaultColumnOffset;

return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
return value->ToInteger(env->context());
}


Expand Down
8 changes: 7 additions & 1 deletion src/node_os.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ using v8::Function;
using v8::FunctionCallbackInfo;
using v8::Integer;
using v8::Local;
using v8::MaybeLocal;
using v8::Null;
using v8::Number;
using v8::Object;
Expand Down Expand Up @@ -339,7 +340,12 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {

if (args[0]->IsObject()) {
Local<Object> options = args[0].As<Object>();
Local<Value> encoding_opt = options->Get(env->encoding_string());
MaybeLocal<Value> maybe_encoding = options->Get(env->context(),
env->encoding_string());
if (maybe_encoding.IsEmpty())
return;

Local<Value> encoding_opt = maybe_encoding.ToLocalChecked();
encoding = ParseEncoding(env->isolate(), encoding_opt, UTF8);
} else {
encoding = UTF8;
Expand Down