From 13a6ac5b4ca8fc08b348f64de64a787982f24250 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Fri, 7 Feb 2025 13:22:25 +0100 Subject: [PATCH] lib: crash handlers must be allowed on threads Blocking all signals on non-main threads is not the way to go, at least the handlers for SIGSEGV, SIGBUS, SIGILL, SIGABRT and SIGFPE need to run so we get backtraces. Otherwise the process just exits. Signed-off-by: David Lamparter --- lib/frr_pthread.c | 8 ++++---- lib/frrcu.c | 15 ++++++++++++++- lib/sigevent.h | 27 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index 3a4bc712fc..0b4d7c77ae 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -20,6 +20,7 @@ #include "zlog.h" #include "libfrr.h" #include "libfrr_trace.h" +#include "sigevent.h" DEFINE_MTYPE_STATIC(LIB, FRR_PTHREAD, "FRR POSIX Thread"); DEFINE_MTYPE_STATIC(LIB, PTHREAD_PRIM, "POSIX sync primitives"); @@ -185,10 +186,9 @@ int frr_pthread_run(struct frr_pthread *fpt, const pthread_attr_t *attr) assert(frr_is_after_fork || !"trying to start thread before fork()"); - /* Ensure we never handle signals on a background thread by blocking - * everything here (new thread inherits signal mask) - */ - sigfillset(&blocksigs); + sigemptyset(&blocksigs); + frr_sigset_add_mainonly(&blocksigs); + /* new thread inherits mask */ pthread_sigmask(SIG_BLOCK, &blocksigs, &oldsigs); frrtrace(1, frr_libfrr, frr_pthread_run, fpt->name); diff --git a/lib/frrcu.c b/lib/frrcu.c index b85c525c58..1e7ed99eff 100644 --- a/lib/frrcu.c +++ b/lib/frrcu.c @@ -42,6 +42,7 @@ #include "frrcu.h" #include "seqlock.h" #include "atomlist.h" +#include "sigevent.h" DEFINE_MTYPE_STATIC(LIB, RCU_THREAD, "RCU thread"); DEFINE_MTYPE_STATIC(LIB, RCU_NEXT, "RCU sequence barrier"); @@ -346,7 +347,19 @@ static void rcu_start(void) */ sigset_t oldsigs, blocksigs; - sigfillset(&blocksigs); + /* technically, the RCU thread is very poorly suited to run even just a + * crashlog handler, since zlog_sigsafe() could deadlock on transiently + * invalid (due to RCU) logging data structures + * + * but given that when we try to write a crashlog, we're already in + * b0rked territory anyway - give the crashlog handler a chance. + * + * (also cf. the SIGALRM usage in writing crashlogs to avoid hung + * processes on any kind of deadlock in crash handlers) + */ + sigemptyset(&blocksigs); + frr_sigset_add_mainonly(&blocksigs); + /* new thread inherits mask */ pthread_sigmask(SIG_BLOCK, &blocksigs, &oldsigs); rcu_active = true; diff --git a/lib/sigevent.h b/lib/sigevent.h index 0b07f594c1..2c51ba3767 100644 --- a/lib/sigevent.h +++ b/lib/sigevent.h @@ -45,6 +45,33 @@ bool frr_sigevent_check(sigset_t *setp); /* check whether there are signals to handle, process any found */ extern int frr_sigevent_process(void); +/* Ensure we don't handle "application-type" signals on a secondary thread by + * blocking these signals when creating threads + * + * NB: SIGSEGV, SIGABRT, etc. must be allowed on all threads or we get no + * crashlogs. Since signals vary a little bit between platforms, below is a + * list of known things to go to the main thread. Any unknown signals should + * stay thread-local. + */ +static inline void frr_sigset_add_mainonly(sigset_t *blocksigs) +{ + /* signals we actively handle */ + sigaddset(blocksigs, SIGHUP); + sigaddset(blocksigs, SIGINT); + sigaddset(blocksigs, SIGTERM); + sigaddset(blocksigs, SIGUSR1); + + /* signals we don't actively use but that semantically belong */ + sigaddset(blocksigs, SIGUSR2); + sigaddset(blocksigs, SIGQUIT); + sigaddset(blocksigs, SIGCHLD); + sigaddset(blocksigs, SIGPIPE); + sigaddset(blocksigs, SIGTSTP); + sigaddset(blocksigs, SIGTTIN); + sigaddset(blocksigs, SIGTTOU); + sigaddset(blocksigs, SIGWINCH); +} + #ifdef __cplusplus } #endif -- 2.39.5