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 cba590ae6231b4f81f05716cf1868586b5497d64..5565380626fd433edf3be246b309fc4f71e1d614 100644 (file)
--- 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