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 <vda.linux@googlemail.com>

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");

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): */

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;

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.
+		/* Non-ignored signals revert to SIG_DFL on exec.
-		 * But we can get signals BEFORE execl(), this is unlikely
+		 * But we can get signals BEFORE execl(), unlikely as that may be.
-		 * but wouldn't be good...
+		 * SIGCHLD is safe (would merely write to selfpipe),
+		 * but SIGTERM would set sigterm = 1 (with vfork, we affect parent).
+		 * Avoid that.
 		 */
-		/*bb_signals(0
+		/*signal(SIGCHLD, SIG_DFL);*/
-			+ (1 << SIGCHLD)
+		signal(SIGTERM, SIG_DFL);
-			+ (1 << 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);
 

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_norestart(1 << SIGTERM, sig_term_handler);
-	bb_signals_recursive_norestart(1 << SIGCHLD, sig_child_handler);
+	bb_signals_norestart(1 << SIGCHLD, sig_child_handler);
-	bb_signals_recursive_norestart(1 << SIGALRM, sig_alarm_handler);
+	bb_signals_norestart(1 << SIGALRM, sig_alarm_handler);
-	bb_signals_recursive_norestart(1 << SIGHUP, sig_hangup_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,

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);