Skip to content

Commit 6998e80

Browse files
tniessenevanlucas
tniessen
authored and
evanlucas
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: #12369 Fixes: #12370 PR-URL: #12371 Reviewed-By: Anna Henningsen <[email protected]>
1 parent 92e239d commit 6998e80

File tree

3 files changed

+157
-48
lines changed

3 files changed

+157
-48
lines changed

‎src/node_contextify.cc‎

Lines changed: 113 additions & 47 deletions
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;
@@ -525,17 +527,20 @@ class ContextifyScript : public BaseObject {
525527
Local<String> code = args[0]->ToString(env->isolate());
526528

527529
Local<Value> options = args[1];
528-
Local<String> filename = GetFilenameArg(env, options);
529-
Local<Integer> lineOffset = GetLineOffsetArg(env, options);
530-
Local<Integer> columnOffset = GetColumnOffsetArg(env, options);
531-
bool display_errors = GetDisplayErrorsArg(env, options);
530+
MaybeLocal<String> filename = GetFilenameArg(env, options);
531+
MaybeLocal<Integer> lineOffset = GetLineOffsetArg(env, options);
532+
MaybeLocal<Integer> columnOffset = GetColumnOffsetArg(env, options);
533+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, options);
532534
MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, options);
533-
bool produce_cached_data = GetProduceCachedData(env, options);
535+
Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
534536
if (try_catch.HasCaught()) {
535537
try_catch.ReThrow();
536538
return;
537539
}
538540

541+
bool display_errors = maybe_display_errors.ToChecked();
542+
bool produce_cached_data = maybe_produce_cached_data.ToChecked();
543+
539544
ScriptCompiler::CachedData* cached_data = nullptr;
540545
Local<Uint8Array> ui8;
541546
if (cached_data_buf.ToLocal(&ui8)) {
@@ -545,7 +550,8 @@ class ContextifyScript : public BaseObject {
545550
ui8->ByteLength());
546551
}
547552

548-
ScriptOrigin origin(filename, lineOffset, columnOffset);
553+
ScriptOrigin origin(filename.ToLocalChecked(), lineOffset.ToLocalChecked(),
554+
columnOffset.ToLocalChecked());
549555
ScriptCompiler::Source source(code, origin, cached_data);
550556
ScriptCompiler::CompileOptions compile_options =
551557
ScriptCompiler::kNoCompileOptions;
@@ -603,14 +609,18 @@ class ContextifyScript : public BaseObject {
603609

604610
// Assemble arguments
605611
TryCatch try_catch(args.GetIsolate());
606-
uint64_t timeout = GetTimeoutArg(env, args[0]);
607-
bool display_errors = GetDisplayErrorsArg(env, args[0]);
608-
bool break_on_sigint = GetBreakOnSigintArg(env, args[0]);
612+
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[0]);
613+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[0]);
614+
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]);
609615
if (try_catch.HasCaught()) {
610616
try_catch.ReThrow();
611617
return;
612618
}
613619

620+
int64_t timeout = maybe_timeout.ToChecked();
621+
bool display_errors = maybe_display_errors.ToChecked();
622+
bool break_on_sigint = maybe_break_on_sigint.ToChecked();
623+
614624
// Do the eval within this context
615625
EvalMachine(env, timeout, display_errors, break_on_sigint, args,
616626
&try_catch);
@@ -633,13 +643,17 @@ class ContextifyScript : public BaseObject {
633643
Local<Object> sandbox = args[0].As<Object>();
634644
{
635645
TryCatch try_catch(env->isolate());
636-
timeout = GetTimeoutArg(env, args[1]);
637-
display_errors = GetDisplayErrorsArg(env, args[1]);
638-
break_on_sigint = GetBreakOnSigintArg(env, args[1]);
646+
Maybe<int64_t> maybe_timeout = GetTimeoutArg(env, args[1]);
647+
Maybe<bool> maybe_display_errors = GetDisplayErrorsArg(env, args[1]);
648+
Maybe<bool> maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]);
639649
if (try_catch.HasCaught()) {
640650
try_catch.ReThrow();
641651
return;
642652
}
653+
654+
timeout = maybe_timeout.ToChecked();
655+
display_errors = maybe_display_errors.ToChecked();
656+
break_on_sigint = maybe_break_on_sigint.ToChecked();
643657
}
644658

