From c29063d75bf815e2ef3a5c27139c667e2aff9051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 12 Apr 2017 20:57:28 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/12369 Fixes: https://github.com/nodejs/node/issues/12370 Backport-PR-URL: https://github.com/nodejs/node/pull/13871 PR-URL: https://github.com/nodejs/node/pull/12371 Reviewed-By: Anna Henningsen --- src/node_contextify.cc | 161 +++++++++++++++++-------- src/node_os.cc | 8 +- test/parallel/test-regress-GH-12371.js | 37 ++++++ 3 files changed, 157 insertions(+), 49 deletions(-) create mode 100644 test/parallel/test-regress-GH-12371.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 741f568259a1a6..1ea8fa0b1cb6bf 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -23,11 +23,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; @@ -495,17 +497,20 @@ class ContextifyScript : public BaseObject { Local code = args[0]->ToString(env->isolate()); Local options = args[1]; - Local filename = GetFilenameArg(env, options); - Local lineOffset = GetLineOffsetArg(env, options); - Local columnOffset = GetColumnOffsetArg(env, options); - bool display_errors = GetDisplayErrorsArg(env, options); + MaybeLocal filename = GetFilenameArg(env, options); + MaybeLocal lineOffset = GetLineOffsetArg(env, options); + MaybeLocal columnOffset = GetColumnOffsetArg(env, options); + Maybe maybe_display_errors = GetDisplayErrorsArg(env, options); MaybeLocal cached_data_buf = GetCachedData(env, options); - bool produce_cached_data = GetProduceCachedData(env, options); + Maybe maybe_produce_cached_data = GetProduceCachedData(env, options); if (try_catch.HasCaught()) { try_catch.ReThrow(); return; } + bool display_errors = maybe_display_errors.FromJust(); + bool produce_cached_data = maybe_produce_cached_data.FromJust(); + ScriptCompiler::CachedData* cached_data = nullptr; if (!cached_data_buf.IsEmpty()) { Local ui8 = cached_data_buf.ToLocalChecked(); @@ -515,7 +520,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; @@ -573,14 +579,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 maybe_timeout = GetTimeoutArg(env, args[0]); + Maybe maybe_display_errors = GetDisplayErrorsArg(env, args[0]); + Maybe maybe_break_on_sigint = GetBreakOnSigintArg(env, args[0]); if (try_catch.HasCaught()) { try_catch.ReThrow(); return; } + int64_t timeout = maybe_timeout.FromJust(); + bool display_errors = maybe_display_errors.FromJust(); + bool break_on_sigint = maybe_break_on_sigint.FromJust(); + // Do the eval within this context EvalMachine(env, timeout, display_errors, break_on_sigint, args, &try_catch); @@ -603,13 +613,17 @@ class ContextifyScript : public BaseObject { Local sandbox = args[0].As(); { TryCatch try_catch(env->isolate()); - timeout = GetTimeoutArg(env, args[1]); - display_errors = GetDisplayErrorsArg(env, args[1]); - break_on_sigint = GetBreakOnSigintArg(env, args[1]); + Maybe maybe_timeout = GetTimeoutArg(env, args[1]); + Maybe maybe_display_errors = GetDisplayErrorsArg(env, args[1]); + Maybe maybe_break_on_sigint = GetBreakOnSigintArg(env, args[1]); if (try_catch.HasCaught()) { try_catch.ReThrow(); return; } + + timeout = maybe_timeout.FromJust(); + display_errors = maybe_display_errors.FromJust(); + break_on_sigint = maybe_break_on_sigint.FromJust(); } // Get the context from the sandbox @@ -679,61 +693,82 @@ class ContextifyScript : public BaseObject { True(env->isolate())); } - static bool GetBreakOnSigintArg(Environment* env, Local options) { + static Maybe GetBreakOnSigintArg(Environment* env, + Local 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(); } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint"); - Local value = options.As()->Get(key); - return value->IsTrue(); + MaybeLocal maybe_value = + options.As()->Get(env->context(), key); + if (maybe_value.IsEmpty()) + return Nothing(); + + Local value = maybe_value.ToLocalChecked(); + return Just(value->IsTrue()); } - static int64_t GetTimeoutArg(Environment* env, Local options) { + static Maybe GetTimeoutArg(Environment* env, Local options) { if (options->IsUndefined() || options->IsString()) { - return -1; + return Just(-1); } if (!options->IsObject()) { env->ThrowTypeError("options must be an object"); - return -1; + return Nothing(); } - Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "timeout"); - Local value = options.As()->Get(key); + MaybeLocal maybe_value = + options.As()->Get(env->context(), env->timeout_string()); + if (maybe_value.IsEmpty()) + return Nothing(); + + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) { - return -1; + return Just(-1); } - int64_t timeout = value->IntegerValue(); - if (timeout <= 0) { + Maybe timeout = value->IntegerValue(env->context()); + + if (timeout.IsJust() && timeout.FromJust() <= 0) { env->ThrowRangeError("timeout must be a positive number"); - return -1; + return Nothing(); } + return timeout; } - static bool GetDisplayErrorsArg(Environment* env, Local options) { + static Maybe GetDisplayErrorsArg(Environment* env, + Local 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(); } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors"); - Local value = options.As()->Get(key); + MaybeLocal maybe_value = + options.As()->Get(env->context(), key); + if (maybe_value.IsEmpty()) + return Nothing(); - return value->IsUndefined() ? true : value->BooleanValue(); + Local value = maybe_value.ToLocalChecked(); + if (value->IsUndefined()) + return Just(true); + + return value->BooleanValue(env->context()); } - static Local GetFilenameArg(Environment* env, Local options) { + static MaybeLocal GetFilenameArg(Environment* env, + Local options) { Local defaultFilename = FIXED_ONE_BYTE_STRING(env->isolate(), "evalmachine."); @@ -749,11 +784,15 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename"); - Local value = options.As()->Get(key); + MaybeLocal maybe_value = + options.As()->Get(env->context(), key); + if (maybe_value.IsEmpty()) + return MaybeLocal(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) return defaultFilename; - return value->ToString(env->isolate()); + return value->ToString(env->context()); } @@ -762,7 +801,13 @@ class ContextifyScript : public BaseObject { if (!options->IsObject()) { return MaybeLocal(); } - Local value = options.As()->Get(env->cached_data_string()); + + MaybeLocal maybe_value = + options.As()->Get(env->context(), env->cached_data_string()); + if (maybe_value.IsEmpty()) + return MaybeLocal(); + + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) { return MaybeLocal(); } @@ -776,19 +821,25 @@ class ContextifyScript : public BaseObject { } - static bool GetProduceCachedData(Environment* env, Local options) { + static Maybe GetProduceCachedData(Environment* env, + Local options) { if (!options->IsObject()) { - return false; + return Just(false); } - Local value = - options.As()->Get(env->produce_cached_data_string()); - return value->IsTrue(); + MaybeLocal maybe_value = + options.As()->Get(env->context(), + env->produce_cached_data_string()); + if (maybe_value.IsEmpty()) + return Nothing(); + + Local value = maybe_value.ToLocalChecked(); + return Just(value->IsTrue()); } - static Local GetLineOffsetArg(Environment* env, - Local options) { + static MaybeLocal GetLineOffsetArg(Environment* env, + Local options) { Local defaultLineOffset = Integer::New(env->isolate(), 0); if (!options->IsObject()) { @@ -796,14 +847,21 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset"); - Local value = options.As()->Get(key); + MaybeLocal maybe_value = + options.As()->Get(env->context(), key); + if (maybe_value.IsEmpty()) + return MaybeLocal(); - return value->IsUndefined() ? defaultLineOffset : value->ToInteger(); + Local value = maybe_value.ToLocalChecked(); + if (value->IsUndefined()) + return defaultLineOffset; + + return value->ToInteger(env->context()); } - static Local GetColumnOffsetArg(Environment* env, - Local options) { + static MaybeLocal GetColumnOffsetArg(Environment* env, + Local options) { Local defaultColumnOffset = Integer::New(env->isolate(), 0); if (!options->IsObject()) { @@ -811,9 +869,16 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset"); - Local value = options.As()->Get(key); + MaybeLocal maybe_value = + options.As()->Get(env->context(), key); + if (maybe_value.IsEmpty()) + return MaybeLocal(); + + Local value = maybe_value.ToLocalChecked(); + if (value->IsUndefined()) + return defaultColumnOffset; - return value->IsUndefined() ? defaultColumnOffset : value->ToInteger(); + return value->ToInteger(env->context()); } diff --git a/src/node_os.cc b/src/node_os.cc index 211ac3d01dd8b2..19eaaeeb301d88 100644 --- a/src/node_os.cc +++ b/src/node_os.cc @@ -35,6 +35,7 @@ using v8::Float64Array; using v8::FunctionCallbackInfo; using v8::Integer; using v8::Local; +using v8::MaybeLocal; using v8::Null; using v8::Number; using v8::Object; @@ -306,7 +307,12 @@ static void GetUserInfo(const FunctionCallbackInfo& args) { if (args[0]->IsObject()) { Local options = args[0].As(); - Local encoding_opt = options->Get(env->encoding_string()); + MaybeLocal maybe_encoding = options->Get(env->context(), + env->encoding_string()); + if (maybe_encoding.IsEmpty()) + return; + + Local encoding_opt = maybe_encoding.ToLocalChecked(); encoding = ParseEncoding(env->isolate(), encoding_opt, UTF8); } else { encoding = UTF8; diff --git a/test/parallel/test-regress-GH-12371.js b/test/parallel/test-regress-GH-12371.js new file mode 100644 index 00000000000000..6ab65a8e348e1e --- /dev/null +++ b/test/parallel/test-regress-GH-12371.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const execFile = require('child_process').execFile; + +const scripts = [ + `os.userInfo({ + get encoding() { + throw new Error('xyz'); + } + })` +]; + +['filename', 'cachedData', 'produceCachedData', 'lineOffset', 'columnOffset'] + .forEach((prop) => { + scripts.push(`vm.createScript('', { + get ${prop} () { + throw new Error('xyz'); + } + })`); + }); + +['breakOnSigint', 'timeout', 'displayErrors'] + .forEach((prop) => { + scripts.push(`vm.createScript('').runInThisContext({ + get ${prop} () { + throw new Error('xyz'); + } + })`); + }); + +scripts.forEach((script) => { + const node = process.execPath; + execFile(node, [ '-e', script ], common.mustCall((err, stdout, stderr) => { + assert(stderr.includes('Error: xyz'), 'createScript crashes'); + })); +});