Re: [PATCH] Ignore SIGPIPE when running a filter driver

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jehan Bing wrote:

> If a filter is not defined or if it fails, git behaves as if the filter
> is a no-op passthru. However, if the filter exits before reading all
> the content, and depending on the timing git, could be kill with
> SIGPIPE instead.
>
> Ignore SIGPIPE while processing the filter to detect when it exits
> early and fallback to using the unfiltered content.
>
> Signed-off-by: Jehan Bing <jehan@orb.com>

For the benefit of the uninitiated ("how would ignoring an error help
me detect an error?"): setting the SIGPIPE handler to SIG_IGN does not
actually ignore the broken pipe condition but causes it to be reported
as an I/O error, errno == EPIPE.  That means instead of being killed
by SIGPIPE, git gets to fall back to passthrough and report the
filter's mistake:

	error: cannot feed the input to external filter <foo>
	error: external filter <foo> failed

[...]
> +++ b/convert.c
[...]
> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)
>  	if (start_command(&child_process))
>  		return error("cannot fork to run external filter %s", params->cmd);
>  
> +	sigchain_push(SIGPIPE, SIG_IGN);

Setting the signal disposition after launching the external filter
which would otherwise inherit it, so the filter does not have to cope
with unfamiliar SIGPIPE handling[*].  Phew.

> +
>  	write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
>  	if (close(child_process.in))
>  		write_err = 1;
>  	if (write_err)
>  		error("cannot feed the input to external filter %s", params->cmd);
>  
> +	sigchain_pop(SIGPIPE);
> +

This happens in an async procedure.  SIGPIPE is ignored in the
following block in the other thread:

	if (strbuf_read(&nbuf, async.out, len) < 0) {
		error("read from external filter %s failed", cmd);
		ret = 0;
	}
	if (close(async.out)) {
		error("read from external filter %s failed", cmd);
		ret = 0;
	}
	if (finish_async(&async)) {

That implies a tiny behavior change: if there is an I/O error reading
from async.out at the right moment and stderr is going to a closed
pipe, inability to report the error can result in the error flag being
set on stderr instead of the process being killed.  I don't think
anyone will notice.

So at least on POSIX-y platforms, this patch looks good to me.  Thanks
for writing it.

Sincerely,
Jonathan

[*] See http://bugs.python.org/issue1652 for some stories about what
we are narrowly escaping here. :)