645659
// Get the context from the sandbox
@@ -709,60 +723,82 @@ class ContextifyScript : public BaseObject {
709723
True(env->isolate()));
710724
}
711725

712-
static bool GetBreakOnSigintArg(Environment* env, Local<Value> options) {
726+
static Maybe<bool> GetBreakOnSigintArg(Environment* env,
727+
Local<Value> options) {
713728
if (options->IsUndefined() || options->IsString()) {
714-
return false;
729+
return Just(false);
715730
}
716731
if (!options->IsObject()) {
717732
env->ThrowTypeError("options must be an object");
718-
return false;
733+
return Nothing<bool>();
719734
}
720735

721736
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint");
722-
Local<Value> value = options.As<Object>()->Get(key);
723-
return value->IsTrue();
737+
MaybeLocal<Value> maybe_value =
738+
options.As<Object>()->Get(env->context(), key);
739+
if (maybe_value.IsEmpty())
740+
return Nothing<bool>();
741+
742+
Local<Value> value = maybe_value.ToLocalChecked();
743+
return Just(value->IsTrue());
724744
}
725745

726-
static int64_t GetTimeoutArg(Environment* env, Local<Value> options) {
746+
static Maybe<int64_t> GetTimeoutArg(Environment* env, Local<Value> options) {
727747
if (options->IsUndefined() || options->IsString()) {
728-
return -1;
748+
return Just<int64_t>(-1);
729749
}
730750
if (!options->IsObject()) {
731751
env->ThrowTypeError("options must be an object");
732-
return -1;
752+
return Nothing<int64_t>();
733753
}
734754

735-
Local<Value> value = options.As<Object>()->Get(env->timeout_string());
755+
MaybeLocal<Value> maybe_value =
756+
options.As<Object>()->Get(env->context(), env->timeout_string());
757+
if (maybe_value.IsEmpty())
758+
return Nothing<int64_t>();
759+
760+
Local<Value> value = maybe_value.ToLocalChecked();
736761
if (value->IsUndefined()) {
737-
return -1;
762+
return Just<int64_t>(-1);
738763
}
739-
int64_t timeout = value->IntegerValue();
740764

741-
if (timeout <= 0) {
765+
Maybe<int64_t> timeout = value->IntegerValue(env->context());
766+
767+
if (timeout.IsJust() && timeout.ToChecked() <= 0) {
742768
env->ThrowRangeError("timeout must be a positive number");
743-
return -1;
769+
return Nothing<int64_t>();
744770
}
771+
745772
return timeout;
746773
}
747774

748775

749-
static bool GetDisplayErrorsArg(Environment* env, Local<Value> options) {
776+
static Maybe<bool> GetDisplayErrorsArg(Environment* env,
777+
Local<Value> options) {
750778
if (options->IsUndefined() || options->IsString()) {
751-
return true;
779+
return Just(true);
752780
}
753781
if (!options->IsObject()) {
754782
env->ThrowTypeError("options must be an object");
755-
return false;
783+
return Nothing<bool>();
756784
}
757785

758786
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors");
759-
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 Nothing<bool>();
760791

761-
return value->IsUndefined() ? true : value->BooleanValue();
792+
Local<Value> value = maybe_value.ToLocalChecked();
793+
if (value->IsUndefined())
794+
return Just(true);
795+
796+
return value->BooleanValue(env->context());
762797
}
763798

764799

765-
static Local<String> GetFilenameArg(Environment* env, Local<Value> options) {
800+
static MaybeLocal<String> GetFilenameArg(Environment* env,
801+
Local<Value> options) {
766802
Local<String> defaultFilename =
767803
FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine.<anonymous>");
768804

@@ -778,11 +814,15 @@ class ContextifyScript : public BaseObject {
778814
}
779815

780816
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename");
781-
Local<Value> value = options.As<Object>()->Get(key);
817+
MaybeLocal<Value> maybe_value =
818+
options.As<Object>()->Get(env->context(), key);
819+
if (maybe_value.IsEmpty())
820+
return MaybeLocal<String>();
782821

822+
Local<Value> value = maybe_value.ToLocalChecked();
783823
if (value->IsUndefined())
784824
return defaultFilename;
785-
return value->ToString(env->isolate());
825+
return value->ToString(env->context());
786826
}
787827

788828

@@ -791,7 +831,13 @@ class ContextifyScript : public BaseObject {
791831
if (!options->IsObject()) {
792832
return MaybeLocal<Uint8Array>();
793833
}
794-
Local<Value> value = options.As<Object>()->Get(env->cached_data_string());
834+
835+
MaybeLocal<Value> maybe_value =
836+
options.As<Object>()->Get(env->context(), env->cached_data_string());
837+
if (maybe_value.IsEmpty())
838+
return MaybeLocal<Uint8Array>();
839+
840+
Local<Value> value = maybe_value.ToLocalChecked();
795841
if (value->IsUndefined()) {
796842
return MaybeLocal<Uint8Array>();
797843
}
@@ -805,44 +851,64 @@ class ContextifyScript : public BaseObject {
805851
}
806852

807853

808-
static bool GetProduceCachedData(Environment* env, Local<Value> options) {
854+
static Maybe<bool> GetProduceCachedData(Environment* env,
855+
Local<Value> options) {
809856
if (!options->IsObject()) {
810-
return false;
857+
return Just(false);
811858
}
812-
Local<Value> value =
813-
options.As<Object>()->Get(env->produce_cached_data_string());
814859

815-
return value->IsTrue();
860+
MaybeLocal<Value> maybe_value =
861+
options.As<Object>()->Get(env->context(),
862+
env->produce_cached_data_string());
863+
if (maybe_value.IsEmpty())
864+
return Nothing<bool>();
865+
866+
Local<Value> value = maybe_value.ToLocalChecked();
867+
return Just(value->IsTrue());
816868
}
817869

818870

819-
static Local<Integer> GetLineOffsetArg(Environment* env,
820-
Local<Value> options) {
871+
static MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
872+
Local<Value> options) {
821873
Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);
822874

823875
if (!options->IsObject()) {
824876
return defaultLineOffset;
825877
}
826878

827879
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset");
828-
Local<Value> value = options.As<Object>()->Get(key);
880+
MaybeLocal<Value> maybe_value =
881+
options.As<Object>()->Get(env->context(), key);
882+
if (maybe_value.IsEmpty())
883+
return MaybeLocal<Integer>();
829884

830-
return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
885+
Local<Value> value = maybe_value.ToLocalChecked();
886+
if (value->IsUndefined())
887+
return defaultLineOffset;
888+
889+
return value->ToInteger(env->context());
831890
}
832891

833892

834-
static Local<Integer> GetColumnOffsetArg(Environment* env,
835-
Local<Value> options) {
893+
static MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
894+
Local<Value> options) {
836895
Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);
837896

838897
if (!options->IsObject()) {
839898
return defaultColumnOffset;
840899
}
841900

842901
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset");
843-
Local<Value> value = options.As<Object>()->Get(key);
902+
MaybeLocal<Value> maybe_value =
903+
options.As<Object>()->Get(env->context(), key);
904+
if (maybe_value.IsEmpty())
905+
return MaybeLocal<Integer>();
906+
907+
Local<Value> value = maybe_value.ToLocalChecked();
908+
if (value->IsUndefined())
909+
return defaultColumnOffset;
844910

845-
return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
911+
return value->ToInteger(env->context());
846912
}
847913

848914

‎src/node_os.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ using v8::Function;
3636
using v8::FunctionCallbackInfo;
3737
using v8::Integer;
3838
using v8::Local;
39+
using v8::MaybeLocal;
3940
using v8::Null;
4041
using v8::Number;
4142
using v8::Object;
@@ -318,7 +319,12 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
318319

319320
if (args[0]->IsObject()) {
320321
Local<Object> options = args[0].As<Object>();
321-
Local<Value> encoding_opt = options->Get(env->encoding_string());
322+
MaybeLocal<Value> maybe_encoding = options->Get(env->context(),
323+
env->encoding_string());
324+
if (maybe_encoding.IsEmpty())
325+
return;
326+
327+
Local<Value> encoding_opt = maybe_encoding.ToLocalChecked();
322328
encoding = ParseEncoding(env->isolate(), encoding_opt, UTF8);
323329
} else {
324330
encoding = UTF8;

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

Lines changed: 37 additions & 0 deletions
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)