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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 11, 2023, 10:00 UTC
Message-ID: <80eb7631-e5c0-497e-b2a9-b1f8c8a4a306@gmail.com>
In-Reply-To: <ZP2U8TBNjKs5ebky@ugly>

On 10/09/2023 11:05, Oswald Buddenhagen wrote:

Show 12 quoted lines

>> 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.

Show 22 quoted lines

> 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