Re: Apparent bug in credential tool running...

Sure, I can submit a patch if the change looks good to you (with my lack of experience in the git source and very rusty C I would, of course, defer to an expert in the area on exactly where to place the SIGPIPE ignore push and pop and such... but what's below seems to avoid the race for us.... so I can submit that as-is).

Thanks for the quick response! Erik

On 3/28/18, 11:46 AM, "Jeff King" <peff@peff.net> wrote:

    On Wed, Mar 28, 2018 at 06:26:08PM +0000, Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco) wrote:
    
    > The location of the problem is in credential.c, run_credential_helper()... this code:
    > 
    >    ...
    >     fp = xfdopen(helper.in, "w");
    >     credential_write(c, fp);
    >     fclose(fp);
    >    ..
    > 
    > Which I think needs to become something like this:
    > 
    >     fp = xfdopen(helper.in, "w");
    >     sigchain_push(SIGPIPE, SIG_IGN);
    >     credential_write(c, fp);
    >     fclose(fp);
    >     sigchain_pop(SIGPIPE);
    > 
    > The basics are that we wrote a credential helper in Go and, for the
    > store action, it simply exits 0.  It is fast.  This is similar to the
    > example here:
    
    Yeah, that makes sense. Generally a pipe buffer would be plenty to hold
    a credential, but we're racing against whether the other process exits
    before we even write anything, so it's bound to fail eventually in a
    racy way.
    
    I don't think we've ever made a promise[1] about whether credential
    helpers have to read their input, but it makes sense to me for Git to be
    friendly and handle this case. We've done similar things for hooks.
    
    Curiously, I have a very similar helper myself, which I did as an inline
    shell snippet in my ~/.gitconfig:
    
      [credential "https://github.com"]
      username = peff
      helper = "!f() { test $1 = get && echo password=`pass peff/github/oauth`; }; f"
    
    I guess I've never lost the race because of the sheer number of
    sub-processes that get spawned (shell to "pass" which is itself a shell
    script, which spawns gpg -- yikes!).
    
    Do you want to send your change as a patch? There's some guidance in
    Documentation/SubmittingPatches.
    
    -Peff
    
    [1] I know you pulled a similar example from the Pro Git book content,
        which we mirror on git-scm.com.  The quality there is usually quite
        good, but I don't consider it as authoritative as the manpages. :)