On 04/13, Eric Wong wrote: > Brandon Williams <bmw...@google.com> wrote: > > v2 does a bit of restructuring based on comments from reviewers. I took the > > patch by Eric and broke it up and tweaked it a bit to flow better with v2. > > I > > left out the part of Eric's patch which did signal manipulation as I wasn't > > experienced enough to know what it was doing or why it was necessary. > > Though I > > believe the code is structured in such a way that Eric could make another > > patch > > on top of this series with just the signal changes. > > Yeah, I think a separate commit message might be necessary to > explain the signal changes.
Perfect! I'll carry the changes along in the reroll. > > -------8<----- > Subject: [PATCH] run-command: block signals between fork and execve > > Signal handlers of the parent firing in the forked child may > have unintended side effects. Rather than auditing every signal > handler we have and will ever have, block signals while forking > and restore default signal handlers in the child before execve. > > Restoring default signal handlers is required because > execve does not unblock signals, it only restores default > signal handlers. So we must restore them with sigprocmask > before execve, leaving a window when signal handlers > we control can fire in the child. Continue ignoring > ignored signals, but reset the rest to defaults. > > Similarly, disable pthread cancellation to future-proof our code > in case we start using cancellation; as cancellation is > implemented with signals in glibc. > > Signed-off-by: Eric Wong <e...@80x24.org> > --- > Changes from my original in <20170411070534.GA10552@whir>: > > - fixed typo in NO_PTHREADS code > > - dropped fflush(NULL) before fork, consider us screwed anyways > if the child uses stdio > > - respect SIG_IGN in child; that seems to be the prevailing > wisdom from reading https://ewontfix.com/7/ and process.c > in ruby (git clone https://github.com/ruby/ruby.git) > > run-command.c | 69 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/run-command.c b/run-command.c > index 1c36e692d..59a8b4806 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -213,6 +213,7 @@ static int child_notifier = -1; > > enum child_errcode { > CHILD_ERR_CHDIR, > + CHILD_ERR_SIGPROCMASK, > CHILD_ERR_ENOENT, > CHILD_ERR_SILENT, > CHILD_ERR_ERRNO, > @@ -277,6 +278,8 @@ static void child_err_spew(struct child_process *cmd, > struct child_err *cerr) > error_errno("exec '%s': cd to '%s' failed", > cmd->argv[0], cmd->dir); > break; > + case CHILD_ERR_SIGPROCMASK: > + error_errno("sigprocmask failed restoring signals"); > case CHILD_ERR_ENOENT: > error_errno("cannot run %s", cmd->argv[0]); > break; > @@ -388,6 +391,53 @@ static char **prep_childenv(const char *const *deltaenv) > } > #endif > > +struct atfork_state { > +#ifndef NO_PTHREADS > + int cs; > +#endif > + sigset_t old; > +}; > + > +#ifndef NO_PTHREADS > +static void bug_die(int err, const char *msg) > +{ > + if (err) { > + errno = err; > + die_errno("BUG: %s", msg); > + } > +} > +#endif > + > +static void atfork_prepare(struct atfork_state *as) > +{ > + sigset_t all; > + > + if (sigfillset(&all)) > + die_errno("sigfillset"); > +#ifdef NO_PTHREADS > + if (sigprocmask(SIG_SETMASK, &all, &as->old)) > + die_errno("sigprocmask"); > +#else > + bug_die(pthread_sigmask(SIG_SETMASK, &all, &as->old), > + "blocking all signals"); > + bug_die(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &as->cs), > + "disabling cancellation"); > +#endif > +} > + > +static void atfork_parent(struct atfork_state *as) > +{ > +#ifdef NO_PTHREADS > + if (sigprocmask(SIG_SETMASK, &as->old, NULL)) > + die_errno("sigprocmask"); > +#else > + bug_die(pthread_setcancelstate(as->cs, NULL), > + "re-enabling cancellation"); > + bug_die(pthread_sigmask(SIG_SETMASK, &as->old, NULL), > + "restoring signal mask"); > +#endif > +} > + > static inline void set_cloexec(int fd) > { > int flags = fcntl(fd, F_GETFD); > @@ -511,6 +561,7 @@ int start_command(struct child_process *cmd) > char **childenv; > struct argv_array argv = ARGV_ARRAY_INIT; > struct child_err cerr; > + struct atfork_state as; > > if (pipe(notify_pipe)) > notify_pipe[0] = notify_pipe[1] = -1; > @@ -524,6 +575,7 @@ int start_command(struct child_process *cmd) > > prepare_cmd(&argv, cmd); > childenv = prep_childenv(cmd->env); > + atfork_prepare(&as); > > /* > * NOTE: In order to prevent deadlocking when using threads special > @@ -537,6 +589,7 @@ int start_command(struct child_process *cmd) > cmd->pid = fork(); > failed_errno = errno; > if (!cmd->pid) { > + int sig; > /* > * Ensure the default die/error/warn routines do not get > * called, they can take stdio locks and malloc. > @@ -584,6 +637,21 @@ int start_command(struct child_process *cmd) > if (cmd->dir && chdir(cmd->dir)) > child_die(CHILD_ERR_CHDIR); > > + /* > + * restore default signal handlers here, in case > + * we catch a signal right before execve below > + */ > + for (sig = 1; sig < NSIG; sig++) { > + sighandler_t old = signal(sig, SIG_DFL); > + > + /* ignored signals get reset to SIG_DFL on execve */ > + if (old == SIG_IGN) > + signal(sig, SIG_IGN); > + } > + > + if (sigprocmask(SIG_SETMASK, &as.old, NULL) != 0) > + child_die(CHILD_ERR_SIGPROCMASK); > + > execve(argv.argv[0], (char *const *) argv.argv, > (char *const *) childenv); > > @@ -595,6 +663,7 @@ int start_command(struct child_process *cmd) > child_die(CHILD_ERR_ERRNO); > } > } > + atfork_parent(&as); > if (cmd->pid < 0) > error_errno("cannot fork() for %s", cmd->argv[0]); > else if (cmd->clean_on_exit) > -- > EW -- Brandon Williams
