Control: tags -1 moreinfo confirmed
On 2021-06-16 22:04:01 +0100, Simon McVittie wrote:
> Package: release.debian.org
> Severity: normal
> User: release.debian.org@packages.debian.org
> Usertags: unblock
>
> Would you be willing to accept an update to flatpak before full freeze?
Assuming that the upload happens soon, please go ahead and remove the
moreinfo tag once the new version is available in unstable.
Cheers
>
> [ Reason ]
> Fix regressions for apps that use the "subsandbox" feature, such as
> Chromium.
>
> [ Impact ]
> Flatpak apps that use the subsandbox interface to run part of themselves
> under tighter restrictions or with a different library stack can cause
> the flatpak-portal service to run out of file descriptors, preventing
> subsequent subsandboxes from starting (#989934, non-RC).
> Subsandboxes might run with unintended environment variables or without
> forwarding an intended file descriptor from the main app, or fail to
> start altogether (#989935, non-RC). I would rate the severity of both
> bugs as important, or maybe normal.
>
> The backported changes also fix some minor memory leaks, which I backported
> in order to make the subsandbox changes apply cleanly. There's no bug
> report for these, but if there was, I'd rate it as minor severity.
>
> [ Tests ]
> This is a straightforward backport of changes from upstream git master
> (soon to be released as 1.11.2), where they are covered by new automated
> tests. The tests are somewhat intrusive so I'm not backporting those
> right now, although they might be included in 1.10.3 upstream.
>
> I've run manual tests for these two bugs on a bullseye system as described
> on <https://github.com/flatpak/flatpak/pull/4322>.
>
> There is autopkgtest coverage for Flatpak generally, although not yet for
> this specific feature. It can't be run on ci.debian.net due to not having
> isolation-machine testbeds available (flatpak doesn't work inside lxc),
> but I run the autopkgtests in a qemu VM before each upload.
>
> [ Risks ]
> The changes are isolated to flatpak-portal (other than a few trivial
> memory-leak fixes elsewhere in the codebase), so the only programs that
> could regress are Flatpak apps that use the subsandbox interface, and
> those are the ones already affected by the bugs that I'm fixing.
>
> [ Checklist ]
> [x] all changes are documented in the d/changelog
> [x] I reviewed all changes and I approve them
> [x] attach debdiff against the package in testing
> (preliminary debdiff, not yet released)
>
> [ Other info ]
> If the SRMs allow it, I would like to follow the Flatpak 1.10.x release
> branch during bullseye until it is discontinued upstream, as I did
> for 0.8.x in stretch and 1.2.x in buster. I am an upstream maintainer
> and will be checking what gets backported into 1.10.x to make sure
> it remains manageable. Like previous stable branches, it will contain
> security fixes, bug fixes, and perhaps targeted feature enhancements
> where they are required for compatibility with apps and runtimes that
> users are likely to install.
>
> We'll hopefully have a 1.10.3 upstream release reasonably soon, which could
> either go to bullseye before release or be saved for a stable update as part
> of the 11.1 point release, depending on timing and what the release team
> think of the diffstat.
>
> Thanks,
> smcv
> diff --git a/debian/changelog b/debian/changelog
> index e4bb4964d..d36ce25db 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,21 @@
> +flatpak (1.10.2-2) UNRELEASED; urgency=medium
> +
> + * Backport changes from upstream git to fix regressions when apps invoke
> + flatpak-spawn --env=... to launch a subsandbox.
> + - d/p/Fix-several-memory-leaks.patch:
> + Fix minor memory leaks so that subsequent backports apply cleanly
> + - d/p/portal-Don-t-leak-fd-used-for-serialized-environment.patch:
> + Don't leak a file descriptor each time flatpak-spawn --env=... is used
> + (Closes: #989934)
> + - d/p/portal-Use-a-GArray-to-store-fds.patch,
> + d/p/portal-Remap-env-fd-into-child-process-s-fd-space.patch:
> + When an app uses flatpak-spawn --env=... --forward-fd=..., ensure
> + that the file descriptors do not collide, which could result in the
> + subsandbox failing to launch or being launched with wrong environment
> + variables. (Closes: #989935)
> +
> + -- Simon McVittie <smcv@debian.org> Wed, 16 Jun 2021 10:48:08 +0100
> +
> flatpak (1.10.2-1) unstable; urgency=medium
>
> * New upstream stable release
> diff --git a/debian/patches/Fix-several-memory-leaks.patch b/debian/patches/Fix-several-memory-leaks.patch
> new file mode 100644
> index 000000000..2f0b53064
> --- /dev/null
> +++ b/debian/patches/Fix-several-memory-leaks.patch
> @@ -0,0 +1,86 @@
> +From: Phaedrus Leeds <mwleeds@protonmail.com>
> +Date: Sun, 2 May 2021 21:53:02 -0500
> +Subject: Fix several memory leaks
> +
> +(cherry picked from commit 404d7c6941baf63d1b3ccbe9ee9d34f3ff12f35f)
> +
> +Applied-upstream: 1.10.3, commit:b912053c6cc556f131465c1fd877d7bd0b433539
> +---
> + app/flatpak-builtins-document-export.c | 6 +++---
> + common/flatpak-dir.c | 7 ++++---
> + common/flatpak-utils.c | 1 +
> + portal/flatpak-portal.c | 2 +-
> + 4 files changed, 9 insertions(+), 7 deletions(-)
> +
> +diff --git a/app/flatpak-builtins-document-export.c b/app/flatpak-builtins-document-export.c
> +index 15f1ad1..e701a82 100644
> +--- a/app/flatpak-builtins-document-export.c
> ++++ b/app/flatpak-builtins-document-export.c
> +@@ -90,8 +90,8 @@ flatpak_builtin_document_export (int argc, char **argv,
> + g_autofree char *dirname = NULL;
> + g_autofree char *doc_path = NULL;
> + XdpDbusDocuments *documents;
> +- int fd, fd_id;
> +- int i;
> ++ glnx_autofd int fd = -1;
> ++ int i, fd_id;
> + GUnixFDList *fd_list = NULL;
> + const char *doc_id;
> + struct stat stbuf;
> +@@ -173,7 +173,7 @@ flatpak_builtin_document_export (int argc, char **argv,
> +
> + fd_list = g_unix_fd_list_new ();
> + fd_id = g_unix_fd_list_append (fd_list, fd, error);
> +- close (fd);
> ++ glnx_close_fd (&fd);
> +
> + if (opt_noexist)
> + {
> +diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
> +index 94a86f4..0724677 100644
> +--- a/common/flatpak-dir.c
> ++++ b/common/flatpak-dir.c
> +@@ -13671,14 +13671,15 @@ parse_ref_file (GKeyFile *keyfile,
> + collection_id = g_key_file_get_string (keyfile, FLATPAK_REF_GROUP,
> + FLATPAK_REF_DEPLOY_COLLECTION_ID_KEY, NULL);
> +
> +- if (collection_id == NULL || *collection_id == '\0')
> ++ if (collection_id != NULL && *collection_id == '\0')
> ++ g_clear_pointer (&collection_id, g_free);
> ++ if (collection_id == NULL)
> + {
> + collection_id = g_key_file_get_string (keyfile, FLATPAK_REF_GROUP,
> + FLATPAK_REF_COLLECTION_ID_KEY, NULL);
> + }
> +-
> + if (collection_id != NULL && *collection_id == '\0')
> +- collection_id = NULL;
> ++ g_clear_pointer (&collection_id, g_free);
> +
> + if (collection_id != NULL && gpg_data == NULL)
> + return flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, _("Collection ID requires GPG key to be provided"));
> +diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c
> +index 56cbb06..84bc6a3 100644
> +--- a/common/flatpak-utils.c
> ++++ b/common/flatpak-utils.c
> +@@ -2285,6 +2285,7 @@ flatpak_parse_repofile (const char *remote_name,
> + decoded = g_base64_decode (gpg_key, &decoded_len);
> + if (decoded_len < 10) /* Check some minimal size so we don't get crap */
> + {
> ++ g_free (decoded);
> + flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, _("Invalid gpg key"));
> + return NULL;
> + }
> +diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c
> +index 7887c57..0539ff2 100644
> +--- a/portal/flatpak-portal.c
> ++++ b/portal/flatpak-portal.c
> +@@ -759,7 +759,7 @@ handle_spawn (PortalFlatpak *object,
> + const gint *fds = NULL;
> + gint fds_len = 0;
> + g_autofree FdMapEntry *fd_map = NULL;
> +- gchar **env;
> ++ g_auto(GStrv) env = NULL;
> + gint32 max_fd;
> + GKeyFile *app_info;
> + g_autoptr(GPtrArray) flatpak_argv = g_ptr_array_new_with_free_func (g_free);
> diff --git a/debian/patches/portal-Don-t-leak-fd-used-for-serialized-environment.patch b/debian/patches/portal-Don-t-leak-fd-used-for-serialized-environment.patch
> new file mode 100644
> index 000000000..cc22894c9
> --- /dev/null
> +++ b/debian/patches/portal-Don-t-leak-fd-used-for-serialized-environment.patch
> @@ -0,0 +1,42 @@
> +From: Simon McVittie <smcv@collabora.com>
> +Date: Thu, 20 May 2021 18:52:38 +0100
> +Subject: portal: Don't leak fd used for serialized environment
> +
> +Otherwise we'll run out of file descriptors eventually, when starting
> +a sufficiently large number of subsandboxes.
> +
> +Resolves: flatpak/flatpak#4285
> +Fixes: aeb6a7ab "portal: Convert --env in extra-args into --env-fd"
> +Signed-off-by: Simon McVittie <smcv@collabora.com>
> +Origin: upstream, 1.11.2, commit:f2fbc75827a58cc6b4cba48a0c895c3313274020
> +Forwarded: https://github.com/flatpak/flatpak/pull/4322
> +---
> + portal/flatpak-portal.c | 7 ++++---
> + 1 file changed, 4 insertions(+), 3 deletions(-)
> +
> +diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c
> +index 0539ff2..e6eeecb 100644
> +--- a/portal/flatpak-portal.c
> ++++ b/portal/flatpak-portal.c
> +@@ -788,6 +788,7 @@ handle_spawn (PortalFlatpak *object,
> + gboolean notify_start;
> + gboolean devel;
> + g_autoptr(GString) env_string = g_string_new ("");
> ++ glnx_autofd int env_fd = -1;
> +
> + child_setup_data.instance_id_fd = -1;
> + child_setup_data.env_fd = -1;
> +@@ -1122,10 +1123,10 @@ handle_spawn (PortalFlatpak *object,
> + return G_DBUS_METHOD_INVOCATION_HANDLED;
> + }
> +
> +- child_setup_data.env_fd = glnx_steal_fd (&env_tmpf.fd);
> ++ env_fd = glnx_steal_fd (&env_tmpf.fd);
> ++ child_setup_data.env_fd = env_fd;
> + g_ptr_array_add (flatpak_argv,
> +- g_strdup_printf ("--env-fd=%d",
> +- child_setup_data.env_fd));
> ++ g_strdup_printf ("--env-fd=%d", env_fd));
> + }
> +
> + for (i = 0; unset_env != NULL && unset_env[i] != NULL; i++)
> diff --git a/debian/patches/portal-Remap-env-fd-into-child-process-s-fd-space.patch b/debian/patches/portal-Remap-env-fd-into-child-process-s-fd-space.patch
> new file mode 100644
> index 000000000..1dc03c395
> --- /dev/null
> +++ b/debian/patches/portal-Remap-env-fd-into-child-process-s-fd-space.patch
> @@ -0,0 +1,48 @@
> +From: Simon McVittie <smcv@collabora.com>
> +Date: Fri, 21 May 2021 13:45:03 +0100
> +Subject: portal: Remap --env-fd into child process's fd space
> +
> +Just because we can allocate a new, unused fd in the portal's fd space,
> +that doesn't mean that fd number is going to be unused in the child
> +process's fd space: we might need to remap it.
> +
> +Resolves: flatpak/flatpak#4286
> +Fixes: aeb6a7ab "portal: Convert --env in extra-args into --env-fd"
> +Signed-off-by: Simon McVittie <smcv@collabora.com>
> +Origin: upstream, 1.11.2, commit:526dae92418616c71517945e810227416f160ce6
> +Forwarded: https://github.com/flatpak/flatpak/pull/4322
> +---
> + portal/flatpak-portal.c | 12 ++++++++++--
> + 1 file changed, 10 insertions(+), 2 deletions(-)
> +
> +diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c
> +index b0664aa..50ebcc8 100644
> +--- a/portal/flatpak-portal.c
> ++++ b/portal/flatpak-portal.c
> +@@ -1085,6 +1085,7 @@ handle_spawn (PortalFlatpak *object,
> +
> + if (env_string->len > 0)
> + {
> ++ FdMapEntry fd_map_entry;
> + g_auto(GLnxTmpfile) env_tmpf = { 0, };
> +
> + if (!flatpak_buffer_to_sealed_memfd_or_tmpfile (&env_tmpf, "environ",
> +@@ -1096,9 +1097,16 @@ handle_spawn (PortalFlatpak *object,
> + }
> +
> + env_fd = glnx_steal_fd (&env_tmpf.fd);
> +- child_setup_data.env_fd = env_fd;
> ++
> ++ /* Use a fd that hasn't been used yet. We might have to reshuffle
> ++ * fd_map_entry.to, a bit later. */
> ++ fd_map_entry.from = env_fd;
> ++ fd_map_entry.to = ++max_fd;
> ++ fd_map_entry.final = fd_map_entry.to;
> ++ g_array_append_val (fd_map, fd_map_entry);
> ++
> + g_ptr_array_add (flatpak_argv,
> +- g_strdup_printf ("--env-fd=%d", env_fd));
> ++ g_strdup_printf ("--env-fd=%d", fd_map_entry.final));
> + }
> +
> + for (i = 0; unset_env != NULL && unset_env[i] != NULL; i++)
> diff --git a/debian/patches/portal-Use-a-GArray-to-store-fds.patch b/debian/patches/portal-Use-a-GArray-to-store-fds.patch
> new file mode 100644
> index 000000000..ef7134e18
> --- /dev/null
> +++ b/debian/patches/portal-Use-a-GArray-to-store-fds.patch
> @@ -0,0 +1,158 @@
> +From: Simon McVittie <smcv@collabora.com>
> +Date: Fri, 21 May 2021 13:38:18 +0100
> +Subject: portal: Use a GArray to store fds
> +
> +This will allow us to add additional mapping entries for fds to be
> +used internally by `flatpak run`, in particular --env-fd.
> +
> +Defer the second pass through the fd array until the last possible
> +moment, so that any extra fds we want to add (like the --env-fd) have
> +already been added by then.
> +
> +Helps: flatpak/flatpak#4286
> +Signed-off-by: Simon McVittie <smcv@collabora.com>
> +Origin: upstream, 1.11.2, commit:a09d07f085d85efdf34934ecc864a6a5ce9af761
> +Forwarded: https://github.com/flatpak/flatpak/pull/4322
> +---
> + portal/flatpak-portal.c | 81 +++++++++++++++++++++++++------------------------
> + 1 file changed, 42 insertions(+), 39 deletions(-)
> +
> +diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c
> +index e6eeecb..b0664aa 100644
> +--- a/portal/flatpak-portal.c
> ++++ b/portal/flatpak-portal.c
> +@@ -253,7 +253,7 @@ typedef struct
> + typedef struct
> + {
> + FdMapEntry *fd_map;
> +- int fd_map_len;
> ++ gsize fd_map_len;
> + int instance_id_fd;
> + gboolean set_tty;
> + int tty;
> +@@ -479,7 +479,7 @@ child_setup_func (gpointer user_data)
> + ChildSetupData *data = (ChildSetupData *) user_data;
> + FdMapEntry *fd_map = data->fd_map;
> + sigset_t set;
> +- int i;
> ++ gsize i;
> +
> + flatpak_close_fds_workaround (3);
> +
> +@@ -758,7 +758,7 @@ handle_spawn (PortalFlatpak *object,
> + gsize i, j, n_fds, n_envs;
> + const gint *fds = NULL;
> + gint fds_len = 0;
> +- g_autofree FdMapEntry *fd_map = NULL;
> ++ g_autoptr(GArray) fd_map = NULL;
> + g_auto(GStrv) env = NULL;
> + gint32 max_fd;
> + GKeyFile *app_info;
> +@@ -923,14 +923,13 @@ handle_spawn (PortalFlatpak *object,
> + n_fds = 0;
> + if (fds != NULL)
> + n_fds = g_variant_n_children (arg_fds);
> +- fd_map = g_new0 (FdMapEntry, n_fds);
> +
> +- child_setup_data.fd_map = fd_map;
> +- child_setup_data.fd_map_len = n_fds;
> ++ fd_map = g_array_sized_new (FALSE, FALSE, sizeof (FdMapEntry), n_fds);
> +
> + max_fd = -1;
> + for (i = 0; i < n_fds; i++)
> + {
> ++ FdMapEntry fd_map_entry;
> + gint32 handle, dest_fd;
> + int handle_fd;
> +
> +@@ -947,9 +946,10 @@ handle_spawn (PortalFlatpak *object,
> +
> + handle_fd = fds[handle];
> +
> +- fd_map[i].to = dest_fd;
> +- fd_map[i].from = handle_fd;
> +- fd_map[i].final = fd_map[i].to;
> ++ fd_map_entry.to = dest_fd;
> ++ fd_map_entry.from = handle_fd;
> ++ fd_map_entry.final = fd_map_entry.to;
> ++ g_array_append_val (fd_map, fd_map_entry);
> +
> + /* If stdin/out/err is a tty we try to set it as the controlling
> + tty for the app, this way we can use this to run in a terminal. */
> +@@ -961,36 +961,8 @@ handle_spawn (PortalFlatpak *object,
> + child_setup_data.tty = handle_fd;
> + }
> +
> +- max_fd = MAX (max_fd, fd_map[i].to);
> +- max_fd = MAX (max_fd, fd_map[i].from);
> +- }
> +-
> +- /* We make a second pass over the fds to find if any "to" fd index
> +- overlaps an already in use fd (i.e. one in the "from" category
> +- that are allocated randomly). If a fd overlaps "to" fd then its
> +- a caller issue and not our fault, so we ignore that. */
> +- for (i = 0; i < n_fds; i++)
> +- {
> +- int to_fd = fd_map[i].to;
> +- gboolean conflict = FALSE;
> +-
> +- /* At this point we're fine with using "from" values for this
> +- value (because we handle to==from in the code), or values
> +- that are before "i" in the fd_map (because those will be
> +- closed at this point when dup:ing). However, we can't
> +- reuse a fd that is in "from" for j > i. */
> +- for (j = i + 1; j < n_fds; j++)
> +- {
> +- int from_fd = fd_map[j].from;
> +- if (from_fd == to_fd)
> +- {
> +- conflict = TRUE;
> +- break;
> +- }
> +- }
> +-
> +- if (conflict)
> +- fd_map[i].to = ++max_fd;
> ++ max_fd = MAX (max_fd, fd_map_entry.to);
> ++ max_fd = MAX (max_fd, fd_map_entry.from);
> + }
> +
> + /* TODO: Ideally we should let `flatpak run` inherit the portal's
> +@@ -1361,6 +1333,37 @@ handle_spawn (PortalFlatpak *object,
> + g_debug ("Starting: %s\n", cmd->str);
> + }
> +
> ++ /* We make a second pass over the fds to find if any "to" fd index
> ++ overlaps an already in use fd (i.e. one in the "from" category
> ++ that are allocated randomly). If a fd overlaps "to" fd then its
> ++ a caller issue and not our fault, so we ignore that. */
> ++ for (i = 0; i < fd_map->len; i++)
> ++ {
> ++ int to_fd = g_array_index (fd_map, FdMapEntry, i).to;
> ++ gboolean conflict = FALSE;
> ++
> ++ /* At this point we're fine with using "from" values for this
> ++ value (because we handle to==from in the code), or values
> ++ that are before "i" in the fd_map (because those will be
> ++ closed at this point when dup:ing). However, we can't
> ++ reuse a fd that is in "from" for j > i. */
> ++ for (j = i + 1; j < fd_map->len; j++)
> ++ {
> ++ int from_fd = g_array_index(fd_map, FdMapEntry, j).from;
> ++ if (from_fd == to_fd)
> ++ {
> ++ conflict = TRUE;
> ++ break;
> ++ }
> ++ }
> ++
> ++ if (conflict)
> ++ g_array_index (fd_map, FdMapEntry, i).to = ++max_fd;
> ++ }
> ++
> ++ 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 */
> + if (!g_spawn_async_with_pipes (NULL,
> + (char **) flatpak_argv->pdata,
> diff --git a/debian/patches/series b/debian/patches/series
> index ee9cda11a..b62fb3796 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -1 +1,5 @@
> tests-Remove-hard-coded-references-to-x86_64.patch
> +Fix-several-memory-leaks.patch
> +portal-Don-t-leak-fd-used-for-serialized-environment.patch
> +portal-Use-a-GArray-to-store-fds.patch
> +portal-Remap-env-fd-into-child-process-s-fd-space.patch
--
Sebastian Ramacher
Attachment:
signature.asc
Description: PGP signature