src: fix race condition in debug signal on exit

Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: #3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Oct 29, 2015 · 3bafe1a · 3bafe1a
1 parent 8a2c4ae
commit 3bafe1a
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 13 deletions.
diff --git a/src/atomic-polyfill.h b/src/atomic-polyfill.h
@@ -0,0 +1,18 @@
+#ifndef SRC_ATOMIC_POLYFILL_H_
+#define SRC_ATOMIC_POLYFILL_H_
+
+#include "util.h"
+
+namespace nonstd {
+
+template <typename T>
+struct atomic {
+  atomic() = default;
+  T exchange(T value) { return __sync_lock_test_and_set(&value_, value); }
+  T value_ = T();
+  DISALLOW_COPY_AND_ASSIGN(atomic);
+};
+
+}  // namespace nonstd
+
+#endif  // SRC_ATOMIC_POLYFILL_H_
diff --git a/src/node.cc b/src/node.cc
@@ -86,6 +86,14 @@ typedef int mode_t;
 extern char **environ;
 #endif
 
+#ifdef __APPLE__
+#include "atomic-polyfill.h"  // NOLINT(build/include_order)
+namespace node { template <typename T> using atomic = nonstd::atomic<T>; }
+#else
+#include <atomic>
+namespace node { template <typename T> using atomic = std::atomic<T>; }
+#endif
+
 namespace node {
 
 using v8::Array;
@@ -153,7 +161,7 @@ static double prog_start_time;
 static bool debugger_running;
 static uv_async_t dispatch_debug_messages_async;
 
-static Isolate* node_isolate = nullptr;
+static node::atomic<Isolate*> node_isolate;
 static v8::Platform* default_platform;
 
 
@@ -3389,28 +3397,46 @@ static void EnableDebug(Environment* env) {
 }
 
 
+// Called from an arbitrary thread.
+static void TryStartDebugger() {
+  // Call only async signal-safe functions here!  Don't retry the exchange,
+  // it will deadlock when the thread is interrupted inside a critical section.
+  if (auto isolate = node_isolate.exchange(nullptr)) {
+    v8::Debug::DebugBreak(isolate);
+    uv_async_send(&dispatch_debug_messages_async);
+    CHECK_EQ(nullptr, node_isolate.exchange(isolate));
+  }
+}
+
+
 // Called from the main thread.
 static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
+  // Synchronize with signal handler, see TryStartDebugger.
+  Isolate* isolate;
+  do {
+    isolate = node_isolate.exchange(nullptr);
+  } while (isolate == nullptr);
+
   if (debugger_running == false) {
     fprintf(stderr, "Starting debugger agent.\n");
 
-    HandleScope scope(node_isolate);
-    Environment* env = Environment::GetCurrent(node_isolate);
+    HandleScope scope(isolate);
+    Environment* env = Environment::GetCurrent(isolate);
     Context::Scope context_scope(env->context());
 
     StartDebug(env, false);
     EnableDebug(env);
   }
-  Isolate::Scope isolate_scope(node_isolate);
+
+  Isolate::Scope isolate_scope(isolate);
   v8::Debug::ProcessDebugMessages();
+  CHECK_EQ(nullptr, node_isolate.exchange(isolate));
 }
 
 
 #ifdef __POSIX__
 static void EnableDebugSignalHandler(int signo) {
-  // Call only async signal-safe functions here!
-  v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
-  uv_async_send(&dispatch_debug_messages_async);
+  TryStartDebugger();
 }
 
 
@@ -3464,8 +3490,7 @@ static int RegisterDebugSignalHandler() {
 
 #ifdef _WIN32
 DWORD WINAPI EnableDebugThreadProc(void* arg) {
-  v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
-  uv_async_send(&dispatch_debug_messages_async);
+  TryStartDebugger();
   return 0;
 }
 
@@ -3986,7 +4011,8 @@ static void StartNodeInstance(void* arg) {
   // Fetch a reference to the main isolate, so we have a reference to it
   // even when we need it to access it from another (debugger) thread.
   if (instance_data->is_main())
-    node_isolate = isolate;
+    CHECK_EQ(nullptr, node_isolate.exchange(isolate));
+
   {
     Locker locker(isolate);
     Isolate::Scope isolate_scope(isolate);
@@ -3996,7 +4022,7 @@ static void StartNodeInstance(void* arg) {
     array_buffer_allocator->set_env(env);
     Context::Scope context_scope(context);
 
-    node_isolate->SetAbortOnUncaughtExceptionCallback(
+    isolate->SetAbortOnUncaughtExceptionCallback(
         ShouldAbortOnUncaughtException);
 
     // Start debug agent when argv has --debug
@@ -4047,12 +4073,15 @@ static void StartNodeInstance(void* arg) {
     env = nullptr;
   }
 
+  if (instance_data->is_main()) {
+    // Synchronize with signal handler, see TryStartDebugger.
+    while (isolate != node_isolate.exchange(nullptr));  // NOLINT
+  }
+
   CHECK_NE(isolate, nullptr);
   isolate->Dispose();
   isolate = nullptr;
   delete array_buffer_allocator;
-  if (instance_data->is_main())
-    node_isolate = nullptr;
 }
 
 int Start(int argc, char** argv) {