From 225fd64259ba09717b95da68954d40776c5baae4 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 1/3] os,contextify: 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 --- src/node_contextify.cc | 160 +++++++++++++++++++++++++++++------------ src/node_os.cc | 8 ++- 2 files changed, 120 insertions(+), 48 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index bc6c468aaf4218..5f3cf78a559d70 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -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; @@ -546,17 +548,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.ToChecked(); + bool produce_cached_data = maybe_produce_cached_data.ToChecked(); + ScriptCompiler::CachedData* cached_data = nullptr; Local ui8; if (cached_data_buf.ToLocal(&ui8)) { @@ -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; @@ -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 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.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); @@ -654,13 +664,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.ToChecked(); + display_errors = maybe_display_errors.ToChecked(); + break_on_sigint = maybe_break_on_sigint.ToChecked(); } // Get the context from the sandbox @@ -730,60 +744,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 maybeValue = + options.As()->Get(env->context(), key); + if (maybeValue.IsEmpty()) + return Nothing(); + + Local value = maybeValue.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 value = options.As()->Get(env->timeout_string()); + MaybeLocal maybeValue = + options.As()->Get(env->context(), env->timeout_string()); + if (maybeValue.IsEmpty()) + return Nothing(); + + Local value = maybeValue.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.ToChecked() <= 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 maybeValue = + options.As()->Get(env->context(), key); + if (maybeValue.IsEmpty()) + return Nothing(); - return value->IsUndefined() ? true : value->BooleanValue(); + Local value = maybeValue.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."); @@ -799,11 +835,15 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename"); - Local value = options.As()->Get(key); + MaybeLocal maybeValue = + options.As()->Get(env->context(), key); + if (maybeValue.IsEmpty()) + return MaybeLocal(); + Local value = maybeValue.ToLocalChecked(); if (value->IsUndefined()) return defaultFilename; - return value->ToString(env->isolate()); + return value->ToString(env->context()); } @@ -812,7 +852,13 @@ class ContextifyScript : public BaseObject { if (!options->IsObject()) { return MaybeLocal(); } - Local value = options.As()->Get(env->cached_data_string()); + + MaybeLocal maybeValue = + options.As()->Get(env->context(), env->cached_data_string()); + if (maybeValue.IsEmpty()) + return MaybeLocal(); + + Local value = maybeValue.ToLocalChecked(); if (value->IsUndefined()) { return MaybeLocal(); } @@ -826,19 +872,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 maybeValue = + options.As()->Get(env->context(), + env->produce_cached_data_string()); + if (maybeValue.IsEmpty()) + return Nothing(); + + Local value = maybeValue.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()) { @@ -846,14 +898,21 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset"); - Local value = options.As()->Get(key); + MaybeLocal maybeValue = + options.As()->Get(env->context(), key); + if (maybeValue.IsEmpty()) + return MaybeLocal(); - return value->IsUndefined() ? defaultLineOffset : value->ToInteger(); + Local value = maybeValue.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()) { @@ -861,9 +920,16 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset"); - Local value = options.As()->Get(key); + MaybeLocal maybeValue = + options.As()->Get(env->context(), key); + if (maybeValue.IsEmpty()) + return MaybeLocal(); + + Local value = maybeValue.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 d7be9095b170c9..fad19469532ee0 100644 --- a/src/node_os.cc +++ b/src/node_os.cc @@ -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; @@ -339,7 +340,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; From 6379d867603c89d4f1f36dd469efa0af188c41ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 12 Apr 2017 22:08:09 +0200 Subject: [PATCH 2/3] Add regression test --- test/parallel/test-regress-GH-12371.js | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 test/parallel/test-regress-GH-12371.js 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'); + })); +}); From bb5ed7b26db530c3bae7d8f17f3533c7c4dda74d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 12 Apr 2017 23:38:04 +0200 Subject: [PATCH 3/3] style: maybeValue -> maybe_value --- src/node_contextify.cc | 48 +++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 5f3cf78a559d70..a112d1ba583bee 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -755,12 +755,12 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "breakOnSigint"); - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), key); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return Nothing(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); return Just(value->IsTrue()); } @@ -773,12 +773,12 @@ class ContextifyScript : public BaseObject { return Nothing(); } - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), env->timeout_string()); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return Nothing(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) { return Just(-1); } @@ -805,12 +805,12 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "displayErrors"); - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), key); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return Nothing(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) return Just(true); @@ -835,12 +835,12 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "filename"); - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), key); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return MaybeLocal(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) return defaultFilename; return value->ToString(env->context()); @@ -853,12 +853,12 @@ class ContextifyScript : public BaseObject { return MaybeLocal(); } - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), env->cached_data_string()); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return MaybeLocal(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) { return MaybeLocal(); } @@ -878,13 +878,13 @@ class ContextifyScript : public BaseObject { return Just(false); } - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), env->produce_cached_data_string()); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return Nothing(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); return Just(value->IsTrue()); } @@ -898,12 +898,12 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "lineOffset"); - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), key); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return MaybeLocal(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) return defaultLineOffset; @@ -920,12 +920,12 @@ class ContextifyScript : public BaseObject { } Local key = FIXED_ONE_BYTE_STRING(env->isolate(), "columnOffset"); - MaybeLocal maybeValue = + MaybeLocal maybe_value = options.As()->Get(env->context(), key); - if (maybeValue.IsEmpty()) + if (maybe_value.IsEmpty()) return MaybeLocal(); - Local value = maybeValue.ToLocalChecked(); + Local value = maybe_value.ToLocalChecked(); if (value->IsUndefined()) return defaultColumnOffset;