Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 21 Oct 2019 01:24:23 +0000 (UTC)
From:      Kyle Evans <kevans@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org
Subject:   svn commit: r353789 - in stable: 11/lib/libc/gen 11/lib/libc/sys 11/sys/kern 11/sys/sys 12/lib/libc/gen 12/lib/libc/sys 12/sys/kern 12/sys/sys
Message-ID:  <201910210124.x9L1ONpm075292@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: kevans
Date: Mon Oct 21 01:24:21 2019
New Revision: 353789
URL: https://svnweb.freebsd.org/changeset/base/353789

Log:
  MFC r352711-r352712: Address posix_spawn(3) signal issues
  
  r352711:
  rfork(2): add RFSPAWN flag
  
  When RFSPAWN is passed, rfork exhibits vfork(2) semantics but also resets
  signal handlers in the child during creation to avoid a point of corruption
  of parent state from the child.
  
  This flag will be used by posix_spawn(3) to handle potential signal issues.
  
  Reviewed by:	jilles, kib
  Differential Revision:	https://reviews.freebsd.org/D19058
  
  r352712:
  posix_spawn(3): handle potential signal issues with vfork
  
  Described in [1], signal handlers running in a vfork child have
  opportunities to corrupt the parent's state. Address this by adding a new
  rfork(2) flag, RFSPAWN, that has vfork(2) semantics but also resets signal
  handlers in the child during creation.
  
  x86 uses rfork_thread(3) instead of a direct rfork(2) because rfork with
  RFMEM/RFSPAWN cannot work when the return address is stored on the stack --
  further information about this problem is described under RFMEM in the
  rfork(2) man page.
  
  Addressing this has been identified as a prerequisite to using posix_spawn
  in subprocess on FreeBSD [2].
  
  [1] https://ewontfix.com/7/
  [2] https://bugs.python.org/issue35823

Modified:
  stable/12/lib/libc/gen/posix_spawn.c
  stable/12/lib/libc/sys/rfork.2
  stable/12/sys/kern/kern_fork.c
  stable/12/sys/kern/kern_sig.c
  stable/12/sys/sys/proc.h
  stable/12/sys/sys/signalvar.h
  stable/12/sys/sys/unistd.h
Directory Properties:
  stable/12/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/11/lib/libc/gen/posix_spawn.c
  stable/11/lib/libc/sys/rfork.2
  stable/11/sys/kern/kern_fork.c
  stable/11/sys/kern/kern_sig.c
  stable/11/sys/sys/proc.h
  stable/11/sys/sys/signalvar.h
  stable/11/sys/sys/unistd.h
Directory Properties:
  stable/11/   (props changed)

Modified: stable/12/lib/libc/gen/posix_spawn.c
==============================================================================
--- stable/12/lib/libc/gen/posix_spawn.c	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/lib/libc/gen/posix_spawn.c	Mon Oct 21 01:24:21 2019	(r353789)
@@ -194,43 +194,115 @@ process_file_actions(const posix_spawn_file_actions_t 
 	return (0);
 }
 
+struct posix_spawn_args {
+	const char *path;
+	const posix_spawn_file_actions_t *fa;
+	const posix_spawnattr_t *sa;
+	char * const * argv;
+	char * const * envp;
+	int use_env_path;
+	int error;
+};
+
+#if defined(__i386__) || defined(__amd64__)
+#define	_RFORK_THREAD_STACK_SIZE	4096
+#endif
+
 static int
