credential-cache: fix two races when daemons start at once

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%[email protected]/

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

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.

---
 builtin/credential-cache--daemon.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 65cc619bec..0dfa11e2ec 100644
@@ -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

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

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

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.

---
 builtin/credential-cache--daemon.c | 29 +++++++++++++++++++++--------
 t/t0301-credential-cache.sh        | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+), 8 deletions(-)

index 0dfa11e2ec..de6720c276 100644
@@ -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;
 }

index 6f7cfd9e33..8e07e55520 100755
@@ -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

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.

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.

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.

The rest of the code changes look as I'd expect.

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%[email protected]/

---
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/[email protected]

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

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

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

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

---
 builtin/credential-cache--daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 65cc619bec..5cc39cd172 100644
@@ -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

Yup, makes sense, and the patch is obviously correct.

Patrick

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.

---
 builtin/credential-cache--daemon.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 5cc39cd172..9e6ed9c922 100644
@@ -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

This sentence is very puzzling. Why do you explain how we create the directory if it's something that your patch doesn't even change? And also, why would anybody even get the idea that it's preferable to use mkdir(dir, 0777) && chmod(0700) over mkdir(dir, 0700)`?

Patrick

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.

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

index 9e6ed9c922..03d314f19f 100644
@@ -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;
 }

index 6f7cfd9e33..8e07e55520 100755
@@ -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

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.

By "discussion" I meant you had flagged some concerns. I considered your response a discussion :)

It was not clear to me as someone unfamiliar with the codebase whether the complexity of handling it was worth it. It seemed like it could only occur during upgrades (old git version).

My comment was meant to say: I did not address it but I can if we think it's necessary.

I am not sure if I follow your "upgrades (old git version)" reference. Doesn't a reasonably serious installation of any software have a babysitter process to notice a dead daemon to automatically respawn to keep the service running?

If you are unfamiliar with the codebase, how would you know which of the careful arrangements the existing code makes can be ignored? If it is not important, what would the reason be for reviewers to spend time and effort to write a message to point it out for you?

I'm not sure how you want me to respond here; I made an attempt at a fix for an issue I came across within the context I felt comfortable to contribute.

I tried to capture that within my cover letter.

I was asking why you needed to question "if it is necessary" in the first place when somebody points out a potential issue in your proposed solution. If it is not necessary to be corrected, I do not see why anybody spends time to point it out.

"It is not necessary to worry about this case because it will not happen for such and such reasons" is a very valid thing to say in response, but "I can address it if we think it's necessary" does not move us forward.