[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#1083090: bookworm-pu: package ostree/2022.7-2+deb12u1



Package: release.debian.org
Severity: normal
Tags: bookworm
X-Debbugs-Cc: ostree@packages.debian.org
Control: affects -1 + src:ostree src:flatpak libcurl3-gnutls
User: release.debian.org@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 <smcv@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 <walters@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 <walters@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 <walters@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 <walters@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 <smcv@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))

Reply to: