From b885b8d4c374b67ea2899050ec3c8f865a7c39af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Marczewski?= Date: Tue, 24 Mar 2020 14:52:57 +0100 Subject: [PATCH] shmoverride: handle Xorg crash leaving shm.id file Instead of opening with O_CREAT|O_EXCL, use flock() to see if the file is held by an alive Xorg process. If we fail for some reason, always clean up and continue instead of calling exit(). See 0ac0c7fcee1dcbc6235718b1b1fcf90131811973 for more context. Solves part of QubesOS/qubes-issues#5273 (qubes-guid failing to start after Xorg crash). (cherry picked from commit 24e947b1f664f1b99a36f6cb5ab2401d4ef528db) --- shmoverride/shmoverride.c | 92 +++++++++++++++++++++++++++------------ 1 file changed, 65 insertions(+), 27 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index 2f5b9672..52204650 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -36,6 +36,8 @@ #include #include #include +#include +#include #include "list.h" #include "shmid.h" #include @@ -55,6 +57,7 @@ static int xc_hnd; static int list_len; static char __shmid_filename[SHMID_FILENAME_LEN]; static char *shmid_filename = NULL; +static int idfd = -1; static char display_str[SHMID_DISPLAY_MAXLEN+1] = ""; void *shmat(int shmid, const void *shmaddr, int shmflg) @@ -120,7 +123,7 @@ int shmctl(int shmid, int cmd, struct shmid_ds *buf) return 0; } -void get_display() +int get_display() { int fd; ssize_t res; @@ -130,14 +133,14 @@ void get_display() fd = open("/proc/self/cmdline", O_RDONLY); if (fd < 0) { perror("cmdline open"); - exit(1); + return -1; } while(1) { res = read(fd, &ch, 1); if (res < 0) { perror("cmdline read"); - exit(1); + return -1; } if (res == 0) break; @@ -152,7 +155,7 @@ void get_display() if (in_arg > 0 && (ch < '0' || ch > '9')) { if (in_arg == 1) { fprintf(stderr, "cmdline DISPLAY parsing failed\n"); - exit(1); + return -1; } in_arg = -1; continue; @@ -169,18 +172,20 @@ void get_display() display_str[2] = '\0'; } else if (display_str[1] == '\0') { fprintf(stderr, "cmdline DISPLAY parsing failed\n"); - exit(1); + return -1; } /* post-processing: drop leading ':' */ res = strlen(display_str); for (in_arg = 0; in_arg < res; in_arg++) display_str[in_arg] = display_str[in_arg+1]; + + return 0; } int __attribute__ ((constructor)) initfunc() { - int idfd, len; + int len; char idbuf[20]; unsetenv("LD_PRELOAD"); fprintf(stderr, "shmoverride constructor running\n"); @@ -189,7 +194,7 @@ int __attribute__ ((constructor)) initfunc() real_shmdt = dlsym(RTLD_NEXT, "shmdt"); if (!real_shmat || !real_shmctl || !real_shmdt) { perror("shmoverride: missing shm API"); - exit(1); + goto cleanup; } addr_list = list_new(); #ifdef XENCTRL_HAS_XC_INTERFACE @@ -200,59 +205,92 @@ int __attribute__ ((constructor)) initfunc() if (xc_hnd < 0) { #endif perror("shmoverride xc_interface_open"); - return 0; //allow it to run when not under Xen + goto cleanup; //allow it to run when not under Xen } - get_display(); + if (get_display() < 0) + goto cleanup; + snprintf(__shmid_filename, SHMID_FILENAME_LEN, SHMID_FILENAME_PREFIX "%s", display_str); shmid_filename = __shmid_filename; - idfd = open(shmid_filename, O_WRONLY | O_CREAT | O_EXCL, 0600); + + /* Try to lock the shm.id file (don't rely on whether it exists, a previous + * process might have crashed). + */ + idfd = open(shmid_filename, O_WRONLY | O_CREAT, 0600); if (idfd < 0) { - fprintf(stderr, "shmoverride creating %s: %s\n", + fprintf(stderr, "shmoverride opening %s: %s\n", shmid_filename, strerror(errno)); - xc_interface_close(xc_hnd); - return 0; + goto cleanup; + } + if (flock(idfd, LOCK_EX | LOCK_NB) < 0) { + fprintf(stderr, "shmoverride flock %s: %s\n", + shmid_filename, strerror(errno)); + /* There is probably an alive process holding the file, give up. */ + goto cleanup; + } + if (ftruncate(idfd, 0) < 0) { + perror("shmoverride ftruncate"); + goto cleanup; } local_shmid = shmget(IPC_PRIVATE, SHM_CMD_NUM_PAGES * 4096, IPC_CREAT | 0700); if (local_shmid == -1) { - unlink(shmid_filename); perror("shmoverride shmget"); - exit(1); + goto cleanup; } sprintf(idbuf, "%d", local_shmid); len = strlen(idbuf); if (write(idfd, idbuf, len) != len) { - unlink(shmid_filename); fprintf(stderr, "shmoverride writing %s: %s\n", shmid_filename, strerror(errno)); - exit(1); - } - if (close(idfd) < 0) { - unlink(shmid_filename); - fprintf(stderr, "shmoverride closing %s: %s\n", - shmid_filename, strerror(errno)); - exit(1); + goto cleanup; } cmd_pages = real_shmat(local_shmid, 0, 0); if (!cmd_pages) { - unlink(shmid_filename); perror("real_shmat"); - exit(1); + goto cleanup; } cmd_pages->shmid = local_shmid; return 0; + +cleanup: + fprintf(stderr, "shmoverride: running without override"); +#ifdef XENCTRL_HAS_XC_INTERFACE + if (!xc_hnd) { + xc_interface_close(xc_hnd); + xc_hnd = NULL; + } +#else + if (xc_hnd >= 0) { + xc_interface_close(xc_hnd); + xc_hnd = -1; + } +#endif + if (idfd >= 0) { + close(idfd); + idfd = -1; + } + if (shmid_filename) { + unlink(shmid_filename); + shmid_filename = NULL; + } + cmd_pages = NULL; + return 0; } int __attribute__ ((destructor)) descfunc() { if (cmd_pages) { + assert(shmid_filename); + assert(idfd >= 0); + real_shmdt(cmd_pages); real_shmctl(local_shmid, IPC_RMID, 0); - if (shmid_filename != NULL) - unlink(shmid_filename); + close(idfd); + unlink(shmid_filename); } return 0; }