From: "Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)" <brady@cisco.com>
To: Jeff King <peff@peff.net>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: Apparent bug in credential tool running...
Date: Wed, 28 Mar 2018 20:31:01 +0000 [thread overview]
Message-ID: <6C6181C9-AA36-4940-94BC-5DB0160C412D@cisco.com> (raw)
In-Reply-To: <20180328184637.GB16931@sigill.intra.peff.net>
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. :)
next prev parent reply other threads:[~2018-03-28 20:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-28 18:26 Apparent bug in credential tool running Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)
2018-03-28 18:46 ` Jeff King
2018-03-28 20:31 ` Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco) [this message]
2018-03-28 23:27 ` Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6C6181C9-AA36-4940-94BC-5DB0160C412D@cisco.com \
--to=brady@cisco.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).