From bcd615f3c539c4c55669728244300b59c824a2dc Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Wed, 9 Apr 2025 18:29:08 +0200 Subject: [PATCH] Make Ctrl-C handling thread-safe (#7603) The Ctrl-C handling is not thread safe, there's a global variable g_obj that is being accessed without any locking. The signal handlers are per-process, not per-thread, so that different threads step over each other's handlers. It is unpredictable in which thread the signal handler runs, so the handler may race with the scoped_ctrl_c destructor. Fix this by introducing the functions signal_lock and signal_unlock. signal_lock blocks the SIGINT signal and then takes a mutex (so that the signal handler can't be called while the mutex is held). signal_unlock drops the mutex and restores the signal mask. We protect all the global variables with signal_lock and signal_unlock. Note that on Windows, the SIGINT handler is being run in a separate thread (and there is no way how to block it), so we can use a simple mutex to synchronize the signal handler with the other threads. In class cancel_eh, the operator () may be called concurrently by the timer code and the Ctrl-C code, but the operator () accesses class' members without any locking. Fix this race condition by using the functions signal_lock() and signal_unlock(). There is this possible call trace: SIGINT signal on_sigint a->m_cancel_eh() cancel_eh::operator() m_obj.inc_cancel reslimit::inc_cancel lock_guard lock(*g_rlimit_mux); Here we take a mutex from a signal - this is subject to deadlock (if the signal interrupted another piece of code where the mutex is already held). To fix this race, we remove g_rlimit_mux and replace it with signal_lock() and signal_unlock(). signal_lock and signal_unlock block the signal before grabbing the mutex, so the signal can't interrupt a piece of code where the mutex is held and the deadlock won't happen. Signed-off-by: Mikulas Patocka --- src/util/cancel_eh.h | 22 ++++++- src/util/rlimit.cpp | 34 +++++----- src/util/scoped_ctrl_c.cpp | 129 +++++++++++++++++++++++++++++++------ src/util/scoped_ctrl_c.h | 5 +- 4 files changed, 149 insertions(+), 41 deletions(-) diff --git a/src/util/cancel_eh.h b/src/util/cancel_eh.h index d609b957a..792eeb010 100644 --- a/src/util/cancel_eh.h +++ b/src/util/cancel_eh.h @@ -18,28 +18,44 @@ Revision History: --*/ #pragma once +#include #include "util/event_handler.h" +#include "util/scoped_ctrl_c.h" /** \brief Generic event handler for invoking cancel method. */ template class cancel_eh : public event_handler { - bool m_canceled = false; + std::atomic m_canceled = false; bool m_auto_cancel = false; T & m_obj; public: cancel_eh(T & o): m_obj(o) {} ~cancel_eh() override { if (m_canceled) m_obj.dec_cancel(); if (m_auto_cancel) m_obj.auto_cancel(); } void operator()(event_handler_caller_t caller_id) override { + signal_lock(); if (!m_canceled) { m_caller_id = caller_id; m_canceled = true; m_obj.inc_cancel(); } + signal_unlock(); + } + bool canceled() { + bool ret; + if (!m_canceled) + return false; + signal_lock(); + ret = m_canceled; + signal_unlock(); + return ret; + } + void reset() { + signal_lock(); + m_canceled = false; + signal_unlock(); } - bool canceled() const { return m_canceled; } - void reset() { m_canceled = false; } T& t() { return m_obj; } void set_auto_cancel() { m_auto_cancel = true; } }; diff --git a/src/util/rlimit.cpp b/src/util/rlimit.cpp index 28656bc63..00036ca6b 100644 --- a/src/util/rlimit.cpp +++ b/src/util/rlimit.cpp @@ -19,16 +19,12 @@ Revision History: #include "util/rlimit.h" #include "util/common_msgs.h" #include "util/mutex.h" - - -static DECLARE_MUTEX(g_rlimit_mux); +#include "util/scoped_ctrl_c.h" void initialize_rlimit() { - ALLOC_MUTEX(g_rlimit_mux); } void finalize_rlimit() { - DEALLOC_MUTEX(g_rlimit_mux); } reslimit::reslimit() { @@ -77,50 +73,57 @@ char const* reslimit::get_cancel_msg() const { } void reslimit::push_child(reslimit* r) { - lock_guard lock(*g_rlimit_mux); + signal_lock(); m_children.push_back(r); + signal_unlock(); } void reslimit::pop_child() { - lock_guard lock(*g_rlimit_mux); + signal_lock(); m_count += m_children.back()->m_count; m_children.back()->m_count = 0; m_children.pop_back(); + signal_unlock(); } void reslimit::pop_child(reslimit* r) { - lock_guard lock(*g_rlimit_mux); + signal_lock(); for (unsigned i = 0; i < m_children.size(); ++i) { if (m_children[i] == r) { m_count += r->m_count; r->m_count = 0; m_children.erase(m_children.begin() + i); - return; + break; } } + signal_unlock(); } void reslimit::cancel() { - lock_guard lock(*g_rlimit_mux); + signal_lock(); set_cancel(m_cancel+1); + signal_unlock(); } void reslimit::reset_cancel() { - lock_guard lock(*g_rlimit_mux); + signal_lock(); set_cancel(0); + signal_unlock(); } void reslimit::inc_cancel() { - lock_guard lock(*g_rlimit_mux); + signal_lock(); set_cancel(m_cancel + 1); + signal_unlock(); } void reslimit::dec_cancel() { - lock_guard lock(*g_rlimit_mux); + signal_lock(); if (m_cancel > 0) { set_cancel(m_cancel-1); } + signal_unlock(); } void reslimit::set_cancel(unsigned f) { @@ -151,8 +154,9 @@ void reslimit::push_timeout(unsigned ms) { } void reslimit::inc_cancel(unsigned k) { - lock_guard lock(*g_rlimit_mux); + signal_lock(); set_cancel(m_cancel + k); + signal_unlock(); } void reslimit::auto_cancel() { @@ -164,4 +168,4 @@ void reslimit::auto_cancel() { void reslimit::auto_cancel() { UNREACHABLE(); } -#endif \ No newline at end of file +#endif diff --git a/src/util/scoped_ctrl_c.cpp b/src/util/scoped_ctrl_c.cpp index a3f5ee772..e5502dc33 100644 --- a/src/util/scoped_ctrl_c.cpp +++ b/src/util/scoped_ctrl_c.cpp @@ -12,46 +12,133 @@ Abstract: Author: Leonardo de Moura (leonardo) 2011-04-27. + Mikulas Patocka 2025-04-05. (rewritten to be thread safe) Revision History: --*/ -#include +#include +#include +#include #include "util/scoped_ctrl_c.h" -static scoped_ctrl_c * g_obj = nullptr; +#ifdef _WINDOWS +#define USE_SIGNAL +#endif -static void on_ctrl_c(int) { - if (g_obj->m_first) { - g_obj->m_cancel_eh(CTRL_C_EH_CALLER); - if (g_obj->m_once) { - g_obj->m_first = false; - signal(SIGINT, on_ctrl_c); // re-install the handler - } +static std::recursive_mutex context_lock; +static std::vector active_contexts; +#ifdef USE_SIGNAL +static void (*old_handler)(int); +#else +static sigset_t context_old_set; +static struct sigaction old_sigaction; +static unsigned signal_lock_depth = 0; +#endif +static bool signal_handled = false; + +void signal_lock(void) { +#ifdef USE_SIGNAL + context_lock.lock(); +#else + sigset_t set, old_set; + sigemptyset(&set); + sigaddset(&set, SIGINT); + if (sigprocmask(SIG_BLOCK, &set, &old_set)) + abort(); + context_lock.lock(); + signal_lock_depth++; + if (signal_lock_depth == 1) + context_old_set = old_set; +#endif +} + +void signal_unlock(void) { +#ifdef USE_SIGNAL + context_lock.unlock(); +#else + bool restore; + sigset_t old_set = context_old_set; + signal_lock_depth--; + restore = !signal_lock_depth; + context_lock.unlock(); + if (restore) { + if (sigprocmask(SIG_SETMASK, &old_set, NULL)) + abort(); } - else { - signal(SIGINT, g_obj->m_old_handler); - raise(SIGINT); +#endif +} + +static void test_and_unhandle(void) { + if (!signal_handled) + return; + for (auto a : active_contexts) { + if (a->m_first) + return; } +#ifdef USE_SIGNAL + signal(SIGINT, old_handler); +#else + if (sigaction(SIGINT, &old_sigaction, NULL)) + abort(); +#endif + signal_handled = false; +} + +static void on_sigint(int) { + signal_lock(); +#ifdef USE_SIGNAL + if (signal_handled) + signal(SIGINT, on_sigint); +#endif + for (auto a : active_contexts) { + if (a->m_first) + a->m_cancel_eh(CTRL_C_EH_CALLER); + if (a->m_once) + a->m_first = false; + } + test_and_unhandle(); + signal_unlock(); } scoped_ctrl_c::scoped_ctrl_c(event_handler & eh, bool once, bool enabled): - m_cancel_eh(eh), + m_cancel_eh(eh), m_first(true), m_once(once), - m_enabled(enabled), - m_old_scoped_ctrl_c(g_obj) { + m_enabled(enabled) { if (m_enabled) { - g_obj = this; - m_old_handler = signal(SIGINT, on_ctrl_c); + signal_lock(); + active_contexts.push_back(this); + if (!signal_handled) { +#ifdef USE_SIGNAL + old_handler = signal(SIGINT, on_sigint); +#else + struct sigaction sa; + memset(&sa, 0, sizeof(struct sigaction)); + sa.sa_handler = on_sigint; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESTART; + if (sigaction(SIGINT, &sa, &old_sigaction)) + abort(); +#endif + signal_handled = true; + } + signal_unlock(); } } -scoped_ctrl_c::~scoped_ctrl_c() { +scoped_ctrl_c::~scoped_ctrl_c() { if (m_enabled) { - g_obj = m_old_scoped_ctrl_c; - if (m_old_handler != SIG_ERR) { - signal(SIGINT, m_old_handler); + signal_lock(); + for (auto it = active_contexts.begin(); it != active_contexts.end(); it++) { + if (*it == this) { + active_contexts.erase(it); + goto found; + } } + abort(); +found: + test_and_unhandle(); + signal_unlock(); } } diff --git a/src/util/scoped_ctrl_c.h b/src/util/scoped_ctrl_c.h index 09bc55c05..f686962f4 100644 --- a/src/util/scoped_ctrl_c.h +++ b/src/util/scoped_ctrl_c.h @@ -21,13 +21,14 @@ Revision History: #include "util/event_handler.h" #include "util/util.h" +void signal_lock(void); +void signal_unlock(void); + struct scoped_ctrl_c { event_handler & m_cancel_eh; bool m_first; bool m_once; bool m_enabled; - void (STD_CALL *m_old_handler)(int); - scoped_ctrl_c * m_old_scoped_ctrl_c; public: // If once == true, then the cancel_eh is invoked only at the first Ctrl-C. // The next time, the old signal handler will take over.