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 <jasnell@gmail.com>

14 files changed, 97 insertions, 20 deletions

diff --git a/lib/vm.js b/lib/vm.js
index b34f10dbee..2f110b2db2 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -22,7 +22,7 @@
 'use strict';
 
 const {
-  ContextifyScript: Script,
+  ContextifyScript,
   kParsingContext,
 
   makeContext,
@@ -39,6 +39,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 98702ddc4a..f9923936fe 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<double>* Environment::destroy_async_id_list() {
   return &destroy_async_id_list_;
 }
diff --git a/src/env.h b/src/env.h
index a853726ef0..47723baa78 100644
--- a/src/env.h
+++ b/src/env.h
@@ -699,6 +699,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<v8::Value> (*fun)(v8::Local<v8::String>),
                          const char* errmsg);
@@ -724,6 +736,8 @@ class Environment {
   AliasedBuffer<uint32_t, v8::Uint32Array> scheduled_immediate_count_;
   AliasedBuffer<uint32_t, v8::Uint32Array> should_abort_on_uncaught_toggle_;
 
+  int should_not_abort_scope_counter_ = 0;
+
   performance::performance_state* performance_state_ = nullptr;
   std::map<std::string, uint64_t> performance_marks_;
 
diff --git a/src/node.cc b/src/node.cc
index 03220b664f..d515e6712b 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -782,7 +782,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();
 }
 
 
@@ -1311,6 +1312,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 5b389f2ff8..09cdfc5cf5 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -621,6 +621,7 @@ class ContextifyScript : public BaseObject {
         new ContextifyScript(env, args.This());
 
     TryCatch try_catch(env->isolate());
+    Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);
     Local<String> code =
         args[0]->ToString(env->context()).FromMaybe(Local<String>());
 
@@ -633,6 +634,7 @@ class ContextifyScript : public BaseObject {
     Maybe<bool> maybe_produce_cached_data = GetProduceCachedData(env, options);
     MaybeLocal<Context> maybe_context = GetContext(env, options);
     if (try_catch.HasCaught()) {
+      no_abort_scope.Close();
       try_catch.ReThrow();
       return;
     }
@@ -669,6 +671,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 4acddf349c..718f456c97 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 aa7e51dc5d..6c725cb1cc 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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([eval]-wrapper:*:*)
     at Module._compile (module.js:*:*)
diff --git a/test/message/stdin_messages.out b/test/message/stdin_messages.out
index d934523a72..d2192050dd 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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([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 93bc1dcc99..ff517cc981 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.<anonymous>: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.<anonymous> (*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 7e116ee896..056ea79f8d 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.<anonymous> (*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.<anonymous> (*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 c64f508c74..f3b9953307 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.<anonymous> (*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.<anonymous> (*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 af634c6dcc..a7e06d49f8 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.<anonymous> (*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 eced3a3f62..5c74c25dd8 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.<anonymous> (*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 0000000000..36f73ea676
--- /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) {}