vm: fix race condition with timeout param

This fixes a race condition in the watchdog timer used for vm timeouts.
The condition would terminate the main stack's execution instead of the
code running under the sandbox.

PR-URL: https://github.com/nodejs/node/pull/13074
Reviewed-By: Anna Henningsen <anna@addaleax.net>

src/node_contextify.cc

@@ -958,27 +958,20 @@ class ContextifyScript : public BaseObject {
     bool timed_out = false;
     bool received_signal = false;
     if (break_on_sigint && timeout != -1) {
-      Watchdog wd(env->isolate(), timeout);
-      SigintWatchdog swd(env->isolate());
+      Watchdog wd(env->isolate(), timeout, &timed_out);
+      SigintWatchdog swd(env->isolate(), &received_signal);
       result = script->Run();
-      timed_out = wd.HasTimedOut();
-      received_signal = swd.HasReceivedSignal();
     } else if (break_on_sigint) {
-      SigintWatchdog swd(env->isolate());
+      SigintWatchdog swd(env->isolate(), &received_signal);
       result = script->Run();
-      received_signal = swd.HasReceivedSignal();
     } else if (timeout != -1) {
-      Watchdog wd(env->isolate(), timeout);
+      Watchdog wd(env->isolate(), timeout, &timed_out);
       result = script->Run();
-      timed_out = wd.HasTimedOut();
     } else {
       result = script->Run();
     }
 
-    if (try_catch->HasCaught()) {
-      if (try_catch->HasTerminated())
-        env->isolate()->CancelTerminateExecution();
-
+    if (timed_out || received_signal) {
       // 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.
@@ -986,7 +979,12 @@ class ContextifyScript : public BaseObject {
         env->ThrowError("Script execution timed out.");
       } else if (received_signal) {
         env->ThrowError("Script execution interrupted.");
-      } else if (display_errors) {
+      }
+      env->isolate()->CancelTerminateExecution();
+    }
+
+    if (try_catch->HasCaught()) {
+      if (!timed_out && !received_signal && display_errors) {
         // We should decorate non-termination exceptions
         DecorateErrorStack(env, *try_catch);
       }

src/node_watchdog.cc

@@ -27,9 +27,9 @@
 
 namespace node {
 
-Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
-                                                        timed_out_(false),
-                                                        destroyed_(false) {
+Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out)
+    : isolate_(isolate), timed_out_(timed_out) {
+
   int rc;
   loop_ = new uv_loop_t;
   CHECK(loop_);
@@ -54,20 +54,6 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
 
 
 Watchdog::~Watchdog() {
-  Destroy();
-}
-
-
-void Watchdog::Dispose() {
-  Destroy();
-}
-
-
-void Watchdog::Destroy() {
-  if (destroyed_) {
-    return;
-  }
-
   uv_async_send(&async_);
   uv_thread_join(&thread_);
 
@@ -80,8 +66,6 @@ void Watchdog::Destroy() {
   CHECK_EQ(0, rc);
   delete loop_;
   loop_ = nullptr;
-
-  destroyed_ = true;
 }
 
 
@@ -93,7 +77,7 @@ void Watchdog::Run(void* arg) {
   uv_run(wd->loop_, UV_RUN_DEFAULT);
 
   // Loop ref count reaches zero when both handles are closed.
-  // Close the timer handle on this side and let Destroy() close async_
+  // Close the timer handle on this side and let ~Watchdog() close async_
   uv_close(reinterpret_cast<uv_handle_t*>(&wd->timer_), nullptr);
 }
 
@@ -106,24 +90,15 @@ void Watchdog::Async(uv_async_t* async) {
 
 void Watchdog::Timer(uv_timer_t* timer) {
   Watchdog* w = ContainerOf(&Watchdog::timer_, timer);
-  w->timed_out_ = true;
-  uv_stop(w->loop_);
+  *w->timed_out_ = true;
   w->isolate()->TerminateExecution();
+  uv_stop(w->loop_);
 }
 
 
-SigintWatchdog::~SigintWatchdog() {
-  Destroy();
-}
-
-
-void SigintWatchdog::Dispose() {
-  Destroy();
-}
-
-
-SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
-    : isolate_(isolate), received_signal_(false), destroyed_(false) {
+SigintWatchdog::SigintWatchdog(
+  v8::Isolate* isolate, bool* received_signal)
+    : isolate_(isolate), received_signal_(received_signal) {
   // Register this watchdog with the global SIGINT/Ctrl+C listener.
   SigintWatchdogHelper::GetInstance()->Register(this);
   // Start the helper thread, if that has not already happened.
@@ -131,20 +106,14 @@ SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
 }
 
 
-void SigintWatchdog::Destroy() {
-  if (destroyed_) {
-    return;
-  }
-
-  destroyed_ = true;
-
+SigintWatchdog::~SigintWatchdog() {
   SigintWatchdogHelper::GetInstance()->Unregister(this);
   SigintWatchdogHelper::GetInstance()->Stop();
 }
 
 
 void SigintWatchdog::HandleSigint() {
-  received_signal_ = true;
+  *received_signal_ = true;
   isolate_->TerminateExecution();
 }
 

src/node_watchdog.h

@@ -37,16 +37,13 @@ namespace node {
 
 class Watchdog {
  public:
-  explicit Watchdog(v8::Isolate* isolate, uint64_t ms);
+  explicit Watchdog(v8::Isolate* isolate,
+                    uint64_t ms,
+                    bool* timed_out = nullptr);
   ~Watchdog();
-
-  void Dispose();
-
   v8::Isolate* isolate() { return isolate_; }
-  bool HasTimedOut() { return timed_out_; }
- private:
-  void Destroy();
 
+ private:
   static void Run(void* arg);
   static void Async(uv_async_t* async);
   static void Timer(uv_timer_t* timer);
@@ -56,27 +53,20 @@ class Watchdog {
   uv_loop_t* loop_;
   uv_async_t async_;
   uv_timer_t timer_;
-  bool timed_out_;
-  bool destroyed_;
+  bool* timed_out_;
 };
 
 class SigintWatchdog {
  public:
-  explicit SigintWatchdog(v8::Isolate* isolate);
+  explicit SigintWatchdog(v8::Isolate* isolate,
+                          bool* received_signal = nullptr);
   ~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_;
+  bool* received_signal_;
 };
 
 class SigintWatchdogHelper {

test/pummel/test-vm-race.js

@@ -0,0 +1,33 @@
+'use strict';
+require('../common');
+const vm = require('vm');
+
+// We're testing a race condition so we just have to spin this in a loop
+// for a little while and see if it breaks. The condition being tested
+// is an `isolate->TerminateExecution()` reaching the main JS stack from
+// the timeout watchdog.
+const sandbox = { timeout: 5 };
+const context = vm.createContext(sandbox);
+const script = new vm.Script(
+  'var d = Date.now() + timeout;while (d > Date.now());'
+);
+const immediate = setImmediate(function() {
+  throw new Error('Detected vm race condition!');
+});
+
+// When this condition was first discovered this test would fail in 50ms
+// or so. A better, but still incorrect implementation would fail after
+// 100 seconds or so. If you're messing with vm timeouts you might
+// consider increasing this timeout to hammer out races.
+const giveUp = Date.now() + 5000;
+do {
+  // The loop adjusts the timeout up or down trying to hit the race
+  try {
+    script.runInContext(context, { timeout: 5 });
+    ++sandbox.timeout;
+  } catch (err) {
+    --sandbox.timeout;
+  }
+} while (Date.now() < giveUp);
+
+clearImmediate(immediate);