[PATCH] Ignore SIGPIPE when running a filter driver

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 <redacted>
---
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(-)

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

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 <redacted>

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

[...]

[...]

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