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.

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

3 files changed, 30 insertions, 73 deletions

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index e1f00ad07f..9d774d2c37 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -906,27 +906,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.
@@ -934,7 +927,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);
       }
diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc
index 5d95c4132f..049cd177ef 100644
--- a/src/node_watchdog.cc
+++ b/src/node_watchdog.cc
@@ -6,9 +6,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_);
@@ -33,20 +33,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_);
 
@@ -59,8 +45,6 @@ void Watchdog::Destroy() {
   CHECK_EQ(0, rc);
   delete loop_;
   loop_ = nullptr;
-
-  destroyed_ = true;
 }
 
 
@@ -72,7 +56,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);
 }
 
@@ -85,24 +69,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.
@@ -110,20 +85,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();
 }
 
diff --git a/src/node_watchdog.h b/src/node_watchdog.h
index 2d55d782d0..ec6285af5d 100644
--- a/src/node_watchdog.h
+++ b/src/node_watchdog.h
@@ -16,16 +16,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);
@@ -35,27 +32,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 {