From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 03 Nov 2026 17:24:13 GMT
Subject: Re: [PATCH 2/2] credential-cache--daemon: do not take over a running daemon's socket
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:

> +	/* 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?