turns out there WAS something fishy in signal handling in the "generic"

reaper. Specifically, the sigprocmask/wait/sigsuspend dance is correct for
the main process, BUT you have to remember to reset the signal mask to
something sane for the child (this was a duh! moment, that bug is very
stupid)


Finally, bluhm@  saw the actual issue. Kudoes to him.

The change to "unify" sequential and parallel make  made the bug reproducible
under some circumstances
(because in the parallel make case, many things may happen in different
successions, so you don't get the wrong signal mask that often, but the
sequential case is reproducible, and using the "streamlined" reaper meant the
fork would occur WITHIN the signal loop instead of OUTSIDE)

So:
- discover signal state early on through Sigset_init;
- introduce reset_signal_mask to get back to the initial state;
- call reset_signal_mask systematically after fork

This organisation thanks to cmd_exec.  SOME cmd_exec happens before Job_Init
happens, some afterwards (variables are still lazy and both !!= and :sh will
occur AFTER parsing), so both fork() need to be protected.

okay bluhm@

thx to sthen@ and naddy@ and mpi@ for helping out.

5 files changed, 29 insertions, 10 deletions

diff --git a/usr.bin/make/cmd_exec.c b/usr.bin/make/cmd_exec.c
index 4a206e9bd5c..ca0ecd25f82 100644
--- a/usr.bin/make/cmd_exec.c
+++ b/usr.bin/make/cmd_exec.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: cmd_exec.c,v 1.10 2016/03/28 11:27:37 chl Exp $ */
+/*	$OpenBSD: cmd_exec.c,v 1.11 2020/01/16 16:07:18 espie Exp $ */
 /*
  * Copyright (c) 2001 Marc Espie.
  *
@@ -35,6 +35,7 @@
 #include "buf.h"
 #include "memory.h"
 #include "pathnames.h"
+#include "job.h"
 
 char *
 Cmd_Exec(const char *cmd, char **err)
@@ -67,6 +68,7 @@ Cmd_Exec(const char *cmd, char **err)
 	/* Fork */
 	switch (cpid = fork()) {
 	case 0:
+		reset_signal_mask();
 		/* Close input side of pipe */
 		(void)close(fds[0]);
 
diff --git a/usr.bin/make/engine.c b/usr.bin/make/engine.c
index 778f165248a..ddfe2d4d9bb 100644
--- a/usr.bin/make/engine.c
+++ b/usr.bin/make/engine.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: engine.c,v 1.67 2020/01/13 15:24:31 espie Exp $ */
+/*	$OpenBSD: engine.c,v 1.68 2020/01/16 16:07:18 espie Exp $ */
 /*
  * Copyright (c) 2012 Marc Espie.
  *
@@ -783,6 +783,7 @@ do_run_command(Job *job, const char *pre)
 		Punt("Could not fork");
 		/*NOTREACHED*/
 	case 0:
+		reset_signal_mask();
 		/* put a random delay unless we're the only job running
 		 * and there's nothing left to do.
 		 */
diff --git a/usr.bin/make/init.c b/usr.bin/make/init.c
index 215839ffca4..db5a224a19e 100644
--- a/usr.bin/make/init.c
+++ b/usr.bin/make/init.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: init.c,v 1.7 2012/10/02 10:29:30 espie Exp $ */
+/*	$OpenBSD: init.c,v 1.8 2020/01/16 16:07:18 espie Exp $ */
 
 /*
  * Copyright (c) 2001 Marc Espie.
@@ -41,6 +41,7 @@
 void
 Init(void)
 {
+	Sigset_Init();
 	Init_Timestamp();
 	Init_Stats();
 	Targ_Init();
diff --git a/usr.bin/make/job.c b/usr.bin/make/job.c
index a1334c17138..ba71b154a68 100644
--- a/usr.bin/make/job.c
+++ b/usr.bin/make/job.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: job.c,v 1.158 2020/01/13 16:03:44 espie Exp $	*/
+/*	$OpenBSD: job.c,v 1.159 2020/01/16 16:07:18 espie Exp $	*/
 /*	$NetBSD: job.c,v 1.16 1996/11/06 17:59:08 christos Exp $	*/
 
 /*
@@ -129,7 +129,7 @@ static volatile sig_atomic_t got_fatal;
 static volatile sig_atomic_t got_SIGINT, got_SIGHUP, got_SIGQUIT, got_SIGTERM, 
     got_SIGINFO;
 
-static sigset_t sigset, emptyset;
+static sigset_t sigset, emptyset, origset;
 
 static void handle_fatal_signal(int);
 static void handle_siginfo(void);
@@ -376,11 +376,17 @@ notice_signal(int sig)
 	}
 }
 
+void
+Sigset_Init()
+{
+	sigemptyset(&emptyset);
+	sigprocmask(SIG_BLOCK, &emptyset, &origset);
+}
+
 static void
 setup_all_signals(void)
 {
 	sigemptyset(&sigset);
-	sigemptyset(&emptyset);
 	/*
 	 * Catch the four signals that POSIX specifies if they aren't ignored.
 	 * handle_signal will take care of calling JobInterrupt if appropriate.
@@ -768,16 +774,21 @@ reap_jobs(void)
 	return reaped;
 }
 
+void 
+reset_signal_mask()
+{
+	sigprocmask(SIG_SETMASK, &origset, NULL);
+}
+
 void
 handle_running_jobs(void)
 {
-	sigset_t old;
 	/* reaping children in the presence of caught signals */
 
 	/* first, we make sure to hold on new signals, to synchronize
 	 * reception of new stuff on sigsuspend
 	 */
-	sigprocmask(SIG_BLOCK, &sigset, &old);
+	sigprocmask(SIG_BLOCK, &sigset, NULL);
 	/* note this will NOT loop until runningJobs == NULL.
 	 * It's merely an optimisation, namely that we don't need to go 
 	 * through the logic if no job is present. As soon as a job 
@@ -796,7 +807,7 @@ handle_running_jobs(void)
 		 */
 		sigsuspend(&emptyset);
 	}
-	sigprocmask(SIG_SETMASK, &old, NULL);
+	reset_signal_mask();
 }
 
 void
diff --git a/usr.bin/make/job.h b/usr.bin/make/job.h
index 03a5abb0dc9..a43a3e209f1 100644
--- a/usr.bin/make/job.h
+++ b/usr.bin/make/job.h
@@ -1,7 +1,7 @@
 #ifndef _JOB_H_
 #define _JOB_H_
 
-/*	$OpenBSD: job.h,v 1.36 2020/01/13 16:01:47 espie Exp $	*/
+/*	$OpenBSD: job.h,v 1.37 2020/01/16 16:07:18 espie Exp $	*/
 /*	$NetBSD: job.h,v 1.5 1996/11/06 17:59:10 christos Exp $ */
 
 /*
@@ -54,6 +54,9 @@ extern void Job_Make(GNode *);
  */
 extern void Job_Init(int);
 
+/* save signal mask at start */
+extern void Sigset_Init();
+
 /* interface with the normal build in make.c */
 /* okay = can_start_job();
  *	can we run new jobs right now ?
@@ -78,6 +81,7 @@ extern void handle_running_jobs(void);
  *	handle running jobs until they're finished.
  */
 extern void loop_handle_running_jobs(void);
+extern void reset_signal_mask(void);
 
 /* handle_all_signals();
  *	if a signal was received, react accordingly.