From 37567e0106ba5e0a26c5171104cd27800d13fa00 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 16 Dec 2019 18:13:05 +1100 Subject: [PATCH 1/4] Call dns_dbiterator_destroy earlier to prevent potential deadlock. (cherry picked from commit 9d8f9cc8f282843d6de6a00815affa51eea9af7c) --- lib/dns/zone.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 92a61f92cc..3e6c0888f6 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15414,6 +15414,8 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { dns_db_detachnode(db, &node); } + dns_dbiterator_destroy(&dbiterator); + /* * Call restore_nsec3param() to create private-type records from * the old nsec3 parameters and insert them into db From a2cf6090b2765736335a4fe6261c5ebb1ed15b4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 18 Dec 2019 10:34:29 +0100 Subject: [PATCH 2/4] Refactor receive_secure_db to make the variables and code flow around the iterator more local (cherry picked from commit 6012479419273fa615a4e20e96d678a4182d2520) --- lib/dns/zone.c | 77 +++++++++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 3e6c0888f6..f6c854183c 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15300,12 +15300,9 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { isc_result_t result; dns_zone_t *zone; dns_db_t *rawdb, *db = NULL; - dns_dbnode_t *rawnode = NULL, *node = NULL; dns_fixedname_t fname; dns_name_t *name; dns_dbiterator_t *dbiterator = NULL; - dns_rdatasetiter_t *rdsit = NULL; - dns_rdataset_t rdataset; dns_dbversion_t *version = NULL; isc_time_t loadtime; unsigned int oldserial = 0; @@ -15313,6 +15310,7 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { nsec3paramlist_t nsec3list; isc_event_t *setnsec3param_event; dns_zone_t *dummy; + dns_rdataset_t rdataset; UNUSED(task); @@ -15323,7 +15321,6 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); name = dns_fixedname_initname(&fname); - dns_rdataset_init(&rdataset); LOCK_ZONE(zone); if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING) || !inline_secure(zone)) { @@ -15369,24 +15366,34 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { if (result != ISC_R_SUCCESS) goto failure; - for (result = dns_dbiterator_first(dbiterator); - result == ISC_R_SUCCESS; - result = dns_dbiterator_next(dbiterator)) { + dns_rdataset_init(&rdataset); + + for (iter_result = dns_dbiterator_first(dbiterator); + iter_result == ISC_R_SUCCESS; + iter_result = dns_dbiterator_next(dbiterator)) + { + dns_dbnode_t *rawnode = NULL, *node = NULL; + dns_rdatasetiter_t *rdsit = NULL; + result = dns_dbiterator_current(dbiterator, &rawnode, name); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { continue; + } result = dns_db_findnode(db, name, true, &node); - if (result != ISC_R_SUCCESS) - goto failure; + if (result != ISC_R_SUCCESS) { + goto iter_cleanup; + } result = dns_db_allrdatasets(rawdb, rawnode, NULL, 0, &rdsit); - if (result != ISC_R_SUCCESS) - goto failure; + if (result != ISC_R_SUCCESS) { + goto iter_cleanup; + } - for (result = dns_rdatasetiter_first(rdsit); - result == ISC_R_SUCCESS; - result = dns_rdatasetiter_next(rdsit)) { + for (isc_result_t rdsit_result = dns_rdatasetiter_first(rdsit); + rdsit_result == ISC_R_SUCCESS; + rdsit_result = dns_rdatasetiter_next(rdsit)) + { dns_rdatasetiter_current(rdsit, &rdataset); if (rdataset.type == dns_rdatatype_nsec || rdataset.type == dns_rdatatype_rrsig || @@ -15400,22 +15407,34 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { have_oldserial) { result = checkandaddsoa(db, node, version, &rdataset, oldserial); - } else + } else { result = dns_db_addrdataset(db, node, version, 0, &rdataset, 0, NULL); - if (result != ISC_R_SUCCESS) - goto failure; - + } dns_rdataset_disassociate(&rdataset); + if (result != ISC_R_SUCCESS) { + break; + } } dns_rdatasetiter_destroy(&rdsit); - dns_db_detachnode(rawdb, &rawnode); - dns_db_detachnode(db, &node); +iter_cleanup: + if (rawnode) { + dns_db_detachnode(rawdb, &rawnode); + } + if (node) { + dns_db_detachnode(db, &node); + } + if (result != ISC_R_SUCCESS) { + break; + } } - dns_dbiterator_destroy(&dbiterator); + if (result != ISC_R_SUCCESS) { + goto failure; + } + /* * Call restore_nsec3param() to create private-type records from * the old nsec3 parameters and insert them into db @@ -15453,9 +15472,10 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { failure: UNLOCK_ZONE(zone); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { dns_zone_log(zone, ISC_LOG_ERROR, "receive_secure_db: %s", dns_result_totext(result)); + } while (!ISC_LIST_EMPTY(nsec3list)) { nsec3param_t *nsec3p; @@ -15463,20 +15483,13 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { ISC_LIST_UNLINK(nsec3list, nsec3p, link); isc_mem_put(zone->mctx, nsec3p, sizeof(nsec3param_t)); } - if (dns_rdataset_isassociated(&rdataset)) - dns_rdataset_disassociate(&rdataset); if (db != NULL) { - if (node != NULL) - dns_db_detachnode(db, &node); - if (version != NULL) + if (version != NULL) { dns_db_closeversion(db, &version, false); + } dns_db_detach(&db); } - if (rawnode != NULL) - dns_db_detachnode(rawdb, &rawnode); dns_db_detach(&rawdb); - if (dbiterator != NULL) - dns_dbiterator_destroy(&dbiterator); dns_zone_idetach(&zone); INSIST(version == NULL); From 364f232da89f8a4bc9d36dc0314bb414ff409c19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 18 Dec 2019 10:54:01 +0100 Subject: [PATCH 3/4] Add failure handling when iterators don't end with ISC_R_NOMORE (cherry picked from commit bff83b9480c9efe88dfde41f664a5bb7c1cad94c) --- lib/dns/zone.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index f6c854183c..c9dd8025c5 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15298,6 +15298,7 @@ restore_nsec3param(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *version, static void receive_secure_db(isc_task_t *task, isc_event_t *event) { isc_result_t result; + isc_result_t iter_result; dns_zone_t *zone; dns_db_t *rawdb, *db = NULL; dns_fixedname_t fname; @@ -15374,6 +15375,7 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { { dns_dbnode_t *rawnode = NULL, *node = NULL; dns_rdatasetiter_t *rdsit = NULL; + isc_result_t rdsit_result; result = dns_dbiterator_current(dbiterator, &rawnode, name); if (result != ISC_R_SUCCESS) { @@ -15390,7 +15392,7 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { goto iter_cleanup; } - for (isc_result_t rdsit_result = dns_rdatasetiter_first(rdsit); + for (rdsit_result = dns_rdatasetiter_first(rdsit); rdsit_result == ISC_R_SUCCESS; rdsit_result = dns_rdatasetiter_next(rdsit)) { @@ -15417,6 +15419,11 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { break; } } + if (rdsit_result != ISC_R_SUCCESS && + rdsit_result != ISC_R_NOMORE) + { + result = rdsit_result; + } dns_rdatasetiter_destroy(&rdsit); iter_cleanup: if (rawnode) { @@ -15429,6 +15436,10 @@ iter_cleanup: break; } } + if (iter_result != ISC_R_SUCCESS && + iter_result != ISC_R_NOMORE) { + result = iter_result; + } dns_dbiterator_destroy(&dbiterator); if (result != ISC_R_SUCCESS) { From 7f04f2f25224ede0cf8c0582442d8ab9f7a44099 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 20 Dec 2019 13:01:49 +1100 Subject: [PATCH 4/4] Refactor loop body as copy_non_dnssec_records. (cherry picked from commit d26e125438622966b60b284af5500046bf29c312) --- lib/dns/zone.c | 181 +++++++++++++++++++++++++------------------------ 1 file changed, 94 insertions(+), 87 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index c9dd8025c5..3923d7f9ab 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15295,23 +15295,89 @@ restore_nsec3param(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *version, return (result); } +static isc_result_t +copy_non_dnssec_records(dns_db_t *db, dns_db_t *version, dns_db_t *rawdb, + dns_dbiterator_t *dbiterator, unsigned int *oldserial) +{ + dns_dbnode_t *rawnode = NULL, *node = NULL; + dns_fixedname_t fixed; + dns_name_t *name = dns_fixedname_initname(&fixed); + dns_rdataset_t rdataset; + dns_rdatasetiter_t *rdsit = NULL; + isc_result_t result; + + result = dns_dbiterator_current(dbiterator, &rawnode, name); + if (result != ISC_R_SUCCESS) { + return (ISC_R_SUCCESS); + } + + result = dns_db_findnode(db, name, true, &node); + if (result != ISC_R_SUCCESS) { + goto cleanup; + } + + result = dns_db_allrdatasets(rawdb, rawnode, NULL, 0, &rdsit); + if (result != ISC_R_SUCCESS) { + goto cleanup; + } + + dns_rdataset_init(&rdataset); + + for (result = dns_rdatasetiter_first(rdsit); + result == ISC_R_SUCCESS; + result = dns_rdatasetiter_next(rdsit)) + { + dns_rdatasetiter_current(rdsit, &rdataset); + if (rdataset.type == dns_rdatatype_nsec || + rdataset.type == dns_rdatatype_rrsig || + rdataset.type == dns_rdatatype_nsec3 || + rdataset.type == dns_rdatatype_dnskey || + rdataset.type == dns_rdatatype_nsec3param) { + dns_rdataset_disassociate(&rdataset); + continue; + } + if (rdataset.type == dns_rdatatype_soa && oldserial != NULL) { + result = checkandaddsoa(db, node, version, + &rdataset, *oldserial); + } else { + result = dns_db_addrdataset(db, node, version, + 0, &rdataset, 0, + NULL); + } + dns_rdataset_disassociate(&rdataset); + if (result != ISC_R_SUCCESS) { + goto cleanup; + } + } + if (result == ISC_R_NOMORE) { + result = ISC_R_SUCCESS; + } + + cleanup: + if (rdsit != NULL) { + dns_rdatasetiter_destroy(&rdsit); + } + if (rawnode) { + dns_db_detachnode(rawdb, &rawnode); + } + if (node) { + dns_db_detachnode(db, &node); + } + return (result); +} + static void receive_secure_db(isc_task_t *task, isc_event_t *event) { isc_result_t result; - isc_result_t iter_result; dns_zone_t *zone; dns_db_t *rawdb, *db = NULL; - dns_fixedname_t fname; - dns_name_t *name; dns_dbiterator_t *dbiterator = NULL; dns_dbversion_t *version = NULL; isc_time_t loadtime; - unsigned int oldserial = 0; - bool have_oldserial = false; + unsigned int oldserial = 0, *oldserialp = NULL; nsec3paramlist_t nsec3list; isc_event_t *setnsec3param_event; dns_zone_t *dummy; - dns_rdataset_t rdataset; UNUSED(task); @@ -15321,8 +15387,6 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { rawdb = ((struct secure_event *)event)->db; isc_event_free(&event); - name = dns_fixedname_initname(&fname); - LOCK_ZONE(zone); if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING) || !inline_secure(zone)) { result = ISC_R_SHUTTINGDOWN; @@ -15333,8 +15397,9 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read); if (zone->db != NULL) { result = dns_db_getsoaserial(zone->db, NULL, &oldserial); - if (result == ISC_R_SUCCESS) - have_oldserial = true; + if (result == ISC_R_SUCCESS) { + oldserialp = &oldserial; + } /* * assemble nsec3parameters from the old zone, and set a flag @@ -15351,8 +15416,9 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { result = dns_db_create(zone->mctx, zone->db_argv[0], &zone->origin, dns_dbtype_zone, zone->rdclass, zone->db_argc - 1, zone->db_argv + 1, &db); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { goto failure; + } result = dns_db_setgluecachestats(db, zone->gluecachestats); if (result != ISC_R_SUCCESS && result != ISC_R_NOTIMPLEMENTED) { @@ -15360,89 +15426,27 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) { } result = dns_db_newversion(db, &version); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { goto failure; + } result = dns_db_createiterator(rawdb, 0, &dbiterator); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { goto failure; - - dns_rdataset_init(&rdataset); - - for (iter_result = dns_dbiterator_first(dbiterator); - iter_result == ISC_R_SUCCESS; - iter_result = dns_dbiterator_next(dbiterator)) - { - dns_dbnode_t *rawnode = NULL, *node = NULL; - dns_rdatasetiter_t *rdsit = NULL; - isc_result_t rdsit_result; - - result = dns_dbiterator_current(dbiterator, &rawnode, name); - if (result != ISC_R_SUCCESS) { - continue; - } - - result = dns_db_findnode(db, name, true, &node); - if (result != ISC_R_SUCCESS) { - goto iter_cleanup; - } - - result = dns_db_allrdatasets(rawdb, rawnode, NULL, 0, &rdsit); - if (result != ISC_R_SUCCESS) { - goto iter_cleanup; - } - - for (rdsit_result = dns_rdatasetiter_first(rdsit); - rdsit_result == ISC_R_SUCCESS; - rdsit_result = dns_rdatasetiter_next(rdsit)) - { - dns_rdatasetiter_current(rdsit, &rdataset); - if (rdataset.type == dns_rdatatype_nsec || - rdataset.type == dns_rdatatype_rrsig || - rdataset.type == dns_rdatatype_nsec3 || - rdataset.type == dns_rdatatype_dnskey || - rdataset.type == dns_rdatatype_nsec3param) { - dns_rdataset_disassociate(&rdataset); - continue; - } - if (rdataset.type == dns_rdatatype_soa && - have_oldserial) { - result = checkandaddsoa(db, node, version, - &rdataset, oldserial); - } else { - result = dns_db_addrdataset(db, node, version, - 0, &rdataset, 0, - NULL); - } - dns_rdataset_disassociate(&rdataset); - if (result != ISC_R_SUCCESS) { - break; - } - } - if (rdsit_result != ISC_R_SUCCESS && - rdsit_result != ISC_R_NOMORE) - { - result = rdsit_result; - } - dns_rdatasetiter_destroy(&rdsit); -iter_cleanup: - if (rawnode) { - dns_db_detachnode(rawdb, &rawnode); - } - if (node) { - dns_db_detachnode(db, &node); - } - if (result != ISC_R_SUCCESS) { - break; - } } - if (iter_result != ISC_R_SUCCESS && - iter_result != ISC_R_NOMORE) { - result = iter_result; + + for (result = dns_dbiterator_first(dbiterator); + result == ISC_R_SUCCESS; + result = dns_dbiterator_next(dbiterator)) + { + result = copy_non_dnssec_records(db, version, rawdb, + dbiterator, oldserialp); + if (result != ISC_R_SUCCESS) { + goto failure; + } } dns_dbiterator_destroy(&dbiterator); - - if (result != ISC_R_SUCCESS) { + if (result != ISC_R_NOMORE) { goto failure; } @@ -15483,6 +15487,9 @@ iter_cleanup: failure: UNLOCK_ZONE(zone); + if (dbiterator != NULL) { + dns_dbiterator_destroy(&dbiterator); + } if (result != ISC_R_SUCCESS) { dns_zone_log(zone, ISC_LOG_ERROR, "receive_secure_db: %s", dns_result_totext(result));