Git media client will hang when using ssh to access repositories

[rubyist]

Steps to reproduce:

Use the cache credential helper
git config --global credental.helper cache

Clone a git media repository via ssh. You will then be prompted for github credentials and the git media client will hang:

~$ git clone git@github.com:user/mediarepo.git
Cloning into 'mediarepo'...
remote: Counting objects: 95, done.
remote: Total 95 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (95/95), 8.88 KiB | 0 bytes/s, done.
Resolving deltas: 100% (25/25), done.
Checking connectivity... done.
Username for 'https://github.com': user
Password for 'https://user@github.com':

[rubyist]

The problem is that git credential approve and reject don't have any output. When we're executing the commands, we're giving it buffers for the process stdout and stderr, so cmd.Wait() will wait for those to be filled. Since they'll never be filled, it waits forever.

We'll need to modify the command execution to not provide buffers for approve.

[rubyist]

This is actually a little bit more involved. This is only triggered when the credential cache daemon is not already running. If it is not running, git credential operations will start it. When it is started, one of the child processes is probably not closing the stderr descriptor. This means that the git media client never receives EOF on the stderr pipe that it's waiting on, essentially blocking forever. It appears that the child process properly closes its stdout descriptor, because you can actually leave the buffer on stdout and everything is fine. This is likely a bug somewhere down the chain of git commands, but it's easy enough to handle here by just not waiting for any output on the commands we know don't have any output.

[rubyist]

With a little more investigation, this seems like it might be a bug in git itself. Things work like this:

The git-media client [A] launches git [B] which, upon detecting the credential cache daemon is not running, launches git-credential-cache--daemon [C]. The process [C] closes its stdout descriptor, but it does not close its stderr descriptor. Therefore, Process [A] will read EOF on the stdout pipe, but it will never read EOF on the stderr pipe, which will cause [A] to wait forever (or until [C] dies or is killed, even though [B] is long gone).

In git's credential-cache.c where it launches the credential cache daemon, you can see that it requests to not keep stdout open. I think it also should ask to not keep stderr open, as such:

diff --git a/credential-cache.c b/credential-cache.c
index 9a03792..9abce0b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -47,7 +47,9 @@ static void spawn_daemon(const char *socket)
        argv[1] = socket;
        daemon.argv = argv;
        daemon.no_stdin = 1;
+       daemon.no_stderr = 1;
        daemon.out = -1;

        if (start_command(&daemon))
                die_errno("unable to start cache daemon");

If I do this, the problem goes away. @github/git - does this seem like a legit git bug to you?

We'll have to work around this anyway by just never waiting on the subprocess's stderr, but if it is a legit git bug we might want to send patches.

Edit the code should not set daemon.err = -1, because it never gets set to a valid file descriptor, and thus the program will try to close(-1). Only daemon.no_stderr should be set.

[peff]

Hrm. I'd question a bit whether it is actually worth capturing stderr from a git credential call in the first place. Normally it's stderr would just go the user via the terminal (or captured in a file or whatever). Yes, it's a little weird that we have a persistent process that may keep writing to stderr. But then, it may still have things to say to stderr (i.e., errors to report while the daemon runs).

But assuming we did want to close stderr, this is definitely the wrong place to do it. Most of the interesting errors would happen while the daemon is starting up, and they would all go to /dev/null with your patch. I'd much prefer to close it in the daemon after it has successfully started up but right before it goes into its event loop:

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 3b370ca..1a5572c 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -211,6 +211,7 @@ static void serve_cache(const char *socket_path)

    printf("ok\n");
    fclose(stdout);
+   fclose(stderr);

    while (serve_cache_loop(fd))
        ; /* nothing */

That at least allows through messages related to opening the socket. We'd still miss errors and warnings that happen when serving individual clients, though. Two things that would make it a little better:

1. Teach the daemon a command-line argument to optionally close stderr (after successfully starting). Use it when auto-starting the daemon from credential-cache. This makes it easy to debug issues by running the daemon manually without that option.
2. Detect pipes via fstat() and only close stderr if it goes to a pipe. This would make things work as now in most cases. It does feel a little dirty, though (e.g., if you piped your stderr to "tee" it would magically drop some messages).

Of the two, I think I prefer the first. But I'd still be curious to hear why you want to pipe stderr in the first place.

[rubyist]

Thanks for taking a look at this @peff, I think the reason we were capturing stderr is because the git media client may not necessarily be started from a terminal, and we'd want to bubble those errors up to whatever is controlling it. However, the stderr that we're really expecting to read from I think would come from the git process itself, not the cache daemon. This problem only shows up in the case where this git credential process is the one that kicks off the cache daemon. If the cache daemon is already running then everything is OK.

The reason I think the code in git has a problem is that I think a daemon should not assume that it has a terminal to write to. Given how this daemon is started from the git process, it could be safe-ish to assume that there is a terminal to write startup errors to, but once it's launched that terminal can go away at any time and the output won't go anywhere (or if some later terminal opens up on the same device it could get the output, and that'd be weird). I suppose the credential cache daemon is not your typical daemon in that it's usually started by the git process, is generally short lived, and probably has a terminal to write to.

With regard to start up errors, you're correct, my patch does prevent them from showing while your patch lets them through. I verified this by chmod'ing ~/.git-credential-cache to 777, which will trigger some output on stderr down in the cache daemon code. If anything, allowing those errors to come through is more useful than not.

I think for now, we will work around the problem by not piping stderr, as you suggest. I think we should be OK with that given the types of errors that might happen at that point. It may be that in the typical use case for the git / cache daemon interaction, allowing the daemon to assume it can write to the terminal is OK, I just wanted to check with some git internals experts to make sure that was the case 😄

[peff]

Yeah, the "bubble up to GUI" thing makes sense. Git-media aside, this could be a problem even for something like GitHub for Mac making a call to git fetch. I think we probably didn't notice it there because they typically use the osxkeychain helper rather than the caching helper.

I just sent a patch to the list.

[rubyist]

@peff Awesome, thanks!

Closing this now, #118 just doesn't hook up stderr.