From 14e542eaf4ba30eef5c67f3ba9170849cbaed1af Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 22 Jun 2016 23:32:24 +0200
Subject: [PATCH] vm: test for abort condition of current invocation

When a vm script aborted after a timeout/signal interruption, test
whether the local timeout/signal watchdog was responsible for
terminating the execution.

Without this, when a shorter timer from an outer `vm.run*` invocation
fires before an inner timeout, the inner timeout would throw an error
instead of the outer one, but because it did not witness the timeout
itself, it would assume the termination was the result of a signal
interruption.

PR-URL: https://github.com/nodejs/node/pull/7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/node_contextify.cc           | 21 ++++++++++++++++++---
 src/node_watchdog.cc             |  3 ++-
 src/node_watchdog.h              |  2 ++
 test/parallel/test-vm-timeout.js | 23 +++++++++++++++++++++++
 4 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index a4a769359412ef..e5cbfd5512cc01 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -836,14 +836,17 @@ class ContextifyScript : public BaseObject {
 
     Local<Value> result;
     bool timed_out = false;
+    bool received_signal = false;
     if (break_on_sigint && timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       SigintWatchdog swd(env->isolate());
       result = script->Run();
       timed_out = wd.HasTimedOut();
+      received_signal = swd.HasReceivedSignal();
     } else if (break_on_sigint) {
       SigintWatchdog swd(env->isolate());
       result = script->Run();
+      received_signal = swd.HasReceivedSignal();
     } else if (timeout != -1) {
       Watchdog wd(env->isolate(), timeout);
       result = script->Run();
@@ -852,14 +855,26 @@ class ContextifyScript : public BaseObject {
       result = script->Run();
     }
 
-    if (try_catch.HasCaught() && try_catch.HasTerminated()) {
-      env->isolate()->CancelTerminateExecution();
+    if (try_catch.HasCaught()) {
+      if (try_catch.HasTerminated())
+        env->isolate()->CancelTerminateExecution();
+
+      // It is possible that execution was terminated by another timeout in
+      // which this timeout is nested, so check whether one of the watchdogs
+      // from this invocation is responsible for termination.
       if (timed_out) {
         env->ThrowError("Script execution timed out.");
-      } else {
+      } else if (received_signal) {
         env->ThrowError("Script execution interrupted.");
       }
+
+      // If there was an exception thrown during script execution, re-throw it.
+      // If one of the above checks threw, re-throw the exception instead of
+      // letting try_catch catch it.
+      // If execution has been terminated, but not by one of the watchdogs from
+      // this invocation, this will re-throw a `null` value.
       try_catch.ReThrow();
+
       return false;
     }
 
diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc
index 8e94bc8d9d22ac..8a067c27f3b6c2 100644
--- a/src/node_watchdog.cc
+++ b/src/node_watchdog.cc
@@ -99,7 +99,7 @@ void SigintWatchdog::Dispose() {
 
 
 SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
-    : isolate_(isolate), destroyed_(false) {
+    : isolate_(isolate), received_signal_(false), destroyed_(false) {
   // Register this watchdog with the global SIGINT/Ctrl+C listener.
   SigintWatchdogHelper::GetInstance()->Register(this);
   // Start the helper thread, if that has not already happened.
@@ -120,6 +120,7 @@ void SigintWatchdog::Destroy() {
 
 
 void SigintWatchdog::HandleSigint() {
+  received_signal_ = true;
   isolate_->TerminateExecution();
 }
 
diff --git a/src/node_watchdog.h b/src/node_watchdog.h
index 65815e2bcddc30..77c2d535340b6f 100644
--- a/src/node_watchdog.h
+++ b/src/node_watchdog.h
@@ -46,11 +46,13 @@ class SigintWatchdog {
     void Dispose();
 
     v8::Isolate* isolate() { return isolate_; }
+    bool HasReceivedSignal() { return received_signal_; }
     void HandleSigint();
   private:
     void Destroy();
 
     v8::Isolate* isolate_;
+    bool received_signal_;
     bool destroyed_;
 };
 
diff --git a/test/parallel/test-vm-timeout.js b/test/parallel/test-vm-timeout.js
index d595bac4c32324..0536ae37a1b489 100644
--- a/test/parallel/test-vm-timeout.js
+++ b/test/parallel/test-vm-timeout.js
@@ -32,3 +32,26 @@ assert.throws(function() {
   vm.runInNewContext('runInVM(10)', context, { timeout: 10000 });
   throw new Error('Test 5 failed');
 }, /Script execution timed out./);
+
+// Test 6: Nested vm timeouts, outer timeout is shorter and fires first.
+assert.throws(function() {
+  const context = {
+    runInVM: function(timeout) {
+      vm.runInNewContext('while(true) {}', context, { timeout: timeout });
+    }
+  };
+  vm.runInNewContext('runInVM(10000)', context, { timeout: 100 });
+  throw new Error('Test 6 failed');
+}, /Script execution timed out./);
+
+// Test 7: Nested vm timeouts, inner script throws an error.
+assert.throws(function() {
+  const context = {
+    runInVM: function(timeout) {
+      vm.runInNewContext('throw new Error(\'foobar\')', context, {
+        timeout: timeout
+      });
+    }
+  };
+  vm.runInNewContext('runInVM(10000)', context, { timeout: 100000 });
+}, /foobar/);