Re: [PATCH 2/2] credential-cache--daemon: do not take over a running daemon's socket
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 3, 2026, 17:24 UTC
- Message-ID
- <xmqqik2dakw2.fsf@gitster.g>
- In-Reply-To
- <20261101-fz-credential-cache-race-v1-2-28f0bac51e11@gmail.com>
Farid Zakaria <farid.m.zakaria@gmail.com> writes:
Show 18 quoted lines
> + /* 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?