[PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd

[Alyssa Ross <hi@alyssa.is>]

vsockserver-socketbinder creates and listens on a socket, and
vsockserverd accepts connections and sets up file descriptors.

vsockserver previously did both of these things in one big program,
but now it just sets up a command line to run vsockserver-socketbinder
followed by vsockserverd.  Having two seperate programs allows one
program to be used in situations where the other is not
suitable (e.g. using vsockserver-socketbinder to create a socket in
situations where accept behaviour more complex than vsockserverd can
provide is required).

This design is taken from s6[1], which uses the same design for its
s6-ipcserver, s6-ipcserver-socketbinder, and s6-ipcserverd programs.

[1]: https://skarnet.org/software/s6/
---

Because vsockserver.c is completely rewritten in this patch, the diff
generated by git was a bit confusing, so the diff here has been
lovingly hand-edited to group the hunks together more and make it easy
to see the new code all at once.  Being able to do this is one of the
nice things about email patches. ;)

The new implementations are much more thoroughly commented.  Think
that's my new style. :)

 .gitignore                      |   2 +
 Makefile.in                     |  14 ++-
 vsockserver-socketbinder.c      |  86 ++++++++++++++++++
 vsockserver.c                   | 149 ++++++++++++++------------------
 vsockserver.c => vsockserverd.c |  65 ++++++--------
 5 files changed, 186 insertions(+), 130 deletions(-)
 create mode 100644 vsockserver-socketbinder.c
 copy vsockserver.c => vsockserverd.c (64%)

diff --git a/.gitignore b/.gitignore
index 89a0408..d29518a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,6 +4,8 @@
 *.o
 *.tmp
 vsockclient
+vsockserver-socketbinder
+vsockserverd
 vsockserver
 config.h
 Makefile
diff --git a/Makefile.in b/Makefile.in
index 3260e85..0e65f1f 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -10,7 +10,7 @@ INSTALL_PROGRAM = $(INSTALL)
 prefix = @PREFIX@
 bindir = @BINDIR@
 
-PROGRAMS = vsockclient vsockserver
+PROGRAMS = vsockclient vsockserver-socketbinder vsockserverd vsockserver
 
 all: $(PROGRAMS)
 .PHONY: all
@@ -26,11 +26,17 @@ config.h: configure
 
 vsockclient: vsockclient.o env.o log.o num.o vsock.o
 	$(CC) $(LDFLAGS) -o $@ $@.o env.o log.o num.o vsock.o $(LDLIBS)
-vsockserver: vsockserver.o env.o log.o num.o vsock.o
-	$(CC) $(LDFLAGS) -o $@ $@.o env.o log.o num.o vsock.o $(LDLIBS)
+vsockserver-socketbinder: vsockserver-socketbinder.o log.o num.o vsock.o
+	$(CC) $(LDFLAGS) -o $@ $@.o log.o num.o vsock.o $(LDLIBS)
+vsockserverd: vsockserverd.o env.o log.o vsock.o
+	$(CC) $(LDFLAGS) -o $@ $@.o env.o log.o vsock.o $(LDLIBS)
+vsockserver: vsockserver.o exec.o log.o
+	$(CC) $(LDFLAGS) -o $@ $@.o exec.o log.o $(LDLIBS)
 
 vsockclient.o: env.h log.h num.h vsock.h
-vsockserver.o: env.h log.h num.h vsock.h
+vsockserver-socketbinder.o: log.h num.h vsock.h
+vsockserverd.o: env.h log.h vsock.h
+vsockserver.o: config.h exec.h log.h
 
 clean:
 	rm -f $(PROGRAMS) *.o
