Skip to content

Commit 408d69b

Browse files
tniessentniessen
committed
os,vm: fix segfaults and CHECK failure
Fixes multiple possible segmentation faults in node_contextify.cc and an assertion failure in node_os.cc Fixes: nodejs#12369 Fixes: nodejs#12370 PR-URL: nodejs#12371 Reviewed-By: Anna Henningsen <[email protected]>
1 parent 6523d14 commit 408d69b

File tree

3 files changed

+157
-49
lines changed

3 files changed

+157
-49
lines changed

‎src/node_contextify.cc

+113-48
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,13 @@ using v8::FunctionCallbackInfo;
2323
using v8::FunctionTemplate;
2424
using v8::HandleScope;
2525
using v8::Integer;
26+
using v8::Just;
2627
using v8::Local;
2728
using v8::Maybe;
2829
using v8::MaybeLocal;
2930
using v8::Name;
3031
using v8::NamedPropertyHandlerConfiguration;
32+
using v8::Nothing;
3133
using v8::Object;
3234
using v8::ObjectTemplate;
3335
using v8::Persistent;
@@ -495,17 +497,20 @@ class ContextifyScript : public BaseObject {
495497
Local<String> code = args[0]->ToString(env->isolate());
496498

497499
Local<Value> options = args[1];
498-
Local<String> filename = GetFilenameArg(env, options);
499-
Local<Integer> lineOffset = GetLineOffsetArg(env, options);
500-
Local<Integer> columnOffset = GetColumnOffsetArg(env, options);
501-
bool display_errors = GetDisplayErrorsArg(env, options);
500+
MaybeLocal<String> filename = GetFilenameArg(env, options);
501+
MaybeLocal<Integer> lineOffset = GetLineOffsetArg(env, options);
502+
MaybeLocal<Integer> columnOffset = GetColumnOffsetArg(env, options);
503+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, options);
502504
MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, options);
503-
bool produce_cached_data = GetProduceCachedData(env, options);
505+
Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
504506
if (try_catch.HasCaught()) {
505507
try_catch.ReThrow();
506508
return;
507509
}
508510

511+
bool display_errors = maybe_display_errors.FromJust();
512+
bool produce_cached_data = maybe_produce_cached_data.FromJust();
513+
509514
ScriptCompiler::CachedData* cached_data = nullptr;
510515
if (!cached_data_buf.IsEmpty()) {
511516
Local<Uint8Array> ui8 = cached_data_buf.ToLocalChecked();
@@ -515,7 +520,8 @@ class ContextifyScript : public BaseObject {
515520
ui8->ByteLength());
516521
}
517522

518-
ScriptOrigin origin(filename, lineOffset, columnOffset);
523+
ScriptOrigin origin(filename.ToLocalChecked(), lineOffset.ToLocalChecked(),
524+
columnOffset.ToLocalChecked());
519525
ScriptCompiler::Source source(code, origin, cached_data);
520526
ScriptCompiler::CompileOptions compile_options =
521527
ScriptCompiler::kNoCompileOptions;
@@ -573,14 +579,18 @@ class ContextifyScript : public BaseObject {
573579

574580
// Assemble arguments
575581
TryCatch try_catch(args.GetIsolate());
576-
uint64_t timeout = GetTimeoutArg(env, args[0]);
577-
bool display_errors = GetDisplayErrorsArg(env, args[0]);
578-
bool break_on_sigint = GetBreakOnSigintArg(env, args[0]);
582+
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[0]);
583+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[0]);
584+
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]);
579585
if (try_catch.HasCaught()) {
580586
try_catch.ReThrow();
581587
return;
582588
}
583589

590+
int64_t timeout = maybe_timeout.FromJust();
591+
bool display_errors = maybe_display_errors.FromJust();
592+
bool break_on_sigint = maybe_break_on_sigint.FromJust();
593+
584594
// Do the eval within this context
585595
EvalMachine(env, timeout, display_errors, break_on_sigint, args,
586596
&try_catch);
@@ -603,13 +613,17 @@ class ContextifyScript : public BaseObject {
603613
Local<Object> sandbox = args[0].As<Object>();
604614
{
605615
TryCatch try_catch(env->isolate());
606-
timeout = GetTimeoutArg(env, args[1]);
607-
display_errors = GetDisplayErrorsArg(env, args[1]);
608-
break_on_sigint = GetBreakOnSigintArg(env, args[1]);
616+
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[1]);
617+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[1]);
618+
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]);
609619
if (try_catch.HasCaught()) {
610620
try_catch.ReThrow();
611621
return;
612622
}
623+
624+
timeout = maybe_timeout.FromJust();
625+
display_errors = maybe_display_errors.FromJust();
626+
break_on_sigint = maybe_break_on_sigint.FromJust();
613627
}
614628

615629
// Get the context from the sandbox
@@ -679,61 +693,82 @@ class ContextifyScript : public BaseObject {
679693
True(env->isolate()));
680694
}
681695

