xwayland: Introduce xwayland_spawn_func_t

Spawning subprocesses is finicky, and the correct way to do it depends
on the current process state which we have no control of. Signal masks,
handlers, limits, scheduling and open file descriptors are a few
examples of things that inadvertently get inherited and cause issues for
either the parent or the child.

We introduced a temporary workaround against common issues by clearing
signal masks and signal handlers known to cause issues for Xwayland, but
this was only a bandaid.

Introduce xwayland_spawn_func_t, which allows the compositor to specify
an implementation of a process spawner that will be used to launch
Xwayland whenever needed. This spawner can then do any cleanup and
accounting as the compositor sees fit.

The spawner takes 4 arguments: The path to Xwayland, the arguments we
have constructed for Xwayland, the environment variables that must be
set for Xwayland, and the file descriptors that must have CLOEXEC
cleared after fork but before exec to be correctly inherited by
Xwayland.

A default implementation is provided both for convenience and as a
sample for implementers to look at.

include/wlr/xwayland/server.h

@@ -14,16 +14,24 @@
 #include <time.h>
 #include <wayland-server-core.h>
 
+/**
+ * Xwayland spawn function. pathname and args are compatible with the execv
+ * family of functions. envp is a NULL terminated list of enironment variables
+ * that must be added before exec, while uncloexec is a -1 terminated list of
+ * file descriptors that must have CLOEXEC unset after fork but before exec.
+ */
+typedef bool (*xwayland_spawn_func_t)(char *pathname, char *args[], char *envp[], int uncloexec[]);
+
 struct wlr_xwayland_server_options {
 	bool lazy;
 	bool enable_wm;
 	bool no_touch_pointer_emulation;
 	bool force_xrandr_emulation;
 	int terminate_delay; // in seconds, 0 to terminate immediately
+	xwayland_spawn_func_t spawn_handler;
 };
 
 struct wlr_xwayland_server {
-	pid_t pid;
 	struct wl_client *client;
 	struct wl_event_source *pipe_source;
 	int wm_fd[2], wl_fd[2];

include/wlr/xwayland/xwayland.h

@@ -15,6 +15,7 @@
 #include <xcb/xcb_ewmh.h>
 #include <xcb/xcb_icccm.h>
 #include <wlr/util/addon.h>
+#include <wlr/xwayland/server.h>
 
 struct wlr_box;
 struct wlr_xwm;
@@ -266,7 +267,7 @@ struct wlr_xwayland_minimize_event {
  * client tries to connect.
  */
 struct wlr_xwayland *wlr_xwayland_create(struct wl_display *wl_display,
-	struct wlr_compositor *compositor, bool lazy);
+	struct wlr_compositor *compositor, bool lazy, xwayland_spawn_func_t spawn_func);
 
 /**
  * Create an XWM from an existing Xwayland server.

xwayland/server.c

@@ -1,3 +1,4 @@
+#define _XOPEN_SOURCE 700 // for putenv
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -23,25 +24,48 @@ static void safe_close(int fd) {
 	}
 }
 
-noreturn static void exec_xwayland(struct wlr_xwayland_server *server,
+static bool default_xwayland_spawn_func(char *program, char *args[], char *envp[], int uncloexec[]) {
-		int notify_fd) {
+	for (int idx = 0; uncloexec[idx] != -1; idx++) {
-	if (!set_cloexec(server->x_fd[0], false) ||
+		if (!set_cloexec(uncloexec[idx], false)) {
-			!set_cloexec(server->x_fd[1], false) ||
+			wlr_log(WLR_ERROR, "Unable to clear CLOEXEC");
-			!set_cloexec(server->wl_fd[1], false)) {
+			_exit(EXIT_FAILURE);
-		wlr_log(WLR_ERROR, "Failed to unset CLOEXEC on FD");
+		}
-		_exit(EXIT_FAILURE);
-	}
-	if (server->options.enable_wm && !set_cloexec(server->wm_fd[1], false)) {
-		wlr_log(WLR_ERROR, "Failed to unset CLOEXEC on FD");
-		_exit(EXIT_FAILURE);
 	}
 
-	// The compositor may have messed with signal handling, try to clean it up
+	for (int idx = 0; envp[idx] != NULL; idx++) {
-	sigset_t set;
+		if (putenv(envp[idx]) != 0) {
-	sigemptyset(&set);
+			wlr_log_errno(WLR_ERROR, "Unable to putenv: %s", envp[idx]);
-	sigprocmask(SIG_SETMASK, &set, NULL);
+			_exit(EXIT_FAILURE);
-	signal(SIGPIPE, SIG_DFL);
+		}
-	signal(SIGCHLD, SIG_DFL);
+	}
+
+	enum wlr_log_importance verbosity = wlr_log_get_verbosity();
+	int devnull = open("/dev/null", O_WRONLY | O_CREAT | O_CLOEXEC, 0666);
+	if (devnull < 0) {
+		wlr_log_errno(WLR_ERROR, "xwayland: failed to open /dev/null");
+		_exit(EXIT_FAILURE);
+	}
+	if (verbosity < WLR_INFO) {
+		dup2(devnull, STDOUT_FILENO);
+	}
+	if (verbosity < WLR_INFO) {
+		dup2(devnull, STDERR_FILENO);
+	}
+
+	execv(program, args);
+	wlr_log_errno(WLR_ERROR, "execve failed");
+	_exit(EXIT_SUCCESS); // Close child process
+}
+
+static bool spawn_xwayland(struct wlr_xwayland_server *server,
+		int notify_fd) {
+	int uncloexec[] = {
+		server->x_fd[0],
+		server->x_fd[1],
+		server->wl_fd[1],
+		server->options.enable_wm ? server->wm_fd[1] : -1,
+		-1,
+	};
 
 	char *argv[64] = {0};
 	size_t i = 0;
@@ -107,27 +131,15 @@ noreturn static void exec_xwayland(struct wlr_xwayland_server *server,
 
 	assert(i <= sizeof(argv) / sizeof(argv[0]));
 
-	char wayland_socket_str[16];
+	char wayland_socket_str[32];
-	snprintf(wayland_socket_str, sizeof(wayland_socket_str), "%d", server->wl_fd[1]);
+	snprintf(wayland_socket_str, sizeof(wayland_socket_str), "WAYLAND_SOCKET=%d", server->wl_fd[1]);
-	setenv("WAYLAND_SOCKET", wayland_socket_str, true);
 
-	wlr_log(WLR_INFO, "Starting Xwayland on :%d", server->display);
+	char *envp[] = {
+		wayland_socket_str,
+		NULL,
+	};
 
-	// Closes stdout/stderr depending on log verbosity
+	char *xwayland_path = getenv("WLR_XWAYLAND");
-	enum wlr_log_importance verbosity = wlr_log_get_verbosity();
-	int devnull = open("/dev/null", O_WRONLY | O_CREAT | O_CLOEXEC, 0666);
-	if (devnull < 0) {
-		wlr_log_errno(WLR_ERROR, "XWayland: failed to open /dev/null");
-		_exit(EXIT_FAILURE);
-	}
-	if (verbosity < WLR_INFO) {
-		dup2(devnull, STDOUT_FILENO);
-	}
-	if (verbosity < WLR_ERROR) {
-		dup2(devnull, STDERR_FILENO);
-	}
-
-	const char *xwayland_path = getenv("WLR_XWAYLAND");
 	if (xwayland_path) {
 		wlr_log(WLR_INFO, "Using Xwayland binary '%s' due to WLR_XWAYLAND",
 			xwayland_path);
@@ -135,12 +147,7 @@ noreturn static void exec_xwayland(struct wlr_xwayland_server *server,
 		xwayland_path = XWAYLAND_PATH;
 	}
 
-	// This returns if and only if the call fails
+	return server->options.spawn_handler(xwayland_path, argv, envp, uncloexec);
-	execvp(xwayland_path, argv);
-
-	wlr_log_errno(WLR_ERROR, "failed to exec %s", xwayland_path);
-	close(devnull);
-	_exit(EXIT_FAILURE);
 }
 
 static void server_finish_process(struct wlr_xwayland_server *server) {
@@ -260,24 +267,6 @@ static int xserver_handle_ready(int fd, uint32_t mask, void *data) {
 		}
 	}
 
-	while (waitpid(server->pid, NULL, 0) < 0) {
-		if (errno == EINTR) {
-			continue;
-		}
-
-		/* If some application has installed a SIGCHLD handler, they
-		 * may race and waitpid() on our child, which will cause this
-		 * waitpid() to fail. We have a signal from the
-		 * notify pipe that things are ready, so this waitpid() is only
-		 * to prevent zombies, which will have already been reaped by
-		 * the application's SIGCHLD handler.
-		 */
-		if (errno == ECHILD) {
-			break;
-		}
-		wlr_log_errno(WLR_ERROR, "waitpid for Xwayland fork failed");
-		goto error;
-	}
 	/* Xwayland will only write on the fd once it has finished its
 	 * initial setup. Getting an event here without READABLE means
 	 * the server end failed.
@@ -387,23 +376,12 @@ static bool server_start(struct wlr_xwayland_server *server) {
 
 	wl_signal_emit_mutable(&server->events.start, NULL);
 
-	server->pid = fork();
+	if (!spawn_xwayland(server, notify_fd[1])) {
-	if (server->pid < 0) {
+		wlr_log(WLR_ERROR, "Spawning Xwayland failed");
-		wlr_log_errno(WLR_ERROR, "fork failed");
 		close(notify_fd[0]);
 		close(notify_fd[1]);
 		server_finish_process(server);
 		return false;
-	} else if (server->pid == 0) {
-		pid_t pid = fork();
-		if (pid < 0) {
-			wlr_log_errno(WLR_ERROR, "second fork failed");
-			_exit(EXIT_FAILURE);
-		} else if (pid == 0) {
-			exec_xwayland(server, notify_fd[1]);
-		}
-
-		_exit(EXIT_SUCCESS);
 	}
 
 	/* close child fds */
