From 24e947b1f664f1b99a36f6cb5ab2401d4ef528db 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). --- shmoverride/shmoverride.c | 94 +++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 28 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index efef3e9d..ae2ab34d 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -37,6 +37,8 @@ #include #include #include +#include +#include #include "list.h" #include "shm-args.h" #include @@ -57,6 +59,7 @@ static xengnttab_handle *xgt; 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] = ""; struct mfns_info { @@ -226,7 +229,7 @@ int shmctl(int shmid, int cmd, struct shmid_ds *buf) return 0; } -void get_display() +int get_display() { int fd; ssize_t res; @@ -236,14 +239,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; @@ -258,7 +261,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; @@ -275,18 +278,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"); @@ -295,7 +300,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 @@ -306,65 +311,98 @@ 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 } xgt = xengnttab_open(NULL, 0); if (xgt == NULL) { perror("shmoverride: xengnttab_open failed"); - 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_ARGS_SIZE, 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; } shm_args = real_shmat(local_shmid, 0, 0); if (!shm_args) { - unlink(shmid_filename); perror("real_shmat"); - exit(1); + goto cleanup; } shm_args->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; + } + shm_args = NULL; + return 0; } int __attribute__ ((destructor)) descfunc() { if (shm_args) { + assert(shmid_filename); + assert(idfd >= 0); + real_shmdt(shm_args); real_shmctl(local_shmid, IPC_RMID, 0); - if (shmid_filename != NULL) - unlink(shmid_filename); + close(idfd); + unlink(shmid_filename); } if (xgt != NULL)