From f053d5b41453aa2099030315b8faebd731e7789a Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 24 Aug 2022 12:21:50 +1000 Subject: [PATCH 1/2] Have dns_zt_apply lock the zone table There were a number of places where the zone table should have been locked, but wasn't, when dns_zt_apply was called. Added a isc_rwlocktype_t type parameter to dns_zt_apply and adjusted all calls to using it. Removed locks in callers. --- bin/named/server.c | 11 ++++++----- bin/named/statschannel.c | 9 +++++---- lib/dns/include/dns/zt.h | 2 +- lib/dns/view.c | 3 ++- lib/dns/zt.c | 28 +++++++++++++++++----------- tests/dns/zt_test.c | 4 ++-- 6 files changed, 33 insertions(+), 24 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index b38e00d027a..698e20d1d0d 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -9835,8 +9835,8 @@ cleanup_viewlist: if (result == ISC_R_SUCCESS && strcmp(view->name, "_bind") != 0) { dns_view_setviewrevert(view); - (void)dns_zt_apply(view->zonetable, false, NULL, - removed, view); + (void)dns_zt_apply(view->zonetable, isc_rwlocktype_read, + false, NULL, removed, view); } dns_view_detach(&view); } @@ -11535,8 +11535,8 @@ add_view_tolist(struct dumpcontext *dctx, dns_view_t *view) { ISC_LIST_INIT(vle->zonelist); ISC_LIST_APPEND(dctx->viewlist, vle, link); if (dctx->dumpzones) { - result = dns_zt_apply(view->zonetable, true, NULL, - add_zone_tolist, dctx); + result = dns_zt_apply(view->zonetable, isc_rwlocktype_read, + true, NULL, add_zone_tolist, dctx); } return (result); } @@ -12862,7 +12862,8 @@ named_server_sync(named_server_t *server, isc_lex_t *lex, isc_buffer_t **text) { for (view = ISC_LIST_HEAD(server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) { - result = dns_zt_apply(view->zonetable, false, NULL, + result = dns_zt_apply(view->zonetable, + isc_rwlocktype_none, false, NULL, synczone, &cleanup); if (result != ISC_R_SUCCESS && tresult == ISC_R_SUCCESS) { diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 801878e2bca..9484c2870ef 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -2254,8 +2254,8 @@ generatexml(named_server_t *server, uint32_t flags, int *buflen, if ((flags & STATS_XML_ZONES) != 0) { TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "zones")); - CHECK(dns_zt_apply(view->zonetable, true, NULL, - zone_xmlrender, writer)); + CHECK(dns_zt_apply(view->zonetable, isc_rwlocktype_read, + true, NULL, zone_xmlrender, writer)); TRY0(xmlTextWriterEndElement(writer)); /* /zones */ } @@ -2985,8 +2985,9 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, CHECKMEM(za); if ((flags & STATS_JSON_ZONES) != 0) { - CHECK(dns_zt_apply(view->zonetable, true, NULL, - zone_jsonrender, za)); + CHECK(dns_zt_apply(view->zonetable, + isc_rwlocktype_read, true, + NULL, zone_jsonrender, za)); } if (json_object_array_length(za) != 0) { diff --git a/lib/dns/include/dns/zt.h b/lib/dns/include/dns/zt.h index 4b001234ba8..077b26ddeac 100644 --- a/lib/dns/include/dns/zt.h +++ b/lib/dns/include/dns/zt.h @@ -168,7 +168,7 @@ dns_zt_freezezones(dns_zt_t *zt, dns_view_t *view, bool freeze); */ isc_result_t -dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, +dns_zt_apply(dns_zt_t *zt, isc_rwlocktype_t lock, bool stop, isc_result_t *sub, isc_result_t (*action)(dns_zone_t *, void *), void *uap); /*%< * Apply a given 'action' to all zone zones in the table. diff --git a/lib/dns/view.c b/lib/dns/view.c index 8fcf23345ca..43bdfc6fe71 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -611,7 +611,8 @@ dns_view_dialup(dns_view_t *view) { REQUIRE(DNS_VIEW_VALID(view)); REQUIRE(view->zonetable != NULL); - (void)dns_zt_apply(view->zonetable, false, NULL, dialup, NULL); + (void)dns_zt_apply(view->zonetable, isc_rwlocktype_read, false, NULL, + dialup, NULL); } void diff --git a/lib/dns/zt.c b/lib/dns/zt.c index aab623329a2..cbc0a8df5fd 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -228,7 +228,8 @@ zt_destroy(dns_zt_t *zt) { isc_refcount_destroy(&zt->loads_pending); if (atomic_load_acquire(&zt->flush)) { - (void)dns_zt_apply(zt, false, NULL, flush, NULL); + (void)dns_zt_apply(zt, isc_rwlocktype_none, false, NULL, flush, + NULL); } dns_rbt_destroy(&zt->table); @@ -263,9 +264,8 @@ dns_zt_load(dns_zt_t *zt, bool stop, bool newonly) { struct zt_load_params params; REQUIRE(VALID_ZT(zt)); params.newonly = newonly; - RWLOCK(&zt->rwlock, isc_rwlocktype_read); - result = dns_zt_apply(zt, stop, NULL, load, ¶ms); - RWUNLOCK(&zt->rwlock, isc_rwlocktype_read); + result = dns_zt_apply(zt, isc_rwlocktype_read, stop, NULL, load, + ¶ms); return (result); } @@ -336,9 +336,8 @@ dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_allloaded_t alldone, zt->loaddone = alldone; zt->loaddone_arg = arg; - RWLOCK(&zt->rwlock, isc_rwlocktype_read); - result = dns_zt_apply(zt, false, NULL, asyncload, zt); - RWUNLOCK(&zt->rwlock, isc_rwlocktype_read); + result = dns_zt_apply(zt, isc_rwlocktype_read, false, NULL, asyncload, + zt); /* * Have all the loads completed? @@ -384,9 +383,8 @@ dns_zt_freezezones(dns_zt_t *zt, dns_view_t *view, bool freeze) { REQUIRE(VALID_ZT(zt)); - RWLOCK(&zt->rwlock, isc_rwlocktype_read); - result = dns_zt_apply(zt, false, &tresult, freezezones, ¶ms); - RWUNLOCK(&zt->rwlock, isc_rwlocktype_read); + result = dns_zt_apply(zt, isc_rwlocktype_read, false, &tresult, + freezezones, ¶ms); if (tresult == ISC_R_NOTFOUND) { tresult = ISC_R_SUCCESS; } @@ -522,7 +520,7 @@ dns_zt_setviewrevert(dns_zt_t *zt) { } isc_result_t -dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, +dns_zt_apply(dns_zt_t *zt, isc_rwlocktype_t lock, bool stop, isc_result_t *sub, isc_result_t (*action)(dns_zone_t *, void *), void *uap) { dns_rbtnode_t *node; dns_rbtnodechain_t chain; @@ -532,6 +530,10 @@ dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, REQUIRE(VALID_ZT(zt)); REQUIRE(action != NULL); + if (lock != isc_rwlocktype_none) { + RWLOCK(&zt->rwlock, lock); + } + dns_rbtnodechain_init(&chain); result = dns_rbtnodechain_first(&chain, zt->table, NULL, NULL); if (result == ISC_R_NOTFOUND) { @@ -568,6 +570,10 @@ cleanup: *sub = tresult; } + if (lock != isc_rwlocktype_none) { + RWUNLOCK(&zt->rwlock, lock); + } + return (result); } diff --git a/tests/dns/zt_test.c b/tests/dns/zt_test.c index b164b74e17d..0f476951034 100644 --- a/tests/dns/zt_test.c +++ b/tests/dns/zt_test.c @@ -66,8 +66,8 @@ ISC_LOOP_TEST_IMPL(apply) { assert_non_null(view->zonetable); assert_int_equal(nzones, 0); - result = dns_zt_apply(view->zonetable, false, NULL, count_zone, - &nzones); + result = dns_zt_apply(view->zonetable, isc_rwlocktype_read, false, NULL, + count_zone, &nzones); assert_int_equal(result, ISC_R_SUCCESS); assert_int_equal(nzones, 1); -- GitLab From a4383c906ce832091b3bea4d9c0df9b0c798ae97 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 24 Aug 2022 12:52:49 +1000 Subject: [PATCH 2/2] Add CHANGES note for [GL #3468] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index e3b3ada5df1..4d73a73b370 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6017. [bug] The view's zone table was not locked when it should + have been leading to race conditions when external + extensions that manipulate the zone table where in + use. [GL #3468] + 6016. [func] Change NSEC3PARAM TTL to match the SOA MINIMUM. [GL #3570] -- GitLab