+_posix_spawn_thr(void *data)
+{
+	struct posix_spawn_args *psa;
+	char * const *envp;
+
+	psa = data;
+	if (psa->sa != NULL) {
+		psa->error = process_spawnattr(*psa->sa);
+		if (psa->error)
+			_exit(127);
+	}
+	if (psa->fa != NULL) {
+		psa->error = process_file_actions(*psa->fa);
+		if (psa->error)
+			_exit(127);
+	}
+	envp = psa->envp != NULL ? psa->envp : environ;
+	if (psa->use_env_path)
+		_execvpe(psa->path, psa->argv, envp);
+	else
+		_execve(psa->path, psa->argv, envp);
+	psa->error = errno;
+
+	/* This is called in such a way that it must not exit. */
+	_exit(127);
+}
+
+static int
 do_posix_spawn(pid_t *pid, const char *path,
     const posix_spawn_file_actions_t *fa,
     const posix_spawnattr_t *sa,
     char * const argv[], char * const envp[], int use_env_path)
 {
+	struct posix_spawn_args psa;
 	pid_t p;
-	volatile int error = 0;
+#ifdef _RFORK_THREAD_STACK_SIZE
+	char *stack;
 
-	p = vfork();
-	switch (p) {
-	case -1:
-		return (errno);
-	case 0:
-		if (sa != NULL) {
-			error = process_spawnattr(*sa);
-			if (error)
-				_exit(127);
-		}
-		if (fa != NULL) {
-			error = process_file_actions(*fa);
-			if (error)
-				_exit(127);
-		}
-		if (use_env_path)
-			_execvpe(path, argv, envp != NULL ? envp : environ);
-		else
-			_execve(path, argv, envp != NULL ? envp : environ);
-		error = errno;
-		_exit(127);
-	default:
-		if (error != 0)
-			_waitpid(p, NULL, WNOHANG);
-		else if (pid != NULL)
-			*pid = p;
-		return (error);
+	stack = malloc(_RFORK_THREAD_STACK_SIZE);
+	if (stack == NULL)
+		return (ENOMEM);
+#endif
+	psa.path = path;
+	psa.fa = fa;
+	psa.sa = sa;
+	psa.argv = argv;
+	psa.envp = envp;
+	psa.use_env_path = use_env_path;
+	psa.error = 0;
+
+	/*
+	 * Passing RFSPAWN to rfork(2) gives us effectively a vfork that drops
+	 * non-ignored signal handlers.  We'll fall back to the slightly less
+	 * ideal vfork(2) if we get an EINVAL from rfork -- this should only
+	 * happen with newer libc on older kernel that doesn't accept
+	 * RFSPAWN.
+	 */
+#ifdef _RFORK_THREAD_STACK_SIZE
+	/*
+	 * x86 stores the return address on the stack, so rfork(2) cannot work
+	 * as-is because the child would clobber the return address om the
+	 * parent.  Because of this, we must use rfork_thread instead while
+	 * almost every other arch stores the return address in a register.
+	 */
+	p = rfork_thread(RFSPAWN, stack + _RFORK_THREAD_STACK_SIZE,
+	    _posix_spawn_thr, &psa);
+	free(stack);
+#else
+	p = rfork(RFSPAWN);
+	if (p == 0)
+		/* _posix_spawn_thr does not return */
+		_posix_spawn_thr(&psa);
+#endif
+	/*
+	 * The above block should leave us in a state where we've either
+	 * succeeded and we're ready to process the results, or we need to
+	 * fallback to vfork() if the kernel didn't like RFSPAWN.
+	 */
+
+	if (p == -1 && errno == EINVAL) {
+		p = vfork();
+		if (p == 0)
+			/* _posix_spawn_thr does not return */
+			_posix_spawn_thr(&psa);
 	}
+	if (p == -1)
+		return (errno);
+	if (psa.error != 0)
+		/* Failed; ready to reap */
+		_waitpid(p, NULL, WNOHANG);
+	else if (pid != NULL)
+		/* exec succeeded */
+		*pid = p;
+	return (psa.error);
 }
 
 int

Modified: stable/12/lib/libc/sys/rfork.2
==============================================================================
--- stable/12/lib/libc/sys/rfork.2	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/lib/libc/sys/rfork.2	Mon Oct 21 01:24:21 2019	(r353789)
@@ -5,7 +5,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd July 12, 2011
+.Dd September 25, 2019
 .Dt RFORK 2
 .Os
 .Sh NAME
@@ -34,7 +34,9 @@ and open files.
 The
 .Fa flags
 argument
-is the logical OR of some subset of:
+is either
+.Dv RFSPAWN
+or the logical OR of some subset of:
 .Bl -tag -width ".Dv RFLINUXTHPN"
 .It Dv RFPROC
 If set a new process is created; otherwise changes affect the
@@ -103,6 +105,17 @@ This is intended to mimic certain Linux clone behaviou
 File descriptors in a shared file descriptor table are kept
 open until either they are explicitly closed
 or all processes sharing the table exit.
+.Pp
+If
+.Dv RFSPAWN
+is passed,
+.Nm
+will use
+.Xr vfork 2
+semantics but reset all signal actions in the child to default.
+This flag is used by the
+.Xr posix_spawn 3
+implementation in libc.
 .Pp
 If
 .Dv RFPROC

Modified: stable/12/sys/kern/kern_fork.c
==============================================================================
--- stable/12/sys/kern/kern_fork.c	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/sys/kern/kern_fork.c	Mon Oct 21 01:24:21 2019	(r353789)
@@ -171,10 +171,18 @@ sys_rfork(struct thread *td, struct rfork_args *uap)
 	/* Don't allow kernel-only flags. */
 	if ((uap->flags & RFKERNELONLY) != 0)
 		return (EINVAL);
+	/* RFSPAWN must not appear with others */
+	if ((uap->flags & RFSPAWN) != 0 && uap->flags != RFSPAWN)
+		return (EINVAL);
 
 	AUDIT_ARG_FFLAGS(uap->flags);
 	bzero(&fr, sizeof(fr));
-	fr.fr_flags = uap->flags;
+	if ((uap->flags & RFSPAWN) != 0) {
+		fr.fr_flags = RFFDG | RFPROC | RFPPWAIT | RFMEM;
+		fr.fr_flags2 = FR2_DROPSIG_CAUGHT;
+	} else {
+		fr.fr_flags = uap->flags;
+	}
 	fr.fr_pidp = &pid;
 	error = fork1(td, &fr);
 	if (error == 0) {
@@ -520,6 +528,11 @@ do_fork(struct thread *td, struct fork_req *fr, struct
 	} else {
 		sigacts_copy(newsigacts, p1->p_sigacts);
 		p2->p_sigacts = newsigacts;
+		if ((fr->fr_flags2 & FR2_DROPSIG_CAUGHT) != 0) {
+			mtx_lock(&p2->p_sigacts->ps_mtx);
+			sig_drop_caught(p2);
+			mtx_unlock(&p2->p_sigacts->ps_mtx);
+		}
 	}
 
 	if (fr->fr_flags & RFTSIGZMB)

Modified: stable/12/sys/kern/kern_sig.c
==============================================================================
--- stable/12/sys/kern/kern_sig.c	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/sys/kern/kern_sig.c	Mon Oct 21 01:24:21 2019	(r353789)
@@ -986,12 +986,7 @@ execsigs(struct proc *p)
 	PROC_LOCK_ASSERT(p, MA_OWNED);
 	ps = p->p_sigacts;
 	mtx_lock(&ps->ps_mtx);
-	while (SIGNOTEMPTY(ps->ps_sigcatch)) {
-		sig = sig_ffs(&ps->ps_sigcatch);
-		sigdflt(ps, sig);
-		if ((sigprop(sig) & SIGPROP_IGNORE) != 0)
-			sigqueue_delete_proc(p, sig);
-	}
+	sig_drop_caught(p);
 
 	/*
 	 * As CloudABI processes cannot modify signal handlers, fully
@@ -3855,4 +3850,21 @@ sigacts_shared(struct sigacts *ps)
 {
 
 	return (ps->ps_refcnt > 1);
+}
+
+void
+sig_drop_caught(struct proc *p)
+{
+	int sig;
+	struct sigacts *ps;
+
+	ps = p->p_sigacts;
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+	mtx_assert(&ps->ps_mtx, MA_OWNED);
+	while (SIGNOTEMPTY(ps->ps_sigcatch)) {
+		sig = sig_ffs(&ps->ps_sigcatch);
+		sigdflt(ps, sig);
+		if ((sigprop(sig) & SIGPROP_IGNORE) != 0)
+			sigqueue_delete_proc(p, sig);
+	}
 }

Modified: stable/12/sys/sys/proc.h
==============================================================================
--- stable/12/sys/sys/proc.h	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/sys/sys/proc.h	Mon Oct 21 01:24:21 2019	(r353789)
@@ -1002,6 +1002,8 @@ struct	fork_req {
 	int 		*fr_pd_fd;
 	int 		fr_pd_flags;
 	struct filecaps	*fr_pd_fcaps;
+	int 		fr_flags2;
+#define	FR2_DROPSIG_CAUGHT	0x00001	/* Drop caught non-DFL signals */
 };
 
 /*

Modified: stable/12/sys/sys/signalvar.h
==============================================================================
--- stable/12/sys/sys/signalvar.h	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/sys/sys/signalvar.h	Mon Oct 21 01:24:21 2019	(r353789)
@@ -381,6 +381,7 @@ void	sigacts_copy(struct sigacts *dest, struct sigacts
 void	sigacts_free(struct sigacts *ps);
 struct sigacts *sigacts_hold(struct sigacts *ps);
 int	sigacts_shared(struct sigacts *ps);
+void	sig_drop_caught(struct proc *p);
 void	sigexit(struct thread *td, int sig) __dead2;
 int	sigev_findtd(struct proc *p, struct sigevent *sigev, struct thread **);
 int	sig_ffs(sigset_t *set);

Modified: stable/12/sys/sys/unistd.h
==============================================================================
--- stable/12/sys/sys/unistd.h	Mon Oct 21 00:52:21 2019	(r353788)
+++ stable/12/sys/sys/unistd.h	Mon Oct 21 01:24:21 2019	(r353789)
@@ -188,11 +188,14 @@
 #define	RFTSIGNUM(flags)	(((flags) >> RFTSIGSHIFT) & RFTSIGMASK)
 #define	RFTSIGFLAGS(signum)	((signum) << RFTSIGSHIFT)
 #define	RFPROCDESC	(1<<28)	/* return a process descriptor */
-#define	RFPPWAIT	(1<<31)	/* parent sleeps until child exits (vfork) */
+/* kernel: parent sleeps until child exits (vfork) */
+#define	RFPPWAIT	(1<<31)
+/* user: vfork(2) semantics, clear signals */
+#define	RFSPAWN		(1U<<31)
 #define	RFFLAGS		(RFFDG | RFPROC | RFMEM | RFNOWAIT | RFCFDG | \
     RFTHREAD | RFSIGSHARE | RFLINUXTHPN | RFSTOPPED | RFHIGHPID | RFTSIGZMB | \
-    RFPROCDESC | RFPPWAIT)
-#define	RFKERNELONLY	(RFSTOPPED | RFHIGHPID | RFPPWAIT | RFPROCDESC)
+    RFPROCDESC | RFSPAWN | RFPPWAIT)
+#define	RFKERNELONLY	(RFSTOPPED | RFHIGHPID | RFPROCDESC)
 
 #endif /* __BSD_VISIBLE */
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201910210124.x9L1ONpm075292>