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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 21, 2012, 03:01 UTC
Message-ID: <20120221030150.GA31737@burratino>
In-Reply-To: <1329771217-9088-1-git-send-email-jehan@orb.com>

Hi,

Jehan Bing wrote:

Show 9 quoted lines

> 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

[...]

Show 5 quoted lines

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

Show 9 quoted lines

> +
>  	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. :)