Re: [PATCH] rebase -i: ignore signals when forking subprocesses

[Phillip Wood <phillip.wood123@gmail.com>]

On 10/09/2023 11:05, Oswald Buddenhagen wrote:
>> The child not dying is tricky, if it is in the same process group as 
>> git then even if git dies the I think the shell will wait for the 
>> child to exit before showing the prompt again so it is not clear to me 
>> that the user is disadvantaged by git ignoring SIGINT in that case.
>>
> there is no such thing as waiting for grandchildren. the grandchild is 
> reparented to init when the child exits.
> 
> there is a situation were one can be deadlocked by a non-exiting 
> grandchild: when doing a blocking read of the child's output past its 
> exit, when the grandchild has inherited stdout. but that's a 
> implementation bug in the parent. and not relevant here.

Yes I got carried away and thought that the shell waited for all the 
processes in the foreground process group, but it can only wait on those 
processes that it created.

> On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>> On 08/09/2023 10:59, Phillip Wood wrote:
>>>> I've never done it before, but from my reading we basically want to do
>>>> (in the forked process before we exec):
>>>>
>>>>    setsid();
>>>>    open("/dev/tty");
>>>
>>> Do we want a whole new session? As I understand it to launch a 
>>> foreground job shells put the child in its own process group and then 
>>> call tcsetpgrp() to change the foreground process group of the 
>>> controlling terminal to that of the child.
>>
> this would indeed be the right way if we wanted to isolate the children 
> more, but ...
> 
>> It is better for handling SIGINT and SIGQUIT when we don't want git to 
>> be killed but in complicates the handling of SIGTSTP and friends. [...]
>>
> ... this shows that we really don't want that; we don't want to 
> replicate interactive shell behavior. that is even before the divergence 
> on windows.

Yeah, I'm not enthusiastic about emulating the shell's job control in git.

> so i think your patch is approaching things the right way.
> though blocking signals doesn't appear right - to ensure git's own clean 
> exit while it has no children, it must catch sigint anyway, and 
> temporarily ignoring it around spawning children sounds racy.

There is an inevitable race between wait() returning and calling 
signal() to restore the handlers for SIGINT and SIGQUIT, it is such a 
small window I'm not sure it is a problem in practice. There is also a 
race when creating the child but if we block signals before calling 
fork, then ignore SIGINT and SIGQUIT in the parent before unblocking 
them we're OK because the child will be killed as soon as it unblocks 
signal by any signal received while the signals were blocks and we'll 
detect that in the parent and exit. Currently editor.c just ignores the 
signals after fork has returned in the parent which means it is 
theoretically  possible to kill git with SIGINT while the child is running.

Best Wishes

Phillip