From 57f8cf9cd4bcbc76cd28f1c0d3cb7c4bd313d1fe Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Thu, 20 Mar 2025 17:07:19 +0100 Subject: [PATCH] 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 | 10 ++- include/wlr/xwayland/xwayland.h | 3 +- xwayland/server.c | 128 ++++++++++++++------------------ xwayland/xwayland.c | 3 +- 4 files changed, 68 insertions(+), 76 deletions(-) diff --git a/include/wlr/xwayland/server.h b/include/wlr/xwayland/server.h index b699b8f4f..182d810ed 100644 --- a/include/wlr/xwayland/server.h +++ b/include/wlr/xwayland/server.h @@ -14,16 +14,24 @@ #include #include +/** + * 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]; diff --git a/include/wlr/xwayland/xwayland.h b/include/wlr/xwayland/xwayland.h index b1b2247e6..1afe9eabe 100644 --- a/include/wlr/xwayland/xwayland.h +++ b/include/wlr/xwayland/xwayland.h @@ -15,6 +15,7 @@ #include #include #include +#include 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. diff --git a/xwayland/server.c b/xwayland/server.c index 3abeb8bb9..6b89bb323 100644 --- a/xwayland/server.c +++ b/xwayland/server.c @@ -1,3 +1,4 @@ +#define _XOPEN_SOURCE 700 // for putenv #include #include #include @@ -23,25 +24,48 @@ static void safe_close(int fd) { } } -noreturn static void exec_xwayland(struct wlr_xwayland_server *server, - int notify_fd) { - if (!set_cloexec(server->x_fd[0], false) || - !set_cloexec(server->x_fd[1], false) || - !set_cloexec(server->wl_fd[1], false)) { - 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); +static bool default_xwayland_spawn_func(char *program, char *args[], char *envp[], int uncloexec[]) { + for (int idx = 0; uncloexec[idx] != -1; idx++) { + if (!set_cloexec(uncloexec[idx], false)) { + wlr_log(WLR_ERROR, "Unable to clear CLOEXEC"); + _exit(EXIT_FAILURE); + } } - // The compositor may have messed with signal handling, try to clean it up - sigset_t set; - sigemptyset(&set); - sigprocmask(SIG_SETMASK, &set, NULL); - signal(SIGPIPE, SIG_DFL); - signal(SIGCHLD, SIG_DFL); + for (int idx = 0; envp[idx] != NULL; idx++) { + if (putenv(envp[idx]) != 0) { + wlr_log_errno(WLR_ERROR, "Unable to putenv: %s", envp[idx]); + _exit(EXIT_FAILURE); + } + } + + 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]; - snprintf(wayland_socket_str, sizeof(wayland_socket_str), "%d", server->wl_fd[1]); - setenv("WAYLAND_SOCKET", wayland_socket_str, true); + char wayland_socket_str[32]; + snprintf(wayland_socket_str, sizeof(wayland_socket_str), "WAYLAND_SOCKET=%d", server->wl_fd[1]); - wlr_log(WLR_INFO, "Starting Xwayland on :%d", server->display); + char *envp[] = { + wayland_socket_str, + NULL, + }; - // Closes stdout/stderr depending on log verbosity - 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"); + 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 - execvp(xwayland_path, argv); - - wlr_log_errno(WLR_ERROR, "failed to exec %s", xwayland_path); - close(devnull); - _exit(EXIT_FAILURE); + return server->options.spawn_handler(xwayland_path, argv, envp, uncloexec); } 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 (server->pid < 0) { - wlr_log_errno(WLR_ERROR, "fork failed"); + if (!spawn_xwayland(server, notify_fd[1])) { + wlr_log(WLR_ERROR, "Spawning Xwayland 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 diff --git a/xwayland/xwayland.c b/xwayland/xwayland.c index 7cef4cc8f..25f494ea1 100644 --- a/xwayland/xwayland.c +++ b/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