{"thread":{"id":"48178","subject":"[PATCH] credential: cred helper fast exit can cause SIGPIPE, crash","startedAt":"2018-03-28T22:21:16Z","lastAt":"2018-03-29T22:35:25Z","messageCount":9,"participants":["Erik E Brady","Jeff King","Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343280","messageId":"20180328222051.23684-1-brady@cisco.com","threadId":"48178","inReplyTo":null,"subject":"[PATCH] credential: cred helper fast exit can cause SIGPIPE, crash","fromName":"Erik E Brady","fromEmail":"brady@cisco.com","sentAt":"2018-03-28T22:20:51Z","receivedAt":"2018-03-28T22:21:16Z","isPatch":true,"sender":{"key":"brady@cisco.com","avatar":null},"body":"credential.c, run_credential_helper(): now ignores SIGPIPE\nwhen writing to credential helper.  Avoids problem with race\nwhere cred helper exits very quickly and, after, git tries\nto write to it, generating SIGPIPE and crashing git.  To\nreproduce this the cred helper must not read from STDIN.\n\nThis was seen with a custom credential helper, written in\nGo, which ignored the store command (STDIN not read) and\nthen did a quick exit.  Even with this fast helper the race\nwas pretty rare, ie: was only seen on some of our older VM's\nrunning 2.6.18-416.el5 #1 SMP linux for whatever reason.  On\nthese VM's it occurred only once every few hundred git cmds.\n---\n credential.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/credential.c b/credential.c\nindex 9747f47b1..62be651b0 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"url.h\"\n #include \"prompt.h\"\n+#include \"sigchain.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,\n \t\treturn -1;\n \n \tfp = xfdopen(helper.in, \"w\");\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \tcredential_write(c, fp);\n \tfclose(fp);\n+\tsigchain_pop(SIGPIPE);\n \n \tif (want_output) {\n \t\tint r;\n-- \n2.16.3.dirty\n\n"},{"id":"343314","messageId":"20180329111915.GA30797@sigill.intra.peff.net","threadId":"48178","inReplyTo":"20180328222051.23684-1-brady@cisco.com","subject":"Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T11:19:15Z","receivedAt":"2018-03-29T11:19:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 03:20:51PM -0700, Erik E Brady wrote:\n\n> Subject: Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash\n\nThanks for sending this. The patch itself looks good to me, but I have a\nfew nits with your commit message.\n\nWe usually write commit messages in the imperative, with the subject\nsummarizing the change. So:\n\n  Subject: credential: ignore SIGPIPE when writing to credential helpers\n\nor similar.\n\n> credential.c, run_credential_helper(): now ignores SIGPIPE\n> when writing to credential helper.  Avoids problem with race\n> where cred helper exits very quickly and, after, git tries\n> to write to it, generating SIGPIPE and crashing git.  To\n> reproduce this the cred helper must not read from STDIN.\n\nWe can stop being terse outside of the subject line. :) I'd probably\nwrite something like:\n\n  The credential subsystem can trigger SIGPIPE when writing to an\n  external helper if that helper closes its stdin before reading the\n  whole input. Normally this is rare, since helpers would need to read\n  that input to make a decision about how to respond, but:\n\n    1. It's reasonable to configure a helper which blindly a \"get\"\n       answer, and trigger it only for certain hosts via config like:\n\n         [credential \"https://example.com\"]\n\t helper = \"!get-example-password\"\n\n    2. A broken or misbehaving helper might exit immediately. That's an\n       error, but it's not reasonable for it to take down the parent Git\n       process with SIGPIPE.\n\n  Even with such a helper, seeing this problem should be rare. Getting\n  SIGPIPE requires the helper racily exiting before we've written the\n  fairly small credential output.\n\nFeel free to steal or adapt any of that as you see fit.\n\n> This was seen with a custom credential helper, written in\n> Go, which ignored the store command (STDIN not read) and\n> then did a quick exit.  Even with this fast helper the race\n> was pretty rare, ie: was only seen on some of our older VM's\n> running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On\n> these VM's it occurred only once every few hundred git cmds.\n> ---\n\nMissing signoff. See Documentation/SubmittingPatches, especially the\n'sign-off' and 'dco' sections.\n\n>  credential.c | 3 +++\n>  1 file changed, 3 insertions(+)\n\nNo test, but I think that's fine here. Any such test would be inherently\nracy.\n\n> @@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,\n>  \t\treturn -1;\n>  \n>  \tfp = xfdopen(helper.in, \"w\");\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n>  \tcredential_write(c, fp);\n>  \tfclose(fp);\n> +\tsigchain_pop(SIGPIPE);\n\nThis looks like the right place to put the push/pop (as you noted\nbefore, we may not write until fclose flushes, so it definitely has to\ngo after that).\n\nThanks again for digging this up. It's pretty subtle. :)\n\n-Peff\n"},{"id":"343346","messageId":"330BFBFA-6B9E-456D-A9B3-2D0EF5383A57@cisco.com","threadId":"48178","inReplyTo":"20180329111915.GA30797@sigill.intra.peff.net","subject":"Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash","fromName":"Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)","fromEmail":"brady@cisco.com","sentAt":"2018-03-29T17:25:04Z","receivedAt":"2018-03-29T17:25:12Z","isPatch":true,"sender":{"key":"brady@cisco.com","avatar":null},"body":"Thanks Jeff.\n\nOK, will retry on the comment.  I guess I misunderstood the guidelines a bit on the signoff as well (ie: non-optional), apologies.  Will resubmit via 'git send-email' after adjusting the comment and recommitting with the -s option.  First time for everything I suppose, doh.\n\nAs to your comment suggestion, appreciated, looks good.  I might reword the #1 item you have just a bit (I removed the host specific stuff since I think the race can occur regardless of host specific or not... but I might be missing something there?).  Anyhow, how about something like this:\n\n--\nSubject: credential: ignore SIGPIPE when writing to credential helpers\n\nThe credential subsystem can trigger SIGPIPE when writing to an\nexternal helper if that helper closes its stdin before reading the\nwhole input. Normally this is rare, since helpers would need to read\nthat input to make a decision about how to respond, but:\n\n1. It's reasonable to configure a helper which only handles \"get\"\n    while ignoring \"store\".  Such a handler might not read stdin \n    for \"store\", thereby rapidly closing stdin upon helper exit.\n\n2. A broken or misbehaving helper might exit immediately. That's an\n     error, but it's not reasonable for it to take down the parent Git\n     process with SIGPIPE.\n    \nEven with such a helper, seeing this problem should be rare. Getting\nSIGPIPE requires the helper racily exiting before we've written the\nfairly small credential output.\n--\n\nAs to testing, yes, that was my thought as well.  Anyhow, I will try the above unless you see a problem or would like any further change (?).\n\nThanks,\nErik\n\nOn 3/29/18, 4:19 AM, \"Jeff King\" <peff@peff.net> wrote:\n\n    On Wed, Mar 28, 2018 at 03:20:51PM -0700, Erik E Brady wrote:\n    \n    > Subject: Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash\n    \n    Thanks for sending this. The patch itself looks good to me, but I have a\n    few nits with your commit message.\n    \n    We usually write commit messages in the imperative, with the subject\n    summarizing the change. So:\n    \n      Subject: credential: ignore SIGPIPE when writing to credential helpers\n    \n    or similar.\n    \n    > credential.c, run_credential_helper(): now ignores SIGPIPE\n    > when writing to credential helper.  Avoids problem with race\n    > where cred helper exits very quickly and, after, git tries\n    > to write to it, generating SIGPIPE and crashing git.  To\n    > reproduce this the cred helper must not read from STDIN.\n    \n    We can stop being terse outside of the subject line. :) I'd probably\n    write something like:\n    \n      The credential subsystem can trigger SIGPIPE when writing to an\n      external helper if that helper closes its stdin before reading the\n      whole input. Normally this is rare, since helpers would need to read\n      that input to make a decision about how to respond, but:\n    \n        1. It's reasonable to configure a helper which blindly a \"get\"\n           answer, and trigger it only for certain hosts via config like:\n    \n             [credential \"https://example.com\"]\n    \t helper = \"!get-example-password\"\n    \n        2. A broken or misbehaving helper might exit immediately. That's an\n           error, but it's not reasonable for it to take down the parent Git\n           process with SIGPIPE.\n    \n      Even with such a helper, seeing this problem should be rare. Getting\n      SIGPIPE requires the helper racily exiting before we've written the\n      fairly small credential output.\n    \n    Feel free to steal or adapt any of that as you see fit.\n    \n    > This was seen with a custom credential helper, written in\n    > Go, which ignored the store command (STDIN not read) and\n    > then did a quick exit.  Even with this fast helper the race\n    > was pretty rare, ie: was only seen on some of our older VM's\n    > running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On\n    > these VM's it occurred only once every few hundred git cmds.\n    > ---\n    \n    Missing signoff. See Documentation/SubmittingPatches, especially the\n    'sign-off' and 'dco' sections.\n    \n    >  credential.c | 3 +++\n    >  1 file changed, 3 insertions(+)\n    \n    No test, but I think that's fine here. Any such test would be inherently\n    racy.\n    \n    > @@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,\n    >  \t\treturn -1;\n    >  \n    >  \tfp = xfdopen(helper.in, \"w\");\n    > +\tsigchain_push(SIGPIPE, SIG_IGN);\n    >  \tcredential_write(c, fp);\n    >  \tfclose(fp);\n    > +\tsigchain_pop(SIGPIPE);\n    \n    This looks like the right place to put the push/pop (as you noted\n    before, we may not write until fclose flushes, so it definitely has to\n    go after that).\n    \n    Thanks again for digging this up. It's pretty subtle. :)\n    \n    -Peff\n    \n\n"},{"id":"343350","messageId":"20180329175501.GE31833@sigill.intra.peff.net","threadId":"48178","inReplyTo":"330BFBFA-6B9E-456D-A9B3-2D0EF5383A57@cisco.com","subject":"Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T17:55:01Z","receivedAt":"2018-03-29T17:55:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 05:25:04PM +0000, Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco) wrote:\n\n> OK, will retry on the comment.  I guess I misunderstood the guidelines\n> a bit on the signoff as well (ie: non-optional), apologies.  Will\n> resubmit via 'git send-email' after adjusting the comment and\n> recommitting with the -s option.  First time for everything I suppose,\n> doh.\n\nThe signoff (for our project) is all about the \"yes, this contribution\ncan go under the gpl\". So that part is very non-optional. :)\n\n> As to your comment suggestion, appreciated, looks good.  I might\n> reword the #1 item you have just a bit (I removed the host specific\n> stuff since I think the race can occur regardless of host specific or\n> not... but I might be missing something there?).  Anyhow, how about\n> something like this:\n\nYeah, it can definitely occur regardless. It was just the plausible\nreason to have a handler which does not bother to look at the incoming\ndata (since otherwise you are spewing your password to any host you\nconnect to; maybe that's OK if you configure it only inside a specific\nrepo and only fetch from one server).\n\nYour update looks fine to me.\n\nThanks.\n\n-Peff\n"},{"id":"343352","messageId":"20180329180056.9941-1-brady@cisco.com","threadId":"48178","inReplyTo":"20180329175501.GE31833@sigill.intra.peff.net","subject":"[PATCH] credential: ignore SIGPIPE when writing to credential helpers","fromName":"Erik E Brady","fromEmail":"brady@cisco.com","sentAt":"2018-03-29T18:00:56Z","receivedAt":"2018-03-29T18:01:43Z","isPatch":true,"sender":{"key":"brady@cisco.com","avatar":null},"body":"The credential subsystem can trigger SIGPIPE when writing to an\nexternal helper if that helper closes its stdin before reading the\nwhole input. Normally this is rare, since helpers would need to read\nthat input to make a decision about how to respond, but:\n\n1. It's reasonable to configure a helper which only handles \"get\"\n   while ignoring \"store\".  Such a handler might not read stdin\n   for \"store\", thereby rapidly closing stdin upon helper exit.\n\n2. A broken or misbehaving helper might exit immediately. That's an\n   error, but it's not reasonable for it to take down the parent Git\n   process with SIGPIPE.\n\nEven with such a helper, seeing this problem should be rare. Getting\nSIGPIPE requires the helper racily exiting before we've written the\nfairly small credential output.\n\nSigned-off-by: Erik E Brady <brady@cisco.com>\n---\n credential.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/credential.c b/credential.c\nindex 9747f47b1..62be651b0 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"url.h\"\n #include \"prompt.h\"\n+#include \"sigchain.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,\n \t\treturn -1;\n \n \tfp = xfdopen(helper.in, \"w\");\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \tcredential_write(c, fp);\n \tfclose(fp);\n+\tsigchain_pop(SIGPIPE);\n \n \tif (want_output) {\n \t\tint r;\n-- \n2.16.3.dirty\n\n"},{"id":"343374","messageId":"20180329215153.GJ2939@sigill.intra.peff.net","threadId":"48178","inReplyTo":"20180329180056.9941-1-brady@cisco.com","subject":"Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T21:51:54Z","receivedAt":"2018-03-29T21:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:\n\n> The credential subsystem can trigger SIGPIPE when writing to an\n> external helper if that helper closes its stdin before reading the\n> whole input. Normally this is rare, since helpers would need to read\n> that input to make a decision about how to respond, but:\n> \n> 1. It's reasonable to configure a helper which only handles \"get\"\n>    while ignoring \"store\".  Such a handler might not read stdin\n>    for \"store\", thereby rapidly closing stdin upon helper exit.\n> \n> 2. A broken or misbehaving helper might exit immediately. That's an\n>    error, but it's not reasonable for it to take down the parent Git\n>    process with SIGPIPE.\n> \n> Even with such a helper, seeing this problem should be rare. Getting\n> SIGPIPE requires the helper racily exiting before we've written the\n> fairly small credential output.\n> \n> Signed-off-by: Erik E Brady <brady@cisco.com>\n> ---\n>  credential.c | 3 +++\n>  1 file changed, 3 insertions(+)\n\nThis version looks good to me. Thanks!\n\n-Peff\n"},{"id":"343379","messageId":"E29A3311-D880-4872-835B-EF082EB2F70C@cisco.com","threadId":"48178","inReplyTo":"20180329215153.GJ2939@sigill.intra.peff.net","subject":"Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers","fromName":"Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)","fromEmail":"brady@cisco.com","sentAt":"2018-03-29T22:20:40Z","receivedAt":"2018-03-29T22:20:46Z","isPatch":true,"sender":{"key":"brady@cisco.com","avatar":null},"body":"Thanks Jeff.\n\nI appreciate your time.  Quick Q... is there a way to track the patch through to release?  If not I can just scan release notes/etc so no worries.\n\nCheers,\nErik\n\nOn 3/29/18, 2:51 PM, \"Jeff King\" <peff@peff.net> wrote:\n\n    On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:\n    \n    > The credential subsystem can trigger SIGPIPE when writing to an\n    > external helper if that helper closes its stdin before reading the\n    > whole input. Normally this is rare, since helpers would need to read\n    > that input to make a decision about how to respond, but:\n    > \n    > 1. It's reasonable to configure a helper which only handles \"get\"\n    >    while ignoring \"store\".  Such a handler might not read stdin\n    >    for \"store\", thereby rapidly closing stdin upon helper exit.\n    > \n    > 2. A broken or misbehaving helper might exit immediately. That's an\n    >    error, but it's not reasonable for it to take down the parent Git\n    >    process with SIGPIPE.\n    > \n    > Even with such a helper, seeing this problem should be rare. Getting\n    > SIGPIPE requires the helper racily exiting before we've written the\n    > fairly small credential output.\n    > \n    > Signed-off-by: Erik E Brady <brady@cisco.com>\n    > ---\n    >  credential.c | 3 +++\n    >  1 file changed, 3 insertions(+)\n    \n    This version looks good to me. Thanks!\n    \n    -Peff\n    \n\n"},{"id":"343380","messageId":"20180329222949.GA13017@sigill.intra.peff.net","threadId":"48178","inReplyTo":"E29A3311-D880-4872-835B-EF082EB2F70C@cisco.com","subject":"Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T22:29:49Z","receivedAt":"2018-03-29T22:30:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 10:20:40PM +0000, Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco) wrote:\n\n> I appreciate your time.  Quick Q... is there a way to track the patch\n> through to release?  If not I can just scan release notes/etc so no\n> worries.\n\nWhen the maintainer picks up the patch, he usually says something like\n\"thanks, will queue\". Then you can track its progress either by:\n\n - fetching from https://github.com/gitster/git, which has all of the\n   topic branches. Yours will be \"eb/something\", depending what Junio\n   names it. And then you can periodically \"git branch -a --contains\n   eb/something\" to see it progress through the various integration\n   branches ('pu', 'next', 'master'). The branches are described in the\n   \"note from the maintainer\" that's sent to the list periodically.\n   E.g.:\n\n     https://public-inbox.org/git/xmqqy3nt40pq.fsf@gitster.mtv.corp.google.com/\n\n - alternatively, you can read the \"What's cooking in git.git\" messages\n   sent out by the maintainer a few times a week. E.g.:\n\n     https://public-inbox.org/git/xmqqefkm6s06.fsf@gitster-ct.c.googlers.com/\n\n   There your eb/something topic will be mentioned, along with the\n   status (where it is in the graduation cycle, what's coming next, and\n   if it's stalled, why).\n\nWe're in a release freeze right for v2.17 right now, so I'd expect your\npatch to probably go to the 'maint' track and end up in v2.17.1.\n\nOf course somebody else may see something wrong I didn't and ask you to\ncorrect it, which would be in a reply to this thread. :)\n\n-Peff\n"},{"id":"343382","messageId":"xmqqzi2q4ijf.fsf@gitster-ct.c.googlers.com","threadId":"48178","inReplyTo":"20180329215153.GJ2939@sigill.intra.peff.net","subject":"Re: [PATCH] credential: ignore SIGPIPE when writing to credential helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-29T22:35:16Z","receivedAt":"2018-03-29T22:35:25Z","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> On Thu, Mar 29, 2018 at 11:00:56AM -0700, Erik E Brady wrote:\n>\n>> The credential subsystem can trigger SIGPIPE when writing to an\n>> external helper if that helper closes its stdin before reading the\n>> whole input. Normally this is rare, since helpers would need to read\n>> that input to make a decision about how to respond, but:\n>> \n>> 1. It's reasonable to configure a helper which only handles \"get\"\n>>    while ignoring \"store\".  Such a handler might not read stdin\n>>    for \"store\", thereby rapidly closing stdin upon helper exit.\n>> \n>> 2. A broken or misbehaving helper might exit immediately. That's an\n>>    error, but it's not reasonable for it to take down the parent Git\n>>    process with SIGPIPE.\n>> \n>> Even with such a helper, seeing this problem should be rare. Getting\n>> SIGPIPE requires the helper racily exiting before we've written the\n>> fairly small credential output.\n>> \n>> Signed-off-by: Erik E Brady <brady@cisco.com>\n>> ---\n>>  credential.c | 3 +++\n>>  1 file changed, 3 insertions(+)\n>\n> This version looks good to me. Thanks!\n\nYup, looks good.  Thanks, both.\n"}]}