From c2f4201ed2a86a4889417ea750cb75c2e96dcc57 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Mon, 11 Apr 2022 19:52:27 +0900 Subject: [PATCH] xwayland: use -displayfd instead of USR1 to signal readiness We want to wait for Xwayland to be ready before issuing it blocking requests, but relying on USR1 is a bit unsafe: - we can't ascertain the signal originated from Xwayland - if weston is started as PID1 (e.g. in its own container), then Xwayland will not send SIGUSR1 and X11 connections will be stuck forever: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1312 Creating a pipe and using -displayfd, even if we don't care about the display value itself, is safe and works for all cases Signed-off-by: Dominique Martinet --- compositor/xwayland.c | 79 +++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/compositor/xwayland.c b/compositor/xwayland.c index 06ca4919..17f53b89 100644 --- a/compositor/xwayland.c +++ b/compositor/xwayland.c @@ -35,6 +35,7 @@ #include "compositor/weston.h" #include #include "shared/helpers.h" +#include "shared/os-compatibility.h" #ifdef HAVE_XWAYLAND_LISTENFD # define LISTEN_STR "-listenfd" @@ -46,24 +47,44 @@ struct wet_xwayland { struct weston_compositor *compositor; const struct weston_xwayland_api *api; struct weston_xwayland *xwayland; - struct wl_event_source *sigusr1_source; + struct wl_event_source *display_fd_source; struct wl_client *client; int wm_fd; struct weston_process process; }; static int -handle_sigusr1(int signal_number, void *data) +handle_display_fd(int fd, uint32_t mask, void *data) { struct wet_xwayland *wxw = data; + char buf[64]; + ssize_t n; + + /* xwayland exited before being ready, don't finish initialization, + * the process watcher will cleanup */ + if (!(mask & WL_EVENT_READABLE)) + goto out; + + /* Xwayland writes to the pipe twice, so if we close it too early + * it's possible the second write will fail and Xwayland shuts down. + * Make sure we read until end of line marker to avoid this. */ + n = read(fd, buf, sizeof buf); + if (n < 0 && errno != EAGAIN) { + weston_log("read from Xwayland display_fd failed: %s\n", + strerror(errno)); + goto out; + } + /* Returning 1 here means recheck and call us again if required. */ + if (n <= 0 || (n > 0 && buf[n - 1] != '\n')) + return 1; - /* We'd be safer if we actually had the struct - * signalfd_siginfo from the signalfd data and could verify - * this came from Xwayland.*/ wxw->api->xserver_loaded(wxw->xwayland, wxw->client, wxw->wm_fd); - wl_event_source_remove(wxw->sigusr1_source); - return 1; +out: + wl_event_source_remove(wxw->display_fd_source); + close(fd); + + return 0; } static pid_t @@ -72,10 +93,12 @@ spawn_xserver(void *user_data, const char *display, int abstract_fd, int unix_fd struct wet_xwayland *wxw = user_data; pid_t pid; char s[12], abstract_fd_str[12], unix_fd_str[12], wm_fd_str[12]; - int sv[2], wm[2], fd; + char display_fd_str[12]; + int sv[2], wm[2], fd, display_fd[2]; char *xserver = NULL; struct weston_config *config = wet_get_config(wxw->compositor); struct weston_config_section *section; + struct wl_event_loop *loop; if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sv) < 0) { weston_log("wl connection socketpair failed\n"); @@ -87,6 +110,16 @@ spawn_xserver(void *user_data, const char *display, int abstract_fd, int unix_fd return 1; } + if (pipe(display_fd) < 0) { + weston_log("pipe creation for displayfd failed\n"); + return 1; + } + + if (os_fd_set_cloexec(display_fd[0]) != 0) { + weston_log("failed setting remaining end of displayfd as cloexec\n"); + return 1; + } + pid = fork(); switch (pid) { case 0: @@ -110,27 +143,20 @@ spawn_xserver(void *user_data, const char *display, int abstract_fd, int unix_fd if (fd < 0) goto fail; snprintf(wm_fd_str, sizeof wm_fd_str, "%d", fd); + snprintf(display_fd_str, sizeof display_fd_str, "%d", display_fd[1]); section = weston_config_get_section(config, "xwayland", NULL, NULL); weston_config_section_get_string(section, "path", &xserver, XSERVER_PATH); - /* Ignore SIGUSR1 in the child, which will make the X - * server send SIGUSR1 to the parent (weston) when - * it's done with initialization. During - * initialization the X server will round trip and - * block on the wayland compositor, so avoid making - * blocking requests (like xcb_connect_to_fd) until - * it's done with that. */ - signal(SIGUSR1, SIG_IGN); - if (execl(xserver, xserver, display, "-rootless", LISTEN_STR, abstract_fd_str, LISTEN_STR, unix_fd_str, + "-displayfd", display_fd_str, "-wm", wm_fd_str, "-terminate", NULL) < 0) @@ -150,6 +176,16 @@ spawn_xserver(void *user_data, const char *display, int abstract_fd, int unix_fd close(wm[1]); wxw->wm_fd = wm[0]; + /* During initialization the X server will round trip + * and block on the wayland compositor, so avoid making + * blocking requests (like xcb_connect_to_fd) until + * it's done with that. */ + close(display_fd[1]); + loop = wl_display_get_event_loop(wxw->compositor->wl_display); + wxw->display_fd_source = + wl_event_loop_add_fd(loop, display_fd[0], WL_EVENT_READABLE, + handle_display_fd, wxw); + wxw->process.pid = pid; wet_watch_process(wxw->compositor, &wxw->process); break; @@ -167,12 +203,8 @@ xserver_cleanup(struct weston_process *process, int status) { struct wet_xwayland *wxw = container_of(process, struct wet_xwayland, process); - struct wl_event_loop *loop = - wl_display_get_event_loop(wxw->compositor->wl_display); wxw->api->xserver_exited(wxw->xwayland, status); - wxw->sigusr1_source = wl_event_loop_add_signal(loop, SIGUSR1, - handle_sigusr1, wxw); wxw->client = NULL; } @@ -182,7 +214,6 @@ wet_load_xwayland(struct weston_compositor *comp) const struct weston_xwayland_api *api; struct weston_xwayland *xwayland; struct wet_xwayland *wxw; - struct wl_event_loop *loop; if (weston_compositor_load_xwayland(comp) < 0) return -1; @@ -210,9 +241,5 @@ wet_load_xwayland(struct weston_compositor *comp) if (api->listen(xwayland, wxw, spawn_xserver) < 0) return -1; - loop = wl_display_get_event_loop(comp->wl_display); - wxw->sigusr1_source = wl_event_loop_add_signal(loop, SIGUSR1, - handle_sigusr1, wxw); - return 0; }