[PATCH 0/2] credential-cache: fix two races when daemons start at once

[PATCH 0/2] credential-cache: fix two races when daemons start at once

[Farid Zakaria]

When no credential-cache daemon is running, two "git credential-cache
store" which run at the same time each try to start a daemon.

The second daemon unlinks the first one's socket and binds its own,
which means any credential stored by the first one is lost.

By products of this race:
  - "git credential-cache get" does not return that credential;
  - "git credential-cache exit" stops only the second daemon, and the
   first keeps the password in memory until it expires;
  - when the first daemon exits, its tempfile handler removes
   the socket of the second daemon, whose credentials are then lost
   too.

This can happen whenever two git commands store a password at the
same moment, for example parallel submodule fetches over HTTPS.

I found this with Rewind [1], a deterministic VM I am building that
replays a program under many thread schedules until one fails. 

Context from some history spelunking with an AI:
  Jeff noted in 2021 that the daemon's socket setup is racy [2], and
  expected the worst case to be a daemon that waits 30 seconds for data
  before it exits. That holds only if the losing daemon gets no
  credential, but its own client stores one right after it starts.

[1] https://rewindvm.dev/
[2] https://lore.kernel.org/git/YCK5K%2FnK4tGnTvou@coredump.intra.peff.net/

---
Farid Zakaria (2):
      credential-cache--daemon: allow a concurrently created socket dir
      credential-cache--daemon: do not take over a running daemon's socket

 builtin/credential-cache--daemon.c | 43 +++++++++++++++++++++++++++++---------
 t/t0301-credential-cache.sh        | 23 ++++++++++++++++++++
 2 files changed, 56 insertions(+), 10 deletions(-)


---
base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
change-id: 20261101-fz-credential-cache-race-4504110c6415

[PATCH 1/2] credential-cache--daemon: allow a concurrently created socket dir

[Farid Zakaria]

When no daemon is running, two "git credential-cache store" run at the
same time each try to start one. If the socket directory does not exist
yet, both daemons try to create it. The one that comes second dies:

    fatal: unable to mkdir '<home>/.cache/git/credential': File exists

That client then fails with "cache daemon did not start".

The fix is to ignore EEXIST from mkdir(), and check the directory's
permissions the same way as for one that already existed.

Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
---
 builtin/credential-cache--daemon.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 65cc619bec..0dfa11e2ec 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -273,8 +273,18 @@ static void init_socket_directory(const char *path)
 		 */
 		if (safe_create_leading_directories_const(the_repository, dir) < 0)
 			die_errno("unable to create directories for '%s'", dir);
-		if (mkdir(dir, 0700) < 0)
-			die_errno("unable to mkdir '%s'", dir);
+		if (mkdir(dir, 0700) < 0) {
+			if (errno != EEXIST)
+				die_errno("unable to mkdir '%s'", dir);
+			/*
+			 * Another daemon created it first; check it like one
+			 * that already existed.
+			 */
+			if (stat(dir, &st))
+				die_errno("unable to stat '%s'", dir);
+			if (st.st_mode & 077)
+				die(_(permissions_advice), dir);
+		}
 	}
 
 	if (chdir(dir))

-- 
2.55.0

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

[Farid Zakaria]

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

Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
---
 builtin/credential-cache--daemon.c | 29 +++++++++++++++++++++--------
 t/t0301-credential-cache.sh        | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+), 8 deletions(-)

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
@@ -11,6 +11,7 @@
 #include "tempfile.h"
 #include "credential.h"
 #include "unix-socket.h"
+#include "unix-stream-server.h"
 
 struct credential_cache_entry {
 	struct credential item;
@@ -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.
+	 */
+	ret = unix_ss_create(socket_path, &opts, -1, &server_socket);
+	if (ret == -2) {
+		printf("ok\n");
+		return;
+	}
+	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);
+
 	printf("ok\n");
 	fclose(stdout);
 	if (!debug) {
@@ -244,10 +259,11 @@ static void serve_cache(const char *socket_path, int debug)
 			die_errno("unable to point stderr to /dev/null");
 	}
 
-	while (serve_cache_loop(fd))
+	while (serve_cache_loop(server_socket->fd_socket))
 		; /* nothing */
 
