Run the deadly signal handlers on the alternative stack

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: error-handling, memory-management, observability | Difficulty: senior
Link: https://github.com/ClickHouse/ClickHouse/pull/113927

The problem

When a thread in ClickHouse overflows its stack, the resulting SIGSEGV is delivered on the exhausted stack. The installed signal handler tried to run on this stack, which often led to the handler itself crashing (faulting again), causing the process to die with no useful diagnostic output—no stack trace, no error log, and no sanitizer report. This made stack overflows silent and hard to debug.

Why it's non-trivial

Simply installing an alternative signal stack (sigaltstack) is not enough; the signal handler must be explicitly registered with the SA_ONSTACK flag so the kernel delivers the signal handler on the alternative stack. Additionally, not all signals should use the alternative stack (e.g., SIGTSTP), and care must be taken to disable and free the alternative stack safely at thread exit, since signals could still arrive in that window.

The approach

1. Add use_alt_stack parameter and set SA_ONSTACK when needed

The handler registration function now takes a use_alt_stack parameter. If true, it sets the SA_ONSTACK flag so the handler will run on the alternative stack:

void HandledSignals::addSignalHandler(
    [[maybe_unused]] const std::vector<int> & signals,
    [[maybe_unused]] signal_function handler,
    [[maybe_unused]] bool register_signal,
-    [[maybe_unused]] const std::vector<int> & additional_masked_signals)
+    [[maybe_unused]] const std::vector<int> & additional_masked_signals,
+    [[maybe_unused]] bool use_alt_stack)
{
#if !defined(OS_HAS_SIGNAL_HANDLERS)
    return;
#endif
    struct sigaction sa{};
    sa.sa_sigaction = handler;
    sa.sa_flags = SA_SIGINFO;

+    if (use_alt_stack)
+        sa.sa_flags |= SA_ONSTACK;

This change makes it possible to selectively run handlers on the alternative stack, which is essential for handling stack overflows reliably.

2. Register deadly signal handlers with SA_ONSTACK, but not SIGTSTP

The deadly signal handlers (SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGSYS, SIGFPE, SIGTRAP) are now registered in a group with use_alt_stack=true. SIGTSTP is registered separately without SA_ONSTACK because it is not related to stack exhaustion and its handler returns:

-    addSignalHandler(
-        {SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGSYS, SIGFPE, SIGTSTP, SIGTRAP}, signalHandler, true, unwind_recovery_signals);
+    const std::vector<int> fault_signals{SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGSYS, SIGFPE, SIGTRAP};
+    /// ...snip...
+    /// Not SIGTSTP: it is never raised by stack exhaustion, and its handler returns.
+    addSignalHandler(fault_signals, signalHandler, true, fault_masked_signals, /*use_alt_stack=*/true);
+    addSignalHandler({SIGTSTP}, signalHandler, true, tstp_masked_signals);

This ensures only the signals that need to run on the alternative stack do so, avoiding unnecessary complexity for others.

3. Properly disable the alternative stack before freeing it at thread exit

When a thread exits, its alternative stack must be disabled before freeing, to prevent signals from being delivered to freed memory:

~ThreadStack()
{
+    /// The deadly signal handlers request SA_ONSTACK, so a signal delivered after this point would
+    /// run on freed memory. The thread keeps executing thread_local destructors after this one.
+    stack_t disable{};
+    disable.ss_flags = SS_DISABLE;
+    /// Darwin rejects a zero size even when disabling.
+    disable.ss_size = getSize();
+    if (sigaltstack(&disable, nullptr) != 0)
+    {
+        const auto saved_errno = errno;
+        /// Leak rather than free a stack signals may still be delivered onto.
+        try
+        {
+            LOG_FATAL(getLogger("ThreadStatus"), "Cannot disable the alternative signal stack, leaking it, {}", errnoToString(saved_errno));
+        }
+        catch (...) // NOLINT(bugprone-empty-catch) Ok: a destructor must not throw, and logging is best-effort
+        {
+        }
+        return;
+    }

This prevents use-after-free bugs if a signal arrives after the stack memory is released.

4. Remove redundant per-thread SIGSEGV patching

Previously, ThreadStatus tried to patch SIGSEGV after installing the alternative stack. This is now unnecessary, since SA_ONSTACK is set process-wide and the kernel chooses the stack at delivery:

-        else
-        {
-            /// Obtain existing sigaction and modify it by adding a flag.
-            struct sigaction action{};
-            if (0 != sigaction(SIGSEGV, nullptr, &action))
-            {
-                LOG_WARNING(log, "Cannot obtain previous signal action to set alternative signal stack for thread, {}", errnoToString());
-            }
-            else if (!(action.sa_flags & SA_ONSTACK))
-            {
-                action.sa_flags |= SA_ONSTACK;
-
-                if (0 != sigaction(SIGSEGV, &action, nullptr))
-                {
-                    LOG_WARNING(log, "Cannot set action with alternative signal stack for thread, {}", errnoToString());
-                }
-            }
-        }

This simplifies the logic and avoids unnecessary signal handler reinstallation.

5. Add a test to verify correct registration and flag use

A new test checks that all deadly signals except SIGTSTP are registered with SA_ONSTACK, and that the handler and mask are consistent:

for (int sig : registered)
{
    const std::string suffix = ", signal " + std::to_string(sig);
    struct sigaction actual{};
    require(sigaction(sig, nullptr, &actual) == 0, "reading the action" + suffix);
    /// SIGTSTP is the one whose handler returns, so it stays off the alternative stack.
    require(bool(actual.sa_flags & SA_ONSTACK) == (sig != SIGTSTP), "SA_ONSTACK" + suffix);
    /// `SA_ONSTACK` must be the only difference the split introduces.
    require(actual.sa_sigaction == reference.sa_sigaction, "same handler" + suffix);
    require((actual.sa_flags & ~SA_ONSTACK) == (reference.sa_flags & ~SA_ONSTACK), "same flags" + suffix);
    // ...mask checks...
}

This test ensures the registration logic is correct and robust to future changes.

From the review thread

groeneai: cc @pamarcos @azat, could you review this? The deadly signal handlers were installed without SA_ONSTACK, so on a stack overflow the handler faulted again while pushing its frame and the process died from the default action with no stack at all; the alternative stack was already installed per thread, just never selected. SIGTSTP is deliberately registered separately and excluded, because its handler returns while that stack is freed at thread exit without SS_DISABLE.

groeneai: Agreed, and fixed in 4d3615b. The finding was correct: the test claimed all eight dispositions share one handler but only compared SIGTSTP against SIGSEGV, so rerouting any of the other six fault signals while keeping SA_ONSTACK passed.

As suggested, handler identity now folds into the loop that already iterated every registered signal. Each of the eight is compared against SIGSEGV as the reference for sa_sigaction, for sa_mask, and for sa_flags minus SA_ONSTACK (the one difference the split is allowed to introduce, which the description also claims). The reference is asserted non-null so a registration collapsing to SIG_DFL cannot satisfy the comparison trivially.

Takeaways

• Always use SA_ONSTACK for signal handlers that must run after stack exhaustion. Installing an alternative stack is not enough; the handler must be registered with this flag.
• Disable and free the alternative stack safely at thread exit. Signals may still arrive after destructors start running, so disable the stack before freeing to avoid use-after-free.
• Test signal registration thoroughly, including handler identity and flags. Automated tests should verify not just the presence of handlers, but also that the correct flags and masks are set for each signal.