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

[Jeff King <peff@peff.net>]

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.

> 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.

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.

> +	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.

> @@ -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.

-Peff