From 0806e2f664ed49e167d2ffb1998face9e4a6d3d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 2 Dec 2024 19:04:51 +0100 Subject: [PATCH 1/4] Move signal registraion earlier In case the service is stopped while Xorg is still starting up (and gui-agent still waits for the Xorg connectin in mkghandles), gui-agent would exit before killing Xorg and Xorg would try connecting back to the gui-agent forever, delaying the shutdown. Fix this by moving signal registration earlier, before Xorg startup. Since ghandles_for_vchan_reinitialize is now set before its fully initialized, initialize x_pid field explicitly and leave all the other fields zeroed (instead of random stack rubble). --- gui-agent/vmside.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/gui-agent/vmside.c b/gui-agent/vmside.c index c3ef4d16..dd34c16a 100644 --- a/gui-agent/vmside.c +++ b/gui-agent/vmside.c @@ -2380,7 +2380,7 @@ int main(int argc, char **argv) { int i; int xfd; - Ghandles g; + Ghandles g = { .x_pid = -1 }; g.created_input_device = access("/run/qubes-service/gui-agent-virtual-input-device", F_OK) == 0; @@ -2454,10 +2454,21 @@ int main(int argc, char **argv) libvchan_wait(g.vchan); saved_argv = argv; vchan_register_at_eof(handle_guid_disconnect); + + ghandles_for_vchan_reinitialize = &g; + signal(SIGCHLD, SIG_IGN); + struct sigaction sigterm_handler = { + .sa_sigaction = handle_sigterm, + .sa_flags = SA_SIGINFO, + }; + sigemptyset(&sigterm_handler.sa_mask); + if (sigaction(SIGTERM, &sigterm_handler, NULL)) + err(1, "sigaction"); + handshake(&g); g.x_pid = get_xconf_and_run_x(&g); + mkghandles(&g); - ghandles_for_vchan_reinitialize = &g; /* Turn on Composite for all children of root window. This way X server * keeps separate buffers for each (root child) window. * There are two modes: @@ -2497,14 +2508,6 @@ int main(int argc, char **argv) fprintf(stderr, "XFixes not available, cursor shape handling off\n"); XAutoRepeatOff(g.display); - signal(SIGCHLD, SIG_IGN); - struct sigaction sigterm_handler = { - .sa_sigaction = handle_sigterm, - .sa_flags = SA_SIGINFO, - }; - sigemptyset(&sigterm_handler.sa_mask); - if (sigaction(SIGTERM, &sigterm_handler, NULL)) - err(1, "sigaction"); windows_list = list_new(); embeder_list = list_new(); XSetErrorHandler(dummy_handler); From eaba72a9a8c79691720a423e52f0ccbe6bb10ee1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 2 Dec 2024 19:33:42 +0100 Subject: [PATCH 2/4] Better handle premature Xorg exit Register proper signal handler for SIGCHLD, and collect the Xorg's zombie in it. This has two effects: 1. The main loop can explicitly exit on Xorg termination, not only via receiving EOF on the socket. 2. Due to not ignoring SIGCHLD anymore, accept() in mkghandles will also notice Xorg early exit and not wait indefinitely (it will fail with EINTR). For this case, improve error message. There is still a small race on startup, if Xorg exits before reaching accept() (or listen()) call. Handle this by checking just before accept() call. It isn't perfect (there is still a few instructions window where it wouldn't notice it in time), but it's good enough for practical purposes. QubesOS/qubes-issues#8060 --- gui-agent/vmside.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/gui-agent/vmside.c b/gui-agent/vmside.c index dd34c16a..dc847cde 100644 --- a/gui-agent/vmside.c +++ b/gui-agent/vmside.c @@ -1558,9 +1558,16 @@ static void wait_for_unix_socket(Ghandles *g) addrlen = sizeof(peer); fprintf (stderr, "Waiting on %s socket...\n", SOCKET_ADDRESS); + if (g->x_pid == (pid_t)-1) { + fprintf(stderr, "Xorg exited in the meantime, aborting\n"); + exit(1); + } g->xserver_fd = accept(g->xserver_listen_fd, (struct sockaddr *) &peer, &addrlen); if (g->xserver_fd == -1) { - perror("unix accept"); + if (errno == EINTR && g->x_pid == (pid_t)-1) + fprintf(stderr, "Xorg exited in the meantime, aborting\n"); + else + perror("unix accept"); exit(1); } fprintf (stderr, "Ok, somebody connected.\n"); @@ -2317,6 +2324,21 @@ static _Noreturn void handle_sigterm(int UNUSED(sig), exit(0); } +static void handle_sigchld(int UNUSED(sig), + siginfo_t *UNUSED(info), void *UNUSED(context)) +{ + Ghandles *g = ghandles_for_vchan_reinitialize; + if (g->x_pid != (pid_t)-1) { + int status; + pid_t pid = waitpid(g->x_pid, &status, WNOHANG); + if (pid == g->x_pid && (WIFEXITED(status) || WIFSIGNALED(status))) + /* TODO: consider saving also exit status, but right now gui-agent + * would handle it the same regardless, so maybe later, just for + * logging purposes */ + g->x_pid = -1; + } +} + static void usage(void) { fprintf(stderr, "Usage: qubes_gui [options]\n"); @@ -2456,7 +2478,13 @@ int main(int argc, char **argv) vchan_register_at_eof(handle_guid_disconnect); ghandles_for_vchan_reinitialize = &g; - signal(SIGCHLD, SIG_IGN); + struct sigaction sigchld_handler = { + .sa_sigaction = handle_sigchld, + .sa_flags = SA_SIGINFO, + }; + sigemptyset(&sigchld_handler.sa_mask); + if (sigaction(SIGCHLD, &sigchld_handler, NULL)) + err(1, "sigaction"); struct sigaction sigterm_handler = { .sa_sigaction = handle_sigterm, .sa_flags = SA_SIGINFO, @@ -2544,6 +2572,11 @@ int main(int argc, char **argv) for (;;) { int busy; + if (g.x_pid == -1) { + fprintf(stderr, "Xorg exited prematurely\n"); + exit(1); + } + fds[0].fd = libvchan_fd_for_select(g.vchan); wait_for_vchan_or_argfd(g.vchan, fds, QUBES_ARRAY_SIZE(fds)); /* first process possible qubes_drv reconnection, otherwise we may be From 1d2026af2544b2f8ef077631c75d6898b8781646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 2 Dec 2024 20:57:11 +0100 Subject: [PATCH 3/4] Fix logging in xf86-input-mfndev Use X's logging function instead of plain perror, to ensure the message is written in appropriate Xorg's log. --- xf86-input-mfndev/src/qubes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xf86-input-mfndev/src/qubes.c b/xf86-input-mfndev/src/qubes.c index 04b7beff..9323c498 100644 --- a/xf86-input-mfndev/src/qubes.c +++ b/xf86-input-mfndev/src/qubes.c @@ -390,7 +390,7 @@ int connect_unix_socket(QubesDevicePtr pQubes) struct sockaddr_un remote; if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { - perror("socket"); + xf86Msg(X_ERROR, "socket(%s): %s\n", pQubes->device, strerror(errno)); return -1; } @@ -399,7 +399,7 @@ int connect_unix_socket(QubesDevicePtr pQubes) strncpy(remote.sun_path, pQubes->device, sizeof(remote.sun_path)); len = strlen(remote.sun_path) + sizeof(remote.sun_family); if (connect(s, (struct sockaddr *) &remote, len) == -1) { - perror("connect"); + xf86Msg(X_ERROR, "connect(%s): %s\n", pQubes->device, strerror(errno)); close(s); return -1; } From 0ad1350ee06c8490eac7eb6f693a0ccf5cdbed0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 8 Dec 2024 05:36:54 +0100 Subject: [PATCH 4/4] xf86-input-mfndev: interrupt attempts to contact gui-agent on SIGTERM If Xorg is going to be terminated, do not try to connect to gui-agent anymore. This avoids infinite loop when handling SIGTERM, and properly shutdown instead. --- xf86-input-mfndev/src/qubes.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xf86-input-mfndev/src/qubes.c b/xf86-input-mfndev/src/qubes.c index 9323c498..350640e1 100644 --- a/xf86-input-mfndev/src/qubes.c +++ b/xf86-input-mfndev/src/qubes.c @@ -506,6 +506,12 @@ static int QubesControl(DeviceIntPtr device, int what) "%s: cannot open device; sleeping...\n", pInfo->name); sleep(1); + if (xf86ServerIsExiting()) { + xf86Msg(X_ERROR, + "%s: cannot open device, server exiting, aborting\n", + pInfo->name); + return BadAlloc; + } } } while (pInfo->fd < 0);