| Message ID | pull.1582.git.1694167540231.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | start_command: reset disposition of all signals in child | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > [3] This is really a work-around for not moving the child into its own > process group and changing the foreground process group of the > controlling terminal. I am puzzled, as I somehow thought that "does the user conceive a subprocess as external and different-from-git entity, or is it merely an implementation detail? many use of subprocesses in our codebase, it is the latter." from Peff was a good argument against such isolation between spawning "git" and spawned subprocesses.
On 08/09/2023 16:42, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> [3] This is really a work-around for not moving the child into its own >> process group and changing the foreground process group of the >> controlling terminal. > > I am puzzled, as I somehow thought that "does the user conceive a > subprocess as external and different-from-git entity, or is it > merely an implementation detail? many use of subprocesses in our > codebase, it is the latter." from Peff was a good argument against > such isolation between spawning "git" and spawned subprocesses. It is and in those cases we do not ignore SIGINT and SIGQUIT in the parent when we fork the subprocess. What I was trying to say is that in the few cases where we do ignore SIGINT and SIGQUIT in the parent when we fork a subprocess we're working round the child being in the same process group at the parent. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > On 08/09/2023 16:42, Junio C Hamano wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> [3] This is really a work-around for not moving the child into its own >>> process group and changing the foreground process group of the >>> controlling terminal. >> I am puzzled, as I somehow thought that "does the user conceive a >> subprocess as external and different-from-git entity, or is it >> merely an implementation detail? many use of subprocesses in our >> codebase, it is the latter." from Peff was a good argument against >> such isolation between spawning "git" and spawned subprocesses. > > It is and in those cases we do not ignore SIGINT and SIGQUIT in the > parent when we fork the subprocess. What I was trying to say is that > in the few cases where we do ignore SIGINT and SIGQUIT in the parent > when we fork a subprocess we're working round the child being in the > same process group at the parent. Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at random, the typical pattern seem to be (this example was taken from builtin/receive-pack.c): code = start_command(&proc); if (code) { ... return code; } sigchain_push(SIGPIPE, SIG_IGN); while (1) { ... } close(proc.in); sigchain_pop(SIGPIPE); return finish_command(&proc); The way we spawn an editor in editor.c looks the same: p.use_shell = 1; p.trace2_child_class = "editor"; if (start_command(&p) < 0) { strbuf_release(&realpath); return error("unable to start editor '%s'", editor); } sigchain_push(SIGINT, SIG_IGN); sigchain_push(SIGQUIT, SIG_IGN); ret = finish_command(&p); IOW, we do not ignore then spawn. We spawn and ignore only in the parent, so there shouldn't be any reason to worry about our child inheriting the "we the parent git process do not want to be killed by \C-c" settings, should there? I have a vague recollection that the "propagate what was already ignored to be ignored down to the child, too" was not about signals we ignored, but inherited from the end-user who started git with certain signals ignored, but it is so old a piece of code that the details of the rationale escapes me.
On 08/09/2023 17:24, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> On 08/09/2023 16:42, Junio C Hamano wrote: >>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> [3] This is really a work-around for not moving the child into its own >>>> process group and changing the foreground process group of the >>>> controlling terminal. >>> I am puzzled, as I somehow thought that "does the user conceive a >>> subprocess as external and different-from-git entity, or is it >>> merely an implementation detail? many use of subprocesses in our >>> codebase, it is the latter." from Peff was a good argument against >>> such isolation between spawning "git" and spawned subprocesses. >> >> It is and in those cases we do not ignore SIGINT and SIGQUIT in the >> parent when we fork the subprocess. What I was trying to say is that >> in the few cases where we do ignore SIGINT and SIGQUIT in the parent >> when we fork a subprocess we're working round the child being in the >> same process group at the parent. > > Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at > random, the typical pattern seem to be (this example was taken from > builtin/receive-pack.c): > > code = start_command(&proc); > if (code) { > ... > return code; > } > sigchain_push(SIGPIPE, SIG_IGN); > while (1) { > ... > } > close(proc.in); > sigchain_pop(SIGPIPE); > return finish_command(&proc); > > The way we spawn an editor in editor.c looks the same: > > p.use_shell = 1; > p.trace2_child_class = "editor"; > if (start_command(&p) < 0) { > strbuf_release(&realpath); > return error("unable to start editor '%s'", editor); > } > > sigchain_push(SIGINT, SIG_IGN); > sigchain_push(SIGQUIT, SIG_IGN); > ret = finish_command(&p); > > IOW, we do not ignore then spawn. We spawn and ignore only in the > parent, so there shouldn't be any reason to worry about our child > inheriting the "we the parent git process do not want to be killed > by \C-c" settings, should there? Oh I should have looked more carefully at the existing uses. It looks like it is only my sequencer patch that does sigchain_push(SIGINT, SIG_IGN); sigchain_push(SIGQUIT, SIG_IGN); res = run_command(...); In that case the existing behavior is a problem but maybe I should change those call sites to use start_command() and finish_command() instead if we decide that other patch is a good idea. > I have a vague recollection that the "propagate what was already > ignored to be ignored down to the child, too" was not about signals > we ignored, but inherited from the end-user who started git with > certain signals ignored, but it is so old a piece of code that the > details of the rationale escapes me. The comment /* ignored signals get reset to SIG_DFL on execve */ in start_command() makes it look like the code assumes ignored signals will be reset to SIG_DFL by execve() which is not what happens. Maybe that comment is just wrong and there is a good reason for the current behavior. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Oh I should have looked more carefully at the existing uses. It looks > like it is only my sequencer patch that does > > sigchain_push(SIGINT, SIG_IGN); > sigchain_push(SIGQUIT, SIG_IGN); > res = run_command(...); Hmph, does it mean this patch would become unnecessary, once you fix the above sequence to follow the pattern "to spawn and then ignore"?
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>
On 08/09/2023 18:38, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Oh I should have looked more carefully at the existing uses. It looks >> like it is only my sequencer patch that does >> >> sigchain_push(SIGINT, SIG_IGN); >> sigchain_push(SIGQUIT, SIG_IGN); >> res = run_command(...); > > Hmph, does it mean this patch would become unnecessary, once you fix > the above sequence to follow the pattern "to spawn and then ignore"? Yes, sorry for the confusion. There are a couple of things that I think we should address though. Firstly we should change the comment in run-command which says execve() resets ignored signals to SIG_DFL to say something like Preserve the set of ignored signals so that running git via a wrapper like nohup works as the user expects The other thing is that we have some instances where we ignore SIGPIPE before calling start_command() which means we're ignoring it in the child process as well. For example in gpg-interface.c we have sigchain_push(SIGPIPE, SIG_IGN); ret = pipe_command(&gpg, sigc->payload, sigc->payload_len, &gpg_stdout, 0, &gpg_stderr, 0); sigchain_pop(SIGPIPE); To fix that one we'd need to change pipe_command() to ignore SIGPIPE after calling start_command() or add a flag to struct child_process to do to that. Another example is in upload-pack.c /* * If the next rev-list --stdin encounters an unknown commit, * it terminates, which will cause SIGPIPE in the write loop * below. */ sigchain_push(SIGPIPE, SIG_IGN); if (start_command(cmd)) goto error; rev-list does not check for errors when writing to stdout unless GIT_FLUSH is set in the environment so if parent process exits early rev-list will keep going until it thinks it has printed everything. I think adding a flag to struct child_process to ignore SIGPIPE in the parent is probably the best way to avoid problems like this. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Preserve the set of ignored signals so that running git via a > wrapper like nohup works as the user expects OK. > The other thing is that we have some instances where we ignore SIGPIPE > before calling start_command() which means we're ignoring it in the > child process as well. For example in gpg-interface.c we have > ... > rev-list does not check for errors when writing to stdout unless > GIT_FLUSH is set in the environment so if parent process exits early > rev-list will keep going until it thinks it has printed everything. Ahh, yeah, that is bad. Thanks for pointing them out. > I think adding a flag to struct child_process to ignore SIGPIPE in the > parent is probably the best way to avoid problems like this. OK. SIGPIPE is special enough that it may deserve to be singled out. A pager reading from us quitting does not have to be "we wanted to write more but got an error" but just "ok, if the user does not want any more output, that is fine". THanks.
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); if (sigprocmask(SIG_SETMASK, &as.old, NULL) != 0) child_die(CHILD_ERR_SIGPROCMASK);