From 352b9b10a8175372213fd9279a075110809bd377 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 2 Sep 2026 22:41:39 +0000 Subject: [PATCH 1/2] inspector: fix abort when two Environments own the inspector Two Environments with default flags alive at the same time (for example the embedding.md example run on two threads, or two `CommonEnvironmentSetup`s) aborted the process: `Agent::Start()` bound one file-level static `uv_async_t` to the current Environment's loop for every Environment with `kOwnsInspector`, which `kDefaultFlags` implies, and CHECKed that nobody else had. Environments created one after another did not abort, but each ran `StartDebugSignalHandler()` again, which re-initialized the semaphore the watchdog waits on and spawned another detached watchdog thread, leaking one thread per Environment. Give every Agent that asks for the debug signal handler its own async handle, keep those Agents in a mutex-protected list that the watchdog (or the Windows remote thread) walks, and set the watchdog up once per process while still unblocking SIGUSR1 on each Environment's thread. The handle is heap-allocated, closed by the cleanup hook or `~Agent()`, whichever runs first, and freed by its close callback. A SIGUSR1 now reaches every Environment that asked for the handler, and no longer starts the inspector of one that passed `kNoStartDebugSignalHandler`. Refs: https://github.com/nodejs/node/pull/25777 Signed-off-by: Shelley Vohr --- src/inspector_agent.cc | 127 ++++++++++++++++++-------------- src/inspector_agent.h | 10 ++- test/cctest/test_environment.cc | 3 +- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index fbafc55f1b74..7dcb4070d9b4 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -72,12 +72,12 @@ using v8_inspector::V8InspectorClient; #ifdef __POSIX__ static uv_sem_t start_io_thread_semaphore; #endif // __POSIX__ -static uv_async_t start_io_thread_async; -// This is just an additional check to make sure start_io_thread_async -// is not accidentally re-used or used when uninitialized. -static std::atomic_bool start_io_thread_async_initialized { false }; -// Protects the Agent* stored in start_io_thread_async.data. -static Mutex start_io_thread_async_mutex; +// Agents that asked for the debug signal handler; SIGUSR1 (or the Windows +// remote thread) starts the io thread of each. The mutex also guards the +// once-per-process watchdog setup. +static Mutex start_io_thread_agents_mutex; +static std::vector start_io_thread_agents; +static bool debug_signal_handler_started = false; template void SyncJavaScriptHookState(Environment* env, @@ -125,12 +125,11 @@ void SyncJavaScriptHookState(Environment* env, } } -// Called on the main thread. -void StartIoThreadAsyncCallback(uv_async_t* handle) { - static_cast(handle->data)->StartIoThread(); +static void RequestIoThreadStartOnAgents() { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart(); } - #ifdef __POSIX__ static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) { uv_sem_post(&start_io_thread_semaphore); @@ -140,16 +139,11 @@ inline void* StartIoThreadMain(void* unused) { uv_thread_setname("SignalInspector"); for (;;) { uv_sem_wait(&start_io_thread_semaphore); - Mutex::ScopedLock lock(start_io_thread_async_mutex); - - CHECK(start_io_thread_async_initialized); - Agent* agent = static_cast(start_io_thread_async.data); - if (agent != nullptr) - agent->RequestIoThreadStart(); + RequestIoThreadStartOnAgents(); } } -static int StartDebugSignalHandler() { +static int StartWatchdogThread() { // Start a watchdog thread for calling v8::Debug::DebugBreak() because // it's not safe to call directly from the signal handler, it can // deadlock with the thread it interrupts. @@ -184,14 +178,28 @@ static int StartDebugSignalHandler() { fprintf(stderr, "node[%u]: pthread_create: %s\n", uv_os_getpid(), strerror(err)); fflush(stderr); - // Leave SIGUSR1 blocked. We don't install a signal handler, - // receiving the signal would terminate the process. + uv_sem_destroy(&start_io_thread_semaphore); return -err; } RegisterSignalHandler(SIGUSR1, StartIoThreadWakeup); // Restore original mask CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr)); - // Unblock SIGUSR1. A pending SIGUSR1 signal will now be delivered. + return 0; +} + +static int StartDebugSignalHandler() { + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + if (!debug_signal_handler_started) { + // Leave SIGUSR1 blocked on failure. We don't install a signal handler, + // receiving the signal would terminate the process. + if (int err = StartWatchdogThread()) return err; + debug_signal_handler_started = true; + } + } + // Unblock SIGUSR1 on this thread; PlatformInit() left it blocked. A pending + // SIGUSR1 signal will now be delivered. + sigset_t sigmask; sigemptyset(&sigmask); sigaddset(&sigmask, SIGUSR1); CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &sigmask, nullptr)); @@ -202,11 +210,7 @@ static int StartDebugSignalHandler() { #ifdef _WIN32 DWORD WINAPI StartIoThreadProc(void* arg) { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - CHECK(start_io_thread_async_initialized); - Agent* agent = static_cast(start_io_thread_async.data); - if (agent != nullptr) - agent->RequestIoThreadStart(); + RequestIoThreadStartOnAgents(); return 0; } @@ -216,6 +220,9 @@ static int GetDebugSignalHandlerMappingName(DWORD pid, wchar_t* buf, } static int StartDebugSignalHandler() { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + if (debug_signal_handler_started) return 0; + debug_signal_handler_started = true; wchar_t mapping_name[32]; HANDLE mapping_handle; DWORD pid; @@ -899,7 +906,21 @@ Agent::Agent(Environment* env) debug_options_(env->options()->debug_options()), host_port_(env->inspector_host_port()) {} -Agent::~Agent() = default; +Agent::~Agent() { + StopAcceptingIoThreadStarts(); +} + +void Agent::StopAcceptingIoThreadStarts() { + if (start_io_thread_async_ == nullptr) return; + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + std::erase(start_io_thread_agents, this); + } + parent_env_->RemoveCleanupHook(StopAcceptingIoThreadStartsHook, this); + parent_env_->CloseHandle(start_io_thread_async_, + [](uv_async_t* handle) { delete handle; }); + start_io_thread_async_ = nullptr; +} bool Agent::Start(const std::string& path, const DebugOptions& options, @@ -911,33 +932,25 @@ bool Agent::Start(const std::string& path, host_port_ = host_port; client_ = std::make_shared(parent_env_, is_main); - if (parent_env_->owns_inspector()) { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - CHECK_EQ(start_io_thread_async_initialized.exchange(true), false); - CHECK_EQ(0, uv_async_init(parent_env_->event_loop(), - &start_io_thread_async, - StartIoThreadAsyncCallback)); - uv_unref(reinterpret_cast(&start_io_thread_async)); - start_io_thread_async.data = this; - if (parent_env_->should_start_debug_signal_handler()) { - // Ignore failure, SIGUSR1 won't work, but that should not block node - // start. - StartDebugSignalHandler(); + if (parent_env_->owns_inspector() && + parent_env_->should_start_debug_signal_handler()) { + start_io_thread_async_ = new uv_async_t; + start_io_thread_async_->data = this; + CHECK_EQ(0, + uv_async_init(parent_env_->event_loop(), + start_io_thread_async_, + [](uv_async_t* handle) { + static_cast(handle->data)->StartIoThread(); + })); + uv_unref(reinterpret_cast(start_io_thread_async_)); + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + start_io_thread_agents.push_back(this); } - - parent_env_->AddCleanupHook([](void* data) { - Environment* env = static_cast(data); - - { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - start_io_thread_async.data = nullptr; - } - - // This is global, will never get freed - env->CloseHandle(&start_io_thread_async, [](uv_async_t*) { - CHECK(start_io_thread_async_initialized.exchange(false)); - }); - }, parent_env_); + parent_env_->AddCleanupHook(StopAcceptingIoThreadStartsHook, this); + // Ignore failure, SIGUSR1 won't work, but that should not block node + // start. + StartDebugSignalHandler(); } AtExit(parent_env_, [](void* env) { @@ -1175,6 +1188,10 @@ void Agent::AllAsyncTasksCanceled() { client_->AllAsyncTasksCanceled(); } +void Agent::StopAcceptingIoThreadStartsHook(void* agent) { + static_cast(agent)->StopAcceptingIoThreadStarts(); +} + void Agent::RequestIoThreadStart() { // We need to attempt to interrupt V8 flow (in case Node is running // continuous JS code) and to wake up libuv thread (in case Node is waiting @@ -1182,14 +1199,10 @@ void Agent::RequestIoThreadStart() { if (!options().allow_attaching_debugger) { return; } - CHECK(start_io_thread_async_initialized); - uv_async_send(&start_io_thread_async); parent_env_->RequestInterrupt([this](Environment*) { StartIoThread(); }); - - CHECK(start_io_thread_async_initialized); - uv_async_send(&start_io_thread_async); + uv_async_send(start_io_thread_async_); } void Agent::ContextCreated(Local context, const ContextInfo& info) { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 73835d2a4d1d..6b92ef2a08fe 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -8,6 +8,7 @@ #endif #include "node_options.h" +#include "uv.h" #include "v8.h" #include @@ -123,7 +124,8 @@ class Agent { // Can only be called from the main thread. bool StartIoThread(); - // Calls StartIoThread() from off the main thread. + // Calls StartIoThread() from off the main thread. Only valid while the + // Environment owns the inspector and has not started cleanup. void RequestIoThreadStart(); const DebugOptions& options() { return debug_options_; } @@ -160,6 +162,12 @@ class Agent { // reconciles the two when it is possible and safe to call into JS. JavaScriptHookState async_hook_state_; + // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(), + // whichever runs first, and freed by its close callback. + uv_async_t* start_io_thread_async_ = nullptr; + void StopAcceptingIoThreadStarts(); + static void StopAcceptingIoThreadStartsHook(void* agent); + // Network tracking uses JS hooks. Reconcile the protocol requested and // applied states after leaving a V8 interrupt. JavaScriptHookState network_tracking_state_; diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index fe9915185b71..a1de1b8c0fb9 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -341,9 +341,8 @@ TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) { TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - // Only one of the Environments can have default flags and own the inspector. Env env1 {handle_scope, argv}; - Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags}; + Env env2{handle_scope, argv}; AtExit(*env1, at_exit_callback1, nullptr); AtExit(*env2, at_exit_callback2, nullptr); From b1848f1e6fb0abcc507d41375ad7d7b1a25dcc93 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Thu, 3 Sep 2026 00:03:53 +0000 Subject: [PATCH 2/2] doc: note that default signal handling resets the signal mask `InitializeOncePerProcess()` without `kNoDefaultSignalHandling` calls `pthread_sigmask(SIG_SETMASK, ...)` with a set containing only SIGUSR1, which unblocks every signal the embedder had blocked on the calling thread. Say so in the flag's documentation. Refs: https://github.com/nodejs/node/pull/44121 Signed-off-by: Shelley Vohr --- src/node.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/node.h b/src/node.h index 8e7d1e6a2516..a898f41b33f9 100644 --- a/src/node.h +++ b/src/node.h @@ -210,8 +210,10 @@ enum Flags : uint32_t { kNoICU = 1 << 3, // Do not modify stdio file descriptor or TTY state. kNoStdioInitialization = 1 << 4, - // Do not register Node.js-specific signal handlers - // and reset other signal handlers to default state. + // Do not register Node.js-specific signal handlers, reset other signal + // handlers to default state, or replace the calling thread's signal mask + // (without this flag, POSIX builds with the inspector set it to block + // SIGUSR1 and nothing else). kNoDefaultSignalHandling = 1 << 5, // Do not perform V8 initialization. kNoInitializeV8 = 1 << 6,