From: Aram Sargsyan <aram@isc.org>
Date: Mon, 5 Jun 2023 14:35:42 +0000 (+0000)
Subject: Fix a lock-order-inversion bug in resolver.c
X-Git-Tag: v9.18.16~8^2
X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=db45cab546042376685e20273c9a398f8be1aae1;p=thirdparty%2Fbind9.git

Fix a lock-order-inversion bug in resolver.c

There is a lock-order-inversion (potential deadlock) in resolver.c,
because in dns_resolver_shutdown() a resolver bucket lock is locked
while the resolver lock itself is already locked, while in
fctx_sendevents() the resolver lock is locked while a bucket lock
is locked before calling that function in fctx__done_detach().

The resolver lock/unlock in dns_resolver_shutdown() was added back in
the 317e36d47e1c5bc60783716a678255b60379590e commit to make sure that
the function is finished before the resolver object is destroyed.

Since res->exiting is atomic, it should be possible to remove the
resolver locking in dns_resolver_shutdown() and add it to the
send_shutdown_events() function which requires it.

Also, since 'res->exiting' is now set while unlocked, the 'INSIST'
in spillattimer_countdown() is wrong, and is removed.
---

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index cba590ae623..5565380626f 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -4441,9 +4441,7 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) {
 	if (bucket_empty && exiting &&
 	    isc_refcount_decrement(&res->activebuckets) == 1)
 	{
-		LOCK(&res->lock);
 		send_shutdown_events(res);
-		UNLOCK(&res->lock);
 	}
 
 	isc_refcount_destroy(&fctx->references);
@@ -10260,10 +10258,7 @@ send_shutdown_events(dns_resolver_t *res) {
 	isc_event_t *event, *next_event;
 	isc_task_t *etask;
 
-	/*
-	 * Caller must be holding the resolver lock.
-	 */
-
+	LOCK(&res->lock);
 	for (event = ISC_LIST_HEAD(res->whenshutdown); event != NULL;
 	     event = next_event)
 	{
@@ -10273,6 +10268,7 @@ send_shutdown_events(dns_resolver_t *res) {
 		event->ev_sender = res;
 		isc_task_sendanddetach(&etask, &event);
 	}
+	UNLOCK(&res->lock);
 }
 
 static void
@@ -10287,7 +10283,6 @@ spillattimer_countdown(isc_task_t *task, isc_event_t *event) {
 	UNUSED(task);
 
 	LOCK(&res->lock);
-	INSIST(!atomic_load_acquire(&res->exiting));
 	if (res->spillat > res->spillatmin) {
 		res->spillat--;
 		logit = true;
@@ -10644,7 +10639,6 @@ dns_resolver_shutdown(dns_resolver_t *res) {
 
 	RTRACE("shutdown");
 
-	LOCK(&res->lock);
 	if (atomic_compare_exchange_strong(&res->exiting, &is_false, true)) {
 		RTRACE("exiting");
 
@@ -10673,7 +10667,6 @@ dns_resolver_shutdown(dns_resolver_t *res) {
 					 true);
 		RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	}
-	UNLOCK(&res->lock);
 }
 
 void