Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash

["Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)" <brady@cisco.com>]

Thanks Jeff.

OK, 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.

As 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:

--
Subject: credential: ignore SIGPIPE when writing to credential helpers

The credential subsystem can trigger SIGPIPE when writing to an
external helper if that helper closes its stdin before reading the
whole input. Normally this is rare, since helpers would need to read
that input to make a decision about how to respond, but:

1. It's reasonable to configure a helper which only handles "get"
    while ignoring "store".  Such a handler might not read stdin 
    for "store", thereby rapidly closing stdin upon helper exit.

2. A broken or misbehaving helper might exit immediately. That's an
     error, but it's not reasonable for it to take down the parent Git
     process with SIGPIPE.
    
Even with such a helper, seeing this problem should be rare. Getting
SIGPIPE requires the helper racily exiting before we've written the
fairly small credential output.
--

As 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 (?).

Thanks,
Erik

On 3/29/18, 4:19 AM, "Jeff King" <peff@peff.net> wrote:

    On Wed, Mar 28, 2018 at 03:20:51PM -0700, Erik E Brady wrote:
    
    > Subject: Re: [PATCH] credential: cred helper fast exit can cause SIGPIPE, crash
    
    Thanks for sending this. The patch itself looks good to me, but I have a
    few nits with your commit message.
    
    We usually write commit messages in the imperative, with the subject
    summarizing the change. So:
    
      Subject: credential: ignore SIGPIPE when writing to credential helpers
    
    or similar.
    
    > credential.c, run_credential_helper(): now ignores SIGPIPE
    > when writing to credential helper.  Avoids problem with race
    > where cred helper exits very quickly and, after, git tries
    > to write to it, generating SIGPIPE and crashing git.  To
    > reproduce this the cred helper must not read from STDIN.
    
    We can stop being terse outside of the subject line. :) I'd probably
    write something like:
    
      The credential subsystem can trigger SIGPIPE when writing to an
      external helper if that helper closes its stdin before reading the
      whole input. Normally this is rare, since helpers would need to read
      that input to make a decision about how to respond, but:
    
        1. It's reasonable to configure a helper which blindly a "get"
           answer, and trigger it only for certain hosts via config like:
    
             [credential "https://example.com"]
    	 helper = "!get-example-password"
    
        2. A broken or misbehaving helper might exit immediately. That's an
           error, but it's not reasonable for it to take down the parent Git
           process with SIGPIPE.
    
      Even with such a helper, seeing this problem should be rare. Getting
      SIGPIPE requires the helper racily exiting before we've written the
      fairly small credential output.
    
    Feel free to steal or adapt any of that as you see fit.
    
    > This was seen with a custom credential helper, written in
    > Go, which ignored the store command (STDIN not read) and
    > then did a quick exit.  Even with this fast helper the race
    > was pretty rare, ie: was only seen on some of our older VM's
    > running 2.6.18-416.el5 #1 SMP linux for whatever reason.  On
    > these VM's it occurred only once every few hundred git cmds.
    > ---
    
    Missing signoff. See Documentation/SubmittingPatches, especially the
    'sign-off' and 'dco' sections.
    
    >  credential.c | 3 +++
    >  1 file changed, 3 insertions(+)
    
    No test, but I think that's fine here. Any such test would be inherently
    racy.
    
    > @@ -227,8 +228,10 @@ static int run_credential_helper(struct credential *c,
    >  		return -1;
    >  
    >  	fp = xfdopen(helper.in, "w");
    > +	sigchain_push(SIGPIPE, SIG_IGN);
    >  	credential_write(c, fp);
    >  	fclose(fp);
    > +	sigchain_pop(SIGPIPE);
    
    This looks like the right place to put the push/pop (as you noted
    before, we may not write until fclose flushes, so it definitely has to
    go after that).
    
    Thanks again for digging this up. It's pretty subtle. :)
    
    -Peff