From 53e64bb29e3b70cf23b345d02fac081de716ef3f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 26 Oct 2015 14:11:03 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/3528 Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- src/atomic-polyfill.h | 18 ++++++++++++++ src/node.cc | 55 +++++++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 13 deletions(-) create mode 100644 src/atomic-polyfill.h diff --git a/src/atomic-polyfill.h b/src/atomic-polyfill.h new file mode 100644 index 00000000000000..1c5f414fa13a81 --- /dev/null +++ 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 +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 index 4de2f97491cf46..d6ba87d72fd23e 100644 --- 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 using atomic = nonstd::atomic; } +#else +#include +namespace node { template using atomic = std::atomic; } +#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 node_isolate; static v8::Platform* default_platform; @@ -3410,28 +3418,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(&node_isolate)); - uv_async_send(&dispatch_debug_messages_async); + TryStartDebugger(); } @@ -3485,8 +3511,7 @@ static int RegisterDebugSignalHandler() { #ifdef _WIN32 DWORD WINAPI EnableDebugThreadProc(void* arg) { - v8::Debug::DebugBreak(*static_cast(&node_isolate)); - uv_async_send(&dispatch_debug_messages_async); + TryStartDebugger(); return 0; } @@ -4006,7 +4031,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); @@ -4016,7 +4042,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 @@ -4070,12 +4096,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) {