From 1bdf325a9a872c401cf967aa9d2e7da53aea347d Mon Sep 17 00:00:00 2001 From: Maurizio Abba Date: Tue, 16 Jan 2018 18:12:28 +0000 Subject: [PATCH] signal: use centralized pthread_sigmask for signals according to its man page, sigprocmask has undefined behavior in multithreaded environments. Instead of explictly blocking the handling of SIGUSR2 in every thread, direct block handling SIGUSR2 before creating the threads and enable again the handling of this signal afterwards. In this way, only the main thread will be able to manage this signal properly. --- src/counters.c | 8 -------- src/detect-engine-loader.c | 4 ---- src/flow-manager.c | 8 -------- src/suricata.c | 17 +++++++++++++++-- src/tm-threads.c | 12 ------------ src/util-signal.c | 21 +++++++++++++++++++-- src/util-signal.h | 1 + 7 files changed, 35 insertions(+), 36 deletions(-) diff --git a/src/counters.c b/src/counters.c index bdced6243a..007b093d99 100644 --- a/src/counters.c +++ b/src/counters.c @@ -323,10 +323,6 @@ static void StatsReleaseCtx(void) */ static void *StatsMgmtThread(void *arg) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif ThreadVars *tv_local = (ThreadVars *)arg; uint8_t run = 1; struct timespec cond_time; @@ -411,10 +407,6 @@ static void *StatsMgmtThread(void *arg) */ static void *StatsWakeupThread(void *arg) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif ThreadVars *tv_local = (ThreadVars *)arg; uint8_t run = 1; ThreadVars *tv = NULL; diff --git a/src/detect-engine-loader.c b/src/detect-engine-loader.c index 233bbce3e4..cb07dc6167 100644 --- a/src/detect-engine-loader.c +++ b/src/detect-engine-loader.c @@ -564,10 +564,6 @@ static TmEcode DetectLoaderThreadDeinit(ThreadVars *t, void *data) static TmEcode DetectLoader(ThreadVars *th_v, void *thread_data) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif DetectLoaderThreadData *ftd = (DetectLoaderThreadData *)thread_data; BUG_ON(ftd == NULL); diff --git a/src/flow-manager.c b/src/flow-manager.c index 47484f9dc5..2c58fd4b87 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -658,10 +658,6 @@ static TmEcode FlowManagerThreadDeinit(ThreadVars *t, void *data) */ static TmEcode FlowManager(ThreadVars *th_v, void *thread_data) { -#ifndef OS_WIN32 - /* block usr2. usr1 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif FlowManagerThreadData *ftd = thread_data; struct timeval ts; uint32_t established_cnt = 0, new_cnt = 0, closing_cnt = 0; @@ -887,10 +883,6 @@ static TmEcode FlowRecyclerThreadDeinit(ThreadVars *t, void *data) */ static TmEcode FlowRecycler(ThreadVars *th_v, void *thread_data) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif struct timeval ts; struct timespec cond_time; int flow_update_delay_sec = FLOW_NORMAL_MODE_UPDATE_DELAY_SEC; diff --git a/src/suricata.c b/src/suricata.c index d78c71a2c8..631a1b6a06 100644 --- a/src/suricata.c +++ b/src/suricata.c @@ -2145,7 +2145,6 @@ static int InitSignalHandler(SCInstance *suri) UtilSignalHandlerSetup(SIGINT, SignalHandlerSigint); UtilSignalHandlerSetup(SIGTERM, SignalHandlerSigterm); #ifndef OS_WIN32 - UtilSignalHandlerSetup(SIGUSR2, SignalHandlerSigusr2); UtilSignalHandlerSetup(SIGHUP, SignalHandlerSigHup); UtilSignalHandlerSetup(SIGPIPE, SIG_IGN); UtilSignalHandlerSetup(SIGSYS, SIG_IGN); @@ -2458,8 +2457,10 @@ static void PostRunStartedDetectSetup(SCInstance *suri) * can't call it during the first sig load phase or while threads are still * starting up. */ if (DetectEngineEnabled() && suri->sig_file == NULL && - suri->delayed_detect == 0) + suri->delayed_detect == 0) { UtilSignalHandlerSetup(SIGUSR2, SignalHandlerSigusr2); + UtilSignalUnblock(SIGUSR2); + } #endif if (suri->delayed_detect) { /* force 'reload', this will load the rules and swap engines */ @@ -2774,6 +2775,18 @@ int main(int argc, char **argv) (void)SCSetThreadName("Suricata-Main"); + /* Ignore SIGUSR2 as early as possble. We redeclare interest + * once we're done launching threads. The goal is to either die + * completely or handle any and all SIGUSR2s correctly. + */ +#ifndef OS_WIN32 + UtilSignalHandlerSetup(SIGUSR2, SIG_IGN); + if (UtilSignalBlock(SIGUSR2)) { + SCLogError(SC_ERR_INITIALIZATION, "SIGUSR2 initialization error"); + exit(EXIT_FAILURE); + } +#endif + ParseSizeInit(); RunModeRegisterRunModes(); diff --git a/src/tm-threads.c b/src/tm-threads.c index 07b2bf1b4f..522b0c9475 100644 --- a/src/tm-threads.c +++ b/src/tm-threads.c @@ -270,10 +270,6 @@ static int TmThreadTimeoutLoop(ThreadVars *tv, TmSlot *s) static void *TmThreadsSlotPktAcqLoop(void *td) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif ThreadVars *tv = (ThreadVars *)td; TmSlot *s = tv->tm_slots; char run = 1; @@ -522,10 +518,6 @@ error: */ static void *TmThreadsSlotVar(void *td) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif ThreadVars *tv = (ThreadVars *)td; TmSlot *s = (TmSlot *)tv->tm_slots; Packet *p = NULL; @@ -684,10 +676,6 @@ error: static void *TmThreadsManagement(void *td) { -#ifndef OS_WIN32 - /* block usr2. usr2 to be handled by the main thread only */ - UtilSignalBlock(SIGUSR2); -#endif ThreadVars *tv = (ThreadVars *)td; TmSlot *s = (TmSlot *)tv->tm_slots; TmEcode r = TM_ECODE_OK; diff --git a/src/util-signal.c b/src/util-signal.c index dada13316a..9e3ec0cdf8 100644 --- a/src/util-signal.c +++ b/src/util-signal.c @@ -34,7 +34,24 @@ int UtilSignalBlock(int signum) return -1; if (sigaddset(&x, signum) < 0) return -1; - if (sigprocmask(SIG_BLOCK, &x, NULL) < 0) + /* don't use sigprocmask(), as it's undefined for + * multithreaded programs. Use phtread_sigmask(). + */ + if (pthread_sigmask(SIG_BLOCK, &x, NULL) != 0) + return -1; +#endif + return 0; +} + +int UtilSignalUnblock(int signum) +{ +#ifndef OS_WIN32 + sigset_t x; + if (sigemptyset(&x) < 0) + return -1; + if (sigaddset(&x, signum) < 0) + return -1; + if (pthread_sigmask(SIG_UNBLOCK, &x, NULL) != 0) return -1; #endif return 0; @@ -42,7 +59,7 @@ int UtilSignalBlock(int signum) void UtilSignalHandlerSetup(int sig, void (*handler)(int)) { -#if defined (OS_WIN32) +#ifdef OS_WIN32 signal(sig, handler); #else struct sigaction action; diff --git a/src/util-signal.h b/src/util-signal.h index 171d1b5536..1d5ba3973e 100644 --- a/src/util-signal.h +++ b/src/util-signal.h @@ -25,6 +25,7 @@ #define __UTIL_SIGNAL_H__ int UtilSignalBlock(int); +int UtilSignalUnblock(int); void UtilSignalHandlerSetup(int, void (*handler)(int)); #if 0 int UtilSignalIsHandler(int sig, void (*handler)(int));