Re: [PATCH 07/10] unix-socket: create gentle version of unix_stream_listen()

[Jeff King <peff@peff.net>]

On Tue, Jan 12, 2021 at 03:31:29PM +0000, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create a gentle version of `unix_stream_listen()`.  This version does
> not call `die()` if a socket-fd cannot be created and does not assume
> that it is safe to `unlink()` an existing socket-inode.

The existing one is meant to be gentle. Maybe it is worth fixing it
instead.

> `unix_stream_listen()` uses `unix_stream_socket()` helper function to
> create the socket-fd.  Avoid that helper because it calls `die()` on
> errors.

Yeah, I think this is just a bug. My thinking in the original was that
socket() would basically never fail. And it generally wouldn't, but
things like EMFILE do happen. There are only two callers, and both would
be one-liners to propagate the error up the stack.

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

The trouble here is that one cannot tell if the existing file is active,
and you are orphaning an existing server, or if there is leftover cruft
from an exited server that did not clean up after itself (you will get
EADDRINUSE either way).

Handling those cases (and especially doing so in a non-racy way) is
probably outside the scope of unix_stream_listen(), but it makes sense
for this to be an option. And it looks like you even made it so here,
so unix_stream_listen() could just become a wrapper that sets the
option. Or since there is only one caller in the whole code-base,
perhaps it could just learn to pass the option struct. :)

Likewise for the no-chdir option added in the follow-on patch.

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

I'll be curious to see how you do this atomically. From my skim of patch
10, you will connect to see if it's active, and unlink if it's not. But
then two simultaneous new processes could both see an inactive one and
race to forcefully create the new one. One of them will lose and be
orphaned with a socket that has no filesystem name.

There might be a solution using link() to have an atomic winner, but it
gets tricky around unlinking the old name out of the way. You might need
a separate dot-lock to make sure only one process does the
unlink-and-create process at a time.

-Peff