Phillip Wood via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > 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@xxxxxxxxx>
