--- Begin Message ---
Package: release.debian.org
Severity: normal
Tags: bookworm
X-Debbugs-Cc: ost...@packages.debian.org
Control: affects -1 + src:ostree src:flatpak libcurl3-gnutls
User: release.debian....@packages.debian.org
Usertags: pu
[ Reason ]
Fix a serious regression in flatpak when libcurl3-gnutls is upgraded to
the version from bookworm-backports
[ Impact ]
For users of pure bookworm, no impact.
For users of bookworm-backports' libcurl3-gnutls, flatpak crashes with
an assertion failure when trying to install or upgrade apps/runtimes,
which is fixed by the proposed package.
[ Tests ]
Unfortunately neither the ostree test suite nor the flatpak test suite
reproduces the assertion failure (they use a simple mock http server
and the bad code path appears to require a fully-featured http server,
possibly HTTP/2).
There is a simple manual reproducer, and I've verified that the proposed
package fixes the assertion failure here:
$ podman run --rm -it debian:bookworm-slim
# echo "deb http://deb.debian.org/debian bookworm-backports main" >
/etc/apt/sources.list.d/debian-backports.list
# apt update
# apt install --no-install-recommends flatpak ca-certificates
# apt install libcurl3-gnutls/bookworm-backports
# flatpak remote-add --if-not-exists flathub
https://dl.flathub.org/repo/flathub.flatpakrepo
# flatpak install flathub org.gnome.Recipes
(It is OK and expected that installation of
org.freedesktop.Platform.openh264 fails with "apply_extra script failed",
and other steps log a warning "bwrap: Creating new namespace failed:
Operation not permitted", when installing inside an unprivileged
container: this is a limitation of nested containers and is unrelated
to the regression.)
I have also confirmed that the proposed ostree version can successfully
install a Flatpak app in a Debian 12 GNOME desktop VM with each of
bookworm libcurl3-gnutls or bookworm-backports libcurl3-gnutls.
[ Risks ]
The actual fix seems very low-risk: it's a straightforward backport
of an upstream change that they specifically called out as suitable
for backporting.
The accompanying change to add an assertion failure if curl_multi_assign()
fails could conceivably make a wrong-but-harmless situation into a crash,
but my understanding is that it's something that should never fail for
reasons other than a programming error, and it does seem valuable to
check this.
[ 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 (old)stable
[x] the issue is verified as fixed in unstable
[ Changes ]
d/control, d/gbp.conf:
Administrivia because this is its first stable update in the bookworm cycle
debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch:
This is the actual bug-fix. The original uses C11 <stdbool.h>, but 2022.7
didn't use that, so I adjusted the patch to use GLib's booleans instead
(the only functional difference is that the struct might be slightly
larger).
debian/patches/curl-Assert-that-curl_multi_assign-worked.patch:
While debugging this assertion failure, upstream added an assertion that
improved their ability to locate the problem.
src/libostree/ostree-fetcher-curl.c:
Modified by each of the patches, see above
diffstat for ostree-2022.7 ostree-2022.7
debian/changelog | 13 ++
debian/control | 2
debian/gbp.conf | 2
debian/patches/curl-Assert-that-curl_multi_assign-worked.patch | 31 ++++
debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch | 64 ++++++++++
debian/patches/series | 2
src/libostree/ostree-fetcher-curl.c | 17 ++
7 files changed, 128 insertions(+), 3 deletions(-)
diff -Nru ostree-2022.7/debian/changelog ostree-2022.7/debian/changelog
--- ostree-2022.7/debian/changelog 2022-12-06 11:11:05.000000000 +0000
+++ ostree-2022.7/debian/changelog 2024-10-01 12:25:32.000000000 +0100
@@ -1,3 +1,16 @@
+ostree (2022.7-2+deb12u1) bookworm; urgency=medium
+
+ * d/control, d/gbp.conf: Configure for stable updates
+ * d/p/curl-Assert-that-curl_multi_assign-worked.patch,
+ d/p/curl-Make-socket-callback-during-cleanup-into-no-op.patch:
+ Add patches from upstream 2024.8 to avoid libflatpak crash with an
+ assertion failure when using curl 8.10.x.
+ This was originally reported in testing/unstable, but can affect
+ bookworm if using libcurl3-gnutls from bookworm-backports.
+ (Closes: #1082121)
+
+ -- Simon McVittie <s...@debian.org> Tue, 01 Oct 2024 12:25:32 +0100
+
ostree (2022.7-2) unstable; urgency=medium
* Skip test-sysroot.js on s390x (Mitigates: #1025532)
diff -Nru ostree-2022.7/debian/control ostree-2022.7/debian/control
--- ostree-2022.7/debian/control 2022-12-06 11:11:05.000000000 +0000
+++ ostree-2022.7/debian/control 2024-10-01 12:25:32.000000000 +0100
@@ -50,7 +50,7 @@
Rules-Requires-Root: no
Standards-Version: 4.6.1
Homepage: https://github.com/ostreedev/ostree/
-Vcs-Git: https://salsa.debian.org/debian/ostree.git
+Vcs-Git: https://salsa.debian.org/debian/ostree.git -b debian/bookworm
Vcs-Browser: https://salsa.debian.org/debian/ostree
Package: gir1.2-ostree-1.0
diff -Nru ostree-2022.7/debian/gbp.conf ostree-2022.7/debian/gbp.conf
--- ostree-2022.7/debian/gbp.conf 2022-12-06 11:11:05.000000000 +0000
+++ ostree-2022.7/debian/gbp.conf 2024-10-01 12:25:32.000000000 +0100
@@ -1,7 +1,7 @@
[DEFAULT]
pristine-tar = True
compression = xz
-debian-branch = debian/latest
+debian-branch = debian/bookworm
upstream-branch = upstream/latest
patch-numbers = False
upstream-vcs-tag = v%(version)s
diff -Nru ostree-2022.7/debian/patches/curl-Assert-that-curl_multi_assign-worked.patch ostree-2022.7/debian/patches/curl-Assert-that-curl_multi_assign-worked.patch
--- ostree-2022.7/debian/patches/curl-Assert-that-curl_multi_assign-worked.patch 1970-01-01 01:00:00.000000000 +0100
+++ ostree-2022.7/debian/patches/curl-Assert-that-curl_multi_assign-worked.patch 2024-10-01 12:25:32.000000000 +0100
@@ -0,0 +1,31 @@
+From: Colin Walters <walt...@verbum.org>
+Date: Wed, 18 Sep 2024 13:21:27 -0400
+Subject: curl: Assert that curl_multi_assign worked
+
+ref https://github.com/ostreedev/ostree/issues/3299
+
+This won't fix that issue, but *if* this assertion triggers
+it should give us a better idea of the possible codepaths
+where it is happening.
+
+Signed-off-by: Colin Walters <walt...@verbum.org>
+Origin: upstream, 2024.8, commit:472d9d493a3e4a08415da4c337a7e831e0c5a5e2
+Bug-Debian: https://bugs.debian.org/1082121
+---
+ src/libostree/ostree-fetcher-curl.c | 3 ++-
+ 1 file changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/src/libostree/ostree-fetcher-curl.c b/src/libostree/ostree-fetcher-curl.c
+index 522eacf..3bbd9ba 100644
+--- a/src/libostree/ostree-fetcher-curl.c
++++ b/src/libostree/ostree-fetcher-curl.c
+@@ -509,7 +509,8 @@ addsock (curl_socket_t s, CURL *easy, int action, OstreeFetcher *fetcher)
+ fdp->refcount = 1;
+ fdp->fetcher = fetcher;
+ setsock (fdp, s, action, fetcher);
+- curl_multi_assign (fetcher->multi, s, fdp);
++ CURLMcode rc = curl_multi_assign (fetcher->multi, s, fdp);
++ g_assert_cmpint (rc, ==, CURLM_OK);
+ g_hash_table_add (fetcher->sockets, fdp);
+ }
+
diff -Nru ostree-2022.7/debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch ostree-2022.7/debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch
--- ostree-2022.7/debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch 1970-01-01 01:00:00.000000000 +0100
+++ ostree-2022.7/debian/patches/curl-Make-socket-callback-during-cleanup-into-no-op.patch 2024-10-01 12:25:32.000000000 +0100
@@ -0,0 +1,64 @@
+From: Colin Walters <walt...@verbum.org>
+Date: Wed, 18 Sep 2024 13:41:59 -0400
+Subject: curl: Make socket callback during cleanup into no-op
+
+Because curl_multi_cleanup may invoke callbacks, we effectively have
+some circular references going on here. See discussion in
+
+https://github.com/curl/curl/issues/14860
+
+Basically what we do is the socket callback libcurl may invoke into a no-op when
+we detect we're finalizing. The data structures are owned by this object and
+not by the callbacks, and will be destroyed below. Note that
+e.g. g_hash_table_unref() may itself invoke callbacks, which is where
+some data is cleaned up.
+
+Signed-off-by: Colin Walters <walt...@verbum.org>
+Origin: upstream, 2024.8, commit:4d755a85225ea0a02d4580d088bb8a97138cb040
+Bug: https://github.com/ostreedev/ostree/issues/3299
+Bug-Debian: https://bugs.debian.org/1082121
+[smcv: Backport to 2022.7 by using gboolean instead of stdbool.h]
+Signed-off-by: Simon McVittie <s...@debian.org>
+---
+ src/libostree/ostree-fetcher-curl.c | 14 ++++++++++++++
+ 1 file changed, 14 insertions(+)
+
+diff --git a/src/libostree/ostree-fetcher-curl.c b/src/libostree/ostree-fetcher-curl.c
+index 3bbd9ba..eae6c4a 100644
+--- a/src/libostree/ostree-fetcher-curl.c
++++ b/src/libostree/ostree-fetcher-curl.c
+@@ -75,6 +75,7 @@ struct OstreeFetcher
+ char *proxy;
+ struct curl_slist *extra_headers;
+ int tmpdir_dfd;
++ gboolean finalizing; // Set if we're in the process of teardown
+ char *custom_user_agent;
+
+ GMainContext *mainctx;
+@@ -174,6 +175,15 @@ _ostree_fetcher_finalize (GObject *object)
+ {
+ OstreeFetcher *self = OSTREE_FETCHER (object);
+
++ // Because curl_multi_cleanup may invoke callbacks, we effectively have
++ // some circular references going on here. See discussion in
++ // https://github.com/curl/curl/issues/14860
++ // Basically what we do is make most callbacks libcurl may invoke into no-ops when
++ // we detect we're finalizing. The data structures are owned by this object and
++ // not by the callbacks, and will be destroyed below. Note that
++ // e.g. g_hash_table_unref() may itself invoke callbacks, which is where
++ // some data is cleaned up.
++ self->finalizing = TRUE;
+ curl_multi_cleanup (self->multi);
+ g_free (self->remote_name);
+ g_free (self->tls_ca_db_path);
+@@ -521,6 +531,10 @@ sock_cb (CURL *easy, curl_socket_t s, int what, void *cbp, void *sockp)
+ OstreeFetcher *fetcher = cbp;
+ SockInfo *fdp = (SockInfo*) sockp;
+
++ // We do nothing if we're in the process of teardown; see below.
++ if (fetcher->finalizing)
++ return 0;
++
+ if (what == CURL_POLL_REMOVE)
+ {
+ if (!g_hash_table_remove (fetcher->sockets, fdp))
diff -Nru ostree-2022.7/debian/patches/series ostree-2022.7/debian/patches/series
--- ostree-2022.7/debian/patches/series 2022-12-06 11:11:05.000000000 +0000
+++ ostree-2022.7/debian/patches/series 2024-10-01 12:25:32.000000000 +0100
@@ -1,3 +1,5 @@
configure-use-pkg-config-with-newer-gpgme-and-gpg-error.patch
+curl-Assert-that-curl_multi_assign-worked.patch
+curl-Make-socket-callback-during-cleanup-into-no-op.patch
debian/Skip-test-pull-repeated-during-CI.patch
debian/test-sysroot-Skip-on-s390x-by-default.patch
diff -Nru ostree-2022.7/src/libostree/ostree-fetcher-curl.c ostree-2022.7/src/libostree/ostree-fetcher-curl.c
--- ostree-2022.7/src/libostree/ostree-fetcher-curl.c 2022-10-29 23:03:49.000000000 +0100
+++ ostree-2022.7/src/libostree/ostree-fetcher-curl.c 2024-10-01 12:37:15.000000000 +0100
@@ -75,6 +75,7 @@
char *proxy;
struct curl_slist *extra_headers;
int tmpdir_dfd;
+ gboolean finalizing; // Set if we're in the process of teardown
char *custom_user_agent;
GMainContext *mainctx;
@@ -174,6 +175,15 @@
{
OstreeFetcher *self = OSTREE_FETCHER (object);
+ // Because curl_multi_cleanup may invoke callbacks, we effectively have
+ // some circular references going on here. See discussion in
+ // https://github.com/curl/curl/issues/14860
+ // Basically what we do is make most callbacks libcurl may invoke into no-ops when
+ // we detect we're finalizing. The data structures are owned by this object and
+ // not by the callbacks, and will be destroyed below. Note that
+ // e.g. g_hash_table_unref() may itself invoke callbacks, which is where
+ // some data is cleaned up.
+ self->finalizing = TRUE;
curl_multi_cleanup (self->multi);
g_free (self->remote_name);
g_free (self->tls_ca_db_path);
@@ -509,7 +519,8 @@
fdp->refcount = 1;
fdp->fetcher = fetcher;
setsock (fdp, s, action, fetcher);
- curl_multi_assign (fetcher->multi, s, fdp);
+ CURLMcode rc = curl_multi_assign (fetcher->multi, s, fdp);
+ g_assert_cmpint (rc, ==, CURLM_OK);
g_hash_table_add (fetcher->sockets, fdp);
}
@@ -520,6 +531,10 @@
OstreeFetcher *fetcher = cbp;
SockInfo *fdp = (SockInfo*) sockp;
+ // We do nothing if we're in the process of teardown; see below.
+ if (fetcher->finalizing)
+ return 0;
+
if (what == CURL_POLL_REMOVE)
{
if (!g_hash_table_remove (fetcher->sockets, fdp))
--- End Message ---
