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

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

Phillip Wood [off-list ref] writes:

quoted

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