diff --git a/vsockserver-socketbinder.c b/vsockserver-socketbinder.c
new file mode 100644
index 0000000..598c01c
--- /dev/null
+++ b/vsockserver-socketbinder.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is>
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdnoreturn.h>
+#include <string.h>
+#include <sysexits.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include <linux/vm_sockets.h>
+
+#include "log.h"
+#include "num.h"
+#include "vsock.h"
+
+noreturn static void ex_usage(void)
+{
+	if (verbosity)
+		fprintf(stderr, "Usage: %s cid port prog...\n",
+			program_invocation_short_name);
+	exit(EX_USAGE);
+}
+
+int main(int argc, char *argv[])
+{
+	int opt, fd;
+	uint32_t cid, port;
+
+	// A skeleton of an option parser to reject any options that
+	// are given, so we can add options in the future without
+	// worrying about breaking backwards compatibility because
+	// they were previously interpreted as a first argument.
+	while ((opt = getopt(argc, argv, "+")) != -1) {
+		switch (opt) {
+		default:
+			ex_usage();
+		}
+	}
+
+	// Check there are enough positional arguments (two for the
+	// address and at least one to exec into).
+	if (optind > argc - 3)
+		ex_usage();
+
+	// Parse the `cid' argument.
+	if (!strcmp(argv[optind], "-1"))
+		cid = VMADDR_CID_ANY;
+	else if (getu32(argv[optind], 0,  UINT32_MAX, &cid))
+		ex_usage();
+	optind++;
+
+	// Parse the `port' argument.
+	if (!strcmp(argv[optind], "-1"))
+		port = VMADDR_PORT_ANY;
+	else if (getu32(argv[optind], 0, UINT32_MAX, &port))
+		ex_usage();
+	optind++;
+
+	// Set up the socket.
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (fd == -1)
+		diee(EX_OSERR, "socket");
+	if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1)
+		diee(EX_OSERR, "fcntl");
+	if (vsock_bind(fd, cid, port) == -1)
+		diee(EX_OSERR, "bind");
+	if (listen(fd ,40) == -1)
+		diee(EX_OSERR, "listen");
+
+	// Place the socket at stdout.
+	if (dup2(fd, STDIN_FILENO) == -1)
+		diee(EX_OSERR, "dup2");
+	if (fd != STDIN_FILENO)
+		close(fd);
+
+	// Finally, exec into `prog'.
+	execvp(argv[optind], &argv[optind]);
+	diee(EX_OSERR, "execvp");
+}
diff --git a/vsockserver.c b/vsockserver.c
index 43307d2..f740a8a 100644
--- a/vsockserver.c
+++ b/vsockserver.c
@@ -1,28 +1,20 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is>
+// SPDX-FileCopyrightText: 2021 Alyssa Ross <hi@alyssa.is>
 
 #define _GNU_SOURCE
 
+#include <argz.h>
 #include <errno.h>
-#include <fcntl.h>
-#include <inttypes.h>
-#include <poll.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdnoreturn.h>
-#include <string.h>
-#include <sys/socket.h>
-#include <sys/wait.h>
 #include <sysexits.h>
 #include <unistd.h>
 
-#include <linux/vm_sockets.h>
-
-#include "env.h"
+#include "config.h"
+#include "exec.h"
 #include "log.h"
