From e2767114ffeccbcd7ff08ccf13aa86c435079c1c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 29 Nov 2017 20:55:43 +0100 Subject: [PATCH] vm: never abort on caught syntax error Keep track of C++ `TryCatch` state to avoid aborting when an exception is thrown inside one, and re-throw in JS to make sure the exception is being picked up a second time by a second uncaught exception handler, if necessary. Add a bit of a hack to `AppendExceptionLine` to avoid overriding the line responsible for re-throwing the exception. PR-URL: https://github.com/nodejs/node/pull/17394 Fixes: https://github.com/nodejs/node/issues/13258 Reviewed-By: James M Snell --- lib/vm.js | 15 ++++++++++++- src/env-inl.h | 21 +++++++++++++++++++ src/env.h | 14 +++++++++++++ src/node.cc | 5 ++++- src/node_contextify.cc | 3 +++ test/abort/test-abort-uncaught-exception.js | 15 +++++++++---- test/message/eval_messages.out | 7 ++++--- test/message/stdin_messages.out | 7 ++++--- .../undefined_reference_in_new_context.out | 4 ++-- test/message/vm_display_runtime_error.out | 4 ++-- test/message/vm_display_syntax_error.out | 4 ++-- .../message/vm_dont_display_runtime_error.out | 2 +- test/message/vm_dont_display_syntax_error.out | 2 +- ...st-vm-parse-abort-on-uncaught-exception.js | 14 +++++++++++++ 14 files changed, 97 insertions(+), 20 deletions(-) create mode 100644 test/parallel/test-vm-parse-abort-on-uncaught-exception.js diff --git a/lib/vm.js b/lib/vm.js index 023dd387cb3acb..669fa23396735f 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -22,7 +22,7 @@ 'use strict'; const { - ContextifyScript: Script, + ContextifyScript, kParsingContext, makeContext, @@ -40,6 +40,19 @@ const { // - isContext(sandbox) // From this we build the entire documented API. +class Script extends ContextifyScript { + constructor(code, options) { + // Calling `ReThrow()` on a native TryCatch does not generate a new + // abort-on-uncaught-exception check. A dummy try/catch in JS land + // protects against that. + try { + super(code, options); + } catch (e) { + throw e; /* node-do-not-add-exception-line */ + } + } +} + const realRunInThisContext = Script.prototype.runInThisContext; const realRunInContext = Script.prototype.runInContext; diff --git a/src/env-inl.h b/src/env-inl.h index 2be6580ca88124..d55530ab241e68 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -408,6 +408,27 @@ Environment::should_abort_on_uncaught_toggle() { return should_abort_on_uncaught_toggle_; } +Environment::ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope( + Environment* env) + : env_(env) { + env_->should_not_abort_scope_counter_++; +} + +Environment::ShouldNotAbortOnUncaughtScope::~ShouldNotAbortOnUncaughtScope() { + Close(); +} + +void Environment::ShouldNotAbortOnUncaughtScope::Close() { + if (env_ != nullptr) { + env_->should_not_abort_scope_counter_--; + env_ = nullptr; + } +} + +bool Environment::inside_should_not_abort_on_uncaught_scope() const { + return should_not_abort_scope_counter_ > 0; +} + inline std::vector* Environment::destroy_async_id_list() { return &destroy_async_id_list_; } diff --git a/src/env.h b/src/env.h index e6c50d66c28d3c..ee2784df28c204 100644 --- a/src/env.h +++ b/src/env.h @@ -681,6 +681,18 @@ class Environment { // This needs to be available for the JS-land setImmediate(). void ActivateImmediateCheck(); + class ShouldNotAbortOnUncaughtScope { + public: + explicit inline ShouldNotAbortOnUncaughtScope(Environment* env); + inline void Close(); + inline ~ShouldNotAbortOnUncaughtScope(); + + private: + Environment* env_; + }; + + inline bool inside_should_not_abort_on_uncaught_scope() const; + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -706,6 +718,8 @@ class Environment { AliasedBuffer scheduled_immediate_count_; AliasedBuffer should_abort_on_uncaught_toggle_; + int should_not_abort_scope_counter_ = 0; + performance::performance_state* performance_state_ = nullptr; std::map performance_marks_; diff --git a/src/node.cc b/src/node.cc index f8c445a678c42a..5f65939160f7ca 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1110,7 +1110,8 @@ namespace { bool ShouldAbortOnUncaughtException(Isolate* isolate) { HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); - return env->should_abort_on_uncaught_toggle()[0]; + return env->should_abort_on_uncaught_toggle()[0] && + !env->inside_should_not_abort_on_uncaught_scope(); } @@ -1639,6 +1640,8 @@ void AppendExceptionLine(Environment* env, // Print line of source code. node::Utf8Value sourceline(env->isolate(), message->GetSourceLine()); const char* sourceline_string = *sourceline; + if (strstr(sourceline_string, "node-do-not-add-exception-line") != nullptr) + return; // Because of how node modules work, all scripts are wrapped with a // "function (module, exports, __filename, ...) {" diff --git a/src/node_contextify.cc b/src/node_contextify.cc index f7c5102f669798..4d3ea122c8bf49 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -654,6 +654,7 @@ class ContextifyScript : public BaseObject { new ContextifyScript(env, args.This()); TryCatch try_catch(env->isolate()); + Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env); Local code = args[0]->ToString(env->context()).FromMaybe(Local()); @@ -666,6 +667,7 @@ class ContextifyScript : public BaseObject { Maybe maybe_produce_cached_data = GetProduceCachedData(env, options); MaybeLocal maybe_context = GetContext(env, options); if (try_catch.HasCaught()) { + no_abort_scope.Close(); try_catch.ReThrow(); return; } @@ -702,6 +704,7 @@ class ContextifyScript : public BaseObject { if (v8_script.IsEmpty()) { DecorateErrorStack(env, try_catch); + no_abort_scope.Close(); try_catch.ReThrow(); return; } diff --git a/test/abort/test-abort-uncaught-exception.js b/test/abort/test-abort-uncaught-exception.js index 4acddf349c227a..718f456c9733c6 100644 --- a/test/abort/test-abort-uncaught-exception.js +++ b/test/abort/test-abort-uncaught-exception.js @@ -3,17 +3,24 @@ const common = require('../common'); const assert = require('assert'); const spawn = require('child_process').spawn; +const vm = require('vm'); const node = process.execPath; if (process.argv[2] === 'child') { throw new Error('child error'); +} else if (process.argv[2] === 'vm') { + // Refs: https://github.com/nodejs/node/issues/13258 + // This *should* still crash. + new vm.Script('[', {}); } else { - run('', null); - run('--abort-on-uncaught-exception', ['SIGABRT', 'SIGTRAP', 'SIGILL']); + run('', 'child', null); + run('--abort-on-uncaught-exception', 'child', + ['SIGABRT', 'SIGTRAP', 'SIGILL']); + run('--abort-on-uncaught-exception', 'vm', ['SIGABRT', 'SIGTRAP', 'SIGILL']); } -function run(flags, signals) { - const args = [__filename, 'child']; +function run(flags, argv2, signals) { + const args = [__filename, argv2]; if (flags) args.unshift(flags); diff --git a/test/message/eval_messages.out b/test/message/eval_messages.out index aa7e51dc5d1584..6c725cb1cc8a14 100644 --- a/test/message/eval_messages.out +++ b/test/message/eval_messages.out @@ -3,6 +3,7 @@ with(this){__filename} ^^^^ SyntaxError: Strict mode code may not include a with statement + at new Script (vm.js:*:*) at createScript (vm.js:*:*) at Object.runInThisContext (vm.js:*:*) at Object. ([eval]-wrapper:*:*) @@ -18,7 +19,7 @@ throw new Error("hello") Error: hello at [eval]:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*:*) + at Script.runInThisContext (vm.js:*:*) at Object.runInThisContext (vm.js:*:*) at Object. ([eval]-wrapper:*:*) at Module._compile (module.js:*:*) @@ -32,7 +33,7 @@ throw new Error("hello") Error: hello at [eval]:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*:*) + at Script.runInThisContext (vm.js:*:*) at Object.runInThisContext (vm.js:*:*) at Object. ([eval]-wrapper:*:*) at Module._compile (module.js:*:*) @@ -46,7 +47,7 @@ var x = 100; y = x; ReferenceError: y is not defined at [eval]:1:16 - at ContextifyScript.Script.runInThisContext (vm.js:*:*) + at Script.runInThisContext (vm.js:*:*) at Object.runInThisContext (vm.js:*:*) at Object. ([eval]-wrapper:*:*) at Module._compile (module.js:*:*) diff --git a/test/message/stdin_messages.out b/test/message/stdin_messages.out index d934523a726772..d2192050dd4196 100644 --- a/test/message/stdin_messages.out +++ b/test/message/stdin_messages.out @@ -3,6 +3,7 @@ with(this){__filename} ^^^^ SyntaxError: Strict mode code may not include a with statement + at new Script (vm.js:*) at createScript (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. ([stdin]-wrapper:*:*) @@ -20,7 +21,7 @@ throw new Error("hello") Error: hello at [stdin]:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. ([stdin]-wrapper:*:*) at Module._compile (module.js:*:*) @@ -35,7 +36,7 @@ throw new Error("hello") Error: hello at [stdin]:1:* - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. ([stdin]-wrapper:*:*) at Module._compile (module.js:*:*) @@ -51,7 +52,7 @@ var x = 100; y = x; ReferenceError: y is not defined at [stdin]:1:16 - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. ([stdin]-wrapper:*:*) at Module._compile (module.js:*:*) diff --git a/test/message/undefined_reference_in_new_context.out b/test/message/undefined_reference_in_new_context.out index 93bc1dcc99c3e1..ff517cc981a255 100644 --- a/test/message/undefined_reference_in_new_context.out +++ b/test/message/undefined_reference_in_new_context.out @@ -5,8 +5,8 @@ foo.bar = 5; ReferenceError: foo is not defined at evalmachine.:1:1 - at ContextifyScript.Script.runInContext (vm.js:*) - at ContextifyScript.Script.runInNewContext (vm.js:*) + at Script.runInContext (vm.js:*) + at Script.runInNewContext (vm.js:*) at Object.runInNewContext (vm.js:*) at Object. (*test*message*undefined_reference_in_new_context.js:*) at Module._compile (module.js:*) diff --git a/test/message/vm_display_runtime_error.out b/test/message/vm_display_runtime_error.out index 7e116ee8964525..056ea79f8d39b4 100644 --- a/test/message/vm_display_runtime_error.out +++ b/test/message/vm_display_runtime_error.out @@ -5,7 +5,7 @@ throw new Error("boo!") Error: boo! at test.vm:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_display_runtime_error.js:*) at Module._compile (module.js:*) @@ -20,7 +20,7 @@ throw new Error("spooky!") Error: spooky! at test.vm:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_display_runtime_error.js:*) at Module._compile (module.js:*) diff --git a/test/message/vm_display_syntax_error.out b/test/message/vm_display_syntax_error.out index c64f508c74db1b..f3b9953307db19 100644 --- a/test/message/vm_display_syntax_error.out +++ b/test/message/vm_display_syntax_error.out @@ -3,6 +3,7 @@ foo.vm:1 var 4; ^ SyntaxError: Unexpected number + at new Script (vm.js:*) at createScript (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_display_syntax_error.js:*) @@ -12,11 +13,11 @@ SyntaxError: Unexpected number at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) - at startup (bootstrap_node.js:*:*) test.vm:1 var 5; ^ SyntaxError: Unexpected number + at new Script (vm.js:*) at createScript (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_display_syntax_error.js:*) @@ -26,4 +27,3 @@ SyntaxError: Unexpected number at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) - at startup (bootstrap_node.js:*:*) diff --git a/test/message/vm_dont_display_runtime_error.out b/test/message/vm_dont_display_runtime_error.out index af634c6dcc4ab1..a7e06d49f85a7c 100644 --- a/test/message/vm_dont_display_runtime_error.out +++ b/test/message/vm_dont_display_runtime_error.out @@ -6,7 +6,7 @@ throw new Error("boo!") Error: boo! at test.vm:1:7 - at ContextifyScript.Script.runInThisContext (vm.js:*) + at Script.runInThisContext (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_dont_display_runtime_error.js:*) at Module._compile (module.js:*) diff --git a/test/message/vm_dont_display_syntax_error.out b/test/message/vm_dont_display_syntax_error.out index eced3a3f626804..5c74c25dd8aa3b 100644 --- a/test/message/vm_dont_display_syntax_error.out +++ b/test/message/vm_dont_display_syntax_error.out @@ -5,6 +5,7 @@ var 5; ^ SyntaxError: Unexpected number + at new Script (vm.js:*) at createScript (vm.js:*) at Object.runInThisContext (vm.js:*) at Object. (*test*message*vm_dont_display_syntax_error.js:*) @@ -14,4 +15,3 @@ SyntaxError: Unexpected number at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) - at startup (bootstrap_node.js:*:*) diff --git a/test/parallel/test-vm-parse-abort-on-uncaught-exception.js b/test/parallel/test-vm-parse-abort-on-uncaught-exception.js new file mode 100644 index 00000000000000..36f73ea676e5a5 --- /dev/null +++ b/test/parallel/test-vm-parse-abort-on-uncaught-exception.js @@ -0,0 +1,14 @@ +// Flags: --abort-on-uncaught-exception +'use strict'; +require('../common'); +const vm = require('vm'); + +// Regression test for https://github.com/nodejs/node/issues/13258 + +try { + new vm.Script({ toString() { throw new Error('foo'); } }, {}); +} catch (err) {} + +try { + new vm.Script('[', {}); +} catch (err) {}