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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 11, 2023, 09:50 UTC
Message-ID: <c64cd85f-f14c-46b7-a0d3-b8e0bfc60053@gmail.com>
In-Reply-To: <xmqq8r9gtviv.fsf@gitster.g>

On 08/09/2023 18:38, Junio C Hamano wrote:

Show 11 quoted lines

> 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