@@ -488,6 +466,10 @@ struct wlr_xwayland_server *wlr_xwayland_server_create(
 	server->wl_display = wl_display;
 	server->options = *options;
 
+	if (server->options.spawn_handler == NULL) {
+		server->options.spawn_handler = default_xwayland_spawn_func;
+	}
+
 #if !HAVE_XWAYLAND_TERMINATE_DELAY
 	server->options.terminate_delay = 0;
 #endif

xwayland/xwayland.c

@@ -139,7 +139,7 @@ struct wlr_xwayland *wlr_xwayland_create_with_server(struct wl_display *wl_displ
 }
 
 struct wlr_xwayland *wlr_xwayland_create(struct wl_display *wl_display,
-		struct wlr_compositor *compositor, bool lazy) {
+		struct wlr_compositor *compositor, bool lazy, xwayland_spawn_func_t spawn_func) {
 	struct wlr_xwayland_shell_v1 *shell_v1 = wlr_xwayland_shell_v1_create(wl_display, 1);
 	if (shell_v1 == NULL) {
 		return NULL;
@@ -148,6 +148,7 @@ struct wlr_xwayland *wlr_xwayland_create(struct wl_display *wl_display,
 	struct wlr_xwayland_server_options options = {
 		.lazy = lazy,
 		.enable_wm = true,
+		.spawn_handler = spawn_func,
 #if HAVE_XCB_XFIXES_SET_CLIENT_DISCONNECT_MODE
 		.terminate_delay = lazy ? 10 : 0,
 #endif