Re: [PATCH 2/2] credential-cache--daemon: do not take over a running daemon's socket

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 3, 2026, 08:01 UTC
Message-ID: <aumUyl6MQa43OFBs@pks.im>
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 16 quoted lines

> When no daemon is running, two "git credential-cache store" run at the
> same time each try to start one. unix_stream_listen() removes whatever
> is at the socket path before it binds, so the second daemon takes over
> the socket of the first. The first daemon keeps running, but no client
> can reach it:
> 
>  - the credential its client stored is lost;
> 
>  - "git credential-cache exit" does not stop it, and it keeps the
>    password in memory until it times out;
> 
>  - when it does exit, it removes the socket of the daemon that is
>    still running, losing that daemon's credentials too.
> 
> The fix is to create the socket with unix_ss_create(), which the
> simple-ipc server already uses.

We usually write commit messages as if instructing the code to change.
So: "Fix that by using `unix_ss_create()` instead...".

Show 20 quoted lines

> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index 0dfa11e2ec..de6720c276 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -231,12 +232,26 @@ static int serve_cache_loop(int fd)
>  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.
> +	 */

This explains what you do without giving much context why you do it, so this comment is somewhat useless. Most importantly...

> +	ret = unix_ss_create(socket_path, &opts, -1, &server_socket);
> +	if (ret == -2) {
> +		printf("ok\n");
> +		return;

... you mention that you write "ok" in your commit message, but you don't explain why that's the right thing to do. So why should the daemon pretend that everything was alright in the case where it fails to start?

Show 6 quoted lines

> +	}
> +	if (ret < 0)
>  		die_errno("unable to bind to '%s'", socket_path);
>  
> +	/* Remove the socket at exit only now that it is ours. */
> +	socket_file = register_tempfile(socket_path);

This comment doesn't really add much value, if you ask me.

Patrick