From: Jeff Hostetler Date: Fri, 05 Feb 2021 23:28:13 GMT Subject: Re: [PATCH v2 11/14] unix-socket: add options to unix_stream_listen() Message-ID: In-Reply-To: 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 >> >> 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 >