From 5dadd497ffd9835a2860cf89ad781d1b513803dc Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sat, 5 Jun 2021 16:20:05 +0200 Subject: [PATCH] runsv: robustify signal handling - SIGTERM to child between vfork and exec could mess things up While at it, rename bb_signals_recursive_norestart() to bb_signals_norestart(): "recursive" was implying we are setting SA_NODEFER allowing signal handler to be entered recursively, but we do not do that. function old new delta bb_signals_norestart - 70 +70 startservice 380 394 +14 bb_signals_recursive_norestart 70 - -70 ------------------------------------------------------------------------------ (add/remove: 1/1 grow/shrink: 1/0 up/down: 84/-70) Total: 14 bytes Signed-off-by: Denys Vlasenko --- console-tools/showkey.c | 2 +- include/libbb.h | 2 +- libbb/signals.c | 2 +- runit/runsv.c | 29 +++++++++++++++++++---------- runit/svlogd.c | 8 ++++---- sysklogd/klogd.c | 2 +- 6 files changed, 27 insertions(+), 18 deletions(-) diff --git a/console-tools/showkey.c b/console-tools/showkey.c index 4d7a9b9e5..84eb38a0a 100644 --- a/console-tools/showkey.c +++ b/console-tools/showkey.c @@ -106,7 +106,7 @@ int showkey_main(int argc UNUSED_PARAM, char **argv) xioctl(STDIN_FILENO, KDSKBMODE, (void *)(ptrdiff_t)((option_mask32 & OPT_k) ? K_MEDIUMRAW : K_RAW)); // we should exit on any signal; signals should interrupt read - bb_signals_recursive_norestart(BB_FATAL_SIGS, record_signo); + bb_signals_norestart(BB_FATAL_SIGS, record_signo); // inform user that program ends after time of inactivity printf(press_keys, "10s after last keypress"); diff --git a/include/libbb.h b/include/libbb.h index 4c9c83bd1..a3f76a206 100644 --- a/include/libbb.h +++ b/include/libbb.h @@ -593,7 +593,7 @@ void bb_signals(int sigs, void (*f)(int)) FAST_FUNC; /* Unlike signal() and bb_signals, sets handler with sigaction() * and in a way that while signal handler is run, no other signals * will be blocked; syscalls will not be restarted: */ -void bb_signals_recursive_norestart(int sigs, void (*f)(int)) FAST_FUNC; +void bb_signals_norestart(int sigs, void (*f)(int)) FAST_FUNC; /* syscalls like read() will be interrupted with EINTR: */ void signal_no_SA_RESTART_empty_mask(int sig, void (*handler)(int)) FAST_FUNC; /* syscalls like read() won't be interrupted (though select/poll will be): */ diff --git a/libbb/signals.c b/libbb/signals.c index d3d84ef6a..0bebc847d 100644 --- a/libbb/signals.c +++ b/libbb/signals.c @@ -56,7 +56,7 @@ void FAST_FUNC bb_signals(int sigs, void (*f)(int)) } } -void FAST_FUNC bb_signals_recursive_norestart(int sigs, void (*f)(int)) +void FAST_FUNC bb_signals_norestart(int sigs, void (*f)(int)) { int sig_no = 0; int bit = 1; diff --git a/runit/runsv.c b/runit/runsv.c index 61ea240ff..7fad563f5 100644 --- a/runit/runsv.c +++ b/runit/runsv.c @@ -149,11 +149,15 @@ static void warn_cannot(const char *m) warn2_cannot(m, ""); } +/* SIGCHLD/TERM handlers are reentrancy-safe because they are unmasked + * only over poll() call, not over memory allocations + * or printouts. Do not need to save/restore errno either, + * as poll() error is not checked there. + */ static void s_child(int sig_no UNUSED_PARAM) { write(selfpipe.wr, "", 1); } - static void s_term(int sig_no UNUSED_PARAM) { sigterm = 1; @@ -380,14 +384,14 @@ static void startservice(struct svdir *s) xdup2(logpipe.wr, 1); } } - /* Non-ignored signals revert to SIG_DFL on exec anyway. - * But we can get signals BEFORE execl(), this is unlikely - * but wouldn't be good... + /* Non-ignored signals revert to SIG_DFL on exec. + * But we can get signals BEFORE execl(), unlikely as that may be. + * SIGCHLD is safe (would merely write to selfpipe), + * but SIGTERM would set sigterm = 1 (with vfork, we affect parent). + * Avoid that. */ - /*bb_signals(0 - + (1 << SIGCHLD) - + (1 << SIGTERM) - , SIG_DFL);*/ + /*signal(SIGCHLD, SIG_DFL);*/ + signal(SIGTERM, SIG_DFL); sig_unblock(SIGCHLD); sig_unblock(SIGTERM); execv(arg[0], (char**) arg); @@ -514,9 +518,13 @@ int runsv_main(int argc UNUSED_PARAM, char **argv) ndelay_on(selfpipe.wr); sig_block(SIGCHLD); - bb_signals_recursive_norestart(1 << SIGCHLD, s_child); sig_block(SIGTERM); - bb_signals_recursive_norestart(1 << SIGTERM, s_term); + /* No particular reason why we don't set SA_RESTART + * (poll() wouldn't restart regardless of that flag), + * we just follow what runit-2.1.2 does: + */ + bb_signals_norestart(1 << SIGCHLD, s_child); + bb_signals_norestart(1 << SIGTERM, s_term); xchdir(dir); /* bss: svd[0].pid = 0; */ @@ -628,6 +636,7 @@ int runsv_main(int argc UNUSED_PARAM, char **argv) sig_unblock(SIGTERM); sig_unblock(SIGCHLD); poll(x, 2 + haslog, 3600*1000); + /* NB: signal handlers can trash errno of poll() */ sig_block(SIGTERM); sig_block(SIGCHLD); diff --git a/runit/svlogd.c b/runit/svlogd.c index 4490492e3..02c305696 100644 --- a/runit/svlogd.c +++ b/runit/svlogd.c @@ -1111,10 +1111,10 @@ int svlogd_main(int argc, char **argv) sigaddset(&blocked_sigset, SIGALRM); sigaddset(&blocked_sigset, SIGHUP); sigprocmask(SIG_BLOCK, &blocked_sigset, NULL); - bb_signals_recursive_norestart(1 << SIGTERM, sig_term_handler); - bb_signals_recursive_norestart(1 << SIGCHLD, sig_child_handler); - bb_signals_recursive_norestart(1 << SIGALRM, sig_alarm_handler); - bb_signals_recursive_norestart(1 << SIGHUP, sig_hangup_handler); + bb_signals_norestart(1 << SIGTERM, sig_term_handler); + bb_signals_norestart(1 << SIGCHLD, sig_child_handler); + bb_signals_norestart(1 << SIGALRM, sig_alarm_handler); + bb_signals_norestart(1 << SIGHUP, sig_hangup_handler); /* Without timestamps, we don't have to print each line * separately, so we can look for _last_ newline, not first, diff --git a/sysklogd/klogd.c b/sysklogd/klogd.c index 82596bc0b..df0edee0a 100644 --- a/sysklogd/klogd.c +++ b/sysklogd/klogd.c @@ -226,7 +226,7 @@ int klogd_main(int argc UNUSED_PARAM, char **argv) signal(SIGHUP, SIG_IGN); /* We want klogd_read to not be restarted, thus _norestart: */ - bb_signals_recursive_norestart(BB_FATAL_SIGS, record_signo); + bb_signals_norestart(BB_FATAL_SIGS, record_signo); syslog(LOG_NOTICE, "klogd started: %s", bb_banner);