Resolve violation of weak referencing dns_view

The dns_view implements weak and strong reference counting.  When strong
reference counting reaches zero, the adb, ntatable and resolver objects
are shut down and detached.

In dns_zone and dns_nta the dns_view was weakly attached, but the
view->resolver reference was accessed directly leading to dereferencing
the NULL pointer.

Add dns_view_getresolver() method which attaches to view->resolver
object under the lock (if it still exists) ensuring the dns_resolver
will be kept referenced until not needed.

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index eff09b660e2022f0cf322fbac975d015b2a98a3d..dad397bb98aa7a3528e6326cbd404f4a10f0054a 100644 (file)
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -1299,4 +1299,7 @@ dns_view_sfd_find(dns_view_t *view, const dns_name_t *name,
  *\li  'foundname' to be valid with a buffer sufficient to hold the name.
  */
 
+isc_result_t
+dns_view_getresolver(dns_view_t *view, dns_resolver_t **resolverp);
+
 ISC_LANG_ENDDECLS

diff --git a/lib/dns/nta.c b/lib/dns/nta.c
index 1d141ea27cb0979f590678cd4f151269a920f151..44714cfa718254d81d895e0ccfd701a77a5ff837 100644 (file)
--- a/lib/dns/nta.c
+++ b/lib/dns/nta.c
@@ -44,6 +44,7 @@
 struct dns_ntatable {
        /* Unlocked. */
        unsigned int magic;
+       isc_mem_t *mctx;
        dns_view_t *view;
        isc_rwlock_t rwlock;
        isc_loopmgr_t *loopmgr;
@@ -52,7 +53,7 @@ struct dns_ntatable {
        isc_refcount_t references;
        /* Locked by rwlock. */
        dns_rbt_t *table;
-       bool shuttingdown;
+       atomic_bool shuttingdown;
 };
 
 struct dns__nta {
@@ -118,16 +119,18 @@ dns_ntatable_create(dns_view_t *view, isc_taskmgr_t *taskmgr,
        ntatable = isc_mem_get(view->mctx, sizeof(*ntatable));
        *ntatable = (dns_ntatable_t){
                .loopmgr = loopmgr,
-               .view = view,
        };
 
+       isc_mem_attach(view->mctx, &ntatable->mctx);
+       dns_view_weakattach(view, &ntatable->view);
+
        result = isc_task_create(taskmgr, &ntatable->task, 0);
        if (result != ISC_R_SUCCESS) {
                goto cleanup_ntatable;
        }
        isc_task_setname(ntatable->task, "ntatable", ntatable);
 
-       result = dns_rbt_create(view->mctx, dns__nta_free, NULL,
+       result = dns_rbt_create(ntatable->mctx, dns__nta_free, NULL,
                                &ntatable->table);
        if (result != ISC_R_SUCCESS) {
                goto cleanup_task;
@@ -146,7 +149,7 @@ cleanup_task:
        isc_task_detach(&ntatable->task);
 
 cleanup_ntatable:
-       isc_mem_put(view->mctx, ntatable, sizeof(*ntatable));
+       isc_mem_putanddetach(&ntatable->mctx, ntatable, sizeof(*ntatable));
 
        return (result);
 }
@@ -158,7 +161,8 @@ dns__ntatable_destroy(dns_ntatable_t *ntatable) {
        dns_rbt_destroy(&ntatable->table);
        isc_rwlock_destroy(&ntatable->rwlock);
        isc_task_detach(&ntatable->task);
-       isc_mem_put(ntatable->view->mctx, ntatable, sizeof(*ntatable));
+       INSIST(ntatable->view == NULL);
+       isc_mem_putanddetach(&ntatable->mctx, ntatable, sizeof(*ntatable));
 }
 
 ISC_REFCOUNT_IMPL(dns_ntatable, dns__ntatable_destroy);
@@ -222,14 +226,13 @@ fetch_done(isc_task_t *task, isc_event_t *event) {
        RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_read);
 
        dns__nta_detach(&nta); /* for dns_resolver_createfetch() */
-       dns_view_weakdetach(&view);
 }
 
 static void
 checkbogus(void *arg) {
        dns__nta_t *nta = arg;
        dns_ntatable_t *ntatable = nta->ntatable;
-       dns_view_t *view = NULL;
+       dns_resolver_t *resolver = NULL;
        isc_result_t result;
 
        if (nta->fetch != NULL) {
@@ -243,17 +246,25 @@ checkbogus(void *arg) {
                dns_rdataset_disassociate(&nta->sigrdataset);
        }
 
+       if (atomic_load(&ntatable->shuttingdown)) {
+               isc_timer_stop(nta->timer);
+               return;
+       }
+
+       result = dns_view_getresolver(ntatable->view, &resolver);
+       if (result != ISC_R_SUCCESS) {
+               return;
+       }
+
        dns__nta_ref(nta); /* for dns_resolver_createfetch */
-       dns_view_weakattach(ntatable->view, &view);
        result = dns_resolver_createfetch(
-               view->resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL,
-               NULL, 0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task,
-               fetch_done, nta, &nta->rdataset, &nta->sigrdataset,
-               &nta->fetch);
+               resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL, NULL,
+               0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task, fetch_done, nta,
+               &nta->rdataset, &nta->sigrdataset, &nta->fetch);
        if (result != ISC_R_SUCCESS) {
                dns__nta_detach(&nta); /* for dns_resolver_createfetch() */
-               dns_view_weakdetach(&view);
        }
+       dns_resolver_detach(&resolver);
 }
 
 static void
@@ -278,21 +289,18 @@ static void
 nta_create(dns_ntatable_t *ntatable, const dns_name_t *name,
           dns__nta_t **target) {
        dns__nta_t *nta = NULL;
-       dns_view_t *view;
 
        REQUIRE(VALID_NTATABLE(ntatable));
        REQUIRE(target != NULL && *target == NULL);
 
-       view = ntatable->view;
-
-       nta = isc_mem_get(view->mctx, sizeof(dns__nta_t));
+       nta = isc_mem_get(ntatable->mctx, sizeof(dns__nta_t));
        *nta = (dns__nta_t){
                .ntatable = ntatable,
                .loop = isc_loop_current(ntatable->loopmgr),
                .magic = NTA_MAGIC,
        };
 
-       isc_mem_attach(view->mctx, &nta->mctx);
+       isc_mem_attach(ntatable->mctx, &nta->mctx);
 
        dns_rdataset_init(&nta->rdataset);
        dns_rdataset_init(&nta->sigrdataset);
@@ -314,12 +322,12 @@ dns_ntatable_add(dns_ntatable_t *ntatable, const dns_name_t *name, bool force,
 
        REQUIRE(VALID_NTATABLE(ntatable));
 
-       RWLOCK(&ntatable->rwlock, isc_rwlocktype_write);
-
-       if (ntatable->shuttingdown) {
-               goto unlock;
+       if (atomic_load(&ntatable->shuttingdown)) {
+               return (ISC_R_SUCCESS);
        }
 
+       RWLOCK(&ntatable->rwlock, isc_rwlocktype_write);
+
        nta_create(ntatable, name, &nta);
 
        nta->expiry = now + lifetime;
@@ -350,7 +358,6 @@ dns_ntatable_add(dns_ntatable_t *ntatable, const dns_name_t *name, bool force,
                break;
        }
 
-unlock:
        RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write);
 
        return (result);
@@ -683,5 +690,7 @@ dns_ntatable_shutdown(dns_ntatable_t *ntatable) {
        }
 
        dns_rbtnodechain_invalidate(&chain);
+
+       dns_view_weakdetach(&ntatable->view);
        RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write);
 }

diff --git a/lib/dns/view.c b/lib/dns/view.c
index b7e3fe83b39d01df996cdbb732d11181f510308d..14123866c6f80deea7776cd0450a0c0cde37691e 100644 (file)
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -507,23 +507,46 @@ dns_view_detach(dns_view_t **viewp) {
        if (isc_refcount_decrement(&view->references) == 1) {
                dns_zone_t *mkzone = NULL, *rdzone = NULL;
                dns_zt_t *zt = NULL;
+               dns_resolver_t *resolver = NULL;
+               dns_adb_t *adb = NULL;
+               dns_requestmgr_t *requestmgr = NULL;
 
                isc_refcount_destroy(&view->references);
 
+               /* Swap the pointers under the lock */
+               LOCK(&view->lock);
                if (view->resolver != NULL) {
-                       dns_resolver_shutdown(view->resolver);
-                       dns_resolver_detach(&view->resolver);
+                       resolver = view->resolver;
+                       view->resolver = NULL;
+                       UNLOCK(&view->lock);
+
+                       dns_resolver_shutdown(resolver);
+                       dns_resolver_detach(&resolver);
+
+                       LOCK(&view->lock);
                }
+
                if (view->adb != NULL) {
-                       dns_adb_shutdown(view->adb);
-                       dns_adb_detach(&view->adb);
+                       adb = view->adb;
+                       view->adb = NULL;
+                       UNLOCK(&view->lock);
+
+                       dns_adb_shutdown(adb);
+                       dns_adb_detach(&adb);
+
+                       LOCK(&view->lock);
                }
+
                if (view->requestmgr != NULL) {
-                       dns_requestmgr_shutdown(view->requestmgr);
-                       dns_requestmgr_detach(&view->requestmgr);
-               }
+                       requestmgr = view->requestmgr;
+                       view->requestmgr = NULL;
+                       UNLOCK(&view->lock);
 
-               LOCK(&view->lock);
+                       dns_requestmgr_shutdown(requestmgr);
+                       dns_requestmgr_detach(&requestmgr);
+
+                       LOCK(&view->lock);
+               }
 
                if (view->zonetable != NULL) {
                        zt = view->zonetable;
@@ -2396,3 +2419,19 @@ dns_view_sfd_find(dns_view_t *view, const dns_name_t *name,
                dns_name_copy(dns_rootname, foundname);
        }
 }
+
+isc_result_t
+dns_view_getresolver(dns_view_t *view, dns_resolver_t **resolverp) {
+       isc_result_t result;
+       REQUIRE(DNS_VIEW_VALID(view));
+       REQUIRE(resolverp != NULL && *resolverp == NULL);
+       LOCK(&view->lock);
+       if (view->resolver != NULL) {
+               dns_resolver_attach(view->resolver, resolverp);
+               result = ISC_R_SUCCESS;
+       } else {
+               result = ISC_R_SHUTTINGDOWN;
+       }
+       UNLOCK(&view->lock);
+       return (result);
+}

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 8cd9765895b361710485b03c24b9b3150a8d5bf2..037bca8dce33183f6f8400adccf88548aa3804e9 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -11298,16 +11298,27 @@ zone_refreshkeys(dns_zone_t *zone) {
 #ifdef ENABLE_AFL
                if (!dns_fuzzing_resolver) {
 #endif /* ifdef ENABLE_AFL */
+                       /*
+                        * We need to unlock and lock zone here because view
+                        * gets locked in the dns_resolver_createfetch() via
+                        * dns_view_findzonecut() and this violates the locking
+                        * hierarchy (view -> zone).
+                        */
                        UNLOCK_ZONE(zone);
-                       result = dns_resolver_createfetch(
-                               zone->view->resolver, kname,
-                               dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0,
-                               DNS_FETCHOPT_NOVALIDATE |
-                                       DNS_FETCHOPT_UNSHARED |
-                                       DNS_FETCHOPT_NOCACHED,
-                               0, NULL, zone->task, keyfetch_done, kfetch,
-                               &kfetch->dnskeyset, &kfetch->dnskeysigset,
-                               &kfetch->fetch);
+                       dns_resolver_t *resolver = NULL;
+                       result = dns_view_getresolver(zone->view, &resolver);
+                       if (result == ISC_R_SUCCESS) {
+                               result = dns_resolver_createfetch(
+                                       resolver, kname, dns_rdatatype_dnskey,
+                                       NULL, NULL, NULL, NULL, 0,
+                                       DNS_FETCHOPT_NOVALIDATE |
+                                               DNS_FETCHOPT_UNSHARED |
+                                               DNS_FETCHOPT_NOCACHED,
+                                       0, NULL, zone->task, keyfetch_done,
+                                       kfetch, &kfetch->dnskeyset,
+                                       &kfetch->dnskeysigset, &kfetch->fetch);
+                               dns_resolver_detach(&resolver);
+                       }
                        LOCK_ZONE(zone);
 #ifdef ENABLE_AFL
                } else {
@@ -14649,6 +14660,7 @@ again:
                result = dns_peerlist_peerbyaddr(zone->view->peers, &primaryip,
                                                 &peer);
                if (result == ISC_R_SUCCESS) {
+                       dns_resolver_t *resolver = NULL;
                        result = dns_peer_getsupportedns(peer, &edns);
                        if (result == ISC_R_SUCCESS && !edns) {
                                DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NOEDNS);
@@ -14662,9 +14674,10 @@ again:
                        if (dscp != -1) {
                                have_xfrdscp = true;
                        }
-                       if (zone->view->resolver != NULL) {
-                               udpsize = dns_resolver_getudpsize(
-                                       zone->view->resolver);
+                       result = dns_view_getresolver(zone->view, &resolver);
+                       if (result == ISC_R_SUCCESS) {
+                               udpsize = dns_resolver_getudpsize(resolver);
+                               dns_resolver_detach(&resolver);
                        }
                        (void)dns_peer_getudpsize(peer, &udpsize);
                        (void)dns_peer_getrequestnsid(peer, &reqnsid);
@@ -14948,6 +14961,7 @@ ns_query(dns_zone_t *zone, dns_rdataset_t *soardataset, dns_stub_t *stub) {
                result = dns_peerlist_peerbyaddr(zone->view->peers, &primaryip,
                                                 &peer);
                if (result == ISC_R_SUCCESS) {
+                       dns_resolver_t *resolver;
                        result = dns_peer_getsupportedns(peer, &edns);
                        if (result == ISC_R_SUCCESS && !edns) {
                                DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NOEDNS);
@@ -14961,9 +14975,10 @@ ns_query(dns_zone_t *zone, dns_rdataset_t *soardataset, dns_stub_t *stub) {
                        if (result == ISC_R_SUCCESS && dscp != -1) {
                                have_xfrdscp = true;
                        }
-                       if (zone->view->resolver != NULL) {
-                               udpsize = dns_resolver_getudpsize(
-                                       zone->view->resolver);
+                       result = dns_view_getresolver(zone->view, &resolver);
+                       if (result == ISC_R_SUCCESS) {
+                               udpsize = dns_resolver_getudpsize(resolver);
+                               dns_resolver_detach(&resolver);
                        }
                        (void)dns_peer_getudpsize(peer, &udpsize);
                        (void)dns_peer_getrequestnsid(peer, &reqnsid);

diff --git a/tests/dns/keytable_test.c b/tests/dns/keytable_test.c
index 1ac1eb042ecee8d39f5eb8bf6deb7f2862ac162c..656bb118bb9f14d2512701403da86b5a3c71882e 100644 (file)
--- a/tests/dns/keytable_test.c
+++ b/tests/dns/keytable_test.c
@@ -211,6 +211,7 @@ create_tables(void) {
 static void
 destroy_tables(void) {
        if (ntatable != NULL) {
+               dns_ntatable_shutdown(ntatable);
                dns_ntatable_detach(&ntatable);
        }
        if (keytable != NULL) {