{"thread":{"id":"29674","subject":"[PATCH] Ignore SIGPIPE when running a filter driver","startedAt":"2012-02-20T20:53:37Z","lastAt":"2012-02-21T20:58:53Z","messageCount":5,"participants":["Jehan Bing","Junio C Hamano","Jonathan Nieder","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185023","messageId":"1329771217-9088-1-git-send-email-jehan@orb.com","threadId":"29674","inReplyTo":null,"subject":"[PATCH] Ignore SIGPIPE when running a filter driver","fromName":"Jehan Bing","fromEmail":"jehan@orb.com","sentAt":"2012-02-20T20:53:37Z","receivedAt":"2012-02-20T20:53:37Z","isPatch":true,"sender":{"key":"jehan@orb.com","avatar":null},"body":"If a filter is not defined or if it fails, git behaves as if the filter\nis a no-op passthru. However, if the filter exits before reading all\nthe content, and depending on the timing git, could be kill with\nSIGPIPE instead.\n\nIgnore SIGPIPE while processing the filter to detect when it exits\nearly and fallback to using the unfiltered content.\n\nSigned-off-by: Jehan Bing \n---\nSince it's not really a problem in the \"required-filter\" patch but a\ngeneral one with filter drivers, I'm submitting this patch\nindependently. I'm also wording it as a pre-patch to \"required-filter\".\n\n-Jehan\n\n convert.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex c06309f..5d312cb 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -2,6 +2,7 @@\n #include \"attr.h\"\n #include \"run-command.h\"\n #include \"quote.h\"\n+#include \"sigchain.h\"\n \n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)\n \tif (start_command(&child_process))\n \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n \n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n \twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n \t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n \n+\tsigchain_pop(SIGPIPE);\n+\n \tstatus = finish_command(&child_process);\n \tif (status)\n \t\terror(\"external filter %s failed %d\", params->cmd, status);\n-- \n1.7.9\n"},{"id":"185041","messageId":"7vsji5jgtv.fsf@alter.siamese.dyndns.org","threadId":"29674","inReplyTo":"1329771217-9088-1-git-send-email-jehan@orb.com","subject":"Re: [PATCH] Ignore SIGPIPE when running a filter driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-20T22:11:24Z","receivedAt":"2012-02-20T22:11:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jehan Bing writes:\n\n> diff --git a/convert.c b/convert.c\n> index c06309f..5d312cb 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -2,6 +2,7 @@\n> #include \"attr.h\"\n> #include \"run-command.h\"\n> #include \"quote.h\"\n> +#include \"sigchain.h\"\n> \n> /*\n> * convert.c - convert a file when checking it out and checking it in.\n> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)\n> \tif (start_command(&child_process))\n> \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n> \n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> \twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n> \tif (close(child_process.in))\n> \t\twrite_err = 1;\n> \tif (write_err)\n> \t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n> \n> +\tsigchain_pop(SIGPIPE);\n> +\n\nThanks.\n\nI think this is OK on a POSIX system where this function is run by\nstart_async() which is implemented with a forked child process.\n\nI do not now if it poses a issue on Windows, though. Johannes, any\ncomments?\n"},{"id":"185051","messageId":"20120221030150.GA31737@burratino","threadId":"29674","inReplyTo":"1329771217-9088-1-git-send-email-jehan@orb.com","subject":"Re: [PATCH] Ignore SIGPIPE when running a filter driver","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-21T03:01:50Z","receivedAt":"2012-02-21T03:01:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJehan Bing wrote:\n\n> If a filter is not defined or if it fails, git behaves as if the filter\n> is a no-op passthru. However, if the filter exits before reading all\n> the content, and depending on the timing git, could be kill with\n> SIGPIPE instead.\n>\n> Ignore SIGPIPE while processing the filter to detect when it exits\n> early and fallback to using the unfiltered content.\n>\n> Signed-off-by: Jehan Bing \n\nFor the benefit of the uninitiated (\"how would ignoring an error help\nme detect an error?\"): setting the SIGPIPE handler to SIG_IGN does not\nactually ignore the broken pipe condition but causes it to be reported\nas an I/O error, errno == EPIPE. That means instead of being killed\nby SIGPIPE, git gets to fall back to passthrough and report the\nfilter's mistake:\n\n\terror: cannot feed the input to external filter \n\terror: external filter failed\n\n[...]\n> +++ b/convert.c\n[...]\n> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)\n> \tif (start_command(&child_process))\n> \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n> \n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n\nSetting the signal disposition after launching the external filter\nwhich would otherwise inherit it, so the filter does not have to cope\nwith unfamiliar SIGPIPE handling[*]. Phew.\n\n> +\n> \twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n> \tif (close(child_process.in))\n> \t\twrite_err = 1;\n> \tif (write_err)\n> \t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n> \n> +\tsigchain_pop(SIGPIPE);\n> +\n\nThis happens in an async procedure. SIGPIPE is ignored in the\nfollowing block in the other thread:\n\n\tif (strbuf_read(&nbuf, async.out, len) < 0) {\n\t\terror(\"read from external filter %s failed\", cmd);\n\t\tret = 0;\n\t}\n\tif (close(async.out)) {\n\t\terror(\"read from external filter %s failed\", cmd);\n\t\tret = 0;\n\t}\n\tif (finish_async(&async)) {\n\nThat implies a tiny behavior change: if there is an I/O error reading\nfrom async.out at the right moment and stderr is going to a closed\npipe, inability to report the error can result in the error flag being\nset on stderr instead of the process being killed. I don't think\nanyone will notice.\n\nSo at least on POSIX-y platforms, this patch looks good to me. Thanks\nfor writing it.\n\nSincerely,\nJonathan\n\n[*] See http://bugs.python.org/issue1652 for some stories about what\nwe are narrowly escaping here. :)\n"},{"id":"185096","messageId":"4F43EE86.3000307@kdbg.org","threadId":"29674","inReplyTo":"7vsji5jgtv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Ignore SIGPIPE when running a filter driver","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-02-21T19:20:38Z","receivedAt":"2012-02-21T19:20:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.02.2012 23:11, schrieb Junio C Hamano:\n> Jehan Bing writes:\n>> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)\n>> \tif (start_command(&child_process))\n>> \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\n>> \n>> +\tsigchain_push(SIGPIPE, SIG_IGN);\n>> +\n>> \twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n>> \tif (close(child_process.in))\n>> \t\twrite_err = 1;\n>> \tif (write_err)\n>> \t\terror(\"cannot feed the input to external filter %s\", params->cmd);\n>> \n>> +\tsigchain_pop(SIGPIPE);\n>> +\n> \n> Thanks.\n> \n> I think this is OK on a POSIX system where this function is run by\n> start_async() which is implemented with a forked child process.\n> \n> I do not now if it poses a issue on Windows, though. Johannes, any\n> comments?\n\nI do not expect the change to cause a problem on Windows.\n\n-- Hannes\n"},{"id":"185102","messageId":"7vlinvgaya.fsf@alter.siamese.dyndns.org","threadId":"29674","inReplyTo":"20120221030150.GA31737@burratino","subject":"Re: [PATCH] Ignore SIGPIPE when running a filter driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T20:58:53Z","receivedAt":"2012-02-21T20:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> If a filter is not defined or if it fails, git behaves as if the filter\n>> is a no-op passthru. However, if the filter exits before reading all\n>> the content, and depending on the timing git, could be kill with\n>> SIGPIPE instead.\n>>\n>> Ignore SIGPIPE while processing the filter to detect when it exits\n>> early and fallback to using the unfiltered content.\n>>\n>> Signed-off-by: Jehan Bing \n>\n> For the benefit of the uninitiated (\"how would ignoring an error help\n> me detect an error?\"): setting the SIGPIPE handler to SIG_IGN does not\n> actually ignore the broken pipe condition but causes it to be reported\n> as an I/O error, errno == EPIPE. That means instead of being killed\n> by SIGPIPE, git gets to fall back to passthrough and report the\n> filter's mistake.\n\nYes. \n\nYou could rephrase bit better to further clarify it, perhaps like this:\n\n Ignore SIGPIPE when running a filter driver\n \n If a filter is not defined or if it fails, git should behave as if the\n filter is a no-op passthru.\n \n However, if the filter exits before reading all the content, depending on\n the timing, git could be killed with SIGPIPE when it tries to write to the\n pipe connected to the filter.\n \n Ignore SIGPIPE while processing the filter to give us a chance to check\n the return value from a failed write, in order to detect and act on this\n mode of failure in a more controlled way.\n \n Signed-off-by: Jehan Bing \n Signed-off-by: Junio C Hamano \n\nalthough I think Jehan's original was already clear enough.\n\n> So at least on POSIX-y platforms, this patch looks good to me. Thanks\n> for writing it.\n\nThank you and Johannes for eyeballing and sanity checking.\n\nWill queue.\n"}]}