Bug#989964: unblock (pre-approval): flatpak/1.10.2-2
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?
[ 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
Reply to: