Merge ~slyon/ubuntu/+source/systemd:slyon/revert-resolved-segfault into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-focal

[Lukas Märdian]

revert/remove resolved-disable-event-sources-before-unreffing-them.patch

    This did not fix the root-cause of the segfault but rather moved it to a
    slightly different location in the code. (LP: #1943982)
    File: debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch

revert/remove resolved-disable-event-sources-before-unreffing-them.patch

    This did not fix the root-cause of the segfault but rather moved it to a
    slightly different location in the code. (LP: #1943982)
    File: debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch

[Lukas Märdian (slyon)]

Actually, looking at the numbers of the new and old crash reports, I think we should let this update in as it seems to improve the situation, even if it is not providing a final fix for the root cause.

https://errors.ubuntu.com/problem/c4e5be3f1c7af9483993c7e6007b9325ab5b78cd (new)
https://errors.ubuntu.com/problem/bb0ce4ff8e6ef86041cfb5647b792823a20b44f7 (old)

As can be seen, the old crash does not happen anymore as of systemd 247.3-3ubuntu3.5/6 (Hirsute) and systemd 245.4-4ubuntu3.12/13 (Focal), but is instead replaced by the new (slightly different, but same root-cause crash). Not a single crash report of this type since the systemd SRU hit hirsute-updates (and started phasing for focal-updates) a week ago on 2021-09-15.

Additionally, we observed only 4 crashes (up to now) on Hirsute (where it's fully phased), whereas there have been 974 crash reports of the old crash in total. This is evidence that the new crash does happen less often, thus improves the situation overall, even though not fixing the root cause. It also supports the statement made by the original bug reporter in https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934221/comments/34 that the crash still happens, but less frequent.

With the old crash not happening anymore and the new crash happening less frequent, I think we should ask the release team to manually approve/phase this SRU, as it improves the overall situation.

[Lukas Märdian (slyon)]

SRU team (RAOF) agreed, so I'm closing this one.

review: Disapprove

Unmerged commits

a75d319... by Lukas Märdian on 2021-09-17

update changelog

87fd092... by Lukas Märdian on 2021-09-17

revert/remove resolved-disable-event-sources-before-unreffing-them.patch

diff --git a/debian/changelog b/debian/changelog
index 98be76e..303329c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+systemd (245.4-4ubuntu3.14) UNRELEASED; urgency=medium
+
+  * revert/remove resolved-disable-event-sources-before-unreffing-them.patch
+    This did not fix the root-cause of the segfault but rather moved it to a
+    slightly different location in the code. (LP: #1943982)
+    File: debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
+    https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=87fd092cbe40be455ef99e6dbd4cef50272da1da
+
+ -- Lukas Märdian <[e-mail]>  Fri, 17 Sep 2021 16:52:06 +0200
+
 systemd (245.4-4ubuntu3.13) focal; urgency=medium
 
   * d/p/dell-clamshell-accel-location-base-with-sku.patch:
diff --git a/debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch b/debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
 deleted file mode 100644
index fa9b484..0000000
--- a/debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
+++ /dev/null
@@ -1,172 +0,0 @@
-From: Denys Fedoryshchenko <[e-mail]>
-Date: Tue, 20 Jul 2021 11:44:01 +0200
-Subject: resolved: disable event sources before unreffing them
-Origin: upstream, https://github.com/systemd/systemd/commit/97935302283729c9206b84f5e00b1aff0f78ad19
-Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934221
-Reviewed-by: Lukas Märdian <[e-mail]>
-
-We generally operate on the assumption that a source is "gone" as soon
-as we unref it. This is generally true because we have the only reference.
-But if something else holds the reference, our unref doesn't really stop
-the source and it could fire again.
-
-In particular, on_query_timeout() is called with DnsQuery* as userdata, and
-it calls dns_query_stop() which invalidates that pointer. If it was ever
-called again, we'd be accessing already-freed memory.
-
-I don't see what would hold the reference. sd-event takes a temporary reference,
-but on the sd_event object, not on the individual sources. And our sources
-are non-floating, so there is no reference from the sd_event object to the
-sources.
-
-For #18427.
----
- src/resolve/resolved-dns-query.c       | 2 +-
- src/resolve/resolved-dns-scope.c       | 8 ++++----
- src/resolve/resolved-dns-stream.c      | 4 ++--
- src/resolve/resolved-dns-stub.c        | 4 ++--
- src/resolve/resolved-dns-transaction.c | 4 ++--
- src/resolve/resolved-llmnr.c           | 8 ++++----
- src/resolve/resolved-mdns.c            | 4 ++--
- 7 files changed, 17 insertions(+), 17 deletions(-)
-
-diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c
-index 4a41921..614cc32 100644
---- a/src/resolve/resolved-dns-query.c
-+++ b/src/resolve/resolved-dns-query.c
-@@ -333,7 +333,7 @@ static void dns_query_stop(DnsQuery *q) {
- 
-         assert(q);
- 
--        q->timeout_event_source = sd_event_source_unref(q->timeout_event_source);
-+        q->timeout_event_source = sd_event_source_disable_unref(q->timeout_event_source);
- 
-         LIST_FOREACH(candidates_by_query, c, q->candidates)
-                 dns_query_candidate_stop(c);
-diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c
-index d7e7b5a..4701736 100644
---- a/src/resolve/resolved-dns-scope.c
-+++ b/src/resolve/resolved-dns-scope.c
-@@ -110,9 +110,9 @@ DnsScope* dns_scope_free(DnsScope *s) {
-         hashmap_free(s->transactions_by_key);
- 
-         ordered_hashmap_free_with_destructor(s->conflict_queue, dns_resource_record_unref);
--        sd_event_source_unref(s->conflict_event_source);
-+        sd_event_source_disable_unref(s->conflict_event_source);
- 
--        sd_event_source_unref(s->announce_event_source);
-+        sd_event_source_disable_unref(s->announce_event_source);
- 
-         dns_cache_flush(&s->cache);
-         dns_zone_flush(&s->zone);
-@@ -998,7 +998,7 @@ static int on_conflict_dispatch(sd_event_source *es, usec_t usec, void *userdata
-         assert(es);
-         assert(scope);
- 
--        scope->conflict_event_source = sd_event_source_unref(scope->conflict_event_source);
-+        scope->conflict_event_source = sd_event_source_disable_unref(scope->conflict_event_source);
- 
-         for (;;) {
-                 _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
-@@ -1204,7 +1204,7 @@ static int on_announcement_timeout(sd_event_source *s, usec_t usec, void *userda
- 
-         assert(s);
- 
--        scope->announce_event_source = sd_event_source_unref(scope->announce_event_source);
-+        scope->announce_event_source = sd_event_source_disable_unref(scope->announce_event_source);
- 
-         (void) dns_scope_announce(scope, false);
-         return 0;
-diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
-index 2a10694..d46302e 100644
---- a/src/resolve/resolved-dns-stream.c
-+++ b/src/resolve/resolved-dns-stream.c
-@@ -17,8 +17,8 @@
- static void dns_stream_stop(DnsStream *s) {
-         assert(s);
- 
--        s->io_event_source = sd_event_source_unref(s->io_event_source);
--        s->timeout_event_source = sd_event_source_unref(s->timeout_event_source);
-+        s->io_event_source = sd_event_source_disable_unref(s->io_event_source);
-+        s->timeout_event_source = sd_event_source_disable_unref(s->timeout_event_source);
-         s->fd = safe_close(s->fd);
- 
-         /* Disconnect us from the server object if we are now not usable anymore */
-diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c
-index ce994a7..851a78a 100644
---- a/src/resolve/resolved-dns-stub.c
-+++ b/src/resolve/resolved-dns-stub.c
-@@ -577,8 +577,8 @@ int manager_dns_stub_start(Manager *m) {
- void manager_dns_stub_stop(Manager *m) {
-         assert(m);
- 
--        m->dns_stub_udp_event_source = sd_event_source_unref(m->dns_stub_udp_event_source);
--        m->dns_stub_tcp_event_source = sd_event_source_unref(m->dns_stub_tcp_event_source);
-+        m->dns_stub_udp_event_source = sd_event_source_disable_unref(m->dns_stub_udp_event_source);
-+        m->dns_stub_tcp_event_source = sd_event_source_disable_unref(m->dns_stub_tcp_event_source);
- 
-         m->dns_stub_udp_fd = safe_close(m->dns_stub_udp_fd);
-         m->dns_stub_tcp_fd = safe_close(m->dns_stub_tcp_fd);
-diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
-index 2f3fb68..5e29275 100644
---- a/src/resolve/resolved-dns-transaction.c
-+++ b/src/resolve/resolved-dns-transaction.c
-@@ -59,14 +59,14 @@ static void dns_transaction_close_connection(DnsTransaction *t) {
-                 t->stream = dns_stream_unref(t->stream);
-         }
- 
--        t->dns_udp_event_source = sd_event_source_unref(t->dns_udp_event_source);
-+        t->dns_udp_event_source = sd_event_source_disable_unref(t->dns_udp_event_source);
-         t->dns_udp_fd = safe_close(t->dns_udp_fd);
- }
- 
- static void dns_transaction_stop_timeout(DnsTransaction *t) {
-         assert(t);
- 
--        t->timeout_event_source = sd_event_source_unref(t->timeout_event_source);
-+        t->timeout_event_source = sd_event_source_disable_unref(t->timeout_event_source);
- }
- 
- DnsTransaction* dns_transaction_free(DnsTransaction *t) {
-diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c
-index 61e5034..5ffa984 100644
---- a/src/resolve/resolved-llmnr.c
-+++ b/src/resolve/resolved-llmnr.c
-@@ -11,16 +11,16 @@
- void manager_llmnr_stop(Manager *m) {
-         assert(m);
- 
--        m->llmnr_ipv4_udp_event_source = sd_event_source_unref(m->llmnr_ipv4_udp_event_source);
-+        m->llmnr_ipv4_udp_event_source = sd_event_source_disable_unref(m->llmnr_ipv4_udp_event_source);
-         m->llmnr_ipv4_udp_fd = safe_close(m->llmnr_ipv4_udp_fd);
- 
--        m->llmnr_ipv6_udp_event_source = sd_event_source_unref(m->llmnr_ipv6_udp_event_source);
-+        m->llmnr_ipv6_udp_event_source = sd_event_source_disable_unref(m->llmnr_ipv6_udp_event_source);
-         m->llmnr_ipv6_udp_fd = safe_close(m->llmnr_ipv6_udp_fd);
- 
--        m->llmnr_ipv4_tcp_event_source = sd_event_source_unref(m->llmnr_ipv4_tcp_event_source);
-+        m->llmnr_ipv4_tcp_event_source = sd_event_source_disable_unref(m->llmnr_ipv4_tcp_event_source);
-         m->llmnr_ipv4_tcp_fd = safe_close(m->llmnr_ipv4_tcp_fd);
- 
--        m->llmnr_ipv6_tcp_event_source = sd_event_source_unref(m->llmnr_ipv6_tcp_event_source);
-+        m->llmnr_ipv6_tcp_event_source = sd_event_source_disable_unref(m->llmnr_ipv6_tcp_event_source);
-         m->llmnr_ipv6_tcp_fd = safe_close(m->llmnr_ipv6_tcp_fd);
- }
- 
-diff --git a/src/resolve/resolved-mdns.c b/src/resolve/resolved-mdns.c
-index 921720f..15c3263 100644
---- a/src/resolve/resolved-mdns.c
-+++ b/src/resolve/resolved-mdns.c
-@@ -15,10 +15,10 @@
- void manager_mdns_stop(Manager *m) {
-         assert(m);
- 
--        m->mdns_ipv4_event_source = sd_event_source_unref(m->mdns_ipv4_event_source);
-+        m->mdns_ipv4_event_source = sd_event_source_disable_unref(m->mdns_ipv4_event_source);
-         m->mdns_ipv4_fd = safe_close(m->mdns_ipv4_fd);
- 
--        m->mdns_ipv6_event_source = sd_event_source_unref(m->mdns_ipv6_event_source);
-+        m->mdns_ipv6_event_source = sd_event_source_disable_unref(m->mdns_ipv6_event_source);
-         m->mdns_ipv6_fd = safe_close(m->mdns_ipv6_fd);
- }
- 
diff --git a/debian/patches/series b/debian/patches/series
index 9da13cd..800d12f 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -146,7 +146,6 @@ lp1891215/0003-fs-util-rename-conservative_rename-conservative_rena.patch
 lp1891215/0004-fs-util-make-sure-conservative_renameat-properly-det.patch
 lp1914740-network-enable-DHCP-broadcast-flag-if-required-by-in.patch
 lp1934981-correct-suspend-then-sleep-string.patch
-lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
 CVE-2020-13529.patch
 CVE-2021-33910.patch
 lp1937117/0001-revert-lp1929560-network-move-set-MAC-and-set-nomaster-operations-out.patch