From 9d8f9cc8f282843d6de6a00815affa51eea9af7c 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. --- lib/dns/zone.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index d9e95f957b..6fa659a34b 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15623,6 +15623,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 6012479419273fa615a4e20e96d678a4182d2520 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 --- 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 6fa659a34b..7bd85fba45 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15503,12 +15503,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; @@ -15516,6 +15513,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); @@ -15526,7 +15524,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)) { @@ -15572,24 +15569,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 || @@ -15609,22 +15616,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 @@ -15662,9 +15681,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; @@ -15672,20 +15692,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 bff83b9480c9efe88dfde41f664a5bb7c1cad94c 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 --- 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 7bd85fba45..f8b93d406a 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15501,6 +15501,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; @@ -15577,6 +15578,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) { @@ -15593,7 +15595,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)) { @@ -15626,6 +15628,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) { @@ -15638,6 +15645,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 d26e125438622966b60b284af5500046bf29c312 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. --- lib/dns/zone.c | 194 +++++++++++++++++++++++++------------------------ 1 file changed, 101 insertions(+), 93 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index f8b93d406a..aeb58365f6 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -15498,23 +15498,96 @@ 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_zone_t *zone, 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) { + /* + * Allow DNSSEC records with dnssec-policy. + * WMM: Perhaps add config option for it. + */ + if (dns_zone_getkasp(zone) == NULL) { + 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); @@ -15524,8 +15597,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; @@ -15536,8 +15607,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 @@ -15554,8 +15626,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) { @@ -15563,95 +15636,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) { - /* - * Allow DNSSEC records with dnssec-policy. - * WMM: Perhaps add config option for it. - */ - if (dns_zone_getkasp(zone) == NULL) { - 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(zone, 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; } @@ -15692,6 +15697,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));