[git commit] ash: jobs: Fix waitcmd busy loop

[Denys Vlasenko]

commit: https://git.busybox.net/busybox/commit/?id=91e11eba6e3ee21f70905bca857b9b216fb764f6
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master

Upstream commit:

    Date: Tue, 2 Jun 2020 23:46:48 +1000
    jobs: Fix waitcmd busy loop

    We need to clear gotsigchld in waitproc because it is used as
    a loop conditional for the waitcmd case.  Without it waitcmd
    may busy loop after a SIGCHLD.

    This patch also changes gotsigchld into a volatile sig_atomic_t
    to prevent compilers from optimising its accesses away.

    Fixes: 6c691b3e5099 ("jobs: Only clear gotsigchld when waiting...")
    Signed-off-by: Herbert Xu <herbert at gondor.apana.org.au>

This change also incorporates other changes to bring us closer to upstream.

function                                             old     new   delta
dowait                                               553     636     +83

Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>
---
 shell/ash.c | 91 +++++++++++++++++++++++--------------------------------------
 1 file changed, 34 insertions(+), 57 deletions(-)

diff --git a/shell/ash.c b/shell/ash.c
index 13470b2fa..07aa2da2e 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -4273,64 +4273,55 @@ sprint_status48(char *os, int status, int sigonly)
 	return s - os;
 }
 
+#define DOWAIT_NONBLOCK 0
+#define DOWAIT_BLOCK    1
+#define DOWAIT_BLOCK_OR_SIG 2
+#if BASH_WAIT_N
+# define DOWAIT_JOBSTATUS 0x10   /* OR this to get job's exitstatus instead of pid */
+#endif
+
 static int
-wait_block_or_sig(int *status)
+waitproc(int block, int *status)
 {
-	int pid;
+	sigset_t oldmask;
+	int flags = block == DOWAIT_BLOCK ? 0 : WNOHANG;
+	int err;
 
-	do {
-		sigset_t mask;
+#if JOBS
+	if (doing_jobctl)
+		flags |= WUNTRACED;
+#endif
 
-		/* Poll all children for changes in their state */
+	do {
 		got_sigchld = 0;
-		/* if job control is active, accept stopped processes too */
-		pid = waitpid(-1, status, doing_jobctl ? (WNOHANG|WUNTRACED) : WNOHANG);
-		if (pid != 0)
-			break; /* Error (e.g. EINTR, ECHILD) or pid */
+		do
+			err = waitpid(-1, status, flags);
+		while (err < 0 && errno == EINTR);
 
-		/* Children exist, but none are ready. Sleep until interesting signal */
-#if 1
-		sigfillset(&mask);
-		sigprocmask2(SIG_SETMASK, &mask); /* mask is updated */
-		while (!got_sigchld && !pending_sig) {
-			sigsuspend(&mask);
-			/* ^^^ add "sigdelset(&mask, SIGCHLD);" before sigsuspend
-			 * to make sure SIGCHLD is not masked off?
-			 * It was reported that this:
-			 *	fn() { : | return; }
-			 *	shopt -s lastpipe
-			 *	fn
-			 *	exec ash SCRIPT
-			 * under bash 4.4.23 runs SCRIPT with SIGCHLD masked,
-			 * making "wait" commands in SCRIPT block forever.
-			 */
-		}
-		sigprocmask(SIG_SETMASK, &mask, NULL);
-#else /* unsafe: a signal can set pending_sig after check, but before pause() */
+		if (err || (err = -!block))
+			break;
+
+		sigfillset(&oldmask);
+		sigprocmask2(SIG_SETMASK, &oldmask); /* mask is updated */
 		while (!got_sigchld && !pending_sig)
-			pause();
-#endif
+			sigsuspend(&oldmask);
+		sigprocmask(SIG_SETMASK, &oldmask, NULL);
+		//simpler, but unsafe: a signal can set pending_sig after check, but before pause():
+		//while (!got_sigchld && !pending_sig)
+		//	pause();
 
-		/* If it was SIGCHLD, poll children again */
 	} while (got_sigchld);
 
-	return pid;
+	return err;
 }
 
-#define DOWAIT_NONBLOCK 0
-#define DOWAIT_BLOCK    1
-#define DOWAIT_BLOCK_OR_SIG 2
-#if BASH_WAIT_N
-# define DOWAIT_JOBSTATUS 0x10   /* OR this to get job's exitstatus instead of pid */
-#endif
-
 static int
 waitone(int block, struct job *job)
 {
 	int pid;
 	int status;
 	struct job *jp;
-	struct job *thisjob;
+	struct job *thisjob = NULL;
 #if BASH_WAIT_N
 	bool want_jobexitstatus = (block & DOWAIT_JOBSTATUS);
 	block = (block & ~DOWAIT_JOBSTATUS);
@@ -4357,21 +4348,8 @@ waitone(int block, struct job *job)
 	 * SIG_DFL handler does not wake sigsuspend().
 	 */
 	INT_OFF;
-	if (block == DOWAIT_BLOCK_OR_SIG) {
-		pid = wait_block_or_sig(&status);
-	} else {
-		int wait_flags = 0;
-		if (block == DOWAIT_NONBLOCK)
-			wait_flags = WNOHANG;
-		/* if job control is active, accept stopped processes too */
-		if (doing_jobctl)
-			wait_flags |= WUNTRACED;
-		/* NB: _not_ safe_waitpid, we need to detect EINTR */
-		pid = waitpid(-1, &status, wait_flags);
-	}
-	TRACE(("wait returns pid=%d, status=0x%x, errno=%d(%s)\n",
-				pid, status, errno, strerror(errno)));
-	thisjob = NULL;
+	pid = waitproc(block, &status);
+	TRACE(("wait returns pid %d, status=%d\n", pid, status));
 	if (pid <= 0)
 		goto out;
 
@@ -4466,7 +4444,6 @@ dowait(int block, struct job *jp)
 	rpid = 1;
 
 	do {
-		got_sigchld = 0;
 		pid = waitone(block, jp);
 		rpid &= !!pid;
 
@@ -4721,7 +4698,7 @@ waitcmd(int argc UNUSED_PARAM, char **argv)
 			job = getjob(*argv, 0);
 		}
 		/* loop until process terminated or stopped */
-		dowait(DOWAIT_BLOCK_OR_SIG, NULL);
+		dowait(DOWAIT_BLOCK_OR_SIG, job);
 		if (pending_sig)
 			goto sigout;
 		job->waited = 1;