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

[Christian Ehrhardt ]

[Christian Ehrhardt  (paelzer)]

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)]

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

[Andreas Hasenack (ahasenack)]

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

[Andreas Hasenack (ahasenack)]

+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)]

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)]

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

[Christian Ehrhardt  (paelzer)]

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

[Andreas Hasenack (ahasenack)]

+1 for including the fix for bug 1797926

[Christian Ehrhardt  (paelzer)]

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)]

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)]

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

[Andreas Hasenack (ahasenack)]

+1

review: Approve

[Christian Ehrhardt  (paelzer)]

tagged and uploaded

diff --git a/debian/changelog b/debian/changelog
index 051027b..3e72fba 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+bind9 (1:9.10.3.dfsg.P4-8ubuntu1.15) xenial; urgency=medium
+
+  * d/p/ubuntu//lp-1833400*: fix race on shutdown (LP: #1833400)
+  * d/p/fix-shutdown-race.diff: dig/host/nslookup could crash when interrupted
+    close to a query timeout (LP: #1797926)
+
+ -- Christian Ehrhardt <[e-mail]>  Mon, 05 Aug 2019 07:30:49 +0200
+
 bind9 (1:9.10.3.dfsg.P4-8ubuntu1.14) xenial-security; urgency=medium
 
   * SECURITY UPDATE: limiting simultaneous TCP clients is ineffective
diff --git a/debian/patches/fix-shutdown-race.diff b/debian/patches/fix-shutdown-race.diff
 new file mode 100644
index 0000000..ae72e03
--- /dev/null
+++ b/debian/patches/fix-shutdown-race.diff
@@ -0,0 +1,41 @@
+From f2ca287330110993609fa0443d3bdb17629bd979 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <[e-mail]>
+Date: Tue, 13 Nov 2018 13:50:47 +0100
+Subject: [PATCH 1/2] Fix a shutdown race in bin/dig/dighost.c
+
+If a tool using the routines defined in bin/dig/dighost.c is sent an
+interruption signal around the time a connection timeout is scheduled to
+fire, connect_timeout() may be executed after destroy_libs() detaches
+from the global task (setting 'global_task' to NULL), which results in a
+crash upon a UDP retry due to bringup_timer() attempting to create a
+timer with 'task' set to NULL.  Fix by preventing connect_timeout() from
+attempting a retry when shutdown is in progress.
+
+(cherry picked from commit 462175659674a10c0d39c7c328f1a5324ce2e38b)
+
+Origin: backport, https://gitlab.isc.org/isc-projects/bind9/merge_requests/1040/diffs
+Bug: https://gitlab.isc.org/isc-projects/bind9/issues/599
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bind9/+bug/1797926
+Last-Update: 2018-12-06
+
+---
+ bin/dig/dighost.c | 5 +++++
+ 1 file changed, 5 insertions(+)
+diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c
+index 39abb9d0fd..17e0328228 100644
+--- a/bin/dig/dighost.c
++++ b/bin/dig/dighost.c
+@@ -3240,6 +3240,11 @@ connect_timeout(isc_task_t *task, isc_event_t *event) {
+ 
+ 	INSIST(!free_now);
+ 
++	if (cancel_now) {
++		UNLOCK_LOOKUP;
++		return;
++	}
++
+ 	if ((query != NULL) && (query->lookup->current_query != NULL) &&
+ 	    (ISC_LIST_NEXT(query->lookup->current_query, link) != NULL)) {
+ 		debug("trying next server...");
+---
+2.18.1
diff --git a/debian/patches/series b/debian/patches/series
index bdcff1c..ef34d45 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -31,3 +31,6 @@ CVE-2018-5745.patch
 CVE-2019-6465.patch
 CVE-2018-5743.patch
 CVE-2018-5743-atomic-fix.patch
+ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch
+ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch
+fix-shutdown-race.diff
diff --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
 new file mode 100644
index 0000000..a2f22b8
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1833400-master-Remove-REQUIRE-preventing-change-4592-from-wo.patch
@@ -0,0 +1,33 @@
+From a94d68ce432b9e11c4ae91d48ee257b1675f86d7 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <[e-mail]>
+Date: Tue, 24 Oct 2017 21:02:05 +0200
+Subject: [PATCH] [master] Remove REQUIRE preventing change 4592 from working
+
+Change 4592 was supposed to replace a REQUIRE with a conditional return.
+While the latter was added, the former was not removed.  Remove the
+relevant REQUIRE to fix RT #43822 for good.
+
+Origin: upstream, https://gitlab.isc.org/isc-projects/bind9/commit/dadca599daec203a24dd6beee0590c038f9301c3
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1833400
+Bug: https://bugs.isc.org/Public/Bug/Display.html?id=43822
+Last-Update: 2019-08-07
+
+---
+ lib/dns/dispatch.c | 1 -
+ 1 file changed, 1 deletion(-)
+
+diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
+index 4f668a102d..607a730395 100644
+--- a/lib/dns/dispatch.c
++++ b/lib/dns/dispatch.c
+@@ -3718,7 +3718,6 @@ dns_dispatch_importrecv(dns_dispatch_t *disp, isc_event_t *event) {
+ 	isc_socketevent_t *sevent, *newsevent;
+ 
+ 	REQUIRE(VALID_DISPATCH(disp));
+-	REQUIRE((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) != 0);
+ 	REQUIRE(event != NULL);
+ 
+ 	if ((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) == 0)
+-- 
+2.22.0
+
diff --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
 new file mode 100644
index 0000000..c009ca7
--- /dev/null
+++ b/debian/patches/ubuntu/lp-1833400-master-fix-dispatch.c-shutdown-race.patch
@@ -0,0 +1,47 @@
+From 019132b70c368bc9abca0034d07b324bb7cb6eb2 Mon Sep 17 00:00:00 2001
+From: Evan Hunt <[e-mail]>
+Date: Thu, 20 Apr 2017 17:41:37 -0700
+Subject: [PATCH] [master] fix dispatch.c shutdown race
+
+4952.	[bug]		A race condition on shutdown could trigger an
+			assertion failure in dispatch.c. [RT #43822]
+
+Origin: backport, https://gitlab.isc.org/isc-projects/bind9/commit/70590c691b410303ac44f879e0a3b2b58711a3de
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1833400
+Bug: https://bugs.isc.org/Public/Bug/Display.html?id=43822
+Last-Update: 2019-08-07
+
+---
+ CHANGES                        | 3 +++
+ lib/dns/dispatch.c             | 5 ++++-
+ lib/dns/include/dns/dispatch.h | 3 +++
+ 3 files changed, 10 insertions(+), 1 deletion(-)
+
+--- a/lib/dns/dispatch.c
++++ b/lib/dns/dispatch.c
+@@ -3691,9 +3691,12 @@ dns_dispatch_importrecv(dns_dispatch_t *
+ 	REQUIRE((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) != 0);
+ 	REQUIRE(event != NULL);
+ 
+-	sevent = (isc_socketevent_t *)event;
++	if ((disp->attributes & DNS_DISPATCHATTR_NOLISTEN) == 0)
++		return;
+ 
++	sevent = (isc_socketevent_t *)event;
+ 	INSIST(sevent->n <= disp->mgr->buffersize);
++
+ 	newsevent = (isc_socketevent_t *)
+ 		    isc_event_allocate(disp->mgr->mctx, NULL,
+ 				      DNS_EVENT_IMPORTRECVDONE, udp_shrecv,
+--- a/lib/dns/include/dns/dispatch.h
++++ b/lib/dns/include/dns/dispatch.h
+@@ -524,6 +524,9 @@ dns_dispatch_importrecv(dns_dispatch_t *
+  * shared between dispatchers and clients.  If the dispatcher fails to copy
+  * or send the event, nothing happens.
+  *
++ * If the attribute DNS_DISPATCHATTR_NOLISTEN is not set, then
++ * the dispatch is already handling a recv; return immediately.
++ *
+  * Requires:
+  *\li 	disp is valid, and the attribute DNS_DISPATCHATTR_NOLISTEN is set.
+  * 	event != NULL