From d91660fe2a4a508479be922140b112e7e8809180 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 24 Sep 2019 09:00:54 +0200 Subject: [PATCH] Work around deadlocks in g_spawn by manually clo-exec:ing fds As per https://gitlab.gnome.org/GNOME/glib/merge_requests/490 there is a bug in glib < 2.60 where g_spawn_* can sometimes deadlock due to using malloc in the child func to close fds. We work around this in places where the code is (potentially) threaded by passing glib flags to leave fds alone and then do a very naive (but safe) fd cloexec loop ourselves. --- common/flatpak-bwrap.c | 3 +++ common/flatpak-dir.c | 3 ++- common/flatpak-run.c | 9 +++++++-- common/flatpak-utils-base-private.h | 1 + common/flatpak-utils-base.c | 16 ++++++++++++++++ portal/Makefile.am.inc | 2 +- portal/flatpak-portal.c | 6 +++++- system-helper/flatpak-system-helper.c | 9 ++++++++- 8 files changed, 43 insertions(+), 6 deletions(-) diff --git a/common/flatpak-bwrap.c b/common/flatpak-bwrap.c index 730f771d..eb3f62a4 100644 --- a/common/flatpak-bwrap.c +++ b/common/flatpak-bwrap.c @@ -39,6 +39,7 @@ #include "flatpak-bwrap-private.h" #include "flatpak-utils-private.h" +#include "flatpak-utils-base-private.h" static void clear_fd (gpointer data) @@ -331,6 +332,8 @@ flatpak_bwrap_child_setup_cb (gpointer user_data) GArray *fd_array = user_data; int i; + flatpak_close_fds_workaround (3); + /* If no fd_array was specified, don't care. */ if (fd_array == NULL) return; diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 9840b9c8..73fec071 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -6281,10 +6281,11 @@ flatpak_dir_run_triggers (FlatpakDir *self, commandline = flatpak_quote_argv ((const char **) bwrap->argv->pdata, -1); g_debug ("Running '%s'", commandline); + /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */ if (!g_spawn_sync ("/", (char **) bwrap->argv->pdata, NULL, - G_SPAWN_SEARCH_PATH, + G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN, flatpak_bwrap_child_setup_cb, bwrap->fds, NULL, NULL, NULL, &trigger_error)) diff --git a/common/flatpak-run.c b/common/flatpak-run.c index 77cbcf95..28bdea09 100644 --- a/common/flatpak-run.c +++ b/common/flatpak-run.c @@ -851,10 +851,11 @@ start_dbus_proxy (FlatpakBwrap *app_bwrap, commandline = flatpak_quote_argv ((const char **) proxy_bwrap->argv->pdata, -1); g_debug ("Running '%s'", commandline); + /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */ if (!g_spawn_async (NULL, (char **) proxy_bwrap->argv->pdata, NULL, - G_SPAWN_SEARCH_PATH, + G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN, flatpak_bwrap_child_setup_cb, proxy_bwrap->fds, NULL, error)) return FALSE; @@ -3122,10 +3123,11 @@ regenerate_ld_cache (GPtrArray *base_argv_array, g_array_append_vals (combined_fd_array, base_fd_array->data, base_fd_array->len); g_array_append_vals (combined_fd_array, bwrap->fds->data, bwrap->fds->len); + /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */ if (!g_spawn_sync (NULL, (char **) bwrap->argv->pdata, bwrap->envp, - G_SPAWN_SEARCH_PATH, + G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN, flatpak_bwrap_child_setup_cb, combined_fd_array, NULL, NULL, &exit_status, @@ -3564,6 +3566,9 @@ flatpak_run_app (const char *app_ref, if (flags & FLATPAK_RUN_FLAG_DO_NOT_REAP) spawn_flags |= G_SPAWN_DO_NOT_REAP_CHILD; + /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */ + spawn_flags |= G_SPAWN_LEAVE_DESCRIPTORS_OPEN; + if (!g_spawn_async (NULL, (char **) bwrap->argv->pdata, bwrap->envp, diff --git a/common/flatpak-utils-base-private.h b/common/flatpak-utils-base-private.h index d7a55273..181a0c95 100644 --- a/common/flatpak-utils-base-private.h +++ b/common/flatpak-utils-base-private.h @@ -30,5 +30,6 @@ char * flatpak_readlink (const char *path, char * flatpak_resolve_link (const char *path, GError **error); char * flatpak_canonicalize_filename (const char *path); +void flatpak_close_fds_workaround (int start_fd); #endif /* __FLATPAK_UTILS_BASE_H__ */ diff --git a/common/flatpak-utils-base.c b/common/flatpak-utils-base.c index 07d7725e..e9786203 100644 --- a/common/flatpak-utils-base.c +++ b/common/flatpak-utils-base.c @@ -98,3 +98,19 @@ flatpak_canonicalize_filename (const char *path) g_autoptr(GFile) file = g_file_new_for_path (path); return g_file_get_path (file); } + +/* There is a dead-lock in glib versions before 2.60 when it closes + * the fds. See: https://gitlab.gnome.org/GNOME/glib/merge_requests/490 + * This was hitting the test-suite a lot, so we work around it by using + * the G_SPAWN_LEAVE_DESCRIPTORS_OPEN/G_SUBPROCESS_FLAGS_INHERIT_FDS flag + * and setting CLOEXEC ourselves. + */ +void +flatpak_close_fds_workaround (int start_fd) +{ + int max_open_fds = sysconf (_SC_OPEN_MAX); + int fd; + + for (fd = start_fd; fd < max_open_fds; fd++) + fcntl (fd, F_SETFD, FD_CLOEXEC); +} diff --git a/portal/Makefile.am.inc b/portal/Makefile.am.inc index 119c1cf9..f10b67b4 100644 --- a/portal/Makefile.am.inc +++ b/portal/Makefile.am.inc @@ -34,6 +34,6 @@ flatpak_portal_SOURCES = \ BUILT_SOURCES += $(nodist_flatpak_portal_SOURCES) CLEANFILES += $(nodist_flatpak_portal_SOURCES) -flatpak_portal_LDADD = $(AM_LDADD) $(BASE_LIBS) +flatpak_portal_LDADD = $(AM_LDADD) $(BASE_LIBS) libflatpak-common-base.la libglnx.la flatpak_portal_CFLAGS = $(AM_CFLAGS) $(BASE_CFLAGS) -DFLATPAK_COMPILATION flatpak_portal_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/portal diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c index 250b674c..ac51b290 100644 --- a/portal/flatpak-portal.c +++ b/portal/flatpak-portal.c @@ -32,6 +32,7 @@ #include "flatpak-portal.h" #include "flatpak-portal-app-info.h" #include "flatpak-portal-error.h" +#include "flatpak-utils-base-private.h" #define IDLE_TIMEOUT_SECS 10 * 60 @@ -176,6 +177,8 @@ child_setup_func (gpointer user_data) sigset_t set; int i; + flatpak_close_fds_workaround (3); + /* Unblock all signals */ sigemptyset (&set); if (pthread_sigmask (SIG_SETMASK, &set, NULL) == -1) @@ -591,10 +594,11 @@ handle_spawn (PortalFlatpak *object, g_debug ("Starting: %s\n", cmd->str); } + /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */ if (!g_spawn_async_with_pipes (NULL, (char **) flatpak_argv->pdata, env, - G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD, + G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_LEAVE_DESCRIPTORS_OPEN, child_setup_func, &child_setup_data, &pid, NULL, diff --git a/system-helper/flatpak-system-helper.c b/system-helper/flatpak-system-helper.c index 7d97c027..b6b484a8 100644 --- a/system-helper/flatpak-system-helper.c +++ b/system-helper/flatpak-system-helper.c @@ -32,12 +32,14 @@ #include #include #include +#include #include "flatpak-dbus-generated.h" #include "flatpak-dir-private.h" #include "flatpak-oci-registry-private.h" #include "flatpak-error.h" #include "flatpak-utils-private.h" +#include "flatpak-utils-base-private.h" static PolkitAuthority *authority = NULL; static FlatpakSystemHelper *helper = NULL; @@ -1465,6 +1467,10 @@ revokefs_fuse_backend_child_setup (gpointer user_data) { struct passwd *passwd = user_data; + /* We use 4 instead of 3 here, because fd 3 is the inerited socket + and got dup2() into place before this by GSubprocess */ + flatpak_close_fds_workaround (4); + if (setgid (passwd->pw_gid) == -1) { g_warning ("Failed to setgid(%d) for revokefs backend: %s", @@ -1548,7 +1554,8 @@ ongoing_pull_new (FlatpakSystemHelper *object, return NULL; } - launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE); + /* We use INHERIT_FDS to work around dead-lock, see flatpak_close_fds_workaround */ + launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_INHERIT_FDS); g_subprocess_launcher_set_child_setup (launcher, revokefs_fuse_backend_child_setup, passwd, NULL); g_subprocess_launcher_take_fd (launcher, sockets[0], 3); fcntl (sockets[1], F_SETFD, FD_CLOEXEC);