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