Re: [PATCH 7/6] run-command: block signals between fork and execve

[Brandon Williams <bmwill@google.com>]

On 04/13, Eric Wong wrote:
> Brandon Williams <bmwill@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