From ae5ba54fbea39df0afc4b0a9090821cc413027fa Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 16:34:41 -0800 Subject: [PATCH 1/8] move dispatchmgr from resolver to view the 'dispatchmgr' member of the resolver object is used by both the dns_resolver and dns_request modules, and may in the future be used by others such as dns_xfrin. it doesn't make sense for it to live in the resolver object; this commit moves it into dns_view. --- bin/named/server.c | 5 ++-- bin/tests/system/pipelined/pipequeries.c | 1 - bin/tools/mdig.c | 1 - lib/dns/client.c | 6 ++-- lib/dns/include/dns/resolver.h | 9 ++---- lib/dns/include/dns/view.h | 36 ++++++++++++++++-------- lib/dns/include/dns/xfrin.h | 8 ++++++ lib/dns/resolver.c | 29 ++++++------------- lib/dns/view.c | 24 ++++++++++++---- lib/dns/xfrin.c | 2 ++ tests/dns/resolver_test.c | 4 ++- 11 files changed, 73 insertions(+), 52 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 44d706bdc0..4330ce00e6 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4746,8 +4746,7 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, ndisp = 4 * ISC_MIN(named_g_udpdisp, MAX_UDP_DISPATCH); CHECK(dns_view_createresolver( view, named_g_loopmgr, ndisp, named_g_netmgr, resopts, - named_g_server->tlsctx_client_cache, named_g_dispatchmgr, - dispatch4, dispatch6)); + named_g_server->tlsctx_client_cache, dispatch4, dispatch6)); if (resstats == NULL) { CHECK(isc_stats_create(mctx, &resstats, @@ -6467,6 +6466,8 @@ create_view(const cfg_obj_t *vconfig, dns_viewlist_t *viewlist, return (result); } + dns_view_setdispatchmgr(view, named_g_dispatchmgr); + isc_nonce_buf(view->secret, sizeof(view->secret)); ISC_LIST_APPEND(*viewlist, view, link); diff --git a/bin/tests/system/pipelined/pipequeries.c b/bin/tests/system/pipelined/pipequeries.c index 8d8cb45372..805e3c5344 100644 --- a/bin/tests/system/pipelined/pipequeries.c +++ b/bin/tests/system/pipelined/pipequeries.c @@ -37,7 +37,6 @@ #include #include #include -#include #include #include #include diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c index b8aa675c83..21b71fcb58 100644 --- a/bin/tools/mdig.c +++ b/bin/tools/mdig.c @@ -48,7 +48,6 @@ #include #include #include -#include #include #include diff --git a/lib/dns/client.c b/lib/dns/client.c index c6f94acab5..a32f33645b 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -211,6 +211,8 @@ createview(isc_mem_t *mctx, dns_rdataclass_t rdclass, isc_loopmgr_t *loopmgr, return (result); } + dns_view_setdispatchmgr(view, dispatchmgr); + /* Initialize view security roots */ result = dns_view_initsecroots(view, mctx); if (result != ISC_R_SUCCESS) { @@ -218,8 +220,8 @@ createview(isc_mem_t *mctx, dns_rdataclass_t rdclass, isc_loopmgr_t *loopmgr, } result = dns_view_createresolver(view, loopmgr, 1, nm, 0, - tlsctx_client_cache, dispatchmgr, - dispatchv4, dispatchv6); + tlsctx_client_cache, dispatchv4, + dispatchv6); if (result != ISC_R_SUCCESS) { goto cleanup_view; } diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index 61429ddaf0..df74cd5e2a 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -183,8 +183,8 @@ isc_result_t dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *nm, unsigned int options, isc_tlsctx_cache_t *tlsctx_cache, - dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, - dns_dispatch_t *dispatchv6, dns_resolver_t **resp); + dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6, + dns_resolver_t **resp); /*%< * Create a resolver. @@ -204,8 +204,6 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, * *\li 'tlsctx_cache' != NULL. * - *\li 'dispatchmgr' != NULL. - * *\li 'dispatchv4' is a dispatch with an IPv4 UDP socket, or is NULL. * If not NULL, 'ndisp' clones of it will be created by the resolver. * @@ -404,9 +402,6 @@ dns_resolver_logfetch(dns_fetch_t *fetch, isc_log_t *lctx, *\li 'fetch' is a valid fetch, and has completed. */ -dns_dispatchmgr_t * -dns_resolver_dispatchmgr(dns_resolver_t *resolver); - dns_dispatch_t * dns_resolver_dispatchv4(dns_resolver_t *resolver); diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index e38a18dd72..b787c01a96 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -82,17 +82,18 @@ ISC_LANG_BEGINDECLS struct dns_view { /* Unlocked. */ - unsigned int magic; - isc_mem_t *mctx; - dns_rdataclass_t rdclass; - char *name; - dns_zt_t *zonetable; - dns_resolver_t *resolver; - dns_adb_t *adb; - dns_requestmgr_t *requestmgr; - dns_cache_t *cache; - dns_db_t *cachedb; - dns_db_t *hints; + unsigned int magic; + isc_mem_t *mctx; + dns_rdataclass_t rdclass; + char *name; + dns_zt_t *zonetable; + dns_resolver_t *resolver; + dns_adb_t *adb; + dns_requestmgr_t *requestmgr; + dns_dispatchmgr_t *dispatchmgr; + dns_cache_t *cache; + dns_db_t *cachedb; + dns_db_t *hints; /* * security roots and negative trust anchors. @@ -389,7 +390,6 @@ isc_result_t dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *netmgr, unsigned int options, isc_tlsctx_cache_t *tlsctx_cache, - dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6); /*%< * Create a resolver and address database for the view. @@ -400,6 +400,9 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, * *\li 'view' does not have a resolver already. * + *\li A dispatch manager has been associated with the view by calling + * dns_view_setdispatchmgr(). + * *\li The requirements of dns_resolver_create() apply to 'ndisp', * 'netmgr', 'options', 'tlsctx_cache', 'dispatchv4', and 'dispatchv6'. * @@ -1316,4 +1319,13 @@ dns_view_getudpsize(dns_view_t *view); * Get the current EDNS UDP buffer size. */ +void +dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr); +dns_dispatchmgr_t * +dns_view_getdispatchmgr(dns_view_t *view); +/*%< + * Set/get the dispatch manager for the view; this wil be used + * by the resolver and request managers to send and receive DNS + * messages. + */ ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/xfrin.h b/lib/dns/include/dns/xfrin.h index 70b601bb57..f37aa7f1c7 100644 --- a/lib/dns/include/dns/xfrin.h +++ b/lib/dns/include/dns/xfrin.h @@ -63,6 +63,14 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, * 'zone' and a result code as arguments when the transfer finishes. * * Requires: + *\li 'xfrp' != NULL and '*xfrp' == NULL. + * + *\li 'done' != NULL. + * + *\li 'primaryaddr' has a non-zero port number. + * + *\li 'zone' is a valid zone and is associated with a view. + * *\li 'xfrtype' is dns_rdatatype_axfr, dns_rdatatype_ixfr * or dns_rdatatype_soa (soa query followed by axfr if * serial is greater than current serial). diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index e043b9a366..b18563a92a 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -549,7 +549,6 @@ struct dns_resolver { bool frozen; unsigned int options; isc_tlsctx_cache_t *tlsctx_cache; - dns_dispatchmgr_t *dispatchmgr; dns_dispatchset_t *dispatches4; dns_dispatchset_t *dispatches6; @@ -2191,7 +2190,7 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, query = isc_mem_get(fctx->mctx, sizeof(*query)); *query = (resquery_t){ .options = options, .addrinfo = addrinfo, - .dispatchmgr = res->dispatchmgr }; + .dispatchmgr = res->view->dispatchmgr }; #if DNS_RESOLVER_TRACE fprintf(stderr, "rctx_init:%s:%s:%d:%p->references = 1\n", __func__, @@ -2256,7 +2255,7 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, } isc_sockaddr_setport(&addr, 0); - result = dns_dispatch_createtcp(res->dispatchmgr, &addr, + result = dns_dispatch_createtcp(res->view->dispatchmgr, &addr, &addrinfo->sockaddr, &query->dispatch); if (result != ISC_R_SUCCESS) { @@ -2266,7 +2265,8 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, FCTXTRACE("connecting via TCP"); } else { if (have_addr) { - result = dns_dispatch_createudp(res->dispatchmgr, &addr, + result = dns_dispatch_createudp(res->view->dispatchmgr, + &addr, &query->dispatch); if (result != ISC_R_SUCCESS) { goto cleanup_query; @@ -3914,20 +3914,17 @@ out: static void possibly_mark(fetchctx_t *fctx, dns_adbaddrinfo_t *addr) { isc_netaddr_t na; - isc_sockaddr_t *sa; + isc_sockaddr_t *sa = &addr->sockaddr; bool aborted = false; bool bogus; dns_acl_t *blackhole; isc_netaddr_t ipaddr; dns_peer_t *peer = NULL; - dns_resolver_t *res; + dns_resolver_t *res = fctx->res; const char *msg = NULL; - sa = &addr->sockaddr; - - res = fctx->res; isc_netaddr_fromsockaddr(&ipaddr, sa); - blackhole = dns_dispatchmgr_getblackhole(res->dispatchmgr); + blackhole = dns_dispatchmgr_getblackhole(res->view->dispatchmgr); (void)dns_peerlist_peerbyaddr(res->view->peers, &ipaddr, &peer); if (blackhole != NULL) { @@ -10116,8 +10113,8 @@ isc_result_t dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *nm, unsigned int options, isc_tlsctx_cache_t *tlsctx_cache, - dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, - dns_dispatch_t *dispatchv6, dns_resolver_t **resp) { + dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6, + dns_resolver_t **resp) { isc_result_t result = ISC_R_SUCCESS; dns_resolver_t *res = NULL; @@ -10129,7 +10126,6 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, REQUIRE(ndisp > 0); REQUIRE(resp != NULL && *resp == NULL); REQUIRE(tlsctx_cache != NULL); - REQUIRE(dispatchmgr != NULL); REQUIRE(dispatchv4 != NULL || dispatchv6 != NULL); RTRACE("create"); @@ -10139,7 +10135,6 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr, .loopmgr = loopmgr, .rdclass = view->rdclass, .nm = nm, - .dispatchmgr = dispatchmgr, .options = options, .tlsctx_cache = tlsctx_cache, .spillatmin = 10, @@ -10848,12 +10843,6 @@ dns_resolver_logfetch(dns_fetch_t *fetch, isc_log_t *lctx, UNLOCK(&fctx->lock); } -dns_dispatchmgr_t * -dns_resolver_dispatchmgr(dns_resolver_t *resolver) { - REQUIRE(VALID_RESOLVER(resolver)); - return (resolver->dispatchmgr); -} - dns_dispatch_t * dns_resolver_dispatchv4(dns_resolver_t *resolver) { REQUIRE(VALID_RESOLVER(resolver)); diff --git a/lib/dns/view.c b/lib/dns/view.c index 35ce0d824b..ce86eaa320 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -637,7 +637,6 @@ isc_result_t dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *netmgr, unsigned int options, isc_tlsctx_cache_t *tlsctx_cache, - dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6) { isc_result_t result; @@ -646,12 +645,13 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, REQUIRE(DNS_VIEW_VALID(view)); REQUIRE(!view->frozen); REQUIRE(view->resolver == NULL); + REQUIRE(view->dispatchmgr != NULL); view->loop = isc_loop_current(loopmgr); result = dns_resolver_create(view, loopmgr, ndisp, netmgr, options, - tlsctx_cache, dispatchmgr, dispatchv4, - dispatchv6, &view->resolver); + tlsctx_cache, dispatchv4, dispatchv6, + &view->resolver); if (result != ISC_R_SUCCESS) { return (result); } @@ -664,9 +664,9 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, goto cleanup_resolver; } - result = dns_requestmgr_create( - view->mctx, dns_resolver_dispatchmgr(view->resolver), - dispatchv4, dispatchv6, &view->requestmgr); + result = dns_requestmgr_create(view->mctx, view->dispatchmgr, + dispatchv4, dispatchv6, + &view->requestmgr); if (result != ISC_R_SUCCESS) { goto cleanup_adb; } @@ -2445,3 +2445,15 @@ dns_view_getudpsize(dns_view_t *view) { REQUIRE(DNS_VIEW_VALID(view)); return (view->udpsize); } + +void +dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr) { + REQUIRE(DNS_VIEW_VALID(view)); + view->dispatchmgr = dispatchmgr; +} + +dns_dispatchmgr_t * +dns_view_getdispatchmgr(dns_view_t *view) { + REQUIRE(DNS_VIEW_VALID(view)); + return (view->dispatchmgr); +} diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index 05e505b188..b693919a2d 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -704,6 +704,8 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, REQUIRE(xfrp != NULL && *xfrp == NULL); REQUIRE(done != NULL); REQUIRE(isc_sockaddr_getport(primaryaddr) != 0); + REQUIRE(zone != NULL); + REQUIRE(dns_zone_getview(zone) != NULL); (void)dns_zone_getdb(zone, &db); diff --git a/tests/dns/resolver_test.c b/tests/dns/resolver_test.c index e5bdea7ce9..6db03141fa 100644 --- a/tests/dns/resolver_test.c +++ b/tests/dns/resolver_test.c @@ -52,6 +52,8 @@ setup_test(void **state) { result = dns_test_makeview("view", false, &view); assert_int_equal(result, ISC_R_SUCCESS); + dns_view_setdispatchmgr(view, dispatchmgr); + isc_sockaddr_any(&local); result = dns_dispatch_createudp(dispatchmgr, &local, &dispatch); assert_int_equal(result, ISC_R_SUCCESS); @@ -75,7 +77,7 @@ mkres(dns_resolver_t **resolverp) { isc_tlsctx_cache_create(mctx, &tlsctx_cache); result = dns_resolver_create(view, loopmgr, 1, netmgr, 0, tlsctx_cache, - dispatchmgr, dispatch, NULL, resolverp); + dispatch, NULL, resolverp); assert_int_equal(result, ISC_R_SUCCESS); } From 9d376210124bb859cb6465a73e66204a6ce77a5e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 18:15:55 -0800 Subject: [PATCH 2/8] remove dead code in dns_request the 'connected' variable in 'dns_request_create()` was always false. --- lib/dns/request.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/lib/dns/request.c b/lib/dns/request.c index 442b98bc4b..e3bef774b5 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -561,7 +561,6 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message, isc_mem_t *mctx = NULL; dns_messageid_t id; bool tcp = false; - bool connected = false; REQUIRE(VALID_REQUESTMGR(requestmgr)); REQUIRE(message != NULL); @@ -672,21 +671,14 @@ again: UNLOCK(&requestmgr->lock); request->destaddr = *destaddr; - if (tcp && connected) { - req_send(request); + request->flags |= DNS_REQUEST_F_CONNECTING; + if (tcp) { + request->flags |= DNS_REQUEST_F_TCP; + } - /* no need to call req_connected(), unref here */ - dns_request_unref(request); - } else { - request->flags |= DNS_REQUEST_F_CONNECTING; - if (tcp) { - request->flags |= DNS_REQUEST_F_TCP; - } - - result = dns_dispatch_connect(request->dispentry); - if (result != ISC_R_SUCCESS) { - goto unlink; - } + result = dns_dispatch_connect(request->dispentry); + if (result != ISC_R_SUCCESS) { + goto unlink; } req_log(ISC_LOG_DEBUG(3), "dns_request_create: request %p", request); From 1dd42a80d6c32f8cd4cc5daf977a8bf4afb37853 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 21:09:05 -0800 Subject: [PATCH 3/8] log the xfrin pointer address in xfrin_log() to make it easier to trace xfrin events in the log, include the address of the dns_xfrin_t object in all xfrin log messages. --- lib/dns/xfrin.c | 40 ++++++++-------------------------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index b693919a2d..be1d41234d 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -251,12 +251,9 @@ static isc_result_t render(dns_message_t *msg, isc_mem_t *mctx, isc_buffer_t *buf); static void -xfrin_logv(int level, const char *zonetext, const isc_sockaddr_t *primaryaddr, - const char *fmt, va_list ap) ISC_FORMAT_PRINTF(4, 0); - -static void -xfrin_log1(int level, const char *zonetext, const isc_sockaddr_t *primaryaddr, - const char *fmt, ...) ISC_FORMAT_PRINTF(4, 5); +xfrin_logv(dns_xfrin_t *xff, int level, const char *zonetext, + const isc_sockaddr_t *primaryaddr, const char *fmt, va_list ap) + ISC_FORMAT_PRINTF(5, 0); static void xfrin_log(dns_xfrin_ctx_t *xfr, int level, const char *fmt, ...) @@ -746,10 +743,7 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, } if (result != ISC_R_SUCCESS) { - char zonetext[DNS_NAME_MAXTEXT + 32]; - dns_zone_name(zone, zonetext, sizeof(zonetext)); - xfrin_log1(ISC_LOG_ERROR, zonetext, primaryaddr, - "zone transfer setup failed"); + xfrin_log(xfr, ISC_LOG_ERROR, "zone transfer setup failed"); } return (result); @@ -1635,8 +1629,8 @@ xfrin_destroy(dns_xfrin_ctx_t *xfr) { * transfer of from
: */ static void -xfrin_logv(int level, const char *zonetext, const isc_sockaddr_t *primaryaddr, - const char *fmt, va_list ap) { +xfrin_logv(dns_xfrin_t *xfr, int level, const char *zonetext, + const isc_sockaddr_t *primaryaddr, const char *fmt, va_list ap) { char primarytext[ISC_SOCKADDR_FORMATSIZE]; char msgtext[2048]; @@ -1644,28 +1638,10 @@ xfrin_logv(int level, const char *zonetext, const isc_sockaddr_t *primaryaddr, vsnprintf(msgtext, sizeof(msgtext), fmt, ap); isc_log_write(dns_lctx, DNS_LOGCATEGORY_XFER_IN, DNS_LOGMODULE_XFER_IN, - level, "transfer of '%s' from %s: %s", zonetext, + level, "%p: transfer of '%s' from %s: %s", xfr, zonetext, primarytext, msgtext); } -/* - * Logging function for use when a xfrin_ctx_t has not yet been created. - */ - -static void -xfrin_log1(int level, const char *zonetext, const isc_sockaddr_t *primaryaddr, - const char *fmt, ...) { - va_list ap; - - if (!isc_log_wouldlog(dns_lctx, level)) { - return; - } - - va_start(ap, fmt); - xfrin_logv(level, zonetext, primaryaddr, fmt, ap); - va_end(ap); -} - /* * Logging function for use when there is a xfrin_ctx_t. */ @@ -1682,6 +1658,6 @@ xfrin_log(dns_xfrin_ctx_t *xfr, int level, const char *fmt, ...) { dns_zone_name(xfr->zone, zonetext, sizeof(zonetext)); va_start(ap, fmt); - xfrin_logv(level, zonetext, &xfr->primaryaddr, fmt, ap); + xfrin_logv(xfr, level, zonetext, &xfr->primaryaddr, fmt, ap); va_end(ap); } From d72419d1f57a908a5c40661c98ab489d5d319e35 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 20:14:30 -0800 Subject: [PATCH 4/8] minor cleanups in dispatch.c - simplified tcp_startrecv() - removed a short function that was only called once - removed an unnecessary if statement --- lib/dns/dispatch.c | 52 +++++++++++----------------------- lib/dns/include/dns/dispatch.h | 1 - 2 files changed, 16 insertions(+), 37 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 995e91d96b..f97655f3b2 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -193,9 +193,6 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, static void tcp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, void *arg); -static void -tcp_recv_done(dns_dispentry_t *resp, isc_result_t eresult, - isc_region_t *region); static uint32_t dns_hash(dns_qid_t *, const isc_sockaddr_t *, dns_messageid_t, in_port_t); static void @@ -210,8 +207,7 @@ qid_destroy(isc_mem_t *mctx, dns_qid_t **qidp); static void udp_startrecv(isc_nmhandle_t *handle, dns_dispentry_t *resp); static void -tcp_startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, - dns_dispentry_t *resp); +tcp_startrecv(dns_dispatch_t *disp, dns_dispentry_t *resp); static void tcp_dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp, int32_t timeout); @@ -769,16 +765,6 @@ tcp_recv_shutdown(dns_dispatch_t *disp, dns_displist_t *resps, disp->state = DNS_DISPATCHSTATE_CANCELED; } -static void -tcp_recv_done(dns_dispentry_t *resp, isc_result_t eresult, - isc_region_t *region) { - dispentry_log(resp, LVL(90), "read callback: %s", - isc_result_totext(eresult)); - - resp->response(eresult, region, resp->arg); - dns_dispentry_detach(&resp); /* DISPENTRY009 */ -} - static void tcp_recv_processall(dns_displist_t *resps, isc_region_t *region) { dns_dispentry_t *resp = NULL, *next = NULL; @@ -786,7 +772,11 @@ tcp_recv_processall(dns_displist_t *resps, isc_region_t *region) { for (resp = ISC_LIST_HEAD(*resps); resp != NULL; resp = next) { next = ISC_LIST_NEXT(resp, rlink); ISC_LIST_UNLINK(*resps, resp, rlink); - tcp_recv_done(resp, resp->result, region); + + dispentry_log(resp, LVL(90), "read callback: %s", + isc_result_totext(resp->result)); + resp->response(resp->result, region, resp->arg); + dns_dispentry_detach(&resp); /* DISPENTRY009 */ } } @@ -864,7 +854,7 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, /* * Phase 3: Trigger timeouts. It's possible that the responses would - * have been timedout out already, but non-matching TCP reads have + * have been timed out out already, but non-matching TCP reads have * prevented this. */ dns_dispentry_t *next = NULL; @@ -910,7 +900,7 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, * Phase 5: Resume reading if there are still active responses */ if (!ISC_LIST_EMPTY(disp->active)) { - tcp_startrecv(NULL, disp, ISC_LIST_HEAD(disp->active)); + tcp_startrecv(disp, ISC_LIST_HEAD(disp->active)); } UNLOCK(&disp->lock); @@ -1739,7 +1729,7 @@ tcp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) { dispentry_log(resp, LVL(90), "final 1 second timeout on %p", disp->handle); - tcp_startrecv(NULL, disp, NULL); + tcp_startrecv(disp, NULL); } #else if (disp->reading) { @@ -1825,14 +1815,10 @@ udp_startrecv(isc_nmhandle_t *handle, dns_dispentry_t *resp) { } static void -tcp_startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, - dns_dispentry_t *resp) { +tcp_startrecv(dns_dispatch_t *disp, dns_dispentry_t *resp) { REQUIRE(VALID_DISPATCH(disp)); REQUIRE(disp->socktype == isc_socktype_tcp); - if (handle != NULL) { - isc_nmhandle_attach(handle, &disp->handle); - } dns_dispatch_ref(disp); /* DISPATCH002 */ if (resp != NULL) { dispentry_log(resp, LVL(90), "reading from %p", disp->handle); @@ -1904,7 +1890,8 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { disp->state = DNS_DISPATCHSTATE_CANCELED; } else if (eresult == ISC_R_SUCCESS) { disp->state = DNS_DISPATCHSTATE_CONNECTED; - tcp_startrecv(handle, disp, resp); + isc_nmhandle_attach(handle, &disp->handle); + tcp_startrecv(disp, resp); } else { disp->state = DNS_DISPATCHSTATE_NONE; } @@ -2043,16 +2030,9 @@ tcp_dispatch_connect(dns_dispatch_t *disp, dns_dispentry_t *resp) { "connecting from %s to %s, timeout %u", localbuf, peerbuf, resp->timeout); - if (transport_type == DNS_TRANSPORT_TLS) { - isc_nm_streamdnsconnect(disp->mgr->nm, &disp->local, - &disp->peer, tcp_connected, - disp, resp->timeout, tlsctx, - sess_cache); - } else { - isc_nm_streamdnsconnect( - disp->mgr->nm, &disp->local, &disp->peer, - tcp_connected, disp, resp->timeout, NULL, NULL); - } + isc_nm_streamdnsconnect(disp->mgr->nm, &disp->local, + &disp->peer, tcp_connected, disp, + resp->timeout, tlsctx, sess_cache); break; case DNS_DISPATCHSTATE_CONNECTING: @@ -2073,7 +2053,7 @@ tcp_dispatch_connect(dns_dispatch_t *disp, dns_dispentry_t *resp) { if (!disp->reading) { /* Restart the reading */ - tcp_startrecv(NULL, disp, resp); + tcp_startrecv(disp, resp); } UNLOCK(&disp->lock); diff --git a/lib/dns/include/dns/dispatch.h b/lib/dns/include/dns/dispatch.h index 2250d2afb2..265918bda4 100644 --- a/lib/dns/include/dns/dispatch.h +++ b/lib/dns/include/dns/dispatch.h @@ -387,5 +387,4 @@ dns_dispatch_getnext(dns_dispentry_t *resp); * Requires: *\li resp is valid */ - ISC_LANG_ENDDECLS From a4c8decc6ad33f3315a0c2630e04ff29ff04b5fa Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 23 Feb 2023 10:29:33 -0800 Subject: [PATCH 5/8] implement refcount tracing in xfrin.c use ISC_REFCOUNT_IMPL for dns_xfrin_ctx_t (which has been renamed to dns_xfrin_t to keep the function names dns_xfrin_attach() and dns_xfrin_detach() unchanged). --- lib/dns/include/dns/xfrin.h | 35 +++++---- lib/dns/xfrin.c | 152 +++++++++++++++--------------------- lib/dns/zone.c | 2 +- 3 files changed, 84 insertions(+), 105 deletions(-) diff --git a/lib/dns/include/dns/xfrin.h b/lib/dns/include/dns/xfrin.h index f37aa7f1c7..7e269c48eb 100644 --- a/lib/dns/include/dns/xfrin.h +++ b/lib/dns/include/dns/xfrin.h @@ -32,6 +32,9 @@ #include #include +/* Define to 1 for detailed reference tracing */ +#undef DNS_XFRIN_TRACE + /*** *** Types ***/ @@ -39,7 +42,7 @@ /*% * A transfer in progress. This is an opaque type. */ -typedef struct dns_xfrin_ctx dns_xfrin_ctx_t; +typedef struct dns_xfrin dns_xfrin_t; /*** *** Functions @@ -53,10 +56,10 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, isc_mem_t *mctx, isc_nm_t *netmgr, dns_xfrindone_t done, - dns_xfrin_ctx_t **xfrp); + dns_xfrin_t **xfrp); /*%< * Attempt to start an incoming zone transfer of 'zone' - * from 'primaryaddr', creating a dns_xfrin_ctx_t object to + * from 'primaryaddr', creating a dns_xfrin_t object to * manage it. Attach '*xfrp' to the newly created object. * * Iff ISC_R_SUCCESS is returned, '*done' is called with @@ -80,24 +83,22 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, */ void -dns_xfrin_shutdown(dns_xfrin_ctx_t *xfr); +dns_xfrin_shutdown(dns_xfrin_t *xfr); /*%< * If the zone transfer 'xfr' has already finished, * do nothing. Otherwise, abort it and cause it to call * its done callback with a status of ISC_R_CANCELED. */ -void -dns_xfrin_detach(dns_xfrin_ctx_t **xfrp); -/*%< - * Detach a reference to a zone transfer object. - * Caller to maintain external locking if required. - */ - -void -dns_xfrin_attach(dns_xfrin_ctx_t *source, dns_xfrin_ctx_t **target); -/*%< - * Caller to maintain external locking if required. - */ - +#if DNS_XFRIN_TRACE +#define dns_xfrin_ref(ptr) dns_xfrin__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_xfrin_unref(ptr) dns_xfrin__unref(ptr, __func__, __FILE__, __LINE__) +#define dns_xfrin_attach(ptr, ptrp) \ + dns_xfrin__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define dns_xfrin_detach(ptrp) \ + dns_xfrin__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_TRACE_DECL(dns_xfrin); +#else +ISC_REFCOUNT_DECL(dns_xfrin); +#endif ISC_LANG_ENDDECLS diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index be1d41234d..c74cce66f9 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -90,7 +90,7 @@ typedef enum { * Incoming zone transfer context. */ -struct dns_xfrin_ctx { +struct dns_xfrin { unsigned int magic; isc_mem_t *mctx; dns_zone_t *zone; @@ -199,43 +199,42 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, - dns_xfrin_ctx_t **xfrp); + dns_xfrin_t **xfrp); static isc_result_t -axfr_init(dns_xfrin_ctx_t *xfr); +axfr_init(dns_xfrin_t *xfr); static isc_result_t -axfr_makedb(dns_xfrin_ctx_t *xfr, dns_db_t **dbp); +axfr_makedb(dns_xfrin_t *xfr, dns_db_t **dbp); static isc_result_t -axfr_putdata(dns_xfrin_ctx_t *xfr, dns_diffop_t op, dns_name_t *name, - dns_ttl_t ttl, dns_rdata_t *rdata); +axfr_putdata(dns_xfrin_t *xfr, dns_diffop_t op, dns_name_t *name, dns_ttl_t ttl, + dns_rdata_t *rdata); static isc_result_t -axfr_apply(dns_xfrin_ctx_t *xfr); +axfr_apply(dns_xfrin_t *xfr); static isc_result_t -axfr_commit(dns_xfrin_ctx_t *xfr); +axfr_commit(dns_xfrin_t *xfr); static isc_result_t -axfr_finalize(dns_xfrin_ctx_t *xfr); +axfr_finalize(dns_xfrin_t *xfr); static isc_result_t -ixfr_init(dns_xfrin_ctx_t *xfr); +ixfr_init(dns_xfrin_t *xfr); static isc_result_t -ixfr_apply(dns_xfrin_ctx_t *xfr); +ixfr_apply(dns_xfrin_t *xfr); static isc_result_t -ixfr_putdata(dns_xfrin_ctx_t *xfr, dns_diffop_t op, dns_name_t *name, - dns_ttl_t ttl, dns_rdata_t *rdata); +ixfr_putdata(dns_xfrin_t *xfr, dns_diffop_t op, dns_name_t *name, dns_ttl_t ttl, + dns_rdata_t *rdata); static isc_result_t -ixfr_commit(dns_xfrin_ctx_t *xfr); +ixfr_commit(dns_xfrin_t *xfr); static isc_result_t -xfr_rr(dns_xfrin_ctx_t *xfr, dns_name_t *name, uint32_t ttl, - dns_rdata_t *rdata); +xfr_rr(dns_xfrin_t *xfr, dns_name_t *name, uint32_t ttl, dns_rdata_t *rdata); static isc_result_t -xfrin_start(dns_xfrin_ctx_t *xfr); +xfrin_start(dns_xfrin_t *xfr); static void xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg); static isc_result_t -xfrin_send_request(dns_xfrin_ctx_t *xfr); +xfrin_send_request(dns_xfrin_t *xfr); static void xfrin_send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg); static void @@ -243,10 +242,10 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, void *cbarg); static void -xfrin_destroy(dns_xfrin_ctx_t *xfr); +xfrin_destroy(dns_xfrin_t *xfr); static void -xfrin_fail(dns_xfrin_ctx_t *xfr, isc_result_t result, const char *msg); +xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg); static isc_result_t render(dns_message_t *msg, isc_mem_t *mctx, isc_buffer_t *buf); @@ -256,7 +255,7 @@ xfrin_logv(dns_xfrin_t *xff, int level, const char *zonetext, ISC_FORMAT_PRINTF(5, 0); static void -xfrin_log(dns_xfrin_ctx_t *xfr, int level, const char *fmt, ...) +xfrin_log(dns_xfrin_t *xfr, int level, const char *fmt, ...) ISC_FORMAT_PRINTF(3, 4); /**************************************************************************/ @@ -265,7 +264,7 @@ xfrin_log(dns_xfrin_ctx_t *xfr, int level, const char *fmt, ...) */ static isc_result_t -axfr_init(dns_xfrin_ctx_t *xfr) { +axfr_init(dns_xfrin_t *xfr) { isc_result_t result; xfr->is_ixfr = false; @@ -283,7 +282,7 @@ failure: } static isc_result_t -axfr_makedb(dns_xfrin_ctx_t *xfr, dns_db_t **dbp) { +axfr_makedb(dns_xfrin_t *xfr, dns_db_t **dbp) { isc_result_t result; result = dns_db_create(xfr->mctx, /* XXX */ @@ -299,8 +298,8 @@ axfr_makedb(dns_xfrin_ctx_t *xfr, dns_db_t **dbp) { } static isc_result_t -axfr_putdata(dns_xfrin_ctx_t *xfr, dns_diffop_t op, dns_name_t *name, - dns_ttl_t ttl, dns_rdata_t *rdata) { +axfr_putdata(dns_xfrin_t *xfr, dns_diffop_t op, dns_name_t *name, dns_ttl_t ttl, + dns_rdata_t *rdata) { isc_result_t result; dns_difftuple_t *tuple = NULL; @@ -325,7 +324,7 @@ failure: * Store a set of AXFR RRs in the database. */ static isc_result_t -axfr_apply(dns_xfrin_ctx_t *xfr) { +axfr_apply(dns_xfrin_t *xfr) { isc_result_t result; uint64_t records; @@ -345,7 +344,7 @@ failure: } static isc_result_t -axfr_commit(dns_xfrin_ctx_t *xfr) { +axfr_commit(dns_xfrin_t *xfr) { isc_result_t result; CHECK(axfr_apply(xfr)); @@ -358,7 +357,7 @@ failure: } static isc_result_t -axfr_finalize(dns_xfrin_ctx_t *xfr) { +axfr_finalize(dns_xfrin_t *xfr) { isc_result_t result; CHECK(dns_zone_replacedb(xfr->zone, xfr->db, true)); @@ -374,7 +373,7 @@ failure: */ static isc_result_t -ixfr_init(dns_xfrin_ctx_t *xfr) { +ixfr_init(dns_xfrin_t *xfr) { isc_result_t result; char *journalfile = NULL; @@ -400,8 +399,8 @@ failure: } static isc_result_t -ixfr_putdata(dns_xfrin_ctx_t *xfr, dns_diffop_t op, dns_name_t *name, - dns_ttl_t ttl, dns_rdata_t *rdata) { +ixfr_putdata(dns_xfrin_t *xfr, dns_diffop_t op, dns_name_t *name, dns_ttl_t ttl, + dns_rdata_t *rdata) { isc_result_t result; dns_difftuple_t *tuple = NULL; @@ -427,7 +426,7 @@ failure: * Apply a set of IXFR changes to the database. */ static isc_result_t -ixfr_apply(dns_xfrin_ctx_t *xfr) { +ixfr_apply(dns_xfrin_t *xfr) { isc_result_t result; uint64_t records; @@ -459,7 +458,7 @@ failure: } static isc_result_t -ixfr_commit(dns_xfrin_ctx_t *xfr) { +ixfr_commit(dns_xfrin_t *xfr) { isc_result_t result; CHECK(ixfr_apply(xfr)); @@ -487,8 +486,7 @@ failure: * state. */ static isc_result_t -xfr_rr(dns_xfrin_ctx_t *xfr, dns_name_t *name, uint32_t ttl, - dns_rdata_t *rdata) { +xfr_rr(dns_xfrin_t *xfr, dns_name_t *name, uint32_t ttl, dns_rdata_t *rdata) { isc_result_t result; xfr->nrecs++; @@ -692,9 +690,9 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, isc_mem_t *mctx, isc_nm_t *netmgr, dns_xfrindone_t done, - dns_xfrin_ctx_t **xfrp) { + dns_xfrin_t **xfrp) { dns_name_t *zonename = dns_zone_getorigin(zone); - dns_xfrin_ctx_t *xfr = NULL; + dns_xfrin_t *xfr = NULL; isc_result_t result; dns_db_t *db = NULL; @@ -749,41 +747,21 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, return (result); } -static void -xfrin_cancelio(dns_xfrin_ctx_t *xfr); - void -dns_xfrin_shutdown(dns_xfrin_ctx_t *xfr) { +dns_xfrin_shutdown(dns_xfrin_t *xfr) { REQUIRE(VALID_XFRIN(xfr)); xfrin_fail(xfr, ISC_R_CANCELED, "shut down"); } -void -dns_xfrin_attach(dns_xfrin_ctx_t *source, dns_xfrin_ctx_t **target) { - REQUIRE(VALID_XFRIN(source)); - REQUIRE(target != NULL && *target == NULL); - (void)isc_refcount_increment(&source->references); - - *target = source; -} - -void -dns_xfrin_detach(dns_xfrin_ctx_t **xfrp) { - dns_xfrin_ctx_t *xfr = NULL; - - REQUIRE(xfrp != NULL && VALID_XFRIN(*xfrp)); - - xfr = *xfrp; - *xfrp = NULL; - - if (isc_refcount_decrement(&xfr->references) == 1) { - xfrin_destroy(xfr); - } -} +#if DNS_XFRIN_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_xfrin, xfrin_destroy); +#else +ISC_REFCOUNT_IMPL(dns_xfrin, xfrin_destroy); +#endif static void -xfrin_cancelio(dns_xfrin_ctx_t *xfr) { +xfrin_cancelio(dns_xfrin_t *xfr) { if (xfr->readhandle == NULL) { return; } @@ -793,7 +771,7 @@ xfrin_cancelio(dns_xfrin_ctx_t *xfr) { } static void -xfrin_reset(dns_xfrin_ctx_t *xfr) { +xfrin_reset(dns_xfrin_t *xfr) { REQUIRE(VALID_XFRIN(xfr)); xfrin_log(xfr, ISC_LOG_INFO, "resetting"); @@ -822,7 +800,7 @@ xfrin_reset(dns_xfrin_ctx_t *xfr) { } static void -xfrin_fail(dns_xfrin_ctx_t *xfr, isc_result_t result, const char *msg) { +xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { /* Make sure only the first xfrin_fail() trumps */ if (atomic_compare_exchange_strong(&xfr->shuttingdown, &(bool){ false }, true)) @@ -858,19 +836,19 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, - dns_xfrin_ctx_t **xfrp) { - dns_xfrin_ctx_t *xfr = NULL; + dns_xfrin_t **xfrp) { + dns_xfrin_t *xfr = NULL; xfr = isc_mem_get(mctx, sizeof(*xfr)); - *xfr = (dns_xfrin_ctx_t){ .netmgr = netmgr, - .shutdown_result = ISC_R_UNSET, - .rdclass = rdclass, - .reqtype = reqtype, - .id = (dns_messageid_t)isc_random16(), - .maxrecords = dns_zone_getmaxrecords(zone), - .primaryaddr = *primaryaddr, - .sourceaddr = *sourceaddr, - .firstsoa = DNS_RDATA_INIT }; + *xfr = (dns_xfrin_t){ .netmgr = netmgr, + .shutdown_result = ISC_R_UNSET, + .rdclass = rdclass, + .reqtype = reqtype, + .id = (dns_messageid_t)isc_random16(), + .maxrecords = dns_zone_getmaxrecords(zone), + .primaryaddr = *primaryaddr, + .sourceaddr = *sourceaddr, + .firstsoa = DNS_RDATA_INIT }; isc_mem_attach(mctx, &xfr->mctx); dns_zone_iattach(zone, &xfr->zone); @@ -923,9 +901,9 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, } static isc_result_t -xfrin_start(dns_xfrin_ctx_t *xfr) { +xfrin_start(dns_xfrin_t *xfr) { isc_result_t result; - dns_xfrin_ctx_t *connect_xfr = NULL; + dns_xfrin_t *connect_xfr = NULL; dns_transport_type_t transport_type = DNS_TRANSPORT_TCP; isc_tlsctx_t *tlsctx = NULL; isc_tlsctx_client_session_cache_t *sess_cache = NULL; @@ -996,7 +974,7 @@ failure: */ static void xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - dns_xfrin_ctx_t *xfr = (dns_xfrin_ctx_t *)cbarg; + dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; char sourcetext[ISC_SOCKADDR_FORMATSIZE]; char signerbuf[DNS_NAME_FORMATSIZE]; const char *signer = "", *sep = ""; @@ -1091,7 +1069,7 @@ tuple2msgname(dns_difftuple_t *tuple, dns_message_t *msg, dns_name_t **target) { * Build an *XFR request and send its length prefix. */ static isc_result_t -xfrin_send_request(dns_xfrin_ctx_t *xfr) { +xfrin_send_request(dns_xfrin_t *xfr) { isc_result_t result; isc_region_t region; dns_rdataset_t *qrdataset = NULL; @@ -1100,7 +1078,7 @@ xfrin_send_request(dns_xfrin_ctx_t *xfr) { dns_name_t *qname = NULL; dns_dbversion_t *ver = NULL; dns_name_t *msgsoaname = NULL; - dns_xfrin_ctx_t *send_xfr = NULL; + dns_xfrin_t *send_xfr = NULL; /* Create the request message */ dns_message_create(xfr->mctx, DNS_MESSAGE_INTENTRENDER, &msg); @@ -1184,8 +1162,8 @@ failure: static void xfrin_send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - dns_xfrin_ctx_t *xfr = (dns_xfrin_ctx_t *)cbarg; - dns_xfrin_ctx_t *recv_xfr = NULL; + dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; + dns_xfrin_t *recv_xfr = NULL; REQUIRE(VALID_XFRIN(xfr)); @@ -1215,7 +1193,7 @@ failure: static void xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, void *cbarg) { - dns_xfrin_ctx_t *xfr = (dns_xfrin_ctx_t *)cbarg; + dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; dns_message_t *msg = NULL; dns_name_t *name = NULL; const dns_name_t *tsigowner = NULL; @@ -1511,7 +1489,7 @@ failure: } static void -xfrin_destroy(dns_xfrin_ctx_t *xfr) { +xfrin_destroy(dns_xfrin_t *xfr) { uint64_t msecs; uint64_t persec; const char *result_str; @@ -1647,7 +1625,7 @@ xfrin_logv(dns_xfrin_t *xfr, int level, const char *zonetext, */ static void -xfrin_log(dns_xfrin_ctx_t *xfr, int level, const char *fmt, ...) { +xfrin_log(dns_xfrin_t *xfr, int level, const char *fmt, ...) { va_list ap; char zonetext[DNS_NAME_MAXTEXT + 32]; diff --git a/lib/dns/zone.c b/lib/dns/zone.c index e60b7547d2..af3bac270c 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -323,7 +323,7 @@ struct dns_zone { isc_sockaddr_t xfrsource4; isc_sockaddr_t xfrsource6; isc_sockaddr_t sourceaddr; - dns_xfrin_ctx_t *xfr; /* loop locked */ + dns_xfrin_t *xfr; /* loop locked */ dns_tsigkey_t *tsigkey; /* key used for xfr */ dns_transport_t *transport; /* transport used for xfr */ /* Access Control Lists */ From f0c766abec3db9ff15dbebc12e3f7a0e25913f7c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 20:14:30 -0800 Subject: [PATCH 6/8] refactor dns_xfrin to use dns_dispatch the dns_xfrin module was still using the network manager directly to manage TCP connections and send and receive messages. this commit changes it to use the dispatch manager instead. --- bin/tests/system/xfer/tests.sh | 2 +- lib/dns/dispatch.c | 11 ++ lib/dns/include/dns/dispatch.h | 11 ++ lib/dns/include/dns/xfrin.h | 3 +- lib/dns/xfrin.c | 269 +++++++++++++++------------------ lib/dns/zone.c | 2 +- 6 files changed, 150 insertions(+), 148 deletions(-) diff --git a/bin/tests/system/xfer/tests.sh b/bin/tests/system/xfer/tests.sh index 7ff58c7bd0..fd0e5ff495 100755 --- a/bin/tests/system/xfer/tests.sh +++ b/bin/tests/system/xfer/tests.sh @@ -431,7 +431,7 @@ $RNDCCMD 10.53.0.4 retransfer nil | sed 's/^/ns4 /' | cat_i sleep 2 -nextpart ns4/named.run | grep "unexpected message id" > /dev/null || { +nextpart ns4/named.run | grep "Transfer status: unexpected error" > /dev/null || { echo_i "failed: expected status was not logged" status=$((status+1)) } diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index f97655f3b2..9e16b5682b 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -2328,3 +2328,14 @@ dns_dispatchset_destroy(dns_dispatchset_t **dsetp) { isc_mutex_destroy(&dset->lock); isc_mem_putanddetach(&dset->mctx, dset, sizeof(dns_dispatchset_t)); } + +isc_result_t +dns_dispatch_checkperm(dns_dispatch_t *disp) { + REQUIRE(VALID_DISPATCH(disp)); + + if (disp->handle == NULL || disp->socktype == isc_socktype_udp) { + return (ISC_R_NOPERM); + } + + return (isc_nm_xfr_checkperm(disp->handle)); +} diff --git a/lib/dns/include/dns/dispatch.h b/lib/dns/include/dns/dispatch.h index 265918bda4..254fda8e32 100644 --- a/lib/dns/include/dns/dispatch.h +++ b/lib/dns/include/dns/dispatch.h @@ -387,4 +387,15 @@ dns_dispatch_getnext(dns_dispentry_t *resp); * Requires: *\li resp is valid */ + +isc_result_t +dns_dispatch_checkperm(dns_dispatch_t *disp); +/*%< + * Check whether it is permitted to do a zone transfer over a dispatch. + * See isc_nm_xfr_checkperm(). + * + * Requires: + *\li disp is valid + */ + ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/xfrin.h b/lib/dns/include/dns/xfrin.h index 7e269c48eb..24ed327de7 100644 --- a/lib/dns/include/dns/xfrin.h +++ b/lib/dns/include/dns/xfrin.h @@ -55,8 +55,7 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, - isc_mem_t *mctx, isc_nm_t *netmgr, dns_xfrindone_t done, - dns_xfrin_t **xfrp); + isc_mem_t *mctx, dns_xfrindone_t done, dns_xfrin_t **xfrp); /*%< * Attempt to start an incoming zone transfer of 'zone' * from 'primaryaddr', creating a dns_xfrin_t object to diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index c74cce66f9..290754f699 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -17,7 +17,6 @@ #include #include -#include #include #include #include @@ -27,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -94,15 +94,10 @@ struct dns_xfrin { unsigned int magic; isc_mem_t *mctx; dns_zone_t *zone; + dns_view_t *view; isc_refcount_t references; - isc_nm_t *netmgr; - - isc_refcount_t connects; /*%< Connect in progress */ - isc_refcount_t sends; /*%< Send in progress */ - isc_refcount_t recvs; /*%< Receive in progress */ - atomic_bool shuttingdown; isc_result_t shutdown_result; @@ -122,9 +117,8 @@ struct dns_xfrin { isc_sockaddr_t primaryaddr; isc_sockaddr_t sourceaddr; - isc_nmhandle_t *handle; - isc_nmhandle_t *readhandle; - isc_nmhandle_t *sendhandle; + dns_dispatch_t *disp; + dns_dispentry_t *dispentry; /*% Buffer for IXFR/AXFR request message */ isc_buffer_t qbuffer; @@ -194,7 +188,7 @@ struct dns_xfrin { */ static void -xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, +xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, dns_name_t *zonename, dns_rdataclass_t rdclass, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, @@ -232,14 +226,13 @@ static isc_result_t xfrin_start(dns_xfrin_t *xfr); static void -xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg); +xfrin_connect_done(isc_result_t result, isc_region_t *region, void *arg); static isc_result_t xfrin_send_request(dns_xfrin_t *xfr); static void -xfrin_send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg); +xfrin_send_done(isc_result_t eresult, isc_region_t *region, void *arg); static void -xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, - isc_region_t *region, void *cbarg); +xfrin_recv_done(isc_result_t result, isc_region_t *region, void *arg); static void xfrin_destroy(dns_xfrin_t *xfr); @@ -689,8 +682,7 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache, - isc_mem_t *mctx, isc_nm_t *netmgr, dns_xfrindone_t done, - dns_xfrin_t **xfrp) { + isc_mem_t *mctx, dns_xfrindone_t done, dns_xfrin_t **xfrp) { dns_name_t *zonename = dns_zone_getorigin(zone); dns_xfrin_t *xfr = NULL; isc_result_t result; @@ -708,9 +700,9 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, REQUIRE(db != NULL); } - xfrin_create(mctx, zone, db, netmgr, zonename, dns_zone_getclass(zone), - xfrtype, primaryaddr, sourceaddr, tsigkey, transport, - tlsctx_cache, &xfr); + xfrin_create(mctx, zone, db, zonename, dns_zone_getclass(zone), xfrtype, + primaryaddr, sourceaddr, tsigkey, transport, tlsctx_cache, + &xfr); if (db != NULL) { xfr->zone_had_db = true; @@ -721,11 +713,9 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, isc_refcount_init(&xfr->references, 1); /* - * Set *xfrp now, before calling xfrin_start(). Asynchronous - * netmgr processing could cause the 'done' callback to run in - * another thread before we reached the end of the present - * function. In that case, if *xfrp hadn't already been - * attached, the 'done' function would be unable to detach it. + * Set *xfrp now, before calling xfrin_start(), otherwise it's + * possible the 'done' callback could be run before *xfrp + * was attached. */ *xfrp = xfr; @@ -752,6 +742,9 @@ dns_xfrin_shutdown(dns_xfrin_t *xfr) { REQUIRE(VALID_XFRIN(xfr)); xfrin_fail(xfr, ISC_R_CANCELED, "shut down"); + + /* we won't reach xfrin_recv_done(), so dereference xfr here */ + dns_xfrin_unref(xfr); } #if DNS_XFRIN_TRACE @@ -762,12 +755,8 @@ ISC_REFCOUNT_IMPL(dns_xfrin, xfrin_destroy); static void xfrin_cancelio(dns_xfrin_t *xfr) { - if (xfr->readhandle == NULL) { - return; - } - - isc_nm_cancelread(xfr->readhandle); - /* The xfr->readhandle detach will happen in xfrin_recv_done callback */ + dns_dispatch_done(&xfr->dispentry); + dns_dispatch_detach(&xfr->disp); } static void @@ -776,9 +765,6 @@ xfrin_reset(dns_xfrin_t *xfr) { xfrin_log(xfr, ISC_LOG_INFO, "resetting"); - REQUIRE(xfr->readhandle == NULL); - REQUIRE(xfr->sendhandle == NULL); - if (xfr->lasttsig != NULL) { isc_buffer_free(&xfr->lasttsig); } @@ -801,6 +787,8 @@ xfrin_reset(dns_xfrin_t *xfr) { static void xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { + dns_xfrin_ref(xfr); + /* Make sure only the first xfrin_fail() trumps */ if (atomic_compare_exchange_strong(&xfr->shuttingdown, &(bool){ false }, true)) @@ -810,12 +798,14 @@ xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { xfrin_log(xfr, ISC_LOG_ERROR, "%s: %s", msg, isc_result_totext(result)); if (xfr->is_ixfr) { - /* Pass special result code to force AXFR retry + /* + * Pass special result code to force AXFR retry */ result = DNS_R_BADIXFR; } } xfrin_cancelio(xfr); + /* * Close the journal. */ @@ -828,10 +818,12 @@ xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { } xfr->shutdown_result = result; } + + dns_xfrin_detach(&xfr); } static void -xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, +xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, dns_name_t *zonename, dns_rdataclass_t rdclass, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, @@ -840,11 +832,9 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, dns_xfrin_t *xfr = NULL; xfr = isc_mem_get(mctx, sizeof(*xfr)); - *xfr = (dns_xfrin_t){ .netmgr = netmgr, - .shutdown_result = ISC_R_UNSET, + *xfr = (dns_xfrin_t){ .shutdown_result = ISC_R_UNSET, .rdclass = rdclass, .reqtype = reqtype, - .id = (dns_messageid_t)isc_random16(), .maxrecords = dns_zone_getmaxrecords(zone), .primaryaddr = *primaryaddr, .sourceaddr = *sourceaddr, @@ -852,12 +842,9 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, isc_mem_attach(mctx, &xfr->mctx); dns_zone_iattach(zone, &xfr->zone); + dns_view_weakattach(dns_zone_getview(zone), &xfr->view); dns_name_init(&xfr->name, NULL); - isc_refcount_init(&xfr->connects, 0); - isc_refcount_init(&xfr->sends, 0); - isc_refcount_init(&xfr->recvs, 0); - atomic_init(&xfr->shuttingdown, false); if (db != NULL) { @@ -902,50 +889,53 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_nm_t *netmgr, static isc_result_t xfrin_start(dns_xfrin_t *xfr) { - isc_result_t result; - dns_xfrin_t *connect_xfr = NULL; - dns_transport_type_t transport_type = DNS_TRANSPORT_TCP; - isc_tlsctx_t *tlsctx = NULL; - isc_tlsctx_client_session_cache_t *sess_cache = NULL; + isc_result_t result = ISC_R_FAILURE; - (void)isc_refcount_increment0(&xfr->connects); - dns_xfrin_attach(xfr, &connect_xfr); + dns_xfrin_ref(xfr); - if (xfr->transport != NULL) { - transport_type = dns_transport_get_type(xfr->transport); + /* + * Reuse an existing TCP connection if possible. For XoT, we can't + * do this because other connections could be using a different + * certificate, so we just create a new dispatch every time. + */ + if (xfr->transport == NULL || + dns_transport_get_type(xfr->transport) == DNS_TRANSPORT_TCP) + { + result = dns_dispatch_gettcp(dns_view_getdispatchmgr(xfr->view), + &xfr->primaryaddr, + &xfr->sourceaddr, &xfr->disp); + } + if (result == ISC_R_SUCCESS) { + char peer[ISC_SOCKADDR_FORMATSIZE]; + isc_sockaddr_format(&xfr->primaryaddr, peer, sizeof(peer)); + xfrin_log(xfr, ISC_LOG_DEBUG(1), + "attached to TCP connection to %s", peer); + } else { + CHECK(dns_dispatch_createtcp(dns_view_getdispatchmgr(xfr->view), + &xfr->sourceaddr, + &xfr->primaryaddr, &xfr->disp)); } /* * XXX: timeouts are hard-coded to 30 seconds; this needs to be * configurable. */ - switch (transport_type) { - case DNS_TRANSPORT_TCP: - isc_nm_streamdnsconnect(xfr->netmgr, &xfr->sourceaddr, - &xfr->primaryaddr, xfrin_connect_done, - connect_xfr, 30000, NULL, NULL); - break; - case DNS_TRANSPORT_TLS: { - result = dns_transport_get_tlsctx( - xfr->transport, &xfr->primaryaddr, xfr->tlsctx_cache, - xfr->mctx, &tlsctx, &sess_cache); - if (result != ISC_R_SUCCESS) { - goto failure; - } - INSIST(tlsctx != NULL); - isc_nm_streamdnsconnect(xfr->netmgr, &xfr->sourceaddr, - &xfr->primaryaddr, xfrin_connect_done, - connect_xfr, 30000, tlsctx, sess_cache); - } break; - default: - UNREACHABLE(); - } - + CHECK(dns_dispatch_add( + xfr->disp, 0, 30000, &xfr->primaryaddr, xfr->transport, + xfr->tlsctx_cache, xfrin_connect_done, xfrin_send_done, + xfrin_recv_done, xfr, &xfr->id, &xfr->dispentry)); + CHECK(dns_dispatch_connect(xfr->dispentry)); return (ISC_R_SUCCESS); failure: - isc_refcount_decrement0(&xfr->connects); - dns_xfrin_detach(&connect_xfr); + if (xfr->dispentry != NULL) { + dns_dispatch_done(&xfr->dispentry); + } + if (xfr->disp != NULL) { + dns_dispatch_detach(&xfr->disp); + } + dns_xfrin_detach(&xfr); + return (result); } @@ -973,26 +963,29 @@ failure: * A connection has been established. */ static void -xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; - char sourcetext[ISC_SOCKADDR_FORMATSIZE]; +xfrin_connect_done(isc_result_t result, isc_region_t *region, void *arg) { + dns_xfrin_t *xfr = (dns_xfrin_t *)arg; + char addrtext[ISC_SOCKADDR_FORMATSIZE]; char signerbuf[DNS_NAME_FORMATSIZE]; const char *signer = "", *sep = ""; - isc_sockaddr_t sockaddr; dns_zonemgr_t *zmgr = NULL; isc_time_t now; - REQUIRE(VALID_XFRIN(xfr)); + UNUSED(region); - isc_refcount_decrement0(&xfr->connects); + REQUIRE(VALID_XFRIN(xfr)); if (atomic_load(&xfr->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; } - CHECK(result); + if (result != ISC_R_SUCCESS) { + xfrin_fail(xfr, result, "failed to connect"); + dns_xfrin_unref(xfr); + return; + } - CHECK(isc_nm_xfr_checkperm(handle)); + CHECK(dns_dispatch_checkperm(xfr->disp)); zmgr = dns_zone_getmgr(xfr->zone); if (zmgr != NULL) { @@ -1007,10 +1000,6 @@ xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { } } - xfr->handle = handle; - sockaddr = isc_nmhandle_peeraddr(handle); - isc_sockaddr_format(&sockaddr, sourcetext, sizeof(sourcetext)); - if (xfr->tsigkey != NULL && xfr->tsigkey->key != NULL) { dns_name_format(dst_key_name(xfr->tsigkey->key), signerbuf, sizeof(signerbuf)); @@ -1018,17 +1007,16 @@ xfrin_connect_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { signer = signerbuf; } - xfrin_log(xfr, ISC_LOG_INFO, "connected using %s%s%s", sourcetext, sep, + isc_sockaddr_format(&xfr->primaryaddr, addrtext, sizeof(addrtext)); + xfrin_log(xfr, ISC_LOG_INFO, "connected using %s%s%s", addrtext, sep, signer); CHECK(xfrin_send_request(xfr)); failure: if (result != ISC_R_SUCCESS) { - xfrin_fail(xfr, result, "failed to connect"); + xfrin_fail(xfr, result, "connected but unable to transfer"); } - - dns_xfrin_detach(&xfr); /* connect_xfr */ } /* @@ -1065,6 +1053,20 @@ tuple2msgname(dns_difftuple_t *tuple, dns_message_t *msg, dns_name_t **target) { *target = name; } +static const char * +request_type(dns_xfrin_t *xfr) { + switch (xfr->reqtype) { + case dns_rdatatype_soa: + return "SOA"; + case dns_rdatatype_axfr: + return "AXFR"; + case dns_rdatatype_ixfr: + return "IXFR"; + default: + ISC_UNREACHABLE(); + } +} + /* * Build an *XFR request and send its length prefix. */ @@ -1078,7 +1080,6 @@ xfrin_send_request(dns_xfrin_t *xfr) { dns_name_t *qname = NULL; dns_dbversion_t *ver = NULL; dns_name_t *msgsoaname = NULL; - dns_xfrin_t *send_xfr = NULL; /* Create the request message */ dns_message_create(xfr->mctx, DNS_MESSAGE_INTENTRENDER, &msg); @@ -1099,7 +1100,6 @@ xfrin_send_request(dns_xfrin_t *xfr) { if (xfr->reqtype == dns_rdatatype_ixfr) { /* Get the SOA and add it to the authority section. */ - /* XXX is using the current version the right thing? */ dns_db_currentversion(xfr->db, &ver); CHECK(dns_db_createsoatuple(xfr->db, ver, xfr->mctx, DNS_DIFFOP_EXISTS, &soatuple)); @@ -1116,7 +1116,6 @@ xfrin_send_request(dns_xfrin_t *xfr) { &xfr->ixfr.request_serial)); } - xfr->id++; xfr->nmsg = 0; xfr->nrecs = 0; xfr->nbytes = 0; @@ -1143,10 +1142,10 @@ xfrin_send_request(dns_xfrin_t *xfr) { isc_buffer_usedregion(&xfr->qbuffer, ®ion); INSIST(region.length <= 65535); - dns_xfrin_attach(xfr, &send_xfr); - isc_nmhandle_attach(send_xfr->handle, &xfr->sendhandle); - isc_refcount_increment0(&send_xfr->sends); - isc_nm_send(xfr->handle, ®ion, xfrin_send_done, send_xfr); + dns_xfrin_ref(xfr); + dns_dispatch_send(xfr->dispentry, ®ion); + xfrin_log(xfr, ISC_LOG_DEBUG(3), "sending %s request, QID %d", + request_type(xfr), xfr->id); failure: dns_message_detach(&msg); @@ -1161,13 +1160,13 @@ failure: } static void -xfrin_send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { - dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; - dns_xfrin_t *recv_xfr = NULL; +xfrin_send_done(isc_result_t result, isc_region_t *region, void *arg) { + dns_xfrin_t *xfr = (dns_xfrin_t *)arg; + + UNUSED(region); REQUIRE(VALID_XFRIN(xfr)); - isc_refcount_decrement0(&xfr->sends); if (atomic_load(&xfr->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; } @@ -1176,34 +1175,24 @@ xfrin_send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { xfrin_log(xfr, ISC_LOG_DEBUG(3), "sent request data"); - dns_xfrin_attach(xfr, &recv_xfr); - isc_nmhandle_attach(handle, &recv_xfr->readhandle); - isc_refcount_increment0(&recv_xfr->recvs); - isc_nm_read(recv_xfr->handle, xfrin_recv_done, recv_xfr); - failure: if (result != ISC_R_SUCCESS) { xfrin_fail(xfr, result, "failed sending request data"); } - isc_nmhandle_detach(&xfr->sendhandle); - dns_xfrin_detach(&xfr); /* send_xfr */ + dns_xfrin_detach(&xfr); } static void -xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, - isc_region_t *region, void *cbarg) { - dns_xfrin_t *xfr = (dns_xfrin_t *)cbarg; +xfrin_recv_done(isc_result_t result, isc_region_t *region, void *arg) { + dns_xfrin_t *xfr = (dns_xfrin_t *)arg; dns_message_t *msg = NULL; dns_name_t *name = NULL; const dns_name_t *tsigowner = NULL; isc_buffer_t buffer; - isc_sockaddr_t peer; REQUIRE(VALID_XFRIN(xfr)); - isc_refcount_decrement0(&xfr->recvs); - if (atomic_load(&xfr->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; } @@ -1228,23 +1217,21 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, isc_buffer_init(&buffer, region->base, region->length); isc_buffer_add(&buffer, region->length); - peer = isc_nmhandle_peeraddr(handle); result = dns_message_parse(msg, &buffer, DNS_MESSAGEPARSE_PRESERVEORDER); if (result == ISC_R_SUCCESS) { - dns_message_logpacket(msg, "received message from", &peer, - DNS_LOGCATEGORY_XFER_IN, - DNS_LOGMODULE_XFER_IN, ISC_LOG_DEBUG(10), - xfr->mctx); + dns_message_logpacket( + msg, "received message from", &xfr->primaryaddr, + DNS_LOGCATEGORY_XFER_IN, DNS_LOGMODULE_XFER_IN, + ISC_LOG_DEBUG(10), xfr->mctx); } else { xfrin_log(xfr, ISC_LOG_DEBUG(10), "dns_message_parse: %s", isc_result_totext(result)); } if (result != ISC_R_SUCCESS || msg->rcode != dns_rcode_noerror || - msg->opcode != dns_opcode_query || msg->rdclass != xfr->rdclass || - msg->id != xfr->id) + msg->opcode != dns_opcode_query || msg->rdclass != xfr->rdclass) { if (result == ISC_R_SUCCESS && msg->rcode != dns_rcode_noerror) { @@ -1270,7 +1257,6 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, xfrin_log(xfr, ISC_LOG_DEBUG(3), "got %s, retrying with AXFR", isc_result_totext(result)); try_axfr: - isc_nmhandle_detach(&xfr->readhandle); dns_message_detach(&msg); xfrin_reset(xfr); xfr->reqtype = dns_rdatatype_soa; @@ -1279,7 +1265,7 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, if (result != ISC_R_SUCCESS) { xfrin_fail(xfr, result, "failed setting up socket"); } - dns_xfrin_detach(&xfr); /* recv_xfr */ + dns_xfrin_detach(&xfr); return; } @@ -1359,7 +1345,7 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, FAIL(DNS_R_NOTAUTHORITATIVE); } - result = dns_message_checksig(msg, dns_zone_getview(xfr->zone)); + result = dns_message_checksig(msg, xfr->view); if (result != ISC_R_SUCCESS) { xfrin_log(xfr, ISC_LOG_DEBUG(3), "TSIG check failed: %s", isc_result_totext(result)); @@ -1468,11 +1454,8 @@ xfrin_recv_done(isc_nmhandle_t *handle, isc_result_t result, /* * Read the next message. */ - /* The readhandle is still attached */ - /* The recv_xfr is still attached */ dns_message_detach(&msg); - isc_refcount_increment0(&xfr->recvs); - isc_nm_read(xfr->handle, xfrin_recv_done, xfr); + dns_dispatch_getnext(xfr->dispentry); return; } @@ -1484,24 +1467,18 @@ failure: if (msg != NULL) { dns_message_detach(&msg); } - isc_nmhandle_detach(&xfr->readhandle); - dns_xfrin_detach(&xfr); /* recv_xfr */ + dns_xfrin_detach(&xfr); } static void xfrin_destroy(dns_xfrin_t *xfr) { - uint64_t msecs; - uint64_t persec; - const char *result_str; + uint64_t msecs, persec; REQUIRE(VALID_XFRIN(xfr)); /* Safe-guards */ REQUIRE(atomic_load(&xfr->shuttingdown)); isc_refcount_destroy(&xfr->references); - isc_refcount_destroy(&xfr->connects); - isc_refcount_destroy(&xfr->recvs); - isc_refcount_destroy(&xfr->sends); INSIST(xfr->shutdown_result != ISC_R_UNSET); @@ -1510,8 +1487,8 @@ xfrin_destroy(dns_xfrin_t *xfr) { * shutting down, we can't know what the transfer status is as * we are only called when the last reference is lost. */ - result_str = isc_result_totext(xfr->shutdown_result); - xfrin_log(xfr, ISC_LOG_INFO, "Transfer status: %s", result_str); + xfrin_log(xfr, ISC_LOG_INFO, "Transfer status: %s", + isc_result_totext(xfr->shutdown_result)); /* * Calculate the length of time the transfer took, @@ -1531,11 +1508,11 @@ xfrin_destroy(dns_xfrin_t *xfr) { (unsigned int)(msecs / 1000), (unsigned int)(msecs % 1000), (unsigned int)persec, xfr->end_serial); - if (xfr->readhandle != NULL) { - isc_nmhandle_detach(&xfr->readhandle); + if (xfr->dispentry != NULL) { + dns_dispatch_done(&xfr->dispentry); } - if (xfr->sendhandle != NULL) { - isc_nmhandle_detach(&xfr->sendhandle); + if (xfr->disp != NULL) { + dns_dispatch_detach(&xfr->disp); } if (xfr->transport != NULL) { @@ -1591,6 +1568,10 @@ xfrin_destroy(dns_xfrin_t *xfr) { dns_zone_idetach(&xfr->zone); } + if (xfr->view != NULL) { + dns_view_weakdetach(&xfr->view); + } + if (xfr->firstsoa_data != NULL) { isc_mem_free(xfr->mctx, xfr->firstsoa_data); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index af3bac270c..449b8485bc 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -17537,7 +17537,7 @@ got_transfer_quota(void *arg) { CHECK(dns_xfrin_create(zone, xfrtype, &primaryaddr, &sourceaddr, zone->tsigkey, zone->transport, zone->zmgr->tlsctx_cache, zone->mctx, - zone->zmgr->netmgr, zone_xfrdone, &zone->xfr)); + zone_xfrdone, &zone->xfr)); LOCK_ZONE(zone); if (xfrtype == dns_rdatatype_axfr) { if (isc_sockaddr_pf(&primaryaddr) == PF_INET) { From 4e93d44c743b903149c7cb80410f5010c99b15a3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 22 Feb 2023 11:04:12 -0800 Subject: [PATCH 7/8] fix a bug in dns_dispatch_getnext() when a message arrives over a TCP connection matching an expected QID, the dispatch is updated so it no longer expects that QID, but continues reading. subsequent messages with the same QID are ignored, unless the dispatch entry has called dns_dispatch_getnext() or dns_dispatch_resume(). however, a coding error caused those functions to have no effect when the dispatch was reading, so streams of messages with the same QID could not be received over a single TCP connection, breaking *XFR. this has been corrected by changing the order of operations in tcp_dispatch_getnext() so that disp->reading isn't checked until after the dispatch entry has been reactivated. --- lib/dns/dispatch.c | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 9e16b5682b..65a3ba77a8 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -531,7 +531,7 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, isc_sockaddr_t peer; isc_netaddr_t netaddr; int match, timeout = 0; - dispatch_cb_t response = NULL; + bool respond = true; REQUIRE(VALID_RESPONSE(resp)); REQUIRE(VALID_DISPATCH(resp->disp)); @@ -542,15 +542,13 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, INSIST(resp->reading); resp->reading = false; - response = resp->response; - if (resp->state == DNS_DISPATCHSTATE_CANCELED) { /* * Nobody is interested in the callback if the response * has been canceled already. Detach from the response * and the handle. */ - response = NULL; + respond = false; eresult = ISC_R_CANCELED; } @@ -648,16 +646,16 @@ next: * Do not invoke the read callback just yet and instead wait for the * proper response to arrive until the original timeout fires. */ - response = NULL; + respond = false; udp_dispatch_getnext(resp, timeout); done: UNLOCK(&disp->lock); - if (response != NULL) { + if (respond) { dispentry_log(resp, LVL(90), "UDP read callback on %p: %s", handle, isc_result_totext(eresult)); - response(eresult, region, resp->arg); + resp->response(eresult, region, resp->arg); } dns_dispentry_detach(&resp); /* DISPENTRY003 */ @@ -725,7 +723,10 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region, dns_qid_t *qid, if (resp->reading) { *respp = resp; } else { - /* We already got our DNS message. */ + /* + * We already got a message for this QID and weren't + * expecting any more. + */ result = ISC_R_UNEXPECTED; } } else { @@ -1576,6 +1577,8 @@ dns_dispatch_getnext(dns_dispentry_t *resp) { isc_result_t result = ISC_R_SUCCESS; int32_t timeout = -1; + dispentry_log(resp, LVL(90), "getnext for QID %d", resp->id); + LOCK(&disp->lock); switch (disp->socktype) { case isc_socktype_udp: { @@ -1608,7 +1611,7 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) { dns_dispatch_t *disp = resp->disp; dns_dispatchmgr_t *mgr = disp->mgr; dns_qid_t *qid = mgr->qid; - dispatch_cb_t response = NULL; + bool respond = false; LOCK(&disp->lock); dispentry_log(resp, LVL(90), @@ -1633,9 +1636,7 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) { case DNS_DISPATCHSTATE_CONNECTED: if (resp->reading) { - dns_dispentry_ref(resp); /* DISPENTRY003 */ - response = resp->response; - + respond = true; dispentry_log(resp, LVL(90), "canceling read on %p", resp->handle); isc_nm_cancelread(resp->handle); @@ -1659,11 +1660,10 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) { unlock: UNLOCK(&disp->lock); - if (response) { + if (respond) { dispentry_log(resp, LVL(90), "read callback: %s", isc_result_totext(result)); - response(result, NULL, resp->arg); - dns_dispentry_detach(&resp); /* DISPENTRY003 */ + resp->response(result, NULL, resp->arg); } } @@ -2117,6 +2117,13 @@ tcp_dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp, int32_t timeout) { REQUIRE(timeout <= INT16_MAX); + dispentry_log(resp, LVL(90), "continue reading"); + + if (!resp->reading) { + ISC_LIST_APPEND(disp->active, resp, alink); + resp->reading = true; + } + if (disp->reading) { return; } @@ -2125,14 +2132,9 @@ tcp_dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp, isc_nmhandle_settimeout(disp->handle, timeout); } - dispentry_log(resp, LVL(90), "continue reading"); - dns_dispatch_ref(disp); /* DISPATCH002 */ isc_nm_read(disp->handle, tcp_recv, disp); disp->reading = true; - - ISC_LIST_APPEND(disp->active, resp, alink); - resp->reading = true; } static void @@ -2161,6 +2163,8 @@ dns_dispatch_resume(dns_dispentry_t *resp, uint16_t timeout) { dns_dispatch_t *disp = resp->disp; + dispentry_log(resp, LVL(90), "resume"); + LOCK(&disp->lock); switch (disp->socktype) { case isc_socktype_udp: { From 55f00de18e4e7d855a93b5112f22848f62b3a7f0 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 21 Feb 2023 22:00:52 -0800 Subject: [PATCH 8/8] CHANGES for [GL #3886] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 620b8b886d..28d75d62e6 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6110. [cleanup] Refactor the dns_xfrin module to use dns_dispatch + to set up TCP connections and send and receive + messages. [GL #3886] + 6109. [func] Infrastructure for QSBR, asynchronous safe memory reclamation for lock-free data structures. [GL !7471]