682-
static bool GetBreakOnSigintArg(Environment* env, Local<Value> options) {
696+
static Maybe<bool> GetBreakOnSigintArg(Environment* env,
697+
Local<Value> options) {
683698
if (options->IsUndefined() || options->IsString()) {
684-
return false;
699+
return Just(false);
685700
}
686701
if (!options->IsObject()) {
687702
env->ThrowTypeError("options must be an object");
688-
return false;
703+
return Nothing<bool>();
689704
}
690705

691706
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint");
692-
Local<Value> value = options.As<Object>()->Get(key);
693-
return value->IsTrue();
707+
MaybeLocal<Value> maybe_value =
708+
options.As<Object>()->Get(env->context(), key);
709+
if (maybe_value.IsEmpty())
710+
return Nothing<bool>();
711+
712+
Local<Value> value = maybe_value.ToLocalChecked();
713+
return Just(value->IsTrue());
694714
}
695715

696-
static int64_t GetTimeoutArg(Environment* env, Local<Value> options) {
716+
static Maybe<int64_t> GetTimeoutArg(Environment* env, Local<Value> options) {
697717
if (options->IsUndefined() || options->IsString()) {
698-
return -1;
718+
return Just<int64_t>(-1);
699719
}
700720
if (!options->IsObject()) {
701721
env->ThrowTypeError("options must be an object");
702-
return -1;
722+
return Nothing<int64_t>();
703723
}
704724

705-
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "timeout");
706-
Local<Value> value = options.As<Object>()->Get(key);
725+
MaybeLocal<Value> maybe_value =
726+
options.As<Object>()->Get(env->context(), env->timeout_string());
727+
if (maybe_value.IsEmpty())
728+
return Nothing<int64_t>();
729+
730+
Local<Value> value = maybe_value.ToLocalChecked();
707731
if (value->IsUndefined()) {
708-
return -1;
732+
return Just<int64_t>(-1);
709733
}
710-
int64_t timeout = value->IntegerValue();
711734

712-
if (timeout <= 0) {
735+
Maybe<int64_t> timeout = value->IntegerValue(env->context());
736+
737+
if (timeout.IsJust() && timeout.FromJust() <= 0) {
713738
env->ThrowRangeError("timeout must be a positive number");
714-
return -1;
739+
return Nothing<int64_t>();
715740
}
741+
716742
return timeout;
717743
}
718744

719745

720-
static bool GetDisplayErrorsArg(Environment* env, Local<Value> options) {
746+
static Maybe<bool> GetDisplayErrorsArg(Environment* env,
747+
Local<Value> options) {
721748
if (options->IsUndefined() || options->IsString()) {
722-
return true;
749+
return Just(true);
723750
}
724751
if (!options->IsObject()) {
725752
env->ThrowTypeError("options must be an object");
726-
return false;
753+
return Nothing<bool>();
727754
}
728755

729756
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors");
730-
Local<Value> value = options.As<Object>()->Get(key);
757+
MaybeLocal<Value> maybe_value =
758+
options.As<Object>()->Get(env->context(), key);
759+
if (maybe_value.IsEmpty())
760+
return Nothing<bool>();
731761

732-
return value->IsUndefined() ? true : value->BooleanValue();
762+
Local<Value> value = maybe_value.ToLocalChecked();
763+
if (value->IsUndefined())
764+
return Just(true);
765+
766+
return value->BooleanValue(env->context());
733767
}
734768

735769

736-
static Local<String> GetFilenameArg(Environment* env, Local<Value> options) {
770+
static MaybeLocal<String> GetFilenameArg(Environment* env,
771+
Local<Value> options) {
737772
Local<String> defaultFilename =
738773
FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine.<anonymous>");
739774

@@ -749,11 +784,15 @@ class ContextifyScript : public BaseObject {
749784
}
750785

751786
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename");
752-
Local<Value> value = options.As<Object>()->Get(key);
787+
MaybeLocal<Value> maybe_value =
788+
options.As<Object>()->Get(env->context(), key);
789+
if (maybe_value.IsEmpty())
790+
return MaybeLocal<String>();
753791

792+
Local<Value> value = maybe_value.ToLocalChecked();
754793
if (value->IsUndefined())
755794
return defaultFilename;
756-
return value->ToString(env->isolate());
795+
return value->ToString(env->context());
757796
}
758797

759798

@@ -762,7 +801,13 @@ class ContextifyScript : public BaseObject {
762801
if (!options->IsObject()) {
763802
return MaybeLocal<Uint8Array>();
764803
}
765-
Local<Value> value = options.As<Object>()->Get(env->cached_data_string());
804+
805+
MaybeLocal<Value> maybe_value =
806+
options.As<Object>()->Get(env->context(), env->cached_data_string());
807+
if (maybe_value.IsEmpty())
808+
return MaybeLocal<Uint8Array>();
809+
810+
Local<Value> value = maybe_value.ToLocalChecked();
766811
if (value->IsUndefined()) {
767812
return MaybeLocal<Uint8Array>();
768813
}
@@ -776,44 +821,64 @@ class ContextifyScript : public BaseObject {
776821
}
777822

778823

779-
static bool GetProduceCachedData(Environment* env, Local<Value> options) {
824+
static Maybe<bool> GetProduceCachedData(Environment* env,
825+
Local<Value> options) {
780826
if (!options->IsObject()) {
781-
return false;
827+
return Just(false);
782828
}
783-
Local<Value> value =
784-
options.As<Object>()->Get(env->produce_cached_data_string());
785829

786-
return value->IsTrue();
830+
MaybeLocal<Value> maybe_value =
831+
options.As<Object>()->Get(env->context(),
832+
env->produce_cached_data_string());
833+
if (maybe_value.IsEmpty())
834+
return Nothing<bool>();
835+
836+
Local<Value> value = maybe_value.ToLocalChecked();
837+
return Just(value->IsTrue());
787838
}
788839

789840

790-
static Local<Integer> GetLineOffsetArg(Environment* env,
791-
Local<Value> options) {
841+
static MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
842+
Local<Value> options) {
792843
Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);
793844

794845
if (!options->IsObject()) {
795846
return defaultLineOffset;
796847
}
797848

798849
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset");
799-
Local<Value> value = options.As<Object>()->Get(key);
850+
MaybeLocal<Value> maybe_value =
851+
options.As<Object>()->Get(env->context(), key);
852+
if (maybe_value.IsEmpty())
853+
return MaybeLocal<Integer>();
800854

801-
return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
855+
Local<Value> value = maybe_value.ToLocalChecked();
856+
if (value->IsUndefined())
857+
return defaultLineOffset;
858+
859+
return value->ToInteger(env->context());
802860
}
803861

804862

805-
static Local<Integer> GetColumnOffsetArg(Environment* env,
806-
Local<Value> options) {
863+
static MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
864+
Local<Value> options) {
807865
Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);
808866

809867
if (!options->IsObject()) {
810868
return defaultColumnOffset;
811869
}
812870

813871
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset");
814-
Local<Value> value = options.As<Object>()->Get(key);
872+
MaybeLocal<Value> maybe_value =
873+
options.As<Object>()->Get(env->context(), key);
874+
if (maybe_value.IsEmpty())
875+
return MaybeLocal<Integer>();
876+
877+
Local<Value> value = maybe_value.ToLocalChecked();
878+
if (value->IsUndefined())
879+
return defaultColumnOffset;
815880

816-
return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
881+
return value->ToInteger(env->context());
817882
}
818883

819884

‎src/node_os.cc

+7-1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ using v8::Float64Array;
3535
using v8::FunctionCallbackInfo;
3636
using v8::Integer;
3737
using v8::Local;
38+
using v8::MaybeLocal;
3839
using v8::Null;
3940
using v8::Number;
4041
using v8::Object;
@@ -306,7 +307,12 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
306307

307308
if (args[0]->IsObject()) {
308309
Local<Object> options = args[0].As<Object>();
309-
Local<Value> encoding_opt = options->Get(env->encoding_string());
310+
MaybeLocal<Value> maybe_encoding = options->Get(env->context(),
311+
env->encoding_string());
312+
if (maybe_encoding.IsEmpty())
313+
return;
314+
315+
Local<Value> encoding_opt = maybe_encoding.ToLocalChecked();
310316
encoding = ParseEncoding(env->isolate(), encoding_opt, UTF8);
311317
} else {
312318
encoding = UTF8;

‎test/parallel/test-regress-GH-12371.js

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const execFile = require('child_process').execFile;
5+
6+
const scripts = [
7+
`os.userInfo({
8+
get encoding() {
9+
throw new Error('xyz');
10+
}
11+
})`
12+
];
13+
14+
['filename', 'cachedData', 'produceCachedData', 'lineOffset', 'columnOffset']
15+
.forEach((prop) => {
16+
scripts.push(`vm.createScript('', {
17+
get ${prop} () {
18+
throw new Error('xyz');
19+
}
20+
})`);
21+
});
22+
23+
['breakOnSigint', 'timeout', 'displayErrors']
24+
.forEach((prop) => {
25+
scripts.push(`vm.createScript('').runInThisContext({
26+
get ${prop} () {
27+
throw new Error('xyz');
28+
}
29+
})`);
30+
});
31+
32+
scripts.forEach((script) => {
33+
const node = process.execPath;
34+
execFile(node, [ '-e', script ], common.mustCall((err, stdout, stderr) => {
35+
assert(stderr.includes('Error: xyz'), 'createScript crashes');
36+
}));
37+
});

0 commit comments

Comments
 (0)