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