Replace flatpak_close_fds_workaround() with g_fdwalk_set_cloexec()

flatpak_close_fds_workaround() wasn't technically async-signal-safe,
because the requirement for sysconf() to be async-signal-safe was
removed in POSIX.1-2008.

It could also leave high fds open in some cases: in practice
sysconf(_SC_OPEN_MAX) returns the soft resource limit, but if our
resource limit has been reduced by an ancestor process, we could
conceivably still have fds open and inherited above that number.

We can fix this by using g_fdwalk_set_cloexec() with GLib >= 2.79.2,
or the backport in libglnx with older GLib. This uses close_range()
if possible, falling back to rummaging in /proc with async-signal-safe
syscalls.

Signed-off-by: Simon McVittie <smcv@collabora.com>
This commit is contained in:
Simon McVittie
2024-02-13 14:08:37 +00:00
parent 2a363d7569
commit 7b1cd20696
8 changed files with 23 additions and 27 deletions

View File

@@ -499,8 +499,14 @@ flatpak_bwrap_child_setup (GArray *fd_array,
{
int i;
/* 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.
*/
if (close_fd_workaround)
flatpak_close_fds_workaround (3);
g_fdwalk_set_cloexec (3);
/* If no fd_array was specified, don't care. */
if (fd_array == NULL)

View File

@@ -7143,7 +7143,8 @@ flatpak_dir_run_triggers (FlatpakDir *self,
commandline = flatpak_quote_argv ((const char **) bwrap->argv->pdata, -1);
g_info ("Running '%s'", commandline);
/* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
/* We use LEAVE_DESCRIPTORS_OPEN and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
if (!g_spawn_sync ("/",
(char **) bwrap->argv->pdata,
NULL,

View File

@@ -161,7 +161,8 @@ flatpak_run_maybe_start_dbus_proxy (FlatpakBwrap *app_bwrap,
commandline = flatpak_quote_argv ((const char **) proxy_bwrap->argv->pdata, -1);
g_info ("Running '%s'", commandline);
/* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
/* We use LEAVE_DESCRIPTORS_OPEN and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
if (!g_spawn_async (NULL,
(char **) proxy_bwrap->argv->pdata,
NULL,

View File

@@ -2643,7 +2643,8 @@ 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 */
/* We use LEAVE_DESCRIPTORS_OPEN and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
if (!g_spawn_sync (NULL,
(char **) bwrap->argv->pdata,
bwrap->envp,
@@ -3453,7 +3454,9 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
else
child_setup = flatpak_bwrap_child_setup_inherit_fds_cb;
/* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
/* Even in the case where we want them closed, we use
* LEAVE_DESCRIPTORS_OPEN and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
spawn_flags |= G_SPAWN_LEAVE_DESCRIPTORS_OPEN;
/* flatpak_bwrap_envp_to_args() moved the environment variables to

View File

@@ -36,6 +36,5 @@ 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__ */

View File

@@ -112,19 +112,3 @@ 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);
}

View File

@@ -482,7 +482,7 @@ child_setup_func (gpointer user_data)
sigset_t set;
gsize i;
flatpak_close_fds_workaround (3);
g_fdwalk_set_cloexec (3);
if (data->instance_id_fd != -1)
drop_cloexec (data->instance_id_fd);
@@ -1518,7 +1518,8 @@ handle_spawn (PortalFlatpak *object,
child_setup_data.fd_map = &g_array_index (fd_map, FdMapEntry, 0);
child_setup_data.fd_map_len = fd_map->len;
/* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
/* We use LEAVE_DESCRIPTORS_OPEN and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
if (!g_spawn_async_with_pipes (NULL,
(char **) flatpak_argv->pdata,
env,
@@ -2612,7 +2613,7 @@ update_child_setup_func (gpointer user_data)
int *socket = user_data;
dup2 (*socket, 3);
flatpak_close_fds_workaround (4);
g_fdwalk_set_cloexec (4);
}
/* This is the meat of the update process, its run out of process (via

View File

@@ -1496,7 +1496,7 @@ revokefs_fuse_backend_child_setup (gpointer user_data)
/* We use 5 instead of 3 here, because fd 3 is the inherited SOCK_SEQPACKET
* socket and fd 4 is the --close-with-fd pipe; both were dup2()'d into place
* before this by GSubprocess */
flatpak_close_fds_workaround (5);
g_fdwalk_set_cloexec (5);
if (setgid (passwd->pw_gid) == -1)
{
@@ -1581,7 +1581,8 @@ ongoing_pull_new (FlatpakSystemHelper *object,
return NULL;
}
/* We use INHERIT_FDS to work around dead-lock, see flatpak_close_fds_workaround */
/* We use INHERIT_FDS and close them in the child_setup
* to work around a deadlock in GLib < 2.60 */
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);