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>