-	close(fd);
+	delete_tempfile(&socket_file);
+	unix_ss_free(server_socket);
 }
 
 static const char permissions_advice[] = N_(
@@ -303,7 +319,6 @@ int cmd_credential_cache_daemon(int argc,
 				const char *prefix,
 				struct repository *repo UNUSED)
 {
-	struct tempfile *socket_file;
 	const char *socket_path;
 	int ignore_sighup = 0;
 	static const char *usage[] = {
@@ -331,13 +346,11 @@ int cmd_credential_cache_daemon(int argc,
 		die("socket directory must be an absolute path");
 
 	init_socket_directory(socket_path);
-	socket_file = register_tempfile(socket_path);
 
 	if (ignore_sighup)
 		signal(SIGHUP, SIG_IGN);
 
 	serve_cache(socket_path, debug);
-	delete_tempfile(&socket_file);
 
 	return 0;
 }
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
+'
+
 XDG_CACHE_HOME="$HOME/xdg"
 export XDG_CACHE_HOME
 # test behavior when XDG_CACHE_HOME is set

-- 
2.55.0

Re: [PATCH 1/2] credential-cache--daemon: allow a concurrently created socket dir

[Patrick Steinhardt]

On Sun, Nov 01, 2026 at 03:29:25PM -0800, Farid Zakaria wrote:
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index 65cc619bec..0dfa11e2ec 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -273,8 +273,18 @@ static void init_socket_directory(const char *path)
>  		 */
>  		if (safe_create_leading_directories_const(the_repository, dir) < 0)
>  			die_errno("unable to create directories for '%s'", dir);

It's quite curious that we use `safe_create_leading_directories()` here.
As documented in "path.h", the `safe_...()` functions should only ever
be used for creating directories in the ".git" directory itself. In
particular, applying "core.sharedRepostiory" to the leading directories
of our credential daemon is never going to result in anything good at
all.

So we should call `safe_create_leadingdirectories_no_share_const()`
instead, which does not apply "core.sharedRepository". I know that's not
introduced by your patch, but would you mind maybe fixing that in a
preparatory commit?

> -		if (mkdir(dir, 0700) < 0)
> -			die_errno("unable to mkdir '%s'", dir);
> +		if (mkdir(dir, 0700) < 0) {
> +			if (errno != EEXIST)
> +				die_errno("unable to mkdir '%s'", dir);

Makes sense.

> +			/*
> +			 * Another daemon created it first; check it like one
> +			 * that already existed.
> +			 */
> +			if (stat(dir, &st))
> +				die_errno("unable to stat '%s'", dir);
> +			if (st.st_mode & 077)
> +				die(_(permissions_advice), dir);
> +		}

A bit of a shame that we duplicate this logic, but I don't see an
obvious way for how to avoid that.

Patrick

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

[Patrick Steinhardt]

On Sun, Nov 01, 2026 at 03:29:26PM -0800, Farid Zakaria wrote:
> 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...".

> 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?

> +	}
> +	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

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

[Junio C Hamano]

Farid Zakaria <farid.m.zakaria@gmail.com> writes:

> +	/* Remove the socket at exit only now that it is ours. */
> +	socket_file = register_tempfile(socket_path);
> +
>  	printf("ok\n");
>  	fclose(stdout);
>  	if (!debug) {
> @@ -244,10 +259,11 @@ static void serve_cache(const char *socket_path, int debug)
>  			die_errno("unable to point stderr to /dev/null");
>  	}
>  
> -	while (serve_cache_loop(fd))
> +	while (serve_cache_loop(server_socket->fd_socket))
>  		; /* nothing */
>  
> -	close(fd);
> +	delete_tempfile(&socket_file);
> +	unix_ss_free(server_socket);
>  }

Is it sane to mix unix_ss_* API with tempfile like this?  For Unix
domain sockets managed by the unix_ss_* API, isn't
register_tempfile() unnecessary and architecturally undesirable?

If the daemon exits abruptly without cleaning up its socket, it is
entirely harmless.  The next time a daemon starts, unix_ss_create()
will find that the socket is stale (is_another_server_alive() will
return 0), and it will safely unlink() the stale socket before
binding.

E.g., fsmonitor--daemon.c calls ipc_server_init_async(), which is
implemented in ipc-unix-socket.c by calling setup_listener_socket(),
which in turn calls create_listener_socket(), which in turn finally
calls unix-stream-server.c:unix_ss_create().  In that function, we
check if somebody else is listening or if the on-filesystem socket
is stale, unlink and rebind by calling unix_stream_listen(), and
continue.

By invoking register_tempfile(), the Unix socket would be managed
by the tempfile API.  At normal exit, delete_tempfile(&socket_file)
blindly unlinks the socket file.  If the daemon is killed abruptly by
a signal (e.g., SIGTERM), the tempfile API's signal handler will
similarly unlink the socket blindly.  Doesn't this completely bypass
the safety built into unix_ss_free(), though?  The API deliberately
calls unix_ss_was_stolen() (which checks the inode) to prevent a
dying daemon from unlinking a socket that has since been taken over
by a new daemon, but delete_tempfile() has no way of knowing that.
It runs first (or runs in the signal handler) and blindly unlinks
the socket, defeating the safety check, no?