Re: Apparent bug in credential tool running...

From: Jeff King <peff@peff.net>
Date: Mar 28, 2018, 18:46 UTC
Message-ID: <20180328184637.GB16931@sigill.intra.peff.net>
In-Reply-To: <7898B9A4-D52E-41AD-9162-5B6B401A1AEA@cisco.com>

On Wed, Mar 28, 2018 at 06:26:08PM +0000, Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco) wrote:

Show 19 quoted lines

> 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. :)