[git commit] runsv: robustify signal handling - SIGTERM to child between vfork and exec could mess things up
Denys Vlasenko
vda.linux at googlemail.com
Sat Jun 5 14:20:05 UTC 2021
commit: https://git.busybox.net/busybox/commit/?id=5dadd497ffd9835a2860cf89ad781d1b513803dc
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master
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 <vda.linux at googlemail.com>
---
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);
More information about the busybox-cvs
mailing list