-#include "num.h"
-#include "vsock.h"
 
 noreturn static void ex_usage(void)
 {
@@ -34,102 +26,87 @@ noreturn static void ex_usage(void)
 
 int main(int argc, char *argv[])
 {
-	bool notify = false;
 	int opt;
-	pid_t child;
-	uint32_t lcid, lport, rcid, rport;
+	bool alloc_failed = false;
+	size_t binder_opts_len = 0, daemon_opts_len = 0;
+	char *binder_opts = NULL, *daemon_opts = NULL;
+
+	// The heavy lifting here is done by vsockserver-socketbinder
+	// and vsockserverd.  All this program does is munge argv
+	// appropriately to set the right options for each of those
+	// programs, and then exec into vsockserver-socketbinder.
+
+	// If allocation fails, we need to keep going until after
+	// we've parsed the arguments, so we know what our verbosity
+	// setting is, and consequently whether we should print an
+	// error message about the allocation failure.
+	alloc_failed |=
+		argz_add(&binder_opts, &binder_opts_len, BINDIR "/vsockserver-socketbinder") ||
+		argz_add(&daemon_opts, &daemon_opts_len, BINDIR "/vsockserverd");
 
 	while ((opt = getopt(argc, argv, "+1qQv")) != -1) {
+		char *arg = NULL;
+
 		switch (opt) {
 		case '1':
-			notify = true;
-			break;
 		case 'q':
 		case 'Q':
 		case 'v':
 			set_verbosity(opt);
+			alloc_failed |=
+				asprintf(&arg, "-%c", opt) == -1 ||
+				argz_add(&daemon_opts, &daemon_opts_len, arg);
+			free(arg);
 			break;
 		default:
 			ex_usage();
 		}
 	}
 
+	// Now that verbosity is set, we can whether we were sitting
+	// on an allocation failure.
+	if (alloc_failed)
+		diee(EX_OSERR, "malloc");
+	// Now we don't have to keep checking alloc_failed before
+	// doing anything, and we can deal with allocation failures
+	// after this point by just terminating immediately.
+
-	// Check there are enough positional arguments (two for the
-	// address and at least one to exec into).
+	// Check there are `cid' and `port' arguments to pass to
+	// vsockserver-socketbinder, and at least one `prog' argument
+	// to pass to vsockserverd.
 	if (optind > argc - 3)
 		ex_usage();
 
-	if (!strcmp(argv[optind], "-1"))
-		lcid = VMADDR_CID_ANY;
-	else if (getu32(argv[optind], 0, UINT32_MAX, &lcid))
-		ex_usage();
-	optind++;
-
-	if (!strcmp(argv[optind], "-1"))
-		lport = VMADDR_PORT_ANY;
-	else if (getu32(argv[optind], 0, UINT32_MAX, &lport))
-		ex_usage();
-	optind++;
-
-	int fd = socket(AF_VSOCK, SOCK_STREAM, 0);
-	if (fd == -1)
-		diee(EX_OSERR, "socket");
-
-	if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1)
-		diee(EX_OSERR, "fcntl");
-
-	if (vsock_bind(fd, lcid, lport) == -1)
-		diee(EX_OSERR, "bind");
-
-	if (listen(fd, 40) == -1)
-		diee(EX_OSERR, "listen");
-
-	if (lport == VMADDR_PORT_ANY)
-		if (vsock_get_cid_and_port(fd, NULL, &lport) == -1)
-			diee(EX_OSERR, "getsockname");
-
-	if (notify) {
-		printf("%" PRIu32 "\n", lport);
-		fclose(stdout);
-	}
-
-	setenvf("VSOCKLOCALCID", 1, "%" PRIu32, lcid);
-	setenvf("VSOCKLOCALPORT", 1, "%" PRIu32, lport);
-
-	ilog("listening as %" PRIu32 " on port %" PRIu32, lcid, lport);
-
-	struct pollfd poll_fd = { .fd = fd, .events = POLL_IN, .revents = 0 };
-	while (poll(&poll_fd, 1, -1) != -1) {
-		// On Linux, conn will be blocking.  On other
-		// platforms, this may not be the case.  If other
-		// platforms are to be supported, we'd probably want
-		// to set O_NONBLOCK here.
-		int conn = vsock_accept(fd, &rcid, &rport);
-		if (conn == -1)
-			diee(EX_OSERR, "accept");
-
-		setenvf("VSOCKREMOTECID", 1, "%" PRIu32, rcid);
-		setenvf("VSOCKREMOTEPORT", 1, "%" PRIu32, rport);
-
-		ilog("connection from %" PRIu32 " port %" PRIu32, rcid, rport);
-
-		switch (child = fork()) {
-		case -1: diee(EX_OSERR, "fork");
-		case 0:
-			if (dup2(conn, STDIN_FILENO) == -1)
-				diee(EX_OSERR, "dup2");
-			if (dup2(conn, STDOUT_FILENO) == -1)
-				diee(EX_OSERR, "dup2");
-			if (conn != STDIN_FILENO && conn != STDOUT_FILENO)
-				close(conn);
-			execvp(argv[optind], &argv[optind]);
-			diee(EX_OSERR, "exec");
-		}
-
-		if (waitpid(child, NULL, 0) == -1)
-			diee(EX_OSERR, "waitpid");
-
-		close(conn);
-	}
-	diee(EX_OSERR, "poll");
+	// Add `cid' and `port' arguments to binder options.
+	if (argz_add(&binder_opts, &binder_opts_len, "--") ||
+	    argz_add(&binder_opts, &binder_opts_len, argv[optind++]) ||
+	    argz_add(&binder_opts, &binder_opts_len, argv[optind++]))
+		diee(EX_OSERR, "malloc");
+
+	// Add all of daemon_opts onto the end of binder_opts.  It's
+	// okay to multiply to find the size because if it would
+	// overflow calloc would have failed earlier.
+	if (argz_append(&binder_opts, &binder_opts_len, daemon_opts, daemon_opts_len))
+		diee(EX_OSERR, "malloc");
+	free(daemon_opts);
+
+	// Append `prog' to binder_opts.  Technically this should be
+	// part of daemon_opts, but then we'd just be copying it into
+	// one place to immediately copy it elsewhere.
+	for (int i = optind; i < argc; i++)
+		if (argz_add(&binder_opts, &binder_opts_len, argv[i]))
+			diee(EX_OSERR, "malloc");
+
+	if (verbosity == all) {
+		char *opt;
+
+		// Log the full argv before we exec it.
+		fprintf(stderr, "%s: executing", program_invocation_short_name);
+		while ((opt = argz_next(binder_opts, binder_opts_len, opt)))
+			fprintf(stderr, " %s", opt);
+		fputc('\n', stderr);
+	}
+
+	execzp(binder_opts, binder_opts, binder_opts_len);
+	diee(EX_OSERR, "execvp");
 }
diff --git a/vsockserver.c b/vsockserverd.c
similarity index 64%
copy from vsockserver.c
copy to vsockserverd.c
index 43307d2..1abfff9 100644
--- a/vsockserver.c
+++ b/vsockserverd.c
@@ -4,30 +4,25 @@
 #define _GNU_SOURCE
 
 #include <errno.h>
-#include <fcntl.h>
 #include <inttypes.h>
 #include <poll.h>
 #include <stdbool.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdnoreturn.h>
-#include <string.h>
-#include <sys/socket.h>
 #include <sys/wait.h>
 #include <sysexits.h>
 #include <unistd.h>
 
-#include <linux/vm_sockets.h>
-
 #include "env.h"
 #include "log.h"
-#include "num.h"
 #include "vsock.h"
 
 noreturn static void ex_usage(void)
 {
 	if (verbosity)
-		fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] cid port prog...\n",
+		fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] prog...\n",
 			program_invocation_short_name);
 	exit(EX_USAGE);
 }
@@ -38,6 +33,8 @@ int main(int argc, char *argv[])
 	int opt;
 	pid_t child;
 	uint32_t lcid, lport, rcid, rport;
+	struct pollfd poll_fd =
+		{ .fd = STDIN_FILENO, .events = POLLIN, .revents = 0 };
 
 	while ((opt = getopt(argc, argv, "+1qQv")) != -1) {
 		switch (opt) {
@@ -54,60 +51,42 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	// Check there are enough positional arguments (two for the
-	// address and at least one to exec into).
-	if (optind > argc - 3)
+	// Check that there is something for us to exec into.
+	if (optind > argc - 1)
 		ex_usage();
 
-	if (!strcmp(argv[optind], "-1"))
-		lcid = VMADDR_CID_ANY;
-	else if (getu32(argv[optind], 0, UINT32_MAX, &lcid))
-		ex_usage();
-	optind++;
-
-	if (!strcmp(argv[optind], "-1"))
-		lport = VMADDR_PORT_ANY;
-	else if (getu32(argv[optind], 0, UINT32_MAX, &lport))
-		ex_usage();
-	optind++;
-
-	int fd = socket(AF_VSOCK, SOCK_STREAM, 0);
-	if (fd == -1)
-		diee(EX_OSERR, "socket");
-
-	if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1)
-		diee(EX_OSERR, "fcntl");
-
-	if (vsock_bind(fd, lcid, lport) == -1)
-		diee(EX_OSERR, "bind");
-
-	if (listen(fd, 40) == -1)
-		diee(EX_OSERR, "listen");
-
-	if (lport == VMADDR_PORT_ANY)
-		if (vsock_get_cid_and_port(fd, NULL, &lport) == -1)
-			diee(EX_OSERR, "getsockname");
+	// Find out the CID and port of the socket.
+	if (vsock_get_cid_and_port(STDIN_FILENO, &lcid, &lport)) {
+		if (errno == ENOTSOCK || errno == EPROTOTYPE)
+			die(EX_USAGE, "stdin is not an AF_VSOCK socket");
+		diee(EX_OSERR, "getsockname");
+	}
 
+	// Notify the user of our port and indicate readiness.
 	if (notify) {
 		printf("%" PRIu32 "\n", lport);
 		fclose(stdout);
 	}
 
+	// Set our CID and port as environment variables so `prog' can
+	// know them.
 	setenvf("VSOCKLOCALCID", 1, "%" PRIu32, lcid);
 	setenvf("VSOCKLOCALPORT", 1, "%" PRIu32, lport);
 
 	ilog("listening as %" PRIu32 " on port %" PRIu32, lcid, lport);
 
-	struct pollfd poll_fd = { .fd = fd, .events = POLL_IN, .revents = 0 };
+	// Wait for a connection.
 	while (poll(&poll_fd, 1, -1) != -1) {
 		// On Linux, conn will be blocking.  On other
 		// platforms, this may not be the case.  If other
 		// platforms are to be supported, we'd probably want
 		// to set O_NONBLOCK here.
-		int conn = vsock_accept(fd, &rcid, &rport);
+		int conn = vsock_accept(STDIN_FILENO, &rcid, &rport);
 		if (conn == -1)
 			diee(EX_OSERR, "accept");
 
+		// Set the client's CID and port as environment
+		// variables so `prog' can know them.
 		setenvf("VSOCKREMOTECID", 1, "%" PRIu32, rcid);
 		setenvf("VSOCKREMOTEPORT", 1, "%" PRIu32, rport);
 
@@ -116,12 +95,18 @@ int main(int argc, char *argv[])
 		switch (child = fork()) {
 		case -1: diee(EX_OSERR, "fork");
 		case 0:
+			// Set up the connection socket on prog's
+			// stdin and stdout.  This has the happy side
+			// effect of closing the listening socket in
+			// the child, since it's also on stdin.
 			if (dup2(conn, STDIN_FILENO) == -1)
 				diee(EX_OSERR, "dup2");
 			if (dup2(conn, STDOUT_FILENO) == -1)
 				diee(EX_OSERR, "dup2");
 			if (conn != STDIN_FILENO && conn != STDOUT_FILENO)
 				close(conn);
+
+			// exec into `prog'.
 			execvp(argv[optind], &argv[optind]);
 			diee(EX_OSERR, "exec");
 		}
-- 
2.30.0