Re: [PATCH] start_command: reset disposition of all signals in child

[Eric Wong <e@80x24.org>]

Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> In order to avoid invoking a signal handler in the child process between
> fork() and exec(), start_command() blocks all signals before calling
> fork and then in the child resets the disposition of all signals that
> are not ignored to SIG_DFL before unblocking them. The exception for
> ignored signals seems to been inspired by ruby's process handling[1]
> based on the misconception that execve() will reset them to
> SIG_DFL. Unfortunately this is not the case [2] and any signals that are
> ignored in the parent will default to being ignored by the program
> executed by start_command().
> 
> When git ignores SIGPIPE before forking a child process it is to stop
> git from being killed if the child exits while git is writing to the
> child's stdin. We do not want to ignore SIGPIPE in the child. When git
> ignores SIGINT and SIGQUIT before forking a child process it is to stop
> git from being killed if the user interrupts the child with Ctrl-C or
> Ctrl-\ we do not want the child to ignore those signals [3].
> Fortunately the fix is easy - reset the disposition of all signals
> regardless of their previous disposition.
> 
> [1] https://lore.kernel.org/git/20170413211428.GA5961@whir/
> 
> [2] The man page for execve(2) states:
> 
>         POSIX.1 specifies that the dispositions of any signals that are
> 	ignored or set to the default are left unchanged.  POSIX.1
> 	specifies one exception: if SIGCHLD is being ignored, then an
> 	implementation may leave the disposition unchanged or reset it
> 	to the default; Linux does the former.

Yeah, the old code seems like an error on my part.  Oops :x

> diff --git a/run-command.c b/run-command.c
> index a558042c876..765775a1f42 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -823,11 +823,8 @@ fail_pipe:
>  		 * restore default signal handlers here, in case
>  		 * we catch a signal right before execve below
>  		 */
> -		for (sig = 1; sig < NSIG; sig++) {
> -			/* ignored signals get reset to SIG_DFL on execve */
> -			if (signal(sig, SIG_DFL) == SIG_IGN)
> -				signal(sig, SIG_IGN);
> -		}
> +		for (sig = 1; sig < NSIG; sig++)
> +			signal(sig, SIG_DFL);

Looks good to me and matches what I did in some other (A)GPL-3
projects, actually.

Acked-by: Eric Wong <e@80x24.org>