From: Farid Zakaria Date: Wed, 04 Nov 2026 19:53:13 GMT Subject: [PATCH v2 0/3] credential-cache: fix two races when daemons start at once Message-ID: <20261104-fz-credential-cache-race-v2-0-ddccd0831eba@gmail.com> In-Reply-To: <20261101-fz-credential-cache-race-v1-0-28f0bac51e11@gmail.com> 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 ## 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 @@ 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