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