[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

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 1/2] credential-cache--daemon: allow a concurrently created socket dir

[Farid Zakaria]

On Tue Nov 3, 2026 at 12:01 AM PST, Patrick Steinhardt wrote:
> 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?
>

Sure, I'll add that as the first patch in v2.

>> -		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 1/2] credential-cache--daemon: allow a concurrently created socket dir

[Jeff King]

On Sun, Nov 01, 2026 at 03:29:25PM -0800, Farid Zakaria wrote:

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

Makes sense. This is what safe_create_leading_directories() does for all
of the other path components. So of course I wondered why we bothered to
do this last one ourselves, but the comment (which is just out of the
context of your patch) explains it:

                /*
                 * We must be sure to create the directory with the correct mode,
                 * not just chmod it after the fact; otherwise, there is a race
                 * condition in which somebody can chdir to it, sleep, then try to open
                 * our protected socket.
                 */

If we find the directory already created, then we are subject to the
same race, but it is reasonable to assume that whoever created it did so
in the correct non-racy way.

-Peff

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

[Farid Zakaria]

On Tue Nov 3, 2026 at 12:01 AM PST, Patrick Steinhardt wrote:
> 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...".
>

When I sent V2 I will fix this.

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

The client in builtin/credential-cache.c:spawn_daemon() reads the
daemon's stdout and dies with "cache daemon did not start" unless it
gets "ok"

"ok" is part of the protocol between the client and the daemon. The
client expects to see "ok" and then it connects to the socket.

I can put this reasoning as a comment in the code. 
Following Peff's suggestion, I'll also exit(0) there
to make clear that this process is done.

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

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

[Jeff King]

On Sun, Nov 01, 2026 at 03:29:26PM -0800, Farid Zakaria wrote:

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

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

> 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

[PATCH v2 0/3] 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/

---
Changes in v2:
- New patch 1: create the socket's leading directories with
  safe_create_leading_directories_no_share_const(), so that
  "core.sharedRepository" does not apply to them (Patrick).
- Patch 2: explain in the commit message why a directory created by
  another daemon can be trusted (Peff).
- Patch 3: explain in a comment why a daemon that finds another one
  listening still prints "ok", and exit(0) there (Patrick, Peff).
  Drop comments that only restated the code (Patrick).
- Reword commit messages in the imperative mood (Patrick).
- Not changed: the socket is still removed at exit through the
  tempfile API, which unlinks it without the unix_ss_was_stolen().
  There was a discussion with Junio about this. I can do that if it's
  a requirement in a V3.
- Link to v1: https://patch.msgid.link/20261101-fz-credential-cache-race-v1-0-28f0bac51e11@gmail.com

---
Farid Zakaria (3):
      credential-cache--daemon: do not apply core.sharedRepository
      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 | 44 ++++++++++++++++++++++++++++----------
 t/t0301-credential-cache.sh        | 23 ++++++++++++++++++++
 2 files changed, 56 insertions(+), 11 deletions(-)

Range-diff versus v1:

-:  ---------- > 1:  b2b02dd510 credential-cache--daemon: do not apply core.sharedRepository
1:  5137f72936 ! 2:  e2310083cd credential-cache--daemon: allow a concurrently created socket dir
    @@ Commit message
     
         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.
    +    Fix this by ignoring EEXIST from mkdir(), and checking the directory's
    +    permissions the same way as for one that already existed. We create
    +    the directory with mode 0700 instead of fixing it up with chmod()
    +    afterwards to avoid a race where another user enters it before the
    +    chmod(). A directory created by another daemon was made the same way,
    +    so trusting it once its permissions check out is as safe as trusting
    +    one that existed before we started.
     
         Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
     
      ## builtin/credential-cache--daemon.c ##
     @@ builtin/credential-cache--daemon.c: static void init_socket_directory(const char *path)
      		 */
    - 		if (safe_create_leading_directories_const(the_repository, dir) < 0)
    + 		if (safe_create_leading_directories_no_share_const(dir) < 0)
      			die_errno("unable to create directories for '%s'", dir);
     -		if (mkdir(dir, 0700) < 0)
     -			die_errno("unable to mkdir '%s'", dir);
2:  0e13ea044c ! 3:  54bc9bd3df credential-cache--daemon: do not take over a running daemon's socket
    @@ Commit message
          - 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
    +    Fix this by creating 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.
    +    other server is listening on the socket. The lock is held only while
    +    binding, so a stale lockfile left behind by a crashed daemon is
    +    unlikely to need manual cleanup.
     
         Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
     
    @@ builtin/credential-cache--daemon.c: static int serve_cache_loop(int fd)
      
     -	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) {
    ++		/*
    ++		 * Another daemon is already listening on the socket. "ok"
    ++		 * tells our client that a daemon is ready, after which the
    ++		 * client connects to the socket and talks to that daemon.
    ++		 */
     +		printf("ok\n");
    -+		return;
    ++		exit(0);
     +	}
     +	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");

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

[PATCH v2 1/3] credential-cache--daemon: do not apply core.sharedRepository

[Farid Zakaria]

init_socket_directory() creates the leading directories of the socket
path with safe_create_leading_directories_const(). That function
honors "core.sharedRepository", which is meant for files inside the
git directory. The socket lives in the user's home directory, e.g.
"~/.cache/git/credential", and the repository the daemon happens to be
started from has no say over its permissions.

Use safe_create_leading_directories_no_share_const() instead, which
ignores "core.sharedRepository".

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

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 65cc619bec..5cc39cd172 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -271,7 +271,7 @@ static void init_socket_directory(const char *path)
 		 * condition in which somebody can chdir to it, sleep, then try to open
 		 * our protected socket.
 		 */
-		if (safe_create_leading_directories_const(the_repository, dir) < 0)
+		if (safe_create_leading_directories_no_share_const(dir) < 0)
 			die_errno("unable to create directories for '%s'", dir);
 		if (mkdir(dir, 0700) < 0)
 			die_errno("unable to mkdir '%s'", dir);

-- 
2.55.0

[PATCH v2 2/3] 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".

Fix this by ignoring EEXIST from mkdir(), and checking the directory's
permissions the same way as for one that already existed. We create
the directory with mode 0700 instead of fixing it up with chmod()
afterwards to avoid a race where another user enters it before the
chmod(). A directory created by another daemon was made the same way,
so trusting it once its permissions check out is as safe as trusting
one that existed before we started.

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 5cc39cd172..9e6ed9c922 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_no_share_const(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 v2 3/3] 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.

Fix this by creating 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. The lock is held only while
binding, so a stale lockfile left behind by a crashed daemon is
unlikely to need manual cleanup.

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

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 9e6ed9c922..03d314f19f 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,25 @@ 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)
+	ret = unix_ss_create(socket_path, &opts, -1, &server_socket);
+	if (ret == -2) {
+		/*
+		 * Another daemon is already listening on the socket. "ok"
+		 * tells our client that a daemon is ready, after which the
+		 * client connects to the socket and talks to that daemon.
+		 */
+		printf("ok\n");
+		exit(0);
+	}
+	if (ret < 0)
 		die_errno("unable to bind to '%s'", socket_path);
 
+	socket_file = register_tempfile(socket_path);
+
 	printf("ok\n");
 	fclose(stdout);
 	if (!debug) {
@@ -244,10 +258,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 +318,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 +345,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 v2 0/3] credential-cache: fix two races when daemons start at once

[Junio C Hamano]

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

> - Not changed: the socket is still removed at exit through the
>   tempfile API, which unlinks it without the unix_ss_was_stolen().
>   There was a discussion with Junio about this. I can do that if it's
>   a requirement in a V3.

Sorry, but I do not recall having any discussion on this.  

I just pointed out a potential problem, and you sent this version
out without responding to my review, so I cannot tell why you
thought it was OK not to address it.  Which means I cannot answer
"if it's a requirement" question.

Perhaps you had a solid technical reason why the "stolen" detection
is not necessary, in which case it might be OK.  Perhaps you were
too lazy to think the issue through, in which case it might not be
OK.  Or there was some other reason why you thought it is OK not to
address the issue.