Merge ~slyon/ubuntu/+source/systemd:slyon/resolved-segfault-root-cause into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-hirsute

[Lukas Märdian]

resolved: cherry-pick upstream commits to fix segfault (LP: #1934221)

Replace d/p/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch with upstream commit from systemd v247-stable branch (d/p/resolved-disable-event-sources-before-unreffing-them.patch) and add d/p/resolved-fix-braino-with-reference-counting-and-linked-li.patch

The additional patch is a fixup of an already included (in systemd-stable v247.3) patch "resolved: use reference counting for DnsQueryCandidate objects" and should help with avoiding the query related segfaults.
There haven't been any other relevant commits to resolved-dns-query.c in v247-stable nor upstream/master.

I have not been able to reproduce this problem locally as it happens only sporadically. Applying this fixup and observing if this crash does still happen in Hirsute should be a good measure of success, before backporting the changes to Focal:
https://errors.ubuntu.com/problem/c4e5be3f1c7af9483993c7e6007b9325ab5b78cd

[Lukas Märdian (slyon)]

It passes the autopkgtest: https://autopkgtest.ubuntu.com/results/autopkgtest-hirsute-slyon-testing/hirsute/s390x/s/systemd/20210920_131017_85656@/log.gz

[Lukas Märdian (slyon)]

I'm also working with the original bug reporter, trying to get confirmation about this issue being fixed in a PPA: https://launchpad.net/~slyon/+archive/ubuntu/lp1934221/+packages

https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934221

Unmerged commits

70bbdb1... by Lukas Märdian on 2021-09-20

update changelog

38b83d3... by Lukas Märdian on 2021-09-20

resolved: cherry-pick upstream commits to fix segfault (LP: #1934221)

diff --git a/debian/changelog b/debian/changelog
index b003153..ad663b6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,14 @@
+systemd (247.3-3ubuntu3.7) UNRELEASED; urgency=medium
+
+  * resolved: cherry-pick upstream commits to fix segfault (LP: #1934221)
+    Files:
+    - debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
+    - debian/patches/resolved-disable-event-sources-before-unreffing-them.patch
+    - debian/patches/resolved-fix-braino-with-reference-counting-and-linked-li.patch
+    https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=38b83d38337046fca83fbf3fef1c02309295ab14
+
+ -- Lukas Märdian <[e-mail]>  Mon, 20 Sep 2021 13:13:11 +0200
+
 systemd (247.3-3ubuntu3.6) hirsute; 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/resolved-disable-event-sources-before-unreffing-them.patch
similarity index 85%
rename from debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
rename to debian/patches/resolved-disable-event-sources-before-unreffing-them.patch
index c916d9a..f77ff1c 100644
--- a/debian/patches/lp1934221-resolved-disable-event-sources-before-unreffing-them.patch
+++ b/debian/patches/resolved-disable-event-sources-before-unreffing-them.patch
@@ -1,8 +1,6 @@
-From: Lukas Märdian <[e-mail]>
-Date: Tue, 20 Jul 2021 14:43:31 +0200
+From: =?utf-8?q?Zbigniew_J=C4=99drzejewski-Szmek?= <[e-mail]>
+Date: Mon, 1 Mar 2021 23:10:06 +0100
 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
 
 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.
@@ -19,15 +17,17 @@ are non-floating, so there is no reference from the sd_event object to the
 sources.
 
 For #18427.
+
+(cherry picked from commit 97935302283729c9206b84f5e00b1aff0f78ad19)
 ---
- 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        | 8 ++++----
- src/resolve/resolved-dns-transaction.c | 4 ++--
- src/resolve/resolved-llmnr.c           | 8 ++++----
- src/resolve/resolved-mdns.c            | 4 ++--
- 7 files changed, 19 insertions(+), 19 deletions(-)
+ 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        | 10 +++++-----
+ src/resolve/resolved-dns-transaction.c |  5 +++--
+ src/resolve/resolved-llmnr.c           |  8 ++++----
+ src/resolve/resolved-mdns.c            |  4 ++--
+ 7 files changed, 21 insertions(+), 20 deletions(-)
 
 diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c
 index 8ee4fd8..f6ff3b5 100644
@@ -92,7 +92,7 @@ index 1aab089..1471628 100644
  
          /* 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 6a3dc99..63a57cb 100644
+index 6a3dc99..5043c60 100644
 --- a/src/resolve/resolved-dns-stub.c
 +++ b/src/resolve/resolved-dns-stub.c
 @@ -79,8 +79,8 @@ DnsStubListenerExtra *dns_stub_listener_extra_free(DnsStubListenerExtra *p) {
@@ -106,7 +106,7 @@ index 6a3dc99..63a57cb 100644
  
          return mfree(p);
  }
-@@ -763,8 +763,8 @@ int manager_dns_stub_start(Manager *m) {
+@@ -763,12 +763,12 @@ int manager_dns_stub_start(Manager *m) {
  void manager_dns_stub_stop(Manager *m) {
          assert(m);
  
@@ -117,16 +117,22 @@ index 6a3dc99..63a57cb 100644
  }
  
  static const char* const dns_stub_listener_mode_table[_DNS_STUB_LISTENER_MODE_MAX] = {
+-        [DNS_STUB_LISTENER_NO] = "no",
++        [DNS_STUB_LISTENER_NO]  = "no",
+         [DNS_STUB_LISTENER_UDP] = "udp",
+         [DNS_STUB_LISTENER_TCP] = "tcp",
+         [DNS_STUB_LISTENER_YES] = "yes",
 diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
-index 303233e..2b8e763 100644
+index 303233e..a754283 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) {
+@@ -59,14 +59,15 @@ 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);
  }
  
diff --git a/debian/patches/resolved-fix-braino-with-reference-counting-and-linked-li.patch b/debian/patches/resolved-fix-braino-with-reference-counting-and-linked-li.patch
new file mode 100644
index 0000000..75b9833
--- /dev/null
+++ b/debian/patches/resolved-fix-braino-with-reference-counting-and-linked-li.patch
@@ -0,0 +1,204 @@
+From: =?utf-8?q?Zbigniew_J=C4=99drzejewski-Szmek?= <[e-mail]>
+Date: Fri, 14 May 2021 10:49:24 +0200
+Subject: resolved: fix braino with reference counting and linked lists
+
+In 0e0fd08fc832b8f42e567d722d388eba086da5ff I added reference counts to keep
+track of the DnsQueryCandidate objects. Unfortunately, dns_query_unref_candidates()
+was written as
+
+     while (q->candidates)
+           dns_query_candidate_unref(q->candidates);
+
+i.e. it would keep dropping the reference count as many times as needed for it
+to hit 0, making the patch less than fully effective.
+
+dns_query_unref_candidates() is renamed to dns_query_detach_candidates() and
+changed to drop exactly one reference from each of the linked candidates.
+
+Example failure:
+==463== Invalid read of size 8
+==463==    at 0x419C93: dns_query_candidate_go (resolved-dns-query.c:159)
+==463==    by 0x41A143: dns_query_candidate_notify (resolved-dns-query.c:304)
+==463==    by 0x434BD6: dns_transaction_complete (resolved-dns-transaction.c:437)
+==463==    by 0x436A0F: dns_transaction_process_dnssec (resolved-dns-transaction.c:976)
+==463==    by 0x4378C1: dns_transaction_process_reply (resolved-dns-transaction.c:1387)
+==463==    by 0x437CE9: on_dns_packet (resolved-dns-transaction.c:1444)
+==463==    by 0x4B2DC9B: source_dispatch (sd-event.c:3512)
+==463==    by 0x4B2FB1F: sd_event_dispatch (sd-event.c:4077)
+==463==    by 0x4B2FFFA: sd_event_run (sd-event.c:4138)
+==463==    by 0x4B301D6: sd_event_loop (sd-event.c:4159)
+==463==    by 0x464A24: run (resolved.c:92)
+==463==    by 0x464B3C: main (resolved.c:99)
+==463==  Address 0x5f409d0 is 32 bytes inside a block of size 72 free'd
+==463==    at 0x48410E4: free (vg_replace_malloc.c:755)
+==463==    by 0x418EDF: mfree (alloc-util.h:48)
+==463==    by 0x4197E8: dns_query_candidate_free (resolved-dns-query.c:67)
+==463==    by 0x4198B7: dns_query_candidate_unref (resolved-dns-query.c:70)
+==463==    by 0x41A2E3: dns_query_unref_candidates (resolved-dns-query.c:337)
+==463==    by 0x41C5FE: dns_query_cname_redirect (resolved-dns-query.c:1028)
+==463==    by 0x41CA04: dns_query_process_cname_one (resolved-dns-query.c:1128)
+==463==    by 0x41CA80: dns_query_process_cname_many (resolved-dns-query.c:1157)
+==463==    by 0x40C0BD: bus_method_resolve_hostname_complete (resolved-bus.c:198)
+==463==    by 0x41B312: dns_query_complete (resolved-dns-query.c:562)
+==463==    by 0x41C1AC: dns_query_accept (resolved-dns-query.c:922)
+==463==    by 0x41C2C4: dns_query_ready (resolved-dns-query.c:955)
+==463==    by 0x41A162: dns_query_candidate_notify (resolved-dns-query.c:314)
+==463==    by 0x434BD6: dns_transaction_complete (resolved-dns-transaction.c:437)
+==463==    by 0x438995: dns_transaction_prepare (resolved-dns-transaction.c:1728)
+==463==    by 0x43921D: dns_transaction_go (resolved-dns-transaction.c:1928)
+==463==    by 0x419C7C: dns_query_candidate_go (resolved-dns-query.c:163)
+==463==    by 0x41A143: dns_query_candidate_notify (resolved-dns-query.c:304)
+==463==    by 0x434BD6: dns_transaction_complete (resolved-dns-transaction.c:437)
+==463==    by 0x436A0F: dns_transaction_process_dnssec (resolved-dns-transaction.c:976)
+==463==    by 0x4378C1: dns_transaction_process_reply (resolved-dns-transaction.c:1387)
+==463==    by 0x437CE9: on_dns_packet (resolved-dns-transaction.c:1444)
+==463==    by 0x4B2DC9B: source_dispatch (sd-event.c:3512)
+==463==    by 0x4B2FB1F: sd_event_dispatch (sd-event.c:4077)
+==463==    by 0x4B2FFFA: sd_event_run (sd-event.c:4138)
+==463==    by 0x4B301D6: sd_event_loop (sd-event.c:4159)
+==463==    by 0x464A24: run (resolved.c:92)
+==463==    by 0x464B3C: main (resolved.c:99)
+==463==  Block was alloc'd at
+==463==    at 0x483E86F: malloc (vg_replace_malloc.c:380)
+==463==    by 0x418F81: malloc_multiply (alloc-util.h:96)
+==463==    by 0x419378: dns_query_candidate_new (resolved-dns-query.c:23)
+==463==    by 0x41B42C: dns_query_add_candidate (resolved-dns-query.c:582)
+==463==    by 0x41BB7A: dns_query_go (resolved-dns-query.c:762)
+==463==    by 0x40CE3A: bus_method_resolve_hostname (resolved-bus.c:464)
+==463==    by 0x4A84B86: method_callbacks_run (bus-objects.c:414)
+==463==    by 0x4A87961: object_find_and_run (bus-objects.c:1323)
+==463==    by 0x4A87FEE: bus_process_object (bus-objects.c:1443)
+==463==    by 0x4AA3434: process_message (sd-bus.c:2964)
+==463==    by 0x4AA3623: process_running (sd-bus.c:3006)
+==463==    by 0x4AA4110: bus_process_internal (sd-bus.c:3226)
+==463==    by 0x4AA41EF: sd_bus_process (sd-bus.c:3253)
+==463==    by 0x4AA5343: io_callback (sd-bus.c:3604)
+==463==    by 0x4B2DC9B: source_dispatch (sd-event.c:3512)
+==463==    by 0x4B2FB1F: sd_event_dispatch (sd-event.c:4077)
+==463==    by 0x4B2FFFA: sd_event_run (sd-event.c:4138)
+==463==    by 0x4B301D6: sd_event_loop (sd-event.c:4159)
+==463==    by 0x464A24: run (resolved.c:92)
+==463==    by 0x464B3C: main (resolved.c:99)
+
+Fixes #19376.
+
+(cherry picked from commit c856ef0457c35e9edfdbf085b69ec81c126d48e5)
+(cherry picked from commit 89324e233eef767334d9bfe5eed96956c973c2ad)
+---
+ src/resolve/resolved-dns-query.c | 42 ++++++++++++++++++++++++++++++----------
+ 1 file changed, 32 insertions(+), 10 deletions(-)
+
+diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c
+index f6ff3b5..6f6f30d 100644
+--- a/src/resolve/resolved-dns-query.c
++++ b/src/resolve/resolved-dns-query.c
+@@ -43,6 +43,8 @@ static void dns_query_candidate_stop(DnsQueryCandidate *c) {
+ 
+         assert(c);
+ 
++        /* Detach all the DnsTransactions attached to this query */
++
+         while ((t = set_steal_first(c->transactions))) {
+                 set_remove(t->notify_query_candidates, c);
+                 set_remove(t->notify_query_candidates_done, c);
+@@ -50,21 +52,34 @@ static void dns_query_candidate_stop(DnsQueryCandidate *c) {
+         }
+ }
+ 
++static DnsQueryCandidate* dns_query_candidate_unlink(DnsQueryCandidate *c) {
++        assert(c);
++
++        /* Detach this DnsQueryCandidate from the Query and Scope objects */
++
++        if (c->query) {
++                LIST_REMOVE(candidates_by_query, c->query->candidates, c);
++                c->query = NULL;
++        }
++
++        if (c->scope) {
++                LIST_REMOVE(candidates_by_scope, c->scope->query_candidates, c);
++                c->scope = NULL;
++        }
++
++        return c;
++}
++
+ static DnsQueryCandidate* dns_query_candidate_free(DnsQueryCandidate *c) {
+         if (!c)
+                 return NULL;
+ 
+         dns_query_candidate_stop(c);
++        dns_query_candidate_unlink(c);
+ 
+         set_free(c->transactions);
+         dns_search_domain_unref(c->search_domain);
+ 
+-        if (c->query)
+-                LIST_REMOVE(candidates_by_query, c->query->candidates, c);
+-
+-        if (c->scope)
+-                LIST_REMOVE(candidates_by_scope, c->scope->query_candidates, c);
+-
+         return mfree(c);
+ }
+ 
+@@ -103,6 +118,7 @@ static int dns_query_candidate_add_transaction(DnsQueryCandidate *c, DnsResource
+ 
+         assert(c);
+         assert(key);
++        assert(c->query); /* We shan't add transactions to a candidate that has been detached already */
+ 
+         t = dns_scope_find_transaction(c->scope, key, true);
+         if (!t) {
+@@ -211,6 +227,7 @@ static int dns_query_candidate_setup_transactions(DnsQueryCandidate *c) {
+         int n = 0, r;
+ 
+         assert(c);
++        assert(c->query); /* We shan't add transactions to a candidate that has been detached already */
+ 
+         dns_query_candidate_stop(c);
+ 
+@@ -253,6 +270,9 @@ void dns_query_candidate_notify(DnsQueryCandidate *c) {
+ 
+         assert(c);
+ 
++        if (!c->query) /* This candidate has been abandoned, do nothing. */
++                return;
++
+         state = dns_query_candidate_state(c);
+ 
+         if (DNS_TRANSACTION_IS_LIVE(state))
+@@ -303,11 +323,13 @@ static void dns_query_stop(DnsQuery *q) {
+                 dns_query_candidate_stop(c);
+ }
+ 
+-static void dns_query_unref_candidates(DnsQuery *q) {
++static void dns_query_unlink_candidates(DnsQuery *q) {
+         assert(q);
+ 
+         while (q->candidates)
+-                dns_query_candidate_unref(q->candidates);
++                /* Here we drop *our* references to each of the candidates. If we had the only reference, the
++                 * DnsQueryCandidate object will be freed. */
++                dns_query_candidate_unref(dns_query_candidate_unlink(q->candidates));
+ }
+ 
+ static void dns_query_reset_answer(DnsQuery *q) {
+@@ -336,7 +358,7 @@ DnsQuery *dns_query_free(DnsQuery *q) {
+                 LIST_REMOVE(auxiliary_queries, q->auxiliary_for->auxiliary_queries, q);
+         }
+ 
+-        dns_query_unref_candidates(q);
++        dns_query_unlink_candidates(q);
+ 
+         dns_question_unref(q->question_idna);
+         dns_question_unref(q->question_utf8);
+@@ -932,7 +954,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname)
+         dns_question_unref(q->question_utf8);
+         q->question_utf8 = TAKE_PTR(nq_utf8);
+ 
+-        dns_query_unref_candidates(q);
++        dns_query_unlink_candidates(q);
+         dns_query_reset_answer(q);
+ 
+         q->state = DNS_TRANSACTION_NULL;
diff --git a/debian/patches/series b/debian/patches/series
index f680ae6..8ae5b7b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -96,8 +96,9 @@ 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
 lp1934147/0001-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch
 lp1934147/0002-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch
+resolved-disable-event-sources-before-unreffing-them.patch
+resolved-fix-braino-with-reference-counting-and-linked-li.patch