threads / patch / 29674

patchIgnore SIGPIPE when running a filter driver

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

## tl;dr

5 messages between Feb 20, 2012 and Feb 21, 2012. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Jehan Bing· Feb 20, 2012, 20:53 UTC · lore

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>
---
Since it's not really a problem in the "required-filter" patch but a
general one with filter drivers, I'm submitting this patch
independently. I'm also wording it as a pre-patch to "required-filter".
-Jehan
 convert.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Show changes to convert.c +5 −0
diff --git a/convert.c b/convert.c
index c06309f..5d312cb 100644
--- a/convert.c
+++ b/convert.c
@@ -2,6 +2,7 @@
 #include "attr.h"
 #include "run-command.h"
 #include "quote.h"
+#include "sigchain.h"
 
 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -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);
+
 	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);
+
 	status = finish_command(&child_process);
 	if (status)
 		error("external filter %s failed %d", params->cmd, status);

-- 
1.7.9
Junio C Hamano· Feb 20, 2012, 22:11 UTC · re: Jehan Bing · lore

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

Jehan Bing <jehan@orb.com> writes:
Show 26 quoted lines
> diff --git a/convert.c b/convert.c
> index c06309f..5d312cb 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -2,6 +2,7 @@
>  #include "attr.h"
>  #include "run-command.h"
>  #include "quote.h"
> +#include "sigchain.h"
>  
>  /*
>   * convert.c - convert a file when checking it out and checking it in.
> @@ -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);
> +
>  	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);
> +
Thanks.

I think this is OK on a POSIX system where this function is run by start_async() which is implemented with a forked child process.

I do not now if it poses a issue on Windows, though. Johannes, any comments?

Johannes Sixt· Feb 21, 2012, 19:20 UTC · re: Junio C Hamano · lore

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

Am 20.02.2012 23:11, schrieb Junio C Hamano:
Show 23 quoted lines
> Jehan Bing <jehan@orb.com> writes:
>> @@ -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);
>> +
>>  	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);
>> +
> 
> Thanks.
> 
> I think this is OK on a POSIX system where this function is run by
> start_async() which is implemented with a forked child process.
> 
> I do not now if it poses a issue on Windows, though.  Johannes, any
> comments?
I do not expect the change to cause a problem on Windows.
-- Hannes
Jonathan Nieder· Feb 21, 2012, 03:01 UTC · re: Jehan Bing · lore

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

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

Junio C Hamano· Feb 21, 2012, 20:58 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 16 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.
Yes.  
You could rephrase  bit better to further clarify it, perhaps like this:
    Ignore SIGPIPE when running a filter driver
    
    If a filter is not defined or if it fails, git should behave as if the
    filter is a no-op passthru.
    
    However, if the filter exits before reading all the content, depending on
    the timing, git could be killed with SIGPIPE when it tries to write to the
    pipe connected to the filter.
    
    Ignore SIGPIPE while processing the filter to give us a chance to check
    the return value from a failed write, in order to detect and act on this
    mode of failure in a more controlled way.
    
    Signed-off-by: Jehan Bing <jehan@orb.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
although I think Jehan's original was already clear enough.
> So at least on POSIX-y platforms, this patch looks good to me.  Thanks
> for writing it.
Thank you and Johannes for eyeballing and sanity checking.
Will queue.

← back to recent threads