vm: fix race condition in watchdog cleanup

apaprocki · bnoordhuis · commit 49e3fcd05852 · 2013-05-30T15:57:25.000+02:00
Previous code was calling uv_loop_delete() directly on a running loop,
which led to race condition aborts/segfaults within libuv.  This change
changes the watchdog thread to call uv_run() with UV_RUN_ONCE so that
the call exits after either the timer times out or uv_async_send() is
called from the main thread in Watchdog::Destroy().  The timer/async
handles are then closed and uv_run() with UV_RUN_DEFAULT is called so
that libuv has a chance to cleanup before the thread exits.  The main
thread meanwhile calls uv_thread_join() and then uv_loop_delete() to
complete the cleanup.
diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc
@@ -20,31 +20,31 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 #include "node_watchdog.h"
+#include <assert.h>
 
 namespace node {
 
 using v8::V8;
 
 
 Watchdog::Watchdog(uint64_t ms)
-  : timer_started_(false)
-  , thread_created_(false)
+  : thread_created_(false)
   , destroyed_(false) {
 
   loop_ = uv_loop_new();
   if (!loop_)
     return;
 
-  int rc = uv_timer_init(loop_, &timer_);
-  if (rc) {
-    return;
-  }
+  int rc = uv_async_init(loop_, &async_, &Watchdog::Async);
+  assert(rc == 0);
+
+  rc = uv_timer_init(loop_, &timer_);
+  assert(rc == 0);
 
   rc = uv_timer_start(&timer_, &Watchdog::Timer, ms, 0);
   if (rc) {
     return;
   }
-  timer_started_ = true;
 
   rc = uv_thread_create(&thread_, &Watchdog::Run, this);
   if (rc) {
@@ -69,28 +69,38 @@ void Watchdog::Destroy() {
     return;
   }
 
-  if (timer_started_) {
-    uv_timer_stop(&timer_);
+  if (thread_created_) {
+    uv_async_send(&async_);
+    uv_thread_join(&thread_);
   }
 
   if (loop_) {
     uv_loop_delete(loop_);
   }
 
-  if (thread_created_) {
-    uv_thread_join(&thread_);
-  }
-
   destroyed_ = true;
 }
 
 
 void Watchdog::Run(void* arg) {
   Watchdog* wd = static_cast<Watchdog*>(arg);
+
+  // UV_RUN_ONCE so async_ or timer_ wakeup exits uv_run() call.
+  uv_run(wd->loop_, UV_RUN_ONCE);
+
+  // Loop ref count reaches zero when both handles are closed.
+  uv_close(reinterpret_cast<uv_handle_t*>(&wd->async_), NULL);
+  uv_close(reinterpret_cast<uv_handle_t*>(&wd->timer_), NULL);
+
+  // UV_RUN_DEFAULT so that libuv has a chance to clean up.
   uv_run(wd->loop_, UV_RUN_DEFAULT);
 }
 
 
+void Watchdog::Async(uv_async_t* async, int status) {
+}
+
+
 void Watchdog::Timer(uv_timer_t* timer, int status) {
   V8::TerminateExecution();
 }
diff --git a/src/node_watchdog.h b/src/node_watchdog.h
@@ -38,12 +38,13 @@ class Watchdog {
   void Destroy();
 
   static void Run(void* arg);
+  static void Async(uv_async_t* async, int status);
   static void Timer(uv_timer_t* timer, int status);
 
   uv_thread_t thread_;
   uv_loop_t* loop_;
+  uv_async_t async_;
   uv_timer_t timer_;
-  bool timer_started_;
   bool thread_created_;
   bool destroyed_;
 };
diff --git a/test/simple/test-vm-run-timeout.js b/test/simple/test-vm-run-timeout.js
@@ -23,15 +23,34 @@ var common = require('../common');
 var assert = require('assert');
 var vm = require('vm');
 
+// Test 1: Timeout of 100ms executing endless loop
 assert.throws(function() {
   vm.runInThisContext('while(true) {}', '', 100);
 });
 
+// Test 2: Timeout must be >= 0ms
 assert.throws(function() {
   vm.runInThisContext('', '', -1);
 });
 
-assert.doesNotThrow(function() {
-  vm.runInThisContext('', '', 0);
-  vm.runInThisContext('', '', 100);
-});
+// Test 3: Timeout of 0ms
+vm.runInThisContext('', '', 0);
+
+// Test 4: Timeout of 1000ms, script finishes first
+vm.runInThisContext('', '', 1000);
+
+// Test 5: Nested vm timeouts, inner timeout propagates out
+try {
+  var context = {
+    log: console.log,
+    runInVM: function(timeout) {
+      vm.runInNewContext('while(true) {}', context, '', timeout);
+    }
+  };
+  vm.runInNewContext('runInVM(10)', context, '', 100);
+  throw new Error('Test 5 failed');
+} catch (e) {
+  if (-1 === e.message.search(/Script execution timed out./)) {
+    throw e;
+  }
+}
