Re: kern/57527: system() waits for last child if SIG_CHLD is ignored

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/57527; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: dholland%NetBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/57527: system() waits for last child if SIG_CHLD is ignored
Date: Sat, 15 Jul 2023 21:48:47 +0000

 This is a multi-part message in MIME format.
 --=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV
 
 > Date: Sat, 15 Jul 2023 20:46:43 +0000
 > From: David Holland <dholland-bugs%netbsd.org@localhost>
 > 
 > On Sat, Jul 15, 2023 at 08:05:02PM +0000, Taylor R Campbell wrote:
 > > However, it doesn't seem to address the possibility of changing
 > > SIGCHLD to SIG_DFL if and only if it was already set to SIG_IGN, if
 > > I'm reading it correctly.  So maybe the attached patch will resolve
 > > the issue in system(3)?
 > 
 > It's worse than that; you could race with another thread setting
 > SIG_IGN.
 > 
 > Maybe that gets into "don't do that" territory though.
 
 Yes, it's in `don't do that' territory.  POSIX says:
 
    Using the system() function in more than one thread in a process or
    when the SIGCHLD signal is being manipulated by more than one
    thread in a process may produce unexpected results.
 
    https://pubs.opengroup.org/onlinepubs/9699919799/functions/system.html
 
 --=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57527-system"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57527-system.patch"
 
 From 51954a3b812c57c57835be1b4b06c5c9918c5783 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Sat, 15 Jul 2023 20:02:28 +0000
 Subject: [PATCH] system(3): Change SIG_IGN to SIG_DFL for SIGCHLD while we
  work.
 
 PR kern/57527
 ---
  lib/libc/stdlib/system.c | 32 ++++++++++++++++++++++++++++++--
  1 file changed, 30 insertions(+), 2 deletions(-)
 
 diff --git a/lib/libc/stdlib/system.c b/lib/libc/stdlib/system.c
 index 5a19bc9e0568..4d3e45f1892a 100644
 --- a/lib/libc/stdlib/system.c
 +++ b/lib/libc/stdlib/system.c
 @@ -54,7 +54,7 @@ int
  system(const char *command)
  {
  	pid_t pid;
 -	struct sigaction intsa, quitsa, sa;
 +	struct sigaction chldsa, intsa, quitsa, sa;
  	sigset_t nmask, omask, sigdefault;
  	int pstat;
  	const char *argp[] =3D {"sh", "-c", "--", command, NULL};
 @@ -90,6 +90,32 @@ system(const char *command)
  		return -1;
  	}
 =20
 +	/*
 +	 * Make sure SIGCHLD is not set to SIG_IGN, because that means
 +	 * waitpid will wait until _all_ children have exited, under a
 +	 * peculiar clause of POSIX enshrining a quirk of SysV
 +	 * semantics.
 +	 *
 +	 * If SIGCHLD was set to anything other than SIG_IGN, we must
 +	 * not interfere with it -- changing the action may clear any
 +	 * queued signals for for processes _other_ than the one that
 +	 * system(3) itself spawns.
 +	 */
 +	if (sigaction(SIGCHLD, NULL, &chldsa) =3D=3D -1) {
 +		(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 +		sigaction(SIGINT, &intsa, NULL);
 +		sigaction(SIGQUIT, &quitsa, NULL);
 +		return -1;
 +	}
 +	if (chldsa.sa_handler =3D=3D SIG_IGN &&
 +	    sigaction(SIGCHLD, &(struct sigaction){ .sa_handler =3D SIG_DFL },
 +		NULL) =3D=3D -1) {
 +		(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 +		sigaction(SIGINT, &intsa, NULL);
 +		sigaction(SIGQUIT, &quitsa, NULL);
 +		return -1;
 +	}
 +
  	/*
  	 * We arrange to inherit all signal handlers from the caller by
  	 * default, except possibly SIGINT and SIGQUIT.  These we have
 @@ -133,7 +159,9 @@ system(const char *command)
  		}
  	}
 =20
 -out:	sigaction(SIGINT, &intsa, NULL);
 +out:	if (chldsa.sa_handler =3D=3D SIG_IGN)
 +		sigaction(SIGCHLD, &chldsa, NULL);
 +	sigaction(SIGINT, &intsa, NULL);
  	sigaction(SIGQUIT, &quitsa, NULL);
  	(void)sigprocmask(SIG_SETMASK, &omask, NULL);
 =20
 
 --=_cMRXY1mpK2XMc2pQZgnqCub6ZuhP0lAV--