From ac444861b0b0212803ef775c366c41c4c020b1f1 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sat, 5 Jun 2021 14:51:53 +0200 Subject: svlogd: if processor's stdin can not be opened, do not try ad infinitum function old new delta processorstart 426 423 -3 Signed-off-by: Denys Vlasenko --- runit/svlogd.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'runit') diff --git a/runit/svlogd.c b/runit/svlogd.c index 294e31aca..4490492e3 100644 --- a/runit/svlogd.c +++ b/runit/svlogd.c @@ -412,19 +412,32 @@ static void processorstart(struct logdir *ld) int fd; /* child */ - /* Non-ignored signals revert to SIG_DFL on exec anyway */ + /* 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... + */ /*bb_signals(0 + (1 << SIGTERM) + //+ (1 << SIGCHLD) + (1 << SIGALRM) + (1 << SIGHUP) , SIG_DFL);*/ - sig_unblock(SIGTERM); - sig_unblock(SIGALRM); - sig_unblock(SIGHUP); + /* runit 2.1.2 does not unblock SIGCHLD, a bug? we do: */ + sigprocmask(SIG_UNBLOCK, &blocked_sigset, NULL); if (verbose) bb_error_msg(INFO"processing: %s/%s", ld->name, ld->fnsave); - fd = xopen(ld->fnsave, O_RDONLY|O_NDELAY); + + fd = open_or_warn(ld->fnsave, O_RDONLY|O_NDELAY); + /* Used to have xopen() above, but it causes infinite restarts of processor + * if file is gone - which can happen even because of _us_! + * Users report that if on reboot, time is reset to before existing + * logfiles creation time, rmoldest() deletes the newest logfile (!) + * and we end up here trying to open this now-deleted file. + */ + if (fd < 0) + _exit(0); /* fake "success": do not run processor again */ + xmove_fd(fd, 0); ld->fnsave[26] = 't'; /* <- that's why we need sv_ch! */ fd = xopen(ld->fnsave, O_WRONLY|O_NDELAY|O_TRUNC|O_CREAT); -- cgit v1.2.3-55-g6feb From d3e1090308b6d3c55e01a2000a743b73605ddd7f Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sat, 5 Jun 2021 15:24:04 +0200 Subject: tcp/udpsvd: robustify SIGCHLD handling function old new delta if_verbose_print_connection_status - 40 +40 tcpudpsvd_main 1798 1794 -4 connection_status 31 - -31 ------------------------------------------------------------------------------ (add/remove: 1/1 grow/shrink: 0/1 up/down: 40/-35) Total: 5 bytes Signed-off-by: Denys Vlasenko --- networking/tcpudp.c | 33 +++++++++++++++++++++++---------- runit/runsv.c | 5 ++++- 2 files changed, 27 insertions(+), 11 deletions(-) (limited to 'runit') diff --git a/networking/tcpudp.c b/networking/tcpudp.c index 8c4afabf6..708e05c2e 100644 --- a/networking/tcpudp.c +++ b/networking/tcpudp.c @@ -216,17 +216,25 @@ enum { OPT_K = (1 << 16), }; -static void connection_status(void) +static void if_verbose_print_connection_status(void) { - /* "only 1 client max" desn't need this */ - if (cmax > 1) - bb_error_msg("status %u/%u", cnum, cmax); + if (verbose) { + /* "only 1 client max" desn't need this */ + if (cmax > 1) + bb_error_msg("status %u/%u", cnum, cmax); + } } +/* SIGCHLD handler is reentrancy-safe because SIGCHLD is unmasked + * only over accept() or recvfrom() calls, not over memory allocations + * or printouts. Do need to save/restore errno in order not to mangle + * these syscalls' error code, if any. + */ static void sig_child_handler(int sig UNUSED_PARAM) { int wstat; pid_t pid; + int sv_errno = errno; while ((pid = wait_any_nohang(&wstat)) > 0) { if (max_per_host) @@ -236,8 +244,8 @@ static void sig_child_handler(int sig UNUSED_PARAM) if (verbose) print_waitstat(pid, wstat); } - if (verbose) - connection_status(); + if_verbose_print_connection_status(); + errno = sv_errno; } int tcpudpsvd_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE; @@ -458,7 +466,7 @@ int tcpudpsvd_main(int argc UNUSED_PARAM, char **argv) xconnect(0, &remote.u.sa, sa_len); /* hole? at this point we have no wildcard udp socket... * can this cause clients to get "port unreachable" icmp? - * Yup, time window is very small, but it exists (is it?) */ + * Yup, time window is very small, but it exists (does it?) */ /* ..."open new socket", continued */ xbind(sock, &lsa->u.sa, sa_len); socket_want_pktinfo(sock); @@ -491,8 +499,7 @@ int tcpudpsvd_main(int argc UNUSED_PARAM, char **argv) if (pid != 0) { /* Parent */ cnum++; - if (verbose) - connection_status(); + if_verbose_print_connection_status(); if (hccp) hccp->pid = pid; /* clean up changes done by vforked child */ @@ -586,8 +593,14 @@ int tcpudpsvd_main(int argc UNUSED_PARAM, char **argv) xdup2(0, 1); + /* Restore signal handling for the to-be-execed process */ signal(SIGPIPE, SIG_DFL); /* this one was SIG_IGNed */ - /* Non-ignored signals revert to SIG_DFL on exec anyway */ + /* Non-ignored signals revert to SIG_DFL on exec anyway + * But we can get signals BEFORE execvp(), this is unlikely + * but it would invoke sig_child_handler(), which would + * check waitpid(WNOHANG), then print "status N/M" if verbose. + * I guess we can live with that possibility. + */ /*signal(SIGCHLD, SIG_DFL);*/ sig_unblock(SIGCHLD); diff --git a/runit/runsv.c b/runit/runsv.c index ecab8cdf5..61ea240ff 100644 --- a/runit/runsv.c +++ b/runit/runsv.c @@ -380,7 +380,10 @@ static void startservice(struct svdir *s) xdup2(logpipe.wr, 1); } } - /* Non-ignored signals revert to SIG_DFL on exec anyway */ + /* 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... + */ /*bb_signals(0 + (1 << SIGCHLD) + (1 << SIGTERM) -- cgit v1.2.3-55-g6feb From 5dadd497ffd9835a2860cf89ad781d1b513803dc Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sat, 5 Jun 2021 16:20:05 +0200 Subject: 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(-) (limited to 'runit') 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); -- cgit v1.2.3-55-g6feb From 5a72b0cd74d6de8ed39a9704798ce1118b0995f1 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sat, 5 Jun 2021 16:36:21 +0200 Subject: runsv: code shrink: use single handler function for SIGTERM and SIGCHLD function old new delta s_chld_term - 36 +36 runsv_main 1677 1662 -15 s_child 22 - -22 s_term 29 - -29 ------------------------------------------------------------------------------ (add/remove: 1/2 grow/shrink: 0/1 up/down: 36/-66) Total: -30 bytes Signed-off-by: Denys Vlasenko --- runit/runsv.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) (limited to 'runit') diff --git a/runit/runsv.c b/runit/runsv.c index 7fad563f5..a4b8af494 100644 --- a/runit/runsv.c +++ b/runit/runsv.c @@ -149,20 +149,17 @@ static void warn_cannot(const char *m) warn2_cannot(m, ""); } -/* SIGCHLD/TERM handlers are reentrancy-safe because they are unmasked +/* SIGCHLD/TERM handler is 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) +static void s_chld_term(int sig_no) { + if (sig_no == SIGTERM) + sigterm = 1; write(selfpipe.wr, "", 1); } -static void s_term(int sig_no UNUSED_PARAM) -{ - sigterm = 1; - write(selfpipe.wr, "", 1); /* XXX */ -} static int open_trunc_or_warn(const char *name) { @@ -523,8 +520,10 @@ int runsv_main(int argc UNUSED_PARAM, char **argv) * (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); + bb_signals_norestart(0 + + (1 << SIGCHLD) + + (1 << SIGTERM) + , s_chld_term); xchdir(dir); /* bss: svd[0].pid = 0; */ -- cgit v1.2.3-55-g6feb