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

Commit Message

Jeff Hostetler Jan. 12, 2021, 3:31 p.m. UTC

From: Jeff Hostetler <[e-mail]>

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.

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

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

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 unix-socket.c | 39 +++++++++++++++++++++++++++++++++++++++
 unix-socket.h |  8 ++++++++
 2 files changed, 47 insertions(+)

Comments

Jeff King Jan. 13, 2021, 2:06 p.m. UTC | #1

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

> From: Jeff Hostetler <[e-mail]>
> 
> 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

Chris Torek Jan. 14, 2021, 1:19 a.m. UTC | #2

I had saved this to comment on, but Peff beat me to it :-)

On Wed, Jan 13, 2021 at 6:07 AM Jeff King <[e-mail]> wrote:
> 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 definitely should be able to do this atomically with link(), but
the cleanup is indeed messy, and there's already existing locking
code, so it's probably better to press that into service here.

Chris

Patch

diff --git a/unix-socket.c b/unix-socket.c
index 19ed48be990..3a9ffc32268 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -121,3 +121,42 @@  int unix_stream_listen(const char *path)
 	errno = saved_errno;
 	return -1;
 }
+
+int unix_stream_listen_gently(const char *path,
+			      const struct unix_stream_listen_opts *opts)
+{
+	int fd = -1;
+	int bind_successful = 0;
+	int saved_errno;
+	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
+
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		goto fail;
+
+	fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (fd < 0)
+		goto fail;
+
+	if (opts->force_unlink_before_bind)
+		unlink(path);
+
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+		goto fail;
+	bind_successful = 1;
+
+	if (listen(fd, opts->listen_backlog_size) < 0)
+		goto fail;
+
+	unix_sockaddr_cleanup(&ctx);
+	return fd;
+
+fail:
+	saved_errno = errno;
+	unix_sockaddr_cleanup(&ctx);
+	close(fd);
+	if (bind_successful)
+		unlink(path);
+	errno = saved_errno;
+	return -1;
+}
diff --git a/unix-socket.h b/unix-socket.h
index e271aeec5a0..253f579f087 100644
--- a/unix-socket.h
+++ b/unix-socket.h
@@ -4,4 +4,12 @@ 
 int unix_stream_connect(const char *path);
 int unix_stream_listen(const char *path);
 
+struct unix_stream_listen_opts {
+	int listen_backlog_size;
+	unsigned int force_unlink_before_bind:1;
+};
+
+int unix_stream_listen_gently(const char *path,
+			      const struct unix_stream_listen_opts *opts);
+
 #endif /* UNIX_SOCKET_H */