← Commit list

5dadd497f

Denys Vlasenko

2021-06-05

SUCCESS Duration: 156s

Subject

runsv: robustify signal handling - SIGTERM to child between vfork and exec could mess things up

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

console-tools/showkey.c +1 -1 include/libbb.h +1 -1 libbb/signals.c +1 -1 runit/runsv.c +19 -10 runit/svlogd.c +4 -4 sysklogd/klogd.c +1 -1

console-tools/showkey.c+1 -1

@@ -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+1 -1

@@ -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+1 -1

@@ -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+19 -10

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

runit/svlogd.c+4 -4

@@ -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,

sysklogd/klogd.c+1 -1

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