{"thread":{"id":"37352","subject":"[PATCH] unblock and unignore SIGPIPE","startedAt":"2014-08-15T05:29:25Z","lastAt":"2014-09-18T15:04:37Z","messageCount":8,"participants":["Patrick Reynolds","Eric Wong","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247734","messageId":"1408080565-33234-1-git-send-email-patrick.reynolds@github.com","threadId":"37352","inReplyTo":null,"subject":"[PATCH] unblock and unignore SIGPIPE","fromName":"Patrick Reynolds","fromEmail":"patrick.reynolds@github.com","sentAt":"2014-08-15T05:29:25Z","receivedAt":"2014-08-15T05:29:25Z","isPatch":true,"sender":{"key":"patrick.reynolds@github.com","avatar":"https://gravatar.com/avatar/883ab411a1f4a4d96fd05b139be699507f11dafe679b551194bd269370bffc61?d=mp&s=160"},"body":"Blocked and ignored signals -- but not caught signals -- are inherited\nacross exec.  Some callers with sloppy signal-handling behavior can call\ngit with SIGPIPE blocked or ignored, even non-deterministically.  When\nSIGPIPE is blocked or ignored, several git commands can run indefinitely,\nignoring EPIPE returns from write() calls, even when the process that\ncalled them has gone away.  Our specific case involved a pipe of git\ndiff-tree output to a script that reads a limited amount of diff data.\n\nIn an ideal world, git would never be called with SIGPIPE blocked or\nignored.  But in the real world, several real potential callers, including\nPerl, Apache, and Unicorn, sometimes spawn subprocesses with SIGPIPE\nignored.  It is easier and more productive to harden git against this\nmistake than to clean it up in every potential parent process.\n\nSigned-off-by: Patrick Reynolds <patrick.reynolds@github.com>\n---\n cache.h            |  1 +\n git.c              |  5 +++++\n setup.c            | 11 +++++++++++\n t/t0012-sigpipe.sh | 27 +++++++++++++++++++++++++++\n 4 files changed, 44 insertions(+)\n create mode 100755 t/t0012-sigpipe.sh\n\ndiff --git a/cache.h b/cache.h\nindex fcb511d..0a89fc1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -463,6 +463,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);\n extern int init_db(const char *template_dir, unsigned int flags);\n \n extern void sanitize_stdfds(void);\n+extern void sanitize_signals(void);\n extern int daemonize(void);\n \n #define alloc_nr(x) (((x)+16)*3/2)\ndiff --git a/git.c b/git.c\nindex 9c49519..d6b221b 100644\n--- a/git.c\n+++ b/git.c\n@@ -611,6 +611,11 @@ int main(int argc, char **av)\n \t */\n \tsanitize_stdfds();\n \n+\t/*\n+\t * Make sure we aren't ignoring or blocking SIGPIPE.\n+\t */\n+\tsanitize_signals();\n+\n \tgit_setup_gettext();\n \n \ttrace_command_performance(argv);\ndiff --git a/setup.c b/setup.c\nindex 0a22f8b..7aa4b01 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -865,3 +865,14 @@ int daemonize(void)\n \treturn 0;\n #endif\n }\n+\n+/* un-ignore and un-block SIGPIPE */\n+void sanitize_signals(void)\n+{\n+\tsigset_t unblock;\n+\n+\tsigemptyset(&unblock);\n+\tsigaddset(&unblock, SIGPIPE);\n+\tsigprocmask(SIG_UNBLOCK, &unblock, NULL);\n+\tsignal(SIGPIPE, SIG_DFL);\n+}\ndiff --git a/t/t0012-sigpipe.sh b/t/t0012-sigpipe.sh\nnew file mode 100755\nindex 0000000..213cde3\n--- /dev/null\n+++ b/t/t0012-sigpipe.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+\n+test_description='check handling of SIGPIPE'\n+. ./test-lib.sh\n+\n+test_expect_success 'create blob' '\n+\ttest-genrandom foo 16384 >file &&\n+\tgit add file\n+'\n+\n+large_git () {\n+\tfor i in $(test_seq 1 100); do\n+\t\tgit diff --staged --binary || return $?\n+\tdone\n+}\n+\n+test_expect_success 'git dies with SIGPIPE' '\n+\tOUT=$( ((large_git; echo $? 1>&3) | true) 3>&1 )\n+\ttest \"$OUT\" -eq 141\n+'\n+\n+test_expect_success 'git dies with SIGPIPE even if parent ignores it' '\n+\tOUT=$( ((trap \"\" PIPE; large_git; echo $? 1>&3) | true) 3>&1 )\n+\ttest \"$OUT\" -eq 141\n+'\n+\n+test_done\n-- \n2.0.1\n"},{"id":"247829","messageId":"20140818011445.GA22180@dcvr.yhbt.net","threadId":"37352","inReplyTo":"1408080565-33234-1-git-send-email-patrick.reynolds@github.com","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-08-18T01:14:45Z","receivedAt":"2014-08-18T01:14:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Reynolds <patrick.reynolds@github.com> wrote:\n> But in the real world, several real potential callers, including\n> Perl, Apache, and Unicorn, sometimes spawn subprocesses with SIGPIPE\n> ignored.\n\ns/Unicorn/Ruby/\n\nBut unicorn would ignore SIGPIPE it if Ruby did not; relying on SIGPIPE\nwhile doing any multiplexed I/O doesn't work well.\n"},{"id":"248106","messageId":"CAJrMUs_P25m68F_Rv4beSUiGB0uybbP=hBGULzZaefiW4gh=XQ@mail.gmail.com","threadId":"37352","inReplyTo":"20140818011445.GA22180@dcvr.yhbt.net","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Patrick Reynolds","fromEmail":"patrick.reynolds@github.com","sentAt":"2014-08-22T05:40:49Z","receivedAt":"2014-08-22T05:40:49Z","isPatch":true,"sender":{"key":"patrick.reynolds@github.com","avatar":"https://gravatar.com/avatar/883ab411a1f4a4d96fd05b139be699507f11dafe679b551194bd269370bffc61?d=mp&s=160"},"body":"On Sun, Aug 17, 2014 at 8:14 PM, Eric Wong <normalperson@yhbt.net> wrote:\n> But unicorn would ignore SIGPIPE it if Ruby did not; relying on SIGPIPE\n> while doing any multiplexed I/O doesn't work well.\n\nExactly.  Callers block SIGPIPE for their own legitimate reasons, but they\ndon't consistently unblock it before spawning a git subprocess that needs\nthe default SIGPIPE behavior.  Easier to fix it in git than in every potential\ncaller.\n\n--Patrick\n"},{"id":"249572","messageId":"xmqqd2av1bsg.fsf@gitster.dls.corp.google.com","threadId":"37352","inReplyTo":"1408080565-33234-1-git-send-email-patrick.reynolds@github.com","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-16T21:43:43Z","receivedAt":"2014-09-16T21:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Reynolds <patrick.reynolds@github.com> writes:\n\n> Blocked and ignored signals -- but not caught signals -- are inherited\n> across exec.  Some callers with sloppy signal-handling behavior can call\n> git with SIGPIPE blocked or ignored, even non-deterministically.  When\n> SIGPIPE is blocked or ignored, several git commands can run indefinitely,\n> ignoring EPIPE returns from write() calls, even when the process that\n> called them has gone away.  Our specific case involved a pipe of git\n> diff-tree output to a script that reads a limited amount of diff data.\n>\n> In an ideal world, git would never be called with SIGPIPE blocked or\n> ignored.  But in the real world, several real potential callers, including\n> Perl, Apache, and Unicorn, sometimes spawn subprocesses with SIGPIPE\n> ignored.  It is easier and more productive to harden git against this\n> mistake than to clean it up in every potential parent process.\n>\n> Signed-off-by: Patrick Reynolds <patrick.reynolds@github.com>\n\nI missed this one when it was posted.\n\nI have a suspicion that $gmane/256544 (relevant parties Cc'ed) is\ncured by this (even though the other topic came much later than this\nchange)?\n\n> diff --git a/git.c b/git.c\n> index 9c49519..d6b221b 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -611,6 +611,11 @@ int main(int argc, char **av)\n>  \t */\n>  \tsanitize_stdfds();\n>  \n> +\t/*\n> +\t * Make sure we aren't ignoring or blocking SIGPIPE.\n> +\t */\n> +\tsanitize_signals();\n> +\n>  \tgit_setup_gettext();\n>  \n>  \ttrace_command_performance(argv);\n> diff --git a/setup.c b/setup.c\n> index 0a22f8b..7aa4b01 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -865,3 +865,14 @@ int daemonize(void)\n>  \treturn 0;\n>  #endif\n>  }\n> +\n> +/* un-ignore and un-block SIGPIPE */\n> +void sanitize_signals(void)\n> +{\n> +\tsigset_t unblock;\n> +\n> +\tsigemptyset(&unblock);\n> +\tsigaddset(&unblock, SIGPIPE);\n> +\tsigprocmask(SIG_UNBLOCK, &unblock, NULL);\n> +\tsignal(SIGPIPE, SIG_DFL);\n\nWith the only caller in git.c, there is not a good reason that we\nwould want to have this as a global in a different file (I think the\npatch merely follows the pattern of sanitize-fds, but that one has\nto be called from many places).\n\nAfter making the function static to git.c, it would also be a good\nidea to rename it to match the comment at the sole callsite,\ne.g. \"restore_sigpipe_to_default()\" or something.  Then the comment\nat the callsite can be removed.\n\nLater somebody else may want to add other signals handled similarly\nfor whatever reason (I do not think of any signal or any good reason\nat this moment).  But they will have to come up with much better\njustification than \"the function name says 'sanitize'\" if we named\nit to something specific to SIGPIPE.  And that good justification\nwill guide us what kind of signals need to be \"sanitized\" and find a\nbetter verb than \"sanitize\", if it ever happens.\n\n> diff --git a/t/t0012-sigpipe.sh b/t/t0012-sigpipe.sh\n> new file mode 100755\n> index 0000000..213cde3\n> --- /dev/null\n> +++ b/t/t0012-sigpipe.sh\n> @@ -0,0 +1,27 @@\n> +#!/bin/sh\n\nHmmm, do we really need to allocate a new test number just for these\ntwo tests, instead of folding it into an existing one?\n\n> +test_expect_success 'create blob' '\n> +\ttest-genrandom foo 16384 >file &&\n> +\tgit add file\n> +'\n> +\n> +large_git () {\n> +\tfor i in $(test_seq 1 100); do\n> +\t\tgit diff --staged --binary || return $?\n\nWrite it more like this:\n\n\tfor i in $(...)\n        do\n\t\tgit diff --cached --binary || return\n\tdone\n\nto (1) avoid unnecessary semicolon before \"do\", (2) prefer the true\noption name over a synonym, and (3) omit unnecessary $? that is\ngiven to \"return\".\n\nSumming them up, something like this squashed in would give us a\ngood end result, perhaps?\n\n---\n cache.h            |  1 -\n git.c              | 25 +++++++++++++++++++++----\n setup.c            | 11 -----------\n t/t0000-basic.sh   | 17 +++++++++++++++++\n t/t0012-sigpipe.sh | 27 ---------------------------\n 5 files changed, 38 insertions(+), 43 deletions(-)\n delete mode 100755 t/t0012-sigpipe.sh\n\ndiff --git a/cache.h b/cache.h\nindex 0a89fc1..fcb511d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -463,7 +463,6 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);\n extern int init_db(const char *template_dir, unsigned int flags);\n \n extern void sanitize_stdfds(void);\n-extern void sanitize_signals(void);\n extern int daemonize(void);\n \n #define alloc_nr(x) (((x)+16)*3/2)\ndiff --git a/git.c b/git.c\nindex d6b221b..c87bacd 100644\n--- a/git.c\n+++ b/git.c\n@@ -592,6 +592,26 @@ static int run_argv(int *argcp, const char ***argv)\n \treturn done_alias;\n }\n \n+/*\n+ * Many parts of Git have subprograms communicate via pipe, expect the\n+ * upstream of the pipe to die with SIGPIPE and the downstream process\n+ * even knows to check and handle EPIPE correctly.  Some third-party\n+ * programs that ignore or block SIGPIPE for their own reason forget\n+ * to restore SIGPIPE handling to the default before spawning Git and\n+ * break this carefully orchestrated machinery.\n+ *\n+ * Restore the way SIGPIPE is handled to default, which is what we\n+ * expect.\n+ */\n+static void restore_sigpipe_to_default(void)\n+{\n+\tsigset_t unblock;\n+\n+\tsigemptyset(&unblock);\n+\tsigaddset(&unblock, SIGPIPE);\n+\tsigprocmask(SIG_UNBLOCK, &unblock, NULL);\n+\tsignal(SIGPIPE, SIG_DFL);\n+}\n \n int main(int argc, char **av)\n {\n@@ -611,10 +631,7 @@ int main(int argc, char **av)\n \t */\n \tsanitize_stdfds();\n \n-\t/*\n-\t * Make sure we aren't ignoring or blocking SIGPIPE.\n-\t */\n-\tsanitize_signals();\n+\trestore_sigpipe_to_default();\n \n \tgit_setup_gettext();\n \ndiff --git a/setup.c b/setup.c\nindex 7aa4b01..0a22f8b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -865,14 +865,3 @@ int daemonize(void)\n \treturn 0;\n #endif\n }\n-\n-/* un-ignore and un-block SIGPIPE */\n-void sanitize_signals(void)\n-{\n-\tsigset_t unblock;\n-\n-\tsigemptyset(&unblock);\n-\tsigaddset(&unblock, SIGPIPE);\n-\tsigprocmask(SIG_UNBLOCK, &unblock, NULL);\n-\tsignal(SIGPIPE, SIG_DFL);\n-}\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex f10ba4a..21a0b19 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1063,4 +1063,21 @@ test_expect_success 'very long name in the index handled sanely' '\n \ttest $len = 4098\n '\n \n+large_git () {\n+\tfor i in $(test_seq 1 100)\n+\tdo\n+\t\tgit ls-files -s || return\n+\tdone\n+}\n+\n+test_expect_success 'a constipated git dies with SIGPIPE' '\n+\tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 )\n+\ttest \"$OUT\" -eq 141\n+'\n+\n+test_expect_success 'a constipated git dies with SIGPIPE even if parent ignores it' '\n+\tOUT=$( ((trap \"\" PIPE; large_git; echo $? 1>&3) | :) 3>&1 )\n+\ttest \"$OUT\" -eq 141\n+'\n+\n test_done\ndiff --git a/t/t0012-sigpipe.sh b/t/t0012-sigpipe.sh\ndeleted file mode 100755\nindex 213cde3..0000000\n--- a/t/t0012-sigpipe.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-\n-test_description='check handling of SIGPIPE'\n-. ./test-lib.sh\n-\n-test_expect_success 'create blob' '\n-\ttest-genrandom foo 16384 >file &&\n-\tgit add file\n-'\n-\n-large_git () {\n-\tfor i in $(test_seq 1 100); do\n-\t\tgit diff --staged --binary || return $?\n-\tdone\n-}\n-\n-test_expect_success 'git dies with SIGPIPE' '\n-\tOUT=$( ((large_git; echo $? 1>&3) | true) 3>&1 )\n-\ttest \"$OUT\" -eq 141\n-'\n-\n-test_expect_success 'git dies with SIGPIPE even if parent ignores it' '\n-\tOUT=$( ((trap \"\" PIPE; large_git; echo $? 1>&3) | true) 3>&1 )\n-\ttest \"$OUT\" -eq 141\n-'\n-\n-test_done\n-- \n2.1.0-403-g099cf47\n"},{"id":"249591","messageId":"20140917081148.GB16200@peff.net","threadId":"37352","inReplyTo":"xmqqd2av1bsg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-17T08:11:49Z","receivedAt":"2014-09-17T08:11:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 16, 2014 at 02:43:43PM -0700, Junio C Hamano wrote:\n\n> > +/* un-ignore and un-block SIGPIPE */\n> > +void sanitize_signals(void)\n> > +{\n> > +\tsigset_t unblock;\n> > +\n> > +\tsigemptyset(&unblock);\n> > +\tsigaddset(&unblock, SIGPIPE);\n> > +\tsigprocmask(SIG_UNBLOCK, &unblock, NULL);\n> > +\tsignal(SIGPIPE, SIG_DFL);\n> \n> With the only caller in git.c, there is not a good reason that we\n> would want to have this as a global in a different file (I think the\n> patch merely follows the pattern of sanitize-fds, but that one has\n> to be called from many places).\n\nWould we want to call it from external C commands, too? For the most\npart, git.c is the entry point for running git commands, and any\nsanitizing it does will be inherited by sub-commands. But it _is_ still\nlegal to call dashed commands individually, and even required in some\ncases (e.g., git-upload-pack for ssh clients).\n\n> > diff --git a/t/t0012-sigpipe.sh b/t/t0012-sigpipe.sh\n> > new file mode 100755\n> > index 0000000..213cde3\n> > --- /dev/null\n> > +++ b/t/t0012-sigpipe.sh\n> > @@ -0,0 +1,27 @@\n> > +#!/bin/sh\n> \n> Hmmm, do we really need to allocate a new test number just for these\n> two tests, instead of folding it into an existing one?\n\nI see in your proposed patch below you put them into t0000. I wonder if\nt0005 would be a more obvious place.\n\n-Peff\n"},{"id":"249626","messageId":"xmqq61glvojc.fsf@gitster.dls.corp.google.com","threadId":"37352","inReplyTo":"20140917081148.GB16200@peff.net","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-17T23:02:31Z","receivedAt":"2014-09-17T23:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I see in your proposed patch below you put them into t0000. I wonder if\n> t0005 would be a more obvious place.\n\nYup.  That is a good idea.\n"},{"id":"249634","messageId":"CAJrMUs-68cNXdh1dbRbjGBdrT0hukR7DH=mf7v05EDjL7qzP-Q@mail.gmail.com","threadId":"37352","inReplyTo":"20140917081148.GB16200@peff.net","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Patrick Reynolds","fromEmail":"piki@github.com","sentAt":"2014-09-18T14:35:05Z","receivedAt":"2014-09-18T14:35:05Z","isPatch":true,"sender":{"key":"piki@github.com","avatar":null},"body":"On Wed, Sep 17, 2014 at 3:11 AM, Jeff King <peff@peff.net> wrote:\n> Would we want to call it from external C commands, too? For the most\n> part, git.c is the entry point for running git commands, and any\n> sanitizing it does will be inherited by sub-commands. But it _is_ still\n> legal to call dashed commands individually, and even required in some\n> cases (e.g., git-upload-pack for ssh clients).\n\ngit-upload-pack is protected pretty well from SIGPIPE shenanigans,\nbecause its stdout all goes through write_or_die, as of cdf4fb8.  We\ndid, long ago, have some EPIPE problems with upload-pack and SSH\nclients, but it all predates cdf4fb8.\n\nSo I think it's redundant to unblock SIGPIPE in git-upload-pack.\n\nI'll tidy up as Junio recommended, recheck the tests, and submit\nan updated patch shortly.\n\n--Patrick\n"},{"id":"249635","messageId":"CAPc5daWHUvV1tyUsz0kN0i53cSF43YeNVsC9hGdudpq60Pfzyg@mail.gmail.com","threadId":"37352","inReplyTo":"CAJrMUs-68cNXdh1dbRbjGBdrT0hukR7DH=mf7v05EDjL7qzP-Q@mail.gmail.com","subject":"Re: [PATCH] unblock and unignore SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-18T15:04:37Z","receivedAt":"2014-09-18T15:04:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; just to save time, you may want to look at what has already\nbeen queued on 'pu'.\n\n\nOn Thu, Sep 18, 2014 at 7:35 AM, Patrick Reynolds <piki@github.com> wrote:\n> On Wed, Sep 17, 2014 at 3:11 AM, Jeff King <peff@peff.net> wrote:\n>> Would we want to call it from external C commands, too? For the most\n>> part, git.c is the entry point for running git commands, and any\n>> sanitizing it does will be inherited by sub-commands. But it _is_ still\n>> legal to call dashed commands individually, and even required in some\n>> cases (e.g., git-upload-pack for ssh clients).\n>\n> git-upload-pack is protected pretty well from SIGPIPE shenanigans,\n> because its stdout all goes through write_or_die, as of cdf4fb8.  We\n> did, long ago, have some EPIPE problems with upload-pack and SSH\n> clients, but it all predates cdf4fb8.\n>\n> So I think it's redundant to unblock SIGPIPE in git-upload-pack.\n>\n> I'll tidy up as Junio recommended, recheck the tests, and submit\n> an updated patch shortly.\n>\n> --Patrick\n"}]}