Re: [PATCH v2 11/14] unix-socket: add options to unix_stream_listen()

[Jeff Hostetler <git@jeffhostetler.com>]

On 2/2/21 5:14 AM, Jeff King wrote:
> On Mon, Feb 01, 2021 at 07:45:44PM +0000, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Update `unix_stream_listen()` to take an options structure to override
>> default behaviors.  This includes the size of the `listen()` backlog
>> and whether it should always unlink the socket file before trying to
>> create a new one.  Also eliminate calls to `die()` if it cannot create
>> a socket.
> 
> I sent a follow-up on the previous patch, but I think this part about
> the die() should be folded in there.
> 
> Likewise I think it would probably be easier to follow if we added the
> backlog parameter and the unlink options in separate patches. The
> backlog thing is small, but the unlink part is subtle and requires
> explanation. That's a good sign it might do better in its own commit.

Yes, that helped having them in 2 patches each with 1 concern.

> 
>> Normally, `unix_stream_listen()` always tries to `unlink()` the
>> socket-path before calling `bind()`.  If there is an existing
>> server/daemon already bound and listening on that socket-path, our
>> `unlink()` would have the effect of disassociating the existing
>> server's bound-socket-fd from the socket-path without notifying the
>> existing server.  The existing server could continue to service
>> existing connections (accepted-socket-fd's), but would not receive any
>> futher new connections (since clients rendezvous via the socket-path).
>> The existing server would effectively be offline but yet appear to be
>> active.
>>
>> Furthermore, `unix_stream_listen()` creates an opportunity for a brief
>> race condition for connecting clients if they try to connect in the
>> interval between the forced `unlink()` and the subsequent `bind()` (which
>> recreates the socket-path that is bound to a new socket-fd in the current
>> process).
> 
> OK. I'm still not sure of the endgame here for writing non-racy code to
> establish the socket (which is going to require either some atomic
> renaming or some dot-locking in the caller).  But it's plausible to me
> that this option will be a useful primitive.

In part 14/14 in `ipc-unix-sockets.c:create_listener_socket()` I have
code in the calling layer to (try to) handle both the startup races
and basic collisions with existing long-running servers already using
the socket.

But you're right, it might be good to revisit that as a primitive at
this layer.  We only have 1 other caller right now and I don't know
enough about `credential-cache--daemon` to know if it would benefit
from this or not.

> 
> The implementation looks correct, though here are a few small
> observations/questions/nits:
> 
>> -int unix_stream_listen(const char *path)
>> +int unix_stream_listen(const char *path,
>> +		       const struct unix_stream_listen_opts *opts)
>>   {
>> -	int fd, saved_errno;
>> +	int fd = -1;
>> +	int saved_errno;
>> +	int bind_successful = 0;
>> +	int backlog;
>>   	struct sockaddr_un sa;
>>   	struct unix_sockaddr_context ctx;
>>   
>> -	unlink(path);
>> -
>>   	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
>>   		return -1;
> 
> We can return directly here, because we know there is nothing to clean
> up. Which I thought mean that here...
> 
>> +
>>   	fd = socket(AF_UNIX, SOCK_STREAM, 0);
>>   	if (fd < 0)
>> -		die_errno("unable to create socket");
>> +		goto fail;
> 
> ...we are in the same boat. We did not create a socket, so we can just
> return. That makes our cleanup code a bit simpler. But we can't do that,
> because unix_sockaddr_init() may have done things that need cleaning up
> (like chdir). So what you have here is correct.
> 
> IMHO that is all the more reason to push this (and the similar code in
> unix_stream_connect() added in patch 13) into the previous patch.

Agreed.

> 
>> +	if (opts->force_unlink_before_bind)
>> +		unlink(path);
>>   
>>   	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
>>   		goto fail;
>> +	bind_successful = 1;
> 
> And this one needs to mark a flag explicitly, because we have no other
> visible way of knowing we need to do the unlink. Makes sense.
> 
>> -	if (listen(fd, 5) < 0)
>> +	if (opts->listen_backlog_size > 0)
>> +		backlog = opts->listen_backlog_size;
>> +	else
>> +		backlog = 5;
>> +	if (listen(fd, backlog) < 0)
> 
> The default-to-5 is a bit funny here. We already set the default to 5 in
> UNIX_STREAM_LISTEN_OPTS_INIT. Should it be "0" there, so callers can
> treat that as "use the default", which we fill in here? It probably
> doesn't matter much in practice, but it seems cleaner to have only one
> spot with the magic number.

I'll refactor this a bit.

> 
>> @@ -114,7 +125,10 @@ int unix_stream_listen(const char *path)
>>   fail:
>>   	saved_errno = errno;
>>   	unix_sockaddr_cleanup(&ctx);
>> -	close(fd);
>> +	if (fd != -1)
>> +		close(fd);
>> +	if (bind_successful)
>> +		unlink(path);
>>   	errno = saved_errno;
>>   	return -1;
>>   }
> 
> Should we unlink before closing? I usually try to undo actions in the
> reverse order that they were done. I thought at first it might even
> matter here, such that we'd atomically relinquish the name without
> having a moment where it still points to a closed socket (which might be
> less confusing to somebody else trying to connect). But I guess there
> will always be such a moment, because it's not like we would ever
> accept() or service a request.

I'm not sure it matters, but it does look better to unwind things
in reverse order.  And yes, unlinking first is a little bit safer.

> 
> -Peff
>