From 97b5a8f66bb7d9574ac285399bb035837a86cc3b Mon Sep 17 00:00:00 2001 From: Simon McVittie <smcv@collabora.com> Date: Thu, 17 Dec 2020 14:28:15 +0000 Subject: [PATCH] pressure-vessel: Let short-term subprocesses inherit non-CLOEXEC fds To work around older versions of GLib having a potential deadlock in their fd-closing code due to use of non-async-signal-safe functions in the forked child process, the Flatpak-derived code we're using here uses a simple, naive implementation of closing fds: it iterates through every possible fd, up to the rlimit. Unfortunately, Wine users sometimes set their fd rlimit very high (typically around a million), because the "esync" mechanism in Proton's Wine needs a lot of fds. The time taken by each individual syscall is not significant, but when we do a million of them, it adds up. This is arguably a misconfiguration - the recommendation is now to have a soft rlimit of 1024 and a hard rlimit of about a million, so that only processes that opt-in to dealing with a million fds have to be aware of them - but a significant number of misconfigured systems probably still exist. We don't actually *need* to close all the fds here: there's no security boundary, so giving the child access to unnecessary fds isn't a privilage gain; the parent process outlives the child, so leaking fds won't result in them never being closed; and in any case we're reasonably careful to set CLOEXEC on all our fds. So let's just not close them. In particular, this can save literally a million syscalls per helper subprocess invocation (it's not often I get to say that!), reducing pressure-vessel-wrap launch time by 90% on a system with the high fd limit (about 70 seconds down to 7). With the recommended soft limit of 1024, the speedup is less dramatic, but it still takes off nearly 10% (about 6 down to 5.5). Resolves: https://github.com/ValveSoftware/steam-runtime/issues/323 Signed-off-by: Simon McVittie <smcv@collabora.com> --- pressure-vessel/adverb.c | 11 +++++++---- pressure-vessel/bwrap.c | 17 ++++++++--------- pressure-vessel/utils.c | 13 +++++-------- pressure-vessel/wrap.c | 2 +- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/pressure-vessel/adverb.c b/pressure-vessel/adverb.c index 1c3f01261..29188fc47 100644 --- a/pressure-vessel/adverb.c +++ b/pressure-vessel/adverb.c @@ -106,12 +106,12 @@ child_setup_cb (gpointer user_data) dup2 (data->original_stdout_fd, STDOUT_FILENO) != STDOUT_FILENO) pv_async_signal_safe_error ("pressure-vessel-adverb: Unable to reinstate original stdout\n", LAUNCH_EX_FAILED); - /* Make all other file descriptors close-on-exec */ - flatpak_close_fds_workaround (3); - /* Make the fds we pass through *not* be close-on-exec */ if (data != NULL && data->pass_fds) { + /* Make all other file descriptors close-on-exec */ + flatpak_close_fds_workaround (3); + for (iter = data->pass_fds; *iter >= 0; iter++) { int fd = *iter; @@ -381,7 +381,10 @@ run_helper_sync (const char *cwd, return glnx_throw_errno_prefix (error, "pthread_sigmask"); /* We use LEAVE_DESCRIPTORS_OPEN to work around a deadlock in older GLib, - * see flatpak_close_fds_workaround */ + * and to avoid wasting a lot of time closing fds if the rlimit for + * maximum open file descriptors is high. Because we're waiting for the + * subprocess to finish anyway, it doesn't really matter that any fds + * that are not close-on-execute will get leaked into the child. */ ret = g_spawn_sync (cwd, (gchar **) argv, (gchar **) envp, diff --git a/pressure-vessel/bwrap.c b/pressure-vessel/bwrap.c index 9ef7f6e33..32726d53e 100644 --- a/pressure-vessel/bwrap.c +++ b/pressure-vessel/bwrap.c @@ -32,8 +32,8 @@ * or -1 if it could not be launched or was killed by a signal * @error: Used to raise an error on failure * - * Try to run a command. Its standard output and standard error go to - * pressure-vessel's own stdout and stderr. + * Try to run a command. It inherits pressure-vessel's own file + * descriptors. * * Returns: %TRUE if the subprocess runs to completion */ @@ -47,7 +47,6 @@ pv_bwrap_run_sync (FlatpakBwrap *bwrap, g_autofree gchar *errors = NULL; guint i; g_autoptr(GString) command = g_string_new (""); - GArray *child_setup_data = NULL; g_return_val_if_fail (bwrap != NULL, FALSE); g_return_val_if_fail (bwrap->argv->len >= 2, FALSE); @@ -69,20 +68,20 @@ pv_bwrap_run_sync (FlatpakBwrap *bwrap, g_string_append_printf (command, " %s", quoted); } - if (bwrap->fds != NULL && bwrap->fds->len > 0) - child_setup_data = bwrap->fds; - g_debug ("run:%s", command->str); /* We use LEAVE_DESCRIPTORS_OPEN to work around a deadlock in older GLib, - * see flatpak_close_fds_workaround */ + * and to avoid wasting a lot of time closing fds if the rlimit for + * maximum open file descriptors is high. Because we're waiting for the + * subprocess to finish anyway, it doesn't really matter that any fds + * that are not close-on-execute will get leaked into the child. */ if (!g_spawn_sync (NULL, /* cwd */ (char **) bwrap->argv->pdata, bwrap->envp, (G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN), - flatpak_bwrap_child_setup_cb, - child_setup_data, + NULL, + NULL, &output, &errors, &exit_status, diff --git a/pressure-vessel/utils.c b/pressure-vessel/utils.c index c1614207d..8de743af3 100644 --- a/pressure-vessel/utils.c +++ b/pressure-vessel/utils.c @@ -42,12 +42,6 @@ #include "flatpak-utils-base-private.h" #include "flatpak-utils-private.h" -static void -child_setup_cb (gpointer user_data) -{ - flatpak_close_fds_workaround (3); -} - /** * pv_envp_cmp: * @p1: a `const char * const *` @@ -196,13 +190,16 @@ pv_capture_output (const char * const * argv, g_debug ("run:%s", command->str); /* We use LEAVE_DESCRIPTORS_OPEN to work around a deadlock in older GLib, - * see flatpak_close_fds_workaround */ + * and to avoid wasting a lot of time closing fds if the rlimit for + * maximum open file descriptors is high. Because we're waiting for the + * subprocess to finish anyway, it doesn't really matter that any fds + * that are not close-on-execute will get leaked into the child. */ if (!g_spawn_sync (NULL, /* cwd */ (char **) argv, NULL, /* env */ (G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN), - child_setup_cb, NULL, + NULL, NULL, &output, &errors, &wait_status, diff --git a/pressure-vessel/wrap.c b/pressure-vessel/wrap.c index b9709f861..e91328060 100644 --- a/pressure-vessel/wrap.c +++ b/pressure-vessel/wrap.c @@ -227,7 +227,7 @@ check_launch_on_host (const char *launch_executable, if (!g_spawn_sync (NULL, /* cwd */ (gchar **) test_argv, NULL, /* environ */ - G_SPAWN_DEFAULT, + G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, /* child setup */ &child_stdout, &child_stderr, -- GitLab