Re: [PATCH 2/2] credential-cache--daemon: do not take over a running daemon's socket
- From
Jeff King <peff@peff.net>
- Date
- Nov 4, 2026, 03:07 UTC
- Message-ID
- <20261104030753.GC1492379@coredump.intra.peff.net>
- In-Reply-To
- <20261101-fz-credential-cache-race-v1-2-28f0bac51e11@gmail.com>
On Sun, Nov 01, 2026 at 03:29:26PM -0800, Farid Zakaria wrote:
Show 6 quoted lines
> The fix is to create the socket with unix_ss_create(), which the > simple-ipc server already uses. It takes a lock and binds only if no > other server is listening on the socket. A daemon that finds one prints > "ok" and exits, and its client talks to the running daemon. Register > the socket for removal only after binding it, so a daemon that loses > never removes the other one's socket.
OK. One of the reasons the original did not take a lock is that a stale lockfile might force the user to clean it up manually (rather than everything eventually Just Working).
But unix_ss_create() only holds the lockfile briefly to make the socket acquisition atomic. So I think the risk of stale files is negligible, and worth the tradeoff to avoid races.
Show 20 quoted lines
> static void serve_cache(const char *socket_path, int debug)
> {
> struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
> - int fd;
> + struct unix_ss_socket *server_socket;
> + struct tempfile *socket_file;
> + int ret;
>
> - fd = unix_stream_listen(socket_path, &opts);
> - if (fd < 0)
> + /*
> + * Two clients may each start a daemon at the same time. Bind only
> + * if no other daemon is listening yet; otherwise leave the socket
> + * to that one, and our client will talk to it.
> + */
> + ret = unix_ss_create(socket_path, &opts, -1, &server_socket);
> + if (ret == -2) {
> + printf("ok\n");
> + return;
> + }So if we found that somebody else beat us to it, we still say "ok", which makes sense: we tell the client that _some_ daemon is ready for them to talk to, even if it's not us.
And then conceptually we are done and can exit. The return here does the same thing. I think that's OK, but it might be more obvious to add a comment for the "ok" and just call exit(0). I am happy enough either way.
> + /* Remove the socket at exit only now that it is ours. */ > + socket_file = register_tempfile(socket_path);
And then tempfile handling moves in here from the caller, because we might or might not actually make a socket. Makes sense.
> [...]
The rest of the code changes look as I'd expect.
Show 30 quoted lines
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh > index 6f7cfd9e33..8e07e55520 100755 > --- a/t/t0301-credential-cache.sh > +++ b/t/t0301-credential-cache.sh > @@ -51,6 +51,29 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' ' > test_path_is_socket "$HOME/.cache/git/credential/socket" > ' > > +test_expect_success 'a second daemon does not take over the socket' ' > + test_when_finished "git credential-cache exit" && > + check approve cache <<-\EOF && > + protocol=https > + host=example.com > + username=store-user > + password=store-pass > + EOF > + git credential-cache--daemon "$HOME/.cache/git/credential/socket" >out && > + echo ok >expect && > + test_cmp expect out && > + check fill cache <<-\EOF > + protocol=https > + host=example.com > + -- > + protocol=https > + host=example.com > + username=store-user > + password=store-pass > + -- > + EOF > +'
I'd have thought this would pass even before the patch, but perhaps not. Usually we rely on the "credential-cache" client doing a ping to avoid starting a second daemon, but here we do it explicitly. So probably it did just take over the other one, but nobody would really hit that case outside of the race between the client pings and the daemons actually starting.
So overall, I think the patch looks good.
-Peff