From 80e66fbd2d8a6dc581387116288f5d5c5cbcb0f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 3 Nov 2022 12:08:35 +0100 Subject: [PATCH] Don't use dns_zone_attach() in zone_refreshkeys() The zone_refreshkeys() could run before the zone_shutdown(), but after the last .erefs has been "detached" causing assertion failure when doing dns_zone_attach(). Remove the use of .erefs (dns_zone_attach/detach) and replace it with using the .irefs and additional checks whether the zone is exiting in the callbacks. --- lib/dns/zone.c | 191 +++++++++++++++++++++++-------------------------- 1 file changed, 90 insertions(+), 101 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index cf2fc489c7..fe7cb5bed3 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -812,6 +812,7 @@ struct dns_keymgmt { */ struct dns_keyfetch { + isc_mem_t *mctx; dns_fixedname_t name; dns_rdataset_t keydataset; dns_rdataset_t dnskeyset; @@ -5797,8 +5798,11 @@ dns_zone_detach(dns_zone_t **zonep) { isc_refcount_destroy(&zone->erefs); /* - * We just detached the last external reference. + * Stop things being restarted after we cancel them below. */ + DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_EXITING); + dns_zone_log(zone, ISC_LOG_INFO, "final reference detached"); + if (zone->loop != NULL) { /* * This zone has a loop; it can clean @@ -5810,24 +5814,18 @@ dns_zone_detach(dns_zone_t **zonep) { /* * This zone is unmanaged; we're probably running in - * named-checkzone or a unit test. There's no loop, so we need - * to free it immediately. + * named-checkzone or a unit test. There's no loop, 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. + * 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(isc_tid() == ISC_TID_UNKNOWN); INSIST(zone->view == NULL); - if (zone->raw != NULL) { - dns_zone_detach(&zone->raw); - } - if (zone->secure != NULL) { - dns_zone_idetach(&zone->secure); - } - zone_free(zone); + zone_shutdown(zone); } } @@ -5872,7 +5870,6 @@ zone_idetach(dns_zone_t **zonep) { void dns_zone_idetach(dns_zone_t **zonep) { dns_zone_t *zone; - bool free_needed; REQUIRE(zonep != NULL && DNS_ZONE_VALID(*zonep)); @@ -5880,6 +5877,7 @@ dns_zone_idetach(dns_zone_t **zonep) { *zonep = NULL; if (isc_refcount_decrement(&zone->irefs) == 1) { + bool free_needed; LOCK_ZONE(zone); free_needed = exit_check(zone); UNLOCK_ZONE(zone); @@ -10552,7 +10550,7 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) { kfetch = event->ev_arg; zone = kfetch->zone; - isc_mem_attach(zone->mctx, &mctx); + mctx = kfetch->mctx; keyname = dns_fixedname_name(&kfetch->name); dnskeys = &kfetch->dnskeyset; dnskeysigs = &kfetch->dnskeysigset; @@ -11145,11 +11143,9 @@ failure: cleanup: dns_db_detach(&kfetch->db); - isc_refcount_decrement(&zone->irefs); - /* The zone must be managed */ INSIST(kfetch->zone->loop != NULL); - dns_zone_detach(&kfetch->zone); + isc_refcount_decrement(&zone->irefs); if (dns_rdataset_isassociated(keydataset)) { dns_rdataset_disassociate(keydataset); @@ -11162,7 +11158,7 @@ cleanup: } dns_name_free(keyname, mctx); - isc_mem_putanddetach(&mctx, kfetch, sizeof(dns_keyfetch_t)); + isc_mem_putanddetach(&kfetch->mctx, kfetch, sizeof(dns_keyfetch_t)); if (secroots != NULL) { dns_keytable_detach(&secroots); @@ -11181,30 +11177,42 @@ cleanup: 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; + bool free_needed; /* * Error during a key fetch; cancel and retry in an hour. */ + LOCK_ZONE(zone); 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)); + isc_mem_putanddetach(&kfetch->mctx, kfetch, sizeof(*kfetch)); 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); - zone->refreshkeytime = timethen; - zone_settimer(zone, &timenow); + if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) { + /* Don't really retry if we are exiting */ + char timebuf[80]; - isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80); - dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", timebuf); + TIME_NOW(&timenow); + DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen); + zone->refreshkeytime = timethen; + zone_settimer(zone, &timenow); - dns_zone_detach(&zone); + isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80); + dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", + timebuf); + } + + free_needed = exit_check(zone); + UNLOCK_ZONE(zone); + + if (free_needed) { + zone_free(zone); + } } static void @@ -11217,11 +11225,24 @@ do_keyfetch(void *arg) { unsigned int options = DNS_FETCHOPT_NOVALIDATE | DNS_FETCHOPT_UNSHARED | DNS_FETCHOPT_NOCACHED; + if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) { + goto retry; + } + result = dns_view_getresolver(zone->view, &resolver); if (result != ISC_R_SUCCESS) { goto retry; } + /* + * Use of DNS_FETCHOPT_NOCACHED is essential here. If it is not + * set and the cache still holds a non-expired, validated version + * of the RRset being queried for by the time the response is + * received, the cached RRset will be passed to keyfetch_done() + * instead of the one received in the response as the latter will + * have a lower trust level due to not being validated until + * keyfetch_done() is called. + */ result = dns_resolver_createfetch( resolver, kname, dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0, options, 0, NULL, zone->task, keyfetch_done, kfetch, @@ -11284,7 +11305,6 @@ zone_refreshkeys(dns_zone_t *zone) { isc_stdtime_t timer = 0xffffffff; dns_name_t *name = NULL, *kname = NULL; dns_rdataset_t *kdset = NULL; - dns_keyfetch_t *kfetch; uint32_t ttl; dns_rriterator_current(&rrit, &name, &ttl, &kdset, NULL); @@ -11336,55 +11356,39 @@ 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++; - dns_zone_attach(zone, &kfetch->zone); - isc_refcount_increment0(&zone->irefs); - kname = dns_fixedname_initname(&kfetch->name); - dns_name_dup(name, zone->mctx, kname); - dns_rdataset_init(&kfetch->dnskeyset); - dns_rdataset_init(&kfetch->dnskeysigset); - dns_rdataset_init(&kfetch->keydataset); - dns_rdataset_clone(kdset, &kfetch->keydataset); - kfetch->db = NULL; - dns_db_attach(db, &kfetch->db); - kfetch->fetch = NULL; - - if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) { - char namebuf[DNS_NAME_FORMATSIZE]; - dns_name_format(kname, namebuf, sizeof(namebuf)); - dnssec_log(zone, ISC_LOG_DEBUG(3), - "Creating key fetch in " - "zone_refreshkeys() for '%s'", - namebuf); - } - - /* - * Use of DNS_FETCHOPT_NOCACHED is essential here. If it is - * not set and the cache still holds a non-expired, validated - * version of the RRset being queried for by the time the - * response is received, the cached RRset will be passed to - * keyfetch_done() instead of the one received in the response - * as the latter will have a lower trust level due to not being - * validated until keyfetch_done() is called. - */ - #ifdef ENABLE_AFL if (!dns_fuzzing_resolver) { #endif /* ifdef ENABLE_AFL */ + dns_keyfetch_t *kfetch = NULL; + + kfetch = isc_mem_get(zone->mctx, + sizeof(dns_keyfetch_t)); + *kfetch = (dns_keyfetch_t){ .zone = zone }; + isc_mem_attach(zone->mctx, &kfetch->mctx); + + zone->refreshkeycount++; + isc_refcount_increment0(&zone->irefs); + kname = dns_fixedname_initname(&kfetch->name); + dns_name_dup(name, zone->mctx, kname); + dns_rdataset_init(&kfetch->dnskeyset); + dns_rdataset_init(&kfetch->dnskeysigset); + dns_rdataset_init(&kfetch->keydataset); + dns_rdataset_clone(kdset, &kfetch->keydataset); + dns_db_attach(db, &kfetch->db); + + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) { + char namebuf[DNS_NAME_FORMATSIZE]; + dns_name_format(kname, namebuf, + sizeof(namebuf)); + dnssec_log(zone, ISC_LOG_DEBUG(3), + "Creating key fetch in " + "zone_refreshkeys() for '%s'", + namebuf); + } + isc_async_run(zone->loop, do_keyfetch, kfetch); 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)); } #endif /* ifdef ENABLE_AFL */ } @@ -11422,34 +11426,24 @@ static void zone_maintenance(dns_zone_t *zone) { isc_time_t now; isc_result_t result; - bool dumping, load_pending, viewok, notify, refreshkeys; - bool sign, resign, rekey, chain, warn_expire; + bool load_pending, exiting, dumping, viewok, notify; + bool refreshkeys, sign, resign, rekey, chain, warn_expire; REQUIRE(DNS_ZONE_VALID(zone)); ENTER; /* - * Are we pending load/reload? + * Are we pending load/reload, exiting, or unconfigured + * (e.g. because of a syntax failure in the config file)? + * If so, don't attempt maintenance. */ LOCK_ZONE(zone); load_pending = DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADPENDING); - UNLOCK_ZONE(zone); - - if (load_pending) { - return; - } - - /* - * Configuring the view of this zone may have - * failed, for example because the config file - * had a syntax error. In that case, the view - * adb or resolver will be NULL, and we had better not try - * to do further maintenance on it. - */ - LOCK_ZONE(zone); + exiting = DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING); viewok = (zone->view != NULL && zone->view->adb != NULL); UNLOCK_ZONE(zone); - if (!viewok) { + + if (load_pending || exiting || !viewok) { return; } @@ -15117,8 +15111,7 @@ unlock: } /* - * Handle the control event. Note that although this event causes the zone - * to shut down, it is not a shutdown event in the sense of the task library. + * Shut the zone down. */ static void zone_shutdown(void *arg) { @@ -15131,13 +15124,6 @@ zone_shutdown(void *arg) { zone_debuglog(zone, __func__, 3, "shutting down"); - /* - * Stop things being restarted after we cancel them below. - */ - LOCK_ZONE(zone); - DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_EXITING); - UNLOCK_ZONE(zone); - /* * If we were waiting for xfrin quota, step out of * the queue. @@ -15285,7 +15271,6 @@ zone_timer_set(dns_zone_t *zone, isc_time_t *next, isc_time_t *now) { static void zone__settimer(void *arg) { zone_settimer_t *data = arg; - dns_zone_t *zone = data->zone; isc_time_t *now = &data->now; isc_time_t next; @@ -15443,6 +15428,10 @@ free: static void zone_settimer(dns_zone_t *zone, isc_time_t *now) { + if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) { + return; + } + zone_settimer_t *arg = isc_mem_get(zone->mctx, sizeof(*arg)); *arg = (zone_settimer_t){ .zone = zone,