Merge ~paelzer/ubuntu/+source/bind9:bug-1833400-race-on-shutdown into ubuntu/+source/bind9:ubuntu/xenial-devel

Proposed by Christian Ehrhardt  on 2019-08-05
Status: Merged
Approved by: Christian Ehrhardt  on 2019-08-09
Approved revision: 3e06372c267368be1b9d74add302d2925367c62c
Merged at revision: 3e06372c267368be1b9d74add302d2925367c62c
Proposed branch: ~paelzer/ubuntu/+source/bind9:bug-1833400-race-on-shutdown
Merge into: ubuntu/+source/bind9:ubuntu/xenial-devel
Diff against target: 166 lines (+132/-0)
5 files modified
debian/changelog (+8/-0)
debian/patches/fix-shutdown-race.diff (+41/-0)
debian/patches/series (+3/-0)
debian/patches/ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch (+33/-0)
debian/patches/ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch (+47/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack 2019-08-05 Approve on 2019-08-07
Canonical Server Team 2019-08-05 Pending
Ubuntu Server Dev import team 2019-08-05 Pending
Review via email: mp+370942@code.launchpad.net
To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

PPA: https://launchpad.net/~paelzer/+archive/ubuntu/bug-1833400-bind-crash

We still miss the repro steps since the upstream bug is inaccessible and the reporters didn't mention yet. So far this is therefore a low prio review, mostly meant that the changes are readily available if anyone wants to continue on this.

Christian Ehrhardt  (paelzer) wrote :

Never the less it can be reviewed for silly errors on my side, so why not opening it up for review ...

Andreas Hasenack (ahasenack) wrote :

I pinged upstream in the commits asking for the report to be made public

Andreas Hasenack (ahasenack) wrote :

+1

I found their own backports of these patches in their v9_10 branch, if you rather have those:

https://gitlab.isc.org/isc-projects/bind9/commit/dadca599daec203a24dd6beee0590c038f9301c3

and

https://gitlab.isc.org/isc-projects/bind9/commit/70590c691b410303ac44f879e0a3b2b58711a3de

I assume you didn't want to point at the upstream bug in the DEP3 headers because it's not public?

review: Approve
Christian Ehrhardt  (paelzer) wrote :

Thank you Andreas,
as you know they made the bug public now (thanks to your ping) so I can reference it now.
I changed the references to the 9_10 backport commits and added the upstream bug references.
Code-wise things stay the same on these compared to what I had (the backport I needed was for the "Changes" File not matching).

Christian Ehrhardt  (paelzer) wrote :

I'll upload this to unapproved after I added a SRU template

Christian Ehrhardt  (paelzer) wrote :

Actually I'm holding back until we talked about adding a potential backport for bug 1797926 as well.

Andreas Hasenack (ahasenack) wrote :

+1 for including the fix for bug 1797926

Christian Ehrhardt  (paelzer) wrote :

Added the fix for bug 1797926 here.
Also pushed the version with it to the PPA.

I'll open a new MP for the same towards Bionic.

Andreas Hasenack (ahasenack) wrote :

The Origin url's hash doesn't match the From one in the DEP3 headers anymore in d/p/ubuntu/*, if that's a concern.

Christian Ehrhardt  (paelzer) wrote :

No concern on the hash mismatch, but yes to marking as backport

Andreas Hasenack (ahasenack) wrote :

+1

review: Approve
Christian Ehrhardt  (paelzer) wrote :

tagged and uploaded

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 051027b..3e72fba 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,11 @@
6+bind9 (1:9.10.3.dfsg.P4-8ubuntu1.15) xenial; urgency=medium
7+
8+ * d/p/ubuntu//lp-1833400*: fix race on shutdown (LP: #1833400)
9+ * d/p/fix-shutdown-race.diff: dig/host/nslookup could crash when interrupted
10+ close to a query timeout (LP: #1797926)
11+
12+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Mon, 05 Aug 2019 07:30:49 +0200
13+
14 bind9 (1:9.10.3.dfsg.P4-8ubuntu1.14) xenial-security; urgency=medium
15
16 * SECURITY UPDATE: limiting simultaneous TCP clients is ineffective
17diff --git a/debian/patches/fix-shutdown-race.diff b/debian/patches/fix-shutdown-race.diff
18new file mode 100644
19index 0000000..ae72e03
20--- /dev/null
21+++ b/debian/patches/fix-shutdown-race.diff
22@@ -0,0 +1,41 @@
23+From f2ca287330110993609fa0443d3bdb17629bd979 Mon Sep 17 00:00:00 2001
24+From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
25+Date: Tue, 13 Nov 2018 13:50:47 +0100
26+Subject: [PATCH 1/2] Fix a shutdown race in bin/dig/dighost.c
27+
28+If a tool using the routines defined in bin/dig/dighost.c is sent an
29+interruption signal around the time a connection timeout is scheduled to
30+fire, connect_timeout() may be executed after destroy_libs() detaches
31+from the global task (setting 'global_task' to NULL), which results in a
32+crash upon a UDP retry due to bringup_timer() attempting to create a
33+timer with 'task' set to NULL. Fix by preventing connect_timeout() from
34+attempting a retry when shutdown is in progress.
35+
36+(cherry picked from commit 462175659674a10c0d39c7c328f1a5324ce2e38b)
37+
38+Origin: backport, https://gitlab.isc.org/isc-projects/bind9/merge_requests/1040/diffs
39+Bug: https://gitlab.isc.org/isc-projects/bind9/issues/599
40+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bind9/+bug/1797926
41+Last-Update: 2018-12-06
42+
43+---
44+ bin/dig/dighost.c | 5 +++++
45+ 1 file changed, 5 insertions(+)
46+diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c
47+index 39abb9d0fd..17e0328228 100644
48+--- a/bin/dig/dighost.c
49++++ b/bin/dig/dighost.c
50+@@ -3240,6 +3240,11 @@ connect_timeout(isc_task_t *task, isc_event_t *event) {
51+
52+ INSIST(!free_now);
53+
54++ if (cancel_now) {
55++ UNLOCK_LOOKUP;
56++ return;
57++ }
58++
59+ if ((query != NULL) && (query->lookup->current_query != NULL) &&
60+ (ISC_LIST_NEXT(query->lookup->current_query, link) != NULL)) {
61+ debug("trying next server...");
62+---
63+2.18.1
64diff --git a/debian/patches/series b/debian/patches/series
65index bdcff1c..ef34d45 100644
66--- a/debian/patches/series
67+++ b/debian/patches/series
68@@ -31,3 +31,6 @@ CVE-2018-5745.patch
69 CVE-2019-6465.patch
70 CVE-2018-5743.patch
71 CVE-2018-5743-atomic-fix.patch
72+ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch
73+ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch
74+fix-shutdown-race.diff
75diff --git a/debian/patches/ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch b/debian/patches/ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch
76new file mode 100644
77index 0000000..a2f22b8
78--- /dev/null
79+++ b/debian/patches/ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch
80@@ -0,0 +1,33 @@
81+From a94d68ce432b9e11c4ae91d48ee257b1675f86d7 Mon Sep 17 00:00:00 2001
82+From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
83+Date: Tue, 24 Oct 2017 21:02:05 +0200
84+Subject: [PATCH] [master] Remove REQUIRE preventing change 4592 from working
85+
86+Change 4592 was supposed to replace a REQUIRE with a conditional return.
87+While the latter was added, the former was not removed. Remove the
88+relevant REQUIRE to fix RT #43822 for good.
89+
90+Origin: upstream, https://gitlab.isc.org/isc-projects/bind9/commit/dadca599daec203a24dd6beee0590c038f9301c3
91+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1833400
92+Bug: https://bugs.isc.org/Public/Bug/Display.html?id=43822
93+Last-Update: 2019-08-07
94+
95+---
96+ lib/dns/dispatch.c | 1 -
97+ 1 file changed, 1 deletion(-)
98+
99+diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
100+index 4f668a102d..607a730395 100644
101+--- a/lib/dns/dispatch.c
102++++ b/lib/dns/dispatch.c
103+@@ -3718,7 +3718,6 @@ dns_dispatch_importrecv(dns_dispatch_t *disp, isc_event_t *event) {
104+ isc_socketevent_t *sevent, *newsevent;
105+
106+ REQUIRE(VALID_DISPATCH(disp));
107+- REQUIRE((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) != 0);
108+ REQUIRE(event != NULL);
109+
110+ if ((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) == 0)
111+--
112+2.22.0
113+
114diff --git a/debian/patches/ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch b/debian/patches/ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch
115new file mode 100644
116index 0000000..c009ca7
117--- /dev/null
118+++ b/debian/patches/ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch
119@@ -0,0 +1,47 @@
120+From 019132b70c368bc9abca0034d07b324bb7cb6eb2 Mon Sep 17 00:00:00 2001
121+From: Evan Hunt <each@isc.org>
122+Date: Thu, 20 Apr 2017 17:41:37 -0700
123+Subject: [PATCH] [master] fix dispatch.c shutdown race
124+
125+4952. [bug] A race condition on shutdown could trigger an
126+ assertion failure in dispatch.c. [RT #43822]
127+
128+Origin: backport, https://gitlab.isc.org/isc-projects/bind9/commit/70590c691b410303ac44f879e0a3b2b58711a3de
129+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1833400
130+Bug: https://bugs.isc.org/Public/Bug/Display.html?id=43822
131+Last-Update: 2019-08-07
132+
133+---
134+ CHANGES | 3 +++
135+ lib/dns/dispatch.c | 5 ++++-
136+ lib/dns/include/dns/dispatch.h | 3 +++
137+ 3 files changed, 10 insertions(+), 1 deletion(-)
138+
139+--- a/lib/dns/dispatch.c
140++++ b/lib/dns/dispatch.c
141+@@ -3691,9 +3691,12 @@ dns_dispatch_importrecv(dns_dispatch_t *
142+ REQUIRE((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) != 0);
143+ REQUIRE(event != NULL);
144+
145+- sevent = (isc_socketevent_t *)event;
146++ if ((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) == 0)
147++ return;
148+
149++ sevent = (isc_socketevent_t *)event;
150+ INSIST(sevent->n <= disp->mgr->buffersize);
151++
152+ newsevent = (isc_socketevent_t *)
153+ isc_event_allocate(disp->mgr->mctx, NULL,
154+ DNS_EVENT_IMPORTRECVDONE, udp_shrecv,
155+--- a/lib/dns/include/dns/dispatch.h
156++++ b/lib/dns/include/dns/dispatch.h
157+@@ -524,6 +524,9 @@ dns_dispatch_importrecv(dns_dispatch_t *
158+ * shared between dispatchers and clients. If the dispatcher fails to copy
159+ * or send the event, nothing happens.
160+ *
161++ * If the attribute DNS_DISPATCHATTR_NOLISTEN is not set, then
162++ * the dispatch is already handling a recv; return immediately.
163++ *
164+ * Requires:
165+ *\li disp is valid, and the attribute DNS_DISPATCHATTR_NOLISTEN is set.
166+ * event != NULL

Subscribers

People subscribed via source and target branches