Call dns_resolver_createfetch() asynchronously in zone_refreshkeys()

Because dns_resolver_createfetch() locks the view, it was necessary
to unlock the zone in zone_refreshkeys() before calling it in order
to maintain the lock order, and relock afterward. this permitted a race
with dns_zone_synckeyzone().

This commit moves the call to dns_resolver_createfetch() into a separate
function which is called asynchronously after the zone has been
unlocked.

The keyfetch object now attaches to the zone to ensure that
it won't be shut down before the asynchronous call completes.

This necessitated refactoring dns_zone_detach() so it always runs
unlocked. For managed zones it schedules zone_shutdown() to
run asynchronously; for unmanaged zones there is no task.

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 8821d1f86260f9cd0fc425e9f72bb7bce5c6c574..3b280ab495c2a2ccc693b7b20b4f72da5d118c5d 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -5756,56 +5756,43 @@ dns_zone_attach(dns_zone_t *source, dns_zone_t **target) {
 void
 dns_zone_detach(dns_zone_t **zonep) {
        REQUIRE(zonep != NULL && DNS_ZONE_VALID(*zonep));
+
        dns_zone_t *zone = *zonep;
        *zonep = NULL;
 
-       bool free_now = false;
-       dns_zone_t *raw = NULL;
-       dns_zone_t *secure = NULL;
        if (isc_refcount_decrement(&zone->erefs) == 1) {
                isc_refcount_destroy(&zone->erefs);
 
-               LOCK_ZONE(zone);
-               INSIST(zone != zone->raw);
                /*
                 * We just detached the last external reference.
                 */
                if (zone->task != NULL) {
                        /*
-                        * This zone is being managed.  Post
-                        * its control event and let it clean
-                        * up synchronously in the context of
-                        * its task.
+                        * This zone has a task; it can clean
+                        * itself up asynchronously.
                         */
                        isc_event_t *ev = &zone->ctlevent;
                        isc_task_send(zone->task, &ev);
-               } else {
-                       /*
-                        * This zone is not being managed; it has
-                        * no task and can have no outstanding
-                        * events.  Free it immediately.
-                        */
-                       /*
-                        * Unmanaged zones should not have non-null views;
-                        * we have no way of detaching from the view here
-                        * without causing deadlock because this code is called
-                        * with the view already locked.
-                        */
-                       INSIST(zone->view == NULL);
-                       free_now = true;
-                       raw = zone->raw;
-                       zone->raw = NULL;
-                       secure = zone->secure;
-                       zone->secure = NULL;
+                       return;
                }
-               UNLOCK_ZONE(zone);
-       }
-       if (free_now) {
-               if (raw != NULL) {
-                       dns_zone_detach(&raw);
+
+               /*
+                * This zone is unmanaged; we're probably running in
+                * named-checkzone or a unit test. There's no task,
+                * so we need to free it immediately.
+                *
+                * Unmanaged zones must not have null views;
+                * we have no way of detaching from the view here
+                * without causing deadlock because this code is
+                * called with the view already locked.
+                */
+               INSIST(zone->view == NULL);
+
+               if (zone->raw != NULL) {
+                       dns_zone_detach(&zone->raw);
                }
-               if (secure != NULL) {
-                       dns_zone_idetach(&secure);
+               if (zone->secure != NULL) {
+                       dns_zone_idetach(&zone->secure);
                }
                zone_free(zone);
        }
@@ -11147,7 +11134,9 @@ cleanup:
 
        isc_refcount_decrement(&zone->irefs);
 
-       kfetch->zone = NULL;
+       /* The zone must be managed */
+       INSIST(kfetch->zone->task != NULL);
+       dns_zone_detach(&kfetch->zone);
 
        if (dns_rdataset_isassociated(keydataset)) {
                dns_rdataset_disassociate(keydataset);
@@ -11168,6 +11157,7 @@ cleanup:
 
        free_needed = exit_check(zone);
        UNLOCK_ZONE(zone);
+
        if (free_needed) {
                zone_free(zone);
        }
@@ -11175,6 +11165,62 @@ cleanup:
        INSIST(ver == NULL);
 }
 
+static void
+retry_keyfetch(dns_keyfetch_t *kfetch, dns_name_t *kname) {
+       isc_time_t timenow, timethen;
+       char timebuf[80];
+       dns_zone_t *zone = kfetch->zone;
+
+       /*
+        * Error during a key fetch; cancel and retry in an hour.
+        */
+       zone->refreshkeycount--;
+       isc_refcount_decrement(&zone->irefs);
+       dns_db_detach(&kfetch->db);
+       dns_rdataset_disassociate(&kfetch->keydataset);
+       dns_name_free(kname, zone->mctx);
+       isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t));
+       dnssec_log(zone, ISC_LOG_WARNING,
+                  "Failed to create fetch for DNSKEY update");
+
+       TIME_NOW(&timenow);
+       DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen);
+
+       LOCK_ZONE(zone);
+       zone->refreshkeytime = timethen;
+       zone_settimer(zone, &timenow);
+       UNLOCK_ZONE(zone);
+
+       isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80);
+       dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", timebuf);
+
+       dns_zone_detach(&zone);
+}
+
+static void
+do_keyfetch(isc_task_t *task, isc_event_t *event) {
+       isc_result_t result;
+       dns_keyfetch_t *kfetch = (dns_keyfetch_t *)event->ev_arg;
+       dns_name_t *kname = dns_fixedname_name(&kfetch->name);
+       dns_zone_t *zone = kfetch->zone;
+
+       UNUSED(task);
+
+       isc_event_free(&event);
+
+       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);
+
+       if (result != ISC_R_SUCCESS) {
+               retry_keyfetch(kfetch, kname);
+       }
+}
+
 /*
  * Refresh the data in the key zone.  Initiate a fetch to look up
  * DNSKEY records at the trust anchor name.
@@ -11191,7 +11237,7 @@ zone_refreshkeys(dns_zone_t *zone) {
        dns_rdata_keydata_t kd;
        isc_stdtime_t now;
        bool commit = false;
-       bool fetching = false, fetch_err = false;
+       bool fetching = false;
        bool timerset = false;
 
        ENTER;
@@ -11276,9 +11322,10 @@ zone_refreshkeys(dns_zone_t *zone) {
                dns_rriterator_pause(&rrit);
 
                kfetch = isc_mem_get(zone->mctx, sizeof(dns_keyfetch_t));
+               *kfetch = (dns_keyfetch_t){ .zone = NULL };
 
                zone->refreshkeycount++;
-               kfetch->zone = zone;
+               dns_zone_attach(zone, &kfetch->zone);
                isc_refcount_increment0(&zone->irefs);
                kname = dns_fixedname_initname(&kfetch->name);
                dns_name_dup(name, zone->mctx, kname);
@@ -11312,35 +11359,22 @@ zone_refreshkeys(dns_zone_t *zone) {
 #ifdef ENABLE_AFL
                if (!dns_fuzzing_resolver) {
 #endif /* ifdef ENABLE_AFL */
-                       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);
-                       LOCK_ZONE(zone);
-#ifdef ENABLE_AFL
-               } else {
-                       result = ISC_R_FAILURE;
-               }
-#endif /* ifdef ENABLE_AFL */
-               if (result == ISC_R_SUCCESS) {
+                       isc_event_t *e = isc_event_allocate(
+                               zone->mctx, NULL, DNS_EVENT_ZONE, do_keyfetch,
+                               kfetch, sizeof(isc_event_t));
+                       isc_task_send(zone->task, &e);
                        fetching = true;
+#ifdef ENABLE_AFL
                } else {
                        zone->refreshkeycount--;
                        isc_refcount_decrement(&zone->irefs);
                        dns_db_detach(&kfetch->db);
                        dns_rdataset_disassociate(&kfetch->keydataset);
                        dns_name_free(kname, zone->mctx);
+                       dns_zone_detach(&kfetch->zone);
                        isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t));
-                       dnssec_log(zone, ISC_LOG_WARNING,
-                                  "Failed to create fetch for DNSKEY update");
-                       fetch_err = true;
                }
+#endif /* ifdef ENABLE_AFL */
        }
        if (!ISC_LIST_EMPTY(diff.tuples)) {
                CHECK(update_soa_serial(zone, db, ver, &diff, zone->mctx,
@@ -11352,22 +11386,7 @@ zone_refreshkeys(dns_zone_t *zone) {
        }
 
 failure:
-       if (fetch_err) {
-               /*
-                * Error during a key fetch; retry in an hour.
-                */
-               isc_time_t timenow, timethen;
-               char timebuf[80];
-
-               TIME_NOW(&timenow);
-               DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen);
-               zone->refreshkeytime = timethen;
-               zone_settimer(zone, &timenow);
-
-               isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80);
-               dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s",
-                          timebuf);
-       } else if (!timerset) {
+       if (!timerset) {
                isc_time_settoepoch(&zone->refreshkeytime);
        }
 

diff --git a/tests/dns/zonemgr_test.c b/tests/dns/zonemgr_test.c
index c26d3fdebd7920dc2e6d6f607795e474df6df0c0..7eeaf770efdc70d11a700e1e7987657a116c783d 100644 (file)
--- a/tests/dns/zonemgr_test.c
+++ b/tests/dns/zonemgr_test.c
@@ -113,9 +113,10 @@ ISC_RUN_TEST_IMPL(dns_zonemgr_createzone) {
        assert_int_equal(result, ISC_R_SUCCESS);
        assert_non_null(zone);
 
-       if (zone != NULL) {
-               dns_zone_detach(&zone);
-       }
+       result = dns_zonemgr_managezone(myzonemgr, zone);
+       assert_int_equal(result, ISC_R_SUCCESS);
+
+       dns_zone_detach(&zone);
 
        dns_zonemgr_shutdown(myzonemgr);
        dns_zonemgr_detach(&myzonemgr);