From f13e71e55167bf9c94f4faf1dab110467158e7b4 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 30 Nov 2022 18:40:27 +1100 Subject: [PATCH 1/6] Suppress duplicate dns_db_updatenotify_register registrations Duplicate dns_db_updatenotify_register registrations need to be suppressed to ensure that dns_db_updatenotify_unregister is successful. --- lib/dns/catz.c | 5 ++--- lib/dns/db.c | 12 +++++++++++- lib/dns/include/dns/db.h | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 2b8b64b5e2..bd740b1685 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -982,9 +982,8 @@ dns__catz_zone_destroy(dns_catz_zone_t *zone) { } if (zone->db_registered) { - INSIST(dns_db_updatenotify_unregister( - zone->db, dns_catz_dbupdate_callback, - zone->catzs) == ISC_R_SUCCESS); + dns_db_updatenotify_unregister( + zone->db, dns_catz_dbupdate_callback, zone->catzs); } if (zone->dbversion) { dns_db_closeversion(zone->db, &zone->dbversion, false); diff --git a/lib/dns/db.c b/lib/dns/db.c index 5ea9532816..9620efb165 100644 --- a/lib/dns/db.c +++ b/lib/dns/db.c @@ -995,7 +995,7 @@ dns_db_rpz_ready(dns_db_t *db) { return ((db->methods->rpz_ready)(db)); } -/** +/* * Attach a notify-on-update function the database */ isc_result_t @@ -1006,6 +1006,16 @@ dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn, REQUIRE(db != NULL); REQUIRE(fn != NULL); + for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL; + listener = ISC_LIST_NEXT(listener, link)) + { + if ((listener->onupdate == fn) && + (listener->onupdate_arg == fn_arg)) + { + return (ISC_R_SUCCESS); + } + } + listener = isc_mem_get(db->mctx, sizeof(dns_dbonupdatelistener_t)); listener->onupdate = fn; diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h index 1831473e51..f1ca17973b 100644 --- a/lib/dns/include/dns/db.h +++ b/lib/dns/include/dns/db.h @@ -1634,11 +1634,11 @@ dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn, void *fn_arg); /*%< * Register a notify-on-update callback function to a database. + * Duplicate callbacks are suppressed. * * Requires: * * \li 'db' is a valid database - * \li 'db' does not have an update callback registered * \li 'fn' is not NULL * */ From 35839e91d84f4c22f3554ff4b6dc53d20359621e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 30 Nov 2022 18:44:37 +1100 Subject: [PATCH 2/6] Call dns_db_updatenotify_unregister earlier dns_db_updatenotify_unregister needed to be called earlier to ensure that listener->onupdate_arg always points to a valid object. The existing lazy cleanup in rbtdb_free did not ensure that. --- lib/dns/include/dns/zone.h | 3 +- lib/dns/rbtdb.c | 10 +------ lib/dns/zone.c | 60 ++++++++++++++++++++++---------------- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 8cf4cc3aaa..a029c6e43f 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -2621,7 +2621,8 @@ dns_zone_catz_enable(dns_zone_t *zone, dns_catz_zones_t *catzs); void dns_zone_catz_disable(dns_zone_t *zone); /*%< - * Disable zone as catalog zone, if it is one. + * Disable zone as catalog zone, if it is one. Also disables any + * registered callbacks for the catalog zone. * * Requires: * diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index cf8170e874..15732ca87b 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1112,7 +1112,6 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { char buf[DNS_NAME_FORMATSIZE]; dns_rbt_t **treep; isc_time_t start; - dns_dbonupdatelistener_t *listener, *listener_next; if (IS_CACHE(rbtdb) && rbtdb->common.rdclass == dns_rdataclass_in) { overmem((dns_db_t *)rbtdb, (bool)-1); @@ -1253,14 +1252,7 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) { rbtdb->common.impmagic = 0; isc_mem_detach(&rbtdb->hmctx); - for (listener = ISC_LIST_HEAD(rbtdb->common.update_listeners); - listener != NULL; listener = listener_next) - { - listener_next = ISC_LIST_NEXT(listener, link); - ISC_LIST_UNLINK(rbtdb->common.update_listeners, listener, link); - isc_mem_put(rbtdb->common.mctx, listener, - sizeof(dns_dbonupdatelistener_t)); - } + INSIST(ISC_LIST_EMPTY(rbtdb->common.update_listeners)); isc_mem_putanddetach(&rbtdb->common.mctx, rbtdb, sizeof(*rbtdb)); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index fb718151cc..bab33f390d 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -1981,6 +1981,31 @@ dns_zone_rpz_disable_db(dns_zone_t *zone, dns_db_t *db) { zone->rpzs->zones[zone->rpz_num]); } +/* + * If a zone is a catalog zone, attach it to update notification in database. + */ +void +dns_zone_catz_enable_db(dns_zone_t *zone, dns_db_t *db) { + REQUIRE(DNS_ZONE_VALID(zone)); + REQUIRE(db != NULL); + + if (zone->catzs != NULL) { + dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, + zone->catzs); + } +} + +static void +dns_zone_catz_disable_db(dns_zone_t *zone, dns_db_t *db) { + REQUIRE(DNS_ZONE_VALID(zone)); + REQUIRE(db != NULL); + + if (zone->catzs != NULL) { + dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, + zone->catzs); + } +} + static void zone_catz_enable(dns_zone_t *zone, dns_catz_zones_t *catzs) { REQUIRE(DNS_ZONE_VALID(zone)); @@ -2007,6 +2032,9 @@ zone_catz_disable(dns_zone_t *zone) { REQUIRE(DNS_ZONE_VALID(zone)); if (zone->catzs != NULL) { + if (zone->db != NULL) { + dns_zone_catz_disable_db(zone, zone->db); + } dns_catz_catzs_detach(&zone->catzs); } } @@ -2027,31 +2055,6 @@ dns_zone_catz_is_enabled(dns_zone_t *zone) { return (zone->catzs != NULL); } -/* - * If a zone is a catalog zone, attach it to update notification in database. - */ -void -dns_zone_catz_enable_db(dns_zone_t *zone, dns_db_t *db) { - REQUIRE(DNS_ZONE_VALID(zone)); - REQUIRE(db != NULL); - - if (zone->catzs != NULL) { - dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, - zone->catzs); - } -} - -static void -dns_zone_catz_disable_db(dns_zone_t *zone, dns_db_t *db) { - REQUIRE(DNS_ZONE_VALID(zone)); - REQUIRE(db != NULL); - - if (zone->catzs != NULL) { - dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, - zone->catzs); - } -} - /* * Set catalog zone ownership of the zone */ @@ -5456,6 +5459,11 @@ cleanup: isc_result_totext(result)); } + if (result != ISC_R_SUCCESS) { + dns_zone_rpz_disable_db(zone, db); + dns_zone_catz_disable_db(zone, db); + } + for (inc = ISC_LIST_HEAD(zone->newincludes); inc != NULL; inc = ISC_LIST_HEAD(zone->newincludes)) { @@ -17706,6 +17714,8 @@ static void zone_detachdb(dns_zone_t *zone) { REQUIRE(zone->db != NULL); + dns_zone_rpz_disable_db(zone, zone->db); + dns_zone_catz_disable_db(zone, zone->db); dns_db_detach(&zone->db); } From b1086a5561c8024fc39b5250063fc901c27eef06 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 1 Dec 2022 12:51:30 +1100 Subject: [PATCH 3/6] Add missing DbC magic checks Checking for value != NULL is not sufficient to detect use after free errors. --- lib/dns/catz.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index bd740b1685..ec048dfd8f 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -791,7 +791,7 @@ cleanup_ht: void dns_catz_catzs_set_view(dns_catz_zones_t *catzs, dns_view_t *view) { REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); - REQUIRE(view != NULL); + REQUIRE(DNS_VIEW_VALID(view)); /* Either it's a new one or it's being reconfigured. */ REQUIRE(catzs->view == NULL || !strcmp(catzs->view->name, view->name)); @@ -1006,7 +1006,7 @@ void dns_catz_catzs_detach(dns_catz_zones_t **catzsp) { dns_catz_zones_t *catzs; - REQUIRE(catzsp != NULL && *catzsp != NULL); + REQUIRE(catzsp != NULL && DNS_CATZ_ZONES_VALID(*catzsp)); catzs = *catzsp; *catzsp = NULL; @@ -1833,7 +1833,7 @@ dns_catz_generate_masterfilename(dns_catz_zone_t *zone, dns_catz_entry_t *entry, bool special = false; REQUIRE(DNS_CATZ_ZONE_VALID(zone)); - REQUIRE(entry != NULL); + REQUIRE(DNS_CATZ_ENTRY_VALID(entry)); REQUIRE(buffer != NULL && *buffer != NULL); isc_buffer_allocate(zone->catzs->mctx, &tbuf, @@ -1931,7 +1931,7 @@ dns_catz_generate_zonecfg(dns_catz_zone_t *zone, dns_catz_entry_t *entry, char zname[DNS_NAME_FORMATSIZE]; REQUIRE(DNS_CATZ_ZONE_VALID(zone)); - REQUIRE(entry != NULL); + REQUIRE(DNS_CATZ_ENTRY_VALID(entry)); REQUIRE(buf != NULL && *buf == NULL); /* @@ -2066,7 +2066,7 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg) { isc_region_t r; REQUIRE(DNS_DB_VALID(db)); - REQUIRE(fn_arg != NULL); + REQUIRE(DNS_CATZ_ZONES_VALID(fn_arg)); catzs = (dns_catz_zones_t *)fn_arg; dns_name_toregion(&db->origin, &r); From bca84c8601a82d1f7d75b670a9ea8922ebecb4d6 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 30 Nov 2022 19:32:11 +1100 Subject: [PATCH 4/6] Check that restored catalog zone works Using a restored catalog zone excercised a use-after-free bug. The test checks that the use-after-free bug is gone and is just a reasonable behaviour check in its own right. --- bin/tests/system/catz/tests.sh | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/bin/tests/system/catz/tests.sh b/bin/tests/system/catz/tests.sh index 697ae9badf..ee95d2e124 100644 --- a/bin/tests/system/catz/tests.sh +++ b/bin/tests/system/catz/tests.sh @@ -2515,6 +2515,45 @@ rndccmd 10.53.0.2 reconfig || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) +######################################################################### + +nextpart ns2/named.run >/dev/null + +n=$((n+1)) +echo_i "Adding a dom19.example. to primary via RNDC ($n)" +ret=0 +# enough initial content for IXFR response when TXT record is added below +echo "@ 3600 IN SOA . . 1 3600 3600 3600 3600" > ns1/dom19.example.db +echo "@ 3600 IN NS invalid." >> ns1/dom19.example.db +echo "foo 3600 IN TXT some content here" >> ns1/dom19.example.db +echo "bar 3600 IN TXT some content here" >> ns1/dom19.example.db +echo "xxx 3600 IN TXT some content here" >> ns1/dom19.example.db +echo "yyy 3600 IN TXT some content here" >> ns1/dom19.example.db +rndccmd 10.53.0.1 addzone dom19.example. in default '{ type primary; file "dom19.example.db"; allow-transfer { key tsig_key; }; allow-update { any; }; notify explicit; also-notify { 10.53.0.2; }; };' || ret=1 +if [ $ret -ne 0 ]; then echo_i "failed"; fi +status=$((status+ret)) + +n=$((n+1)) +echo_i "add an entry to the restored catalog zone ($n)" +ret=0 +$NSUPDATE -d <> nsupdate.out.test$n 2>&1 || ret=1 + server 10.53.0.1 ${PORT} + update add 09da0a318e5333a9a7f6c14c385d69f6933e8b72.zones.catalog1.example. 3600 IN PTR dom19.example. + update add label1.primaries.ext.09da0a318e5333a9a7f6c14c385d69f6933e8b72.zones.catalog1.example. 3600 IN A 10.53.0.1 + update add label1.primaries.ext.09da0a318e5333a9a7f6c14c385d69f6933e8b72.zones.catalog1.example. 3600 IN TXT "tsig_key" + send +END +if [ $ret -ne 0 ]; then echo_i "failed"; fi +status=$((status+ret)) + +n=$((n+1)) +echo_i "waiting for secondary to sync up ($n)" +ret=0 +wait_for_message ns2/named.run "catz: adding zone 'dom19.example' from catalog 'catalog1.example'" && +wait_for_message ns2/named.run "transfer of 'dom19.example/IN/default' from 10.53.0.1#${PORT}: Transfer status: success" || ret=1 +if [ $ret -ne 0 ]; then echo_i "failed"; fi +status=$((status+ret)) + ########################################################################## n=$((n+1)) echo_i "Adding a domain tls1.example. to primary via RNDC ($n)" From 72402e1710b05b384fc44b546fdbd1ece029ac7d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 30 Nov 2022 18:54:18 +1100 Subject: [PATCH 5/6] Add CHANGES note for [GL #3683] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index acbbaf255a..b3ea4ca14e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6039. [bug] Removing a catalog zone from catalog-zones without + also removing the referenced zone could leave a + dangling pointer. [GL #3683] + 6038. [bug] In some serve stale scenarios, like when following an expired CNAME record, named could return SERVFAIL if the previous request wasn't successful. Consider non-stale From 9843da3423cafff09d65b5bcea453029c020bbf7 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 30 Nov 2022 18:56:53 +1100 Subject: [PATCH 6/6] Add release note for [GL #3683] --- doc/notes/notes-current.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 3f03dd0bd1..49e2931b8d 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -71,6 +71,10 @@ Bug Fixes names in zone transfers that should have been compressed, so zone transfers were larger than before. :gl:`#3706` +- When a catalog zone is removed from the configuration, in some + cases a dangling pointer could cause a :iscman:`named` process + crash. This has been fixed. :gl:`#3683` + Known Issues ~~~~~~~~~~~~