svn commit: r352712 - in head/lib/libc: gen sys

[Kyle Evans]

Author: kevans
Date: Wed Sep 25 19:22:03 2019
New Revision: 352712
URL: https://svnweb.freebsd.org/changeset/base/352712

Log:
  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
  
  Reviewed by:	jilles, kib
  Differential Revision:	https://reviews.freebsd.org/D19058

Modified:
  head/lib/libc/gen/posix_spawn.c
  head/lib/libc/sys/rfork.2

Modified: head/lib/libc/gen/posix_spawn.c
==============================================================================
--- head/lib/libc/gen/posix_spawn.c	Wed Sep 25 19:20:41 2019	(r352711)
+++ head/lib/libc/gen/posix_spawn.c	Wed Sep 25 19:22:03 2019	(r352712)
@@ -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: head/lib/libc/sys/rfork.2
==============================================================================
--- head/lib/libc/sys/rfork.2	Wed Sep 25 19:20:41 2019	(r352711)
+++ head/lib/libc/sys/rfork.2	Wed Sep 25 19:22:03 2019	(r352712)
@@ -113,6 +113,9 @@ is passed,
 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