diff mbox series

[v5,10/12] unix-stream-server: create unix domain socket under lock

Message ID 1ee9de55a106e46dab6126fe8ca2a0aeace57b1a.1615302157.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Simple IPC Mechanism | expand

Commit Message

Jeff Hostetler March 9, 2021, 3:02 p.m. UTC
From: Jeff Hostetler <jeffhost@microsoft.com>

Create a wrapper class for `unix_stream_listen()` that uses a ".lock"
lockfile to create the unix domain socket in a race-free manner.

Unix domain sockets have a fundamental problem on Unix systems because
they persist in the filesystem until they are deleted.  This is
independent of whether a server is actually listening for connections.
Well-behaved servers are expected to delete the socket when they
shutdown.  A new server cannot easily tell if a found socket is
attached to an active server or is leftover cruft from a dead server.
The traditional solution used by `unix_stream_listen()` is to force
delete the socket pathname and then create a new socket.  This solves
the latter (cruft) problem, but in the case of the former, it orphans
the existing server (by stealing the pathname associated with the
socket it is listening on).

We cannot directly use a .lock lockfile to create the socket because
the socket is created by `bind(2)` rather than the `open(2)` mechanism
used by `tempfile.c`.

As an alternative, we hold a plain lockfile ("<path>.lock") as a
mutual exclusion device.  Under the lock, we test if an existing
socket ("<path>") is has an active server.  If not, we create a new
socket and begin listening.  Then we use "rollback" to delete the
lockfile in all cases.

This wrapper code conceptually exists at a higher-level than the core
unix_stream_connect() and unix_stream_listen() routines that it
consumes.  It is isolated in a wrapper class for clarity.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                            |   1 +
 contrib/buildsystems/CMakeLists.txt |   2 +-
 unix-stream-server.c                | 128 ++++++++++++++++++++++++++++
 unix-stream-server.h                |  36 ++++++++
 4 files changed, 166 insertions(+), 1 deletion(-)
 create mode 100644 unix-stream-server.c
 create mode 100644 unix-stream-server.h

Comments

Junio C Hamano March 10, 2021, 12:18 a.m. UTC | #1
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +struct unix_stream_server_socket {
> +int unix_stream_server__create(
> +void unix_stream_server__free(
> +int unix_stream_server__was_stolen(

I think we reserve __ in our API for names of symbols that normal
callers never have to write (both data like git_attr__true[] and
functions like cmd_bisect__helper()).

It seems that list-objects-filter.h may have introduced the
"name_space" followed by "__" followed by "name" convention,
but I am not sure if that is a desirable convention to spread
throughout our codebase.

Also "unix_stream_server" is quite a mouthful.  Perhaps abbreviate
it to uss_ or something?  I dunno if that is too short and invite
confusion with other kinds of uss.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index d3c42d3f4f9f..012694276f6d 100644
--- a/Makefile
+++ b/Makefile
@@ -1665,6 +1665,7 @@  ifdef NO_UNIX_SOCKETS
 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
+	LIB_OBJS += unix-stream-server.o
 endif
 
 ifdef USE_WIN32_IPC
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 40c9e8e3bd9d..c94011269ebb 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -243,7 +243,7 @@  if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
 	add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY )
-	list(APPEND compat_SOURCES unix-socket.c)
+	list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c)
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
diff --git a/unix-stream-server.c b/unix-stream-server.c
new file mode 100644
index 000000000000..5dfe2a9ac2c0
--- /dev/null
+++ b/unix-stream-server.c
@@ -0,0 +1,128 @@ 
+#include "cache.h"
+#include "lockfile.h"
+#include "unix-socket.h"
+#include "unix-stream-server.h"
+
+#define DEFAULT_LOCK_TIMEOUT (100)
+
+/*
+ * Try to connect to a unix domain socket at `path` (if it exists) and
+ * see if there is a server listening.
+ *
+ * We don't know if the socket exists, whether a server died and
+ * failed to cleanup, or whether we have a live server listening, so
+ * we "poke" it.
+ *
+ * We immediately hangup without sending/receiving any data because we
+ * don't know anything about the protocol spoken and don't want to
+ * block while writing/reading data.  It is sufficient to just know
+ * that someone is listening.
+ */
+static int is_another_server_alive(const char *path,
+				   const struct unix_stream_listen_opts *opts)
+{
+	int fd = unix_stream_connect(path, opts->disallow_chdir);
+	if (fd >= 0) {
+		close(fd);
+		return 1;
+	}
+
+	return 0;
+}
+
+int unix_stream_server__create(
+	const char *path,
+	const struct unix_stream_listen_opts *opts,
+	long timeout_ms,
+	struct unix_stream_server_socket **new_server_socket)
+{
+	struct lock_file lock = LOCK_INIT;
+	int fd_socket;
+	struct unix_stream_server_socket *server_socket;
+
+	*new_server_socket = NULL;
+
+	if (timeout_ms < 0)
+		timeout_ms = DEFAULT_LOCK_TIMEOUT;
+
+	/*
+	 * Create a lock at "<path>.lock" if we can.
+	 */
+	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout_ms) < 0)
+		return -1;
+
+	/*
+	 * If another server is listening on "<path>" give up.  We do not
+	 * want to create a socket and steal future connections from them.
+	 */
+	if (is_another_server_alive(path, opts)) {
+		rollback_lock_file(&lock);
+		errno = EADDRINUSE;
+		return -2;
+	}
+
+	/*
+	 * Create and bind to a Unix domain socket at "<path>".
+	 */
+	fd_socket = unix_stream_listen(path, opts);
+	if (fd_socket < 0) {
+		int saved_errno = errno;
+		rollback_lock_file(&lock);
+		errno = saved_errno;
+		return -1;
+	}
+
+	server_socket = xcalloc(1, sizeof(*server_socket));
+	server_socket->path_socket = strdup(path);
+	server_socket->fd_socket = fd_socket;
+	lstat(path, &server_socket->st_socket);
+
+	*new_server_socket = server_socket;
+
+	/*
+	 * Always rollback (just delete) "<path>.lock" because we already created
+	 * "<path>" as a socket and do not want to commit_lock to do the atomic
+	 * rename trick.
+	 */
+	rollback_lock_file(&lock);
+
+	return 0;
+}
+
+void unix_stream_server__free(
+	struct unix_stream_server_socket *server_socket)
+{
+	if (!server_socket)
+		return;
+
+	if (server_socket->fd_socket >= 0) {
+		if (!unix_stream_server__was_stolen(server_socket))
+			unlink(server_socket->path_socket);
+		close(server_socket->fd_socket);
+	}
+
+	free(server_socket->path_socket);
+	free(server_socket);
+}
+
+int unix_stream_server__was_stolen(
+	struct unix_stream_server_socket *server_socket)
+{
+	struct stat st_now;
+
+	if (!server_socket)
+		return 0;
+
+	if (lstat(server_socket->path_socket, &st_now) == -1)
+		return 1;
+
+	if (st_now.st_ino != server_socket->st_socket.st_ino)
+		return 1;
+	if (st_now.st_dev != server_socket->st_socket.st_dev)
+		return 1;
+
+	if (!S_ISSOCK(st_now.st_mode))
+		return 1;
+
+	return 0;
+}
diff --git a/unix-stream-server.h b/unix-stream-server.h
new file mode 100644
index 000000000000..ef9241d0ef70
--- /dev/null
+++ b/unix-stream-server.h
@@ -0,0 +1,36 @@ 
+#ifndef UNIX_STREAM_SERVER_H
+#define UNIX_STREAM_SERVER_H
+
+#include "unix-socket.h"
+
+struct unix_stream_server_socket {
+	char *path_socket;
+	struct stat st_socket;
+	int fd_socket;
+};
+
+/*
+ * Create a Unix Domain Socket at the given path under the protection
+ * of a '.lock' lockfile.
+ *
+ * Returns 0 on success, -1 on error, -2 if socket is in use.
+ */
+int unix_stream_server__create(
+	const char *path,
+	const struct unix_stream_listen_opts *opts,
+	long timeout_ms,
+	struct unix_stream_server_socket **server_socket);
+
+/*
+ * Close and delete the socket.
+ */
+void unix_stream_server__free(
+	struct unix_stream_server_socket *server_socket);
+
+/*
+ * Return 1 if the inode of the pathname to our socket changes.
+ */
+int unix_stream_server__was_stolen(
+	struct unix_stream_server_socket *server_socket);
+
+#endif /* UNIX_STREAM_SERVER_H */