From 5948aa77669b60e93d90b268c3aea28af1c437c1 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 3 Oct 2021 13:58:41 -0700 Subject: [PATCH 1/4] clarify fctx_cancelquery() behavior Cleaned up dereferencing of query objects, and added a comment explaining it better. --- lib/dns/resolver.c | 75 +++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 395aa4f92b..4ca5d8ab54 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -1214,8 +1214,9 @@ update_edns_stats(resquery_t *query) { } static void -fctx_cancelquery(resquery_t *query, isc_time_t *finish, bool no_response, +fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response, bool age_untried) { + resquery_t *query = NULL; fetchctx_t *fctx = NULL; unsigned int rtt, rttms; unsigned int factor; @@ -1223,6 +1224,9 @@ fctx_cancelquery(resquery_t *query, isc_time_t *finish, bool no_response, dns_adbaddrinfo_t *addrinfo; isc_stdtime_t now; + REQUIRE(queryp != NULL); + + query = *queryp; fctx = query->fctx; FCTXTRACE("cancelquery"); @@ -1401,7 +1405,7 @@ fctx_cancelquery(resquery_t *query, isc_time_t *finish, bool no_response, ISC_LIST_UNLINK(fctx->queries, query, link); } - resquery_detach(&query); + resquery_detach(queryp); } static void @@ -1451,7 +1455,7 @@ fctx_cancelqueries(fetchctx_t *fctx, bool no_response, bool age_untried) { for (query = ISC_LIST_HEAD(fctx->queries); query != NULL; query = next_query) { next_query = ISC_LIST_NEXT(query, link); - fctx_cancelquery(query, NULL, no_response, age_untried); + fctx_cancelquery(&query, NULL, no_response, age_untried); } } @@ -1725,6 +1729,7 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) { static void resquery_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { resquery_t *query = (resquery_t *)arg; + resquery_t *copy = query; fetchctx_t *fctx = NULL; QTRACE("senddone"); @@ -1737,6 +1742,10 @@ resquery_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { goto detach; } + /* + * See the note in resquery_connected() about reference + * counting on error conditions. + */ switch (eresult) { case ISC_R_SUCCESS: case ISC_R_CANCELED: @@ -1748,17 +1757,13 @@ resquery_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { case ISC_R_NOPERM: case ISC_R_ADDRNOTAVAIL: case ISC_R_CONNREFUSED: + /* No route to remote. */ FCTXTRACE3("query canceled in resquery_senddone(): " "no route to host; no response", eresult); - - /* - * No route to remote. - */ add_bad(fctx, query->rmessage, query->addrinfo, eresult, badns_unreachable); - fctx_cancelquery(query, NULL, true, false); - + fctx_cancelquery(©, NULL, true, false); FCTX_ATTR_CLR(fctx, FCTX_ATTR_ADDRWAIT); fctx_try(fctx, true, false); break; @@ -1767,7 +1772,7 @@ resquery_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { FCTXTRACE3("query canceled in resquery_senddone() " "due to unexpected result; responding", eresult); - fctx_cancelquery(query, NULL, false, false); + fctx_cancelquery(©, NULL, false, false); fctx_done(fctx, eresult, __LINE__); break; } @@ -2744,6 +2749,7 @@ cleanup_temps: static void resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { resquery_t *query = (resquery_t *)arg; + resquery_t *copy = query; isc_result_t result; fetchctx_t *fctx = NULL; dns_resolver_t *res = NULL; @@ -2766,6 +2772,22 @@ resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { eresult = ISC_R_SHUTTINGDOWN; } + /* + * The reference counting of resquery objects is complex: + * + * 1. attached in fctx_query() + * 2. attached prior to dns_dispatch_connect(), detached in + * resquery_connected() + * 3. attached prior to dns_dispatch_send(), detached in + * resquery_senddone() + * 4. finally detached in fctx_cancelquery() + * + * On error conditions, it's necessary to call fctx_cancelquery() + * from resquery_connected() or _senddone(), detaching twice + * within the same function. To make it clear that's what's + * happening, we cancel-and-detach 'copy' and detach 'query', + * which are both pointing to the same object. + */ switch (eresult) { case ISC_R_SUCCESS: /* @@ -2777,7 +2799,7 @@ resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { FCTXTRACE("query canceled: resquery_send() failed; " "responding"); - fctx_cancelquery(query, NULL, false, false); + fctx_cancelquery(©, NULL, false, false); fctx_done(fctx, result, __LINE__); break; } @@ -2797,12 +2819,9 @@ resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { break; case ISC_R_CANCELED: - break; - case ISC_R_SHUTTINGDOWN: - FCTXTRACE3("shutdown in resquery_connected(): no response", - eresult); - fctx_cancelquery(query, NULL, true, false); + FCTXTRACE3("shutdown in resquery_connected()", eresult); + fctx_cancelquery(©, NULL, true, false); fctx_done(fctx, eresult, __LINE__); break; @@ -2813,31 +2832,26 @@ resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { case ISC_R_ADDRNOTAVAIL: case ISC_R_CONNECTIONRESET: case ISC_R_TIMEDOUT: - FCTXTRACE3("query canceled in " - "resquery_connected(): " - "no route to host; no response", - eresult); - /* - * Do not query this server again in this fetch - * context if the server is unavailable over - * TCP. + * Do not query this server again in this fetch context. */ + FCTXTRACE3("query failed in resquery_connected(): " + "no response", + eresult); add_bad(fctx, query->rmessage, query->addrinfo, eresult, badns_unreachable); - fctx_cancelquery(query, NULL, true, false); + fctx_cancelquery(©, NULL, true, false); FCTX_ATTR_CLR(fctx, FCTX_ATTR_ADDRWAIT); fctx_try(fctx, true, false); break; default: - FCTXTRACE3("query canceled in " - "resquery_connected() " + FCTXTRACE3("query canceled in resquery_connected() " "due to unexpected result; responding", eresult); - fctx_cancelquery(query, NULL, false, false); + fctx_cancelquery(©, NULL, false, false); fctx_done(fctx, eresult, __LINE__); break; } @@ -9421,7 +9435,8 @@ rctx_done(respctx_t *rctx, isc_result_t result) { if (dns_fuzzing_resolver && (rctx->next_server || rctx->resend || rctx->nextitem)) { - fctx_cancelquery(query, rctx->finish, rctx->no_response, false); + fctx_cancelquery(&query, rctx->finish, rctx->no_response, + false); fctx_done(fctx, DNS_R_SERVFAIL, __LINE__); goto detach; } @@ -9438,7 +9453,7 @@ rctx_done(respctx_t *rctx, isc_result_t result) { } /* Cancel the query */ - fctx_cancelquery(query, rctx->finish, rctx->no_response, false); + fctx_cancelquery(&query, rctx->finish, rctx->no_response, false); /* * If nobody's waiting for results, don't resend. From 2653800e0baac026310893173c04695cd47dd8a7 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 3 Oct 2021 17:24:35 -0700 Subject: [PATCH 2/4] simplify sending request events The function send_if_done() was just a front-end for req_sendevents(). --- lib/dns/request.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/lib/dns/request.c b/lib/dns/request.c index 9214225e04..2085575661 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -891,21 +891,6 @@ cleanup: return (result); } -/* - * If this request is no longer waiting for events, - * send the completion event. This will ultimately - * cause the request to be destroyed. - * - * Requires: - * 'request' is locked by the caller. - */ -static void -send_if_done(dns_request_t *request, isc_result_t result) { - if (request->event != NULL) { - req_sendevent(request, result); - } -} - void request_cancel(dns_request_t *request) { if (!DNS_REQUEST_CANCELED(request)) { @@ -931,7 +916,7 @@ dns_request_cancel(dns_request_t *request) { req_log(ISC_LOG_DEBUG(3), "dns_request_cancel: request %p", request); LOCK(&request->requestmgr->locks[request->hash]); request_cancel(request); - send_if_done(request, ISC_R_CANCELED); + req_sendevent(request, ISC_R_CANCELED); UNLOCK(&request->requestmgr->locks[request->hash]); } @@ -1027,14 +1012,14 @@ req_connected(isc_result_t eresult, isc_region_t *region, void *arg) { if (eresult == ISC_R_TIMEDOUT) { dns_dispatch_removeresponse(&request->dispentry); dns_dispatch_detach(&request->dispatch); - send_if_done(request, eresult); + req_sendevent(request, eresult); } else if (DNS_REQUEST_CANCELED(request)) { - send_if_done(request, ISC_R_CANCELED); + req_sendevent(request, ISC_R_CANCELED); } else if (eresult == ISC_R_SUCCESS) { req_send(request); } else { request_cancel(request); - send_if_done(request, ISC_R_CANCELED); + req_sendevent(request, ISC_R_CANCELED); } UNLOCK(&request->requestmgr->locks[request->hash]); @@ -1057,13 +1042,13 @@ req_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { if (DNS_REQUEST_CANCELED(request)) { if (eresult == ISC_R_TIMEDOUT) { - send_if_done(request, eresult); + req_sendevent(request, eresult); } else { - send_if_done(request, ISC_R_CANCELED); + req_sendevent(request, ISC_R_CANCELED); } } else if (eresult != ISC_R_SUCCESS) { request_cancel(request); - send_if_done(request, ISC_R_CANCELED); + req_sendevent(request, ISC_R_CANCELED); } UNLOCK(&request->requestmgr->locks[request->hash]); @@ -1125,16 +1110,20 @@ done: /* * Send completion event. */ - send_if_done(request, result); + req_sendevent(request, result); UNLOCK(&request->requestmgr->locks[request->hash]); } static void req_sendevent(dns_request_t *request, isc_result_t result) { - isc_task_t *task; + isc_task_t *task = NULL; REQUIRE(VALID_REQUEST(request)); + if (request->event == NULL) { + return; + } + req_log(ISC_LOG_DEBUG(3), "req_sendevent: request %p", request); /* From 24dbf9849e94a31f56073d8b4043bb08ef519bc9 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 3 Oct 2021 15:15:50 -0700 Subject: [PATCH 3/4] refactor dispatch cancellation Renamed some functions for clarity and readability: - dns_dispatch_addresponse() -> dns_dispatch_add() - dns_dispatch_removeresponse() -> dns_dispatch_done() The dns_dispatch_cancel() function now calls dns_dispatch_done() directly, so it is no longer ever necessary to call both functions. dns_dispatch_cancel() is used to terminate dispatch connections that are still pending, while dns_dispatch_done() is used when they are complete. --- lib/dns/dispatch.c | 109 +++++++++++++++++++-------------- lib/dns/include/dns/dispatch.h | 44 +++++++------ lib/dns/request.c | 24 +++----- lib/dns/resolver.c | 15 +++-- lib/dns/tests/dispatch_test.c | 42 ++++++------- 5 files changed, 126 insertions(+), 108 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 41751ed890..c702656431 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -1288,11 +1288,11 @@ dns_dispatch_detach(dns_dispatch_t **dispp) { } isc_result_t -dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, - unsigned int timeout, const isc_sockaddr_t *dest, - dispatch_cb_t connected, dispatch_cb_t sent, - dispatch_cb_t response, void *arg, - dns_messageid_t *idp, dns_dispentry_t **resp) { +dns_dispatch_add(dns_dispatch_t *disp, unsigned int options, + unsigned int timeout, const isc_sockaddr_t *dest, + dispatch_cb_t connected, dispatch_cb_t sent, + dispatch_cb_t response, void *arg, dns_messageid_t *idp, + dns_dispentry_t **resp) { dns_dispentry_t *res = NULL; dns_qid_t *qid = NULL; in_port_t localport = 0; @@ -1476,7 +1476,65 @@ dns_dispatch_getnext(dns_dispentry_t *resp) { } void -dns_dispatch_removeresponse(dns_dispentry_t **respp) { +dns_dispatch_cancel(dns_dispentry_t **respp) { + dns_dispentry_t *resp = NULL; + + REQUIRE(respp != NULL); + + resp = *respp; + + REQUIRE(VALID_RESPONSE(resp)); + + resp->canceled = true; + + /* Connected UDP. */ + if (resp->handle != NULL) { + isc_nm_cancelread(resp->handle); + goto done; + } + + /* TCP pending connection. */ + if (ISC_LINK_LINKED(resp, plink)) { + dns_dispentry_t *copy = resp; + + ISC_LIST_UNLINK(resp->disp->pending, resp, plink); + if (resp->connected != NULL) { + resp->connected(ISC_R_CANCELED, NULL, resp->arg); + } + + /* + * We need to detach twice if we were pending + * connection - once to take the place of the + * detach in tcp_connected() or udp_connected() + * that we won't reach, and again later in + * dns_dispatch_done(). + */ + dispentry_detach(©); + goto done; + } + + /* + * Connected TCP, or unconnected UDP. + * + * If TCP, we don't want to cancel the dispatch + * unless this is the last resp waiting. + */ + if (ISC_LINK_LINKED(resp, alink)) { + ISC_LIST_UNLINK(resp->disp->active, resp, alink); + if (ISC_LIST_EMPTY(resp->disp->active) && + resp->disp->handle != NULL) { + isc_nm_cancelread(resp->disp->handle); + } else if (resp->response != NULL) { + resp->response(ISC_R_CANCELED, NULL, resp->arg); + } + } + +done: + dns_dispatch_done(respp); +} + +void +dns_dispatch_done(dns_dispentry_t **respp) { dns_dispatchmgr_t *mgr = NULL; dns_dispatch_t *disp = NULL; dns_dispentry_t *resp = NULL; @@ -1762,45 +1820,6 @@ dns_dispatch_send(dns_dispentry_t *resp, isc_region_t *r, isc_dscp_t dscp) { isc_nm_send(handle, r, send_done, resp); } -void -dns_dispatch_cancel(dns_dispentry_t *resp) { - REQUIRE(VALID_RESPONSE(resp)); - - resp->canceled = true; - - /* Connected UDP */ - if (resp->handle != NULL) { - isc_nm_cancelread(resp->handle); - return; - } - - /* TCP pending connection. */ - if (ISC_LINK_LINKED(resp, plink)) { - ISC_LIST_UNLINK(resp->disp->pending, resp, plink); - if (resp->connected != NULL) { - resp->connected(ISC_R_CANCELED, NULL, resp->arg); - dispentry_detach(&resp); - } - return; - } - - /* - * Connected TCP, or unconnected UDP. - * - * If TCP, we don't want to cancel the dispatch - * unless this is the last resp waiting. - */ - if (ISC_LINK_LINKED(resp, alink)) { - ISC_LIST_UNLINK(resp->disp->active, resp, alink); - if (ISC_LIST_EMPTY(resp->disp->active) && - resp->disp->handle != NULL) { - isc_nm_cancelread(resp->disp->handle); - } else if (resp->response != NULL) { - resp->response(ISC_R_CANCELED, NULL, resp->arg); - } - } -} - isc_result_t dns_dispatch_getlocaladdress(dns_dispatch_t *disp, isc_sockaddr_t *addrp) { REQUIRE(VALID_DISPATCH(disp)); diff --git a/lib/dns/include/dns/dispatch.h b/lib/dns/include/dns/dispatch.h index 93f676f5f9..572fa95d19 100644 --- a/lib/dns/include/dns/dispatch.h +++ b/lib/dns/include/dns/dispatch.h @@ -25,7 +25,7 @@ * MP: * *\li All locking is performed internally to each dispatch. - * Restrictions apply to dns_dispatch_removeresponse(). + * Restrictions apply to dns_dispatch_done(). * * Reliability: * @@ -225,17 +225,7 @@ isc_result_t dns_dispatch_connect(dns_dispentry_t *resp); /*%< * Connect to the remote server configured in 'resp' and run the - * connect callback that was set up via dns_dispatch_addresponse(). - * - * Requires: - *\li 'resp' is valid. - */ - -void -dns_dispatch_cancel(dns_dispentry_t *resp); -/*%< - * Cancel pending connects in 'resp', by setting a flag so that - * a read is not started when the connect handler runs. + * connect callback that was set up via dns_dispatch_add(). * * Requires: *\li 'resp' is valid. @@ -274,11 +264,11 @@ typedef void (*dispatch_cb_t)(isc_result_t eresult, isc_region_t *region, void *cbarg); isc_result_t -dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, - unsigned int timeout, const isc_sockaddr_t *dest, - dispatch_cb_t connected, dispatch_cb_t sent, - dispatch_cb_t response, void *arg, - dns_messageid_t *idp, dns_dispentry_t **resp); +dns_dispatch_add(dns_dispatch_t *disp, unsigned int options, + unsigned int timeout, const isc_sockaddr_t *dest, + dispatch_cb_t connected, dispatch_cb_t sent, + dispatch_cb_t response, void *arg, dns_messageid_t *idp, + dns_dispentry_t **resp); /*%< * Add a response entry for this dispatch. * @@ -316,13 +306,27 @@ dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, */ void -dns_dispatch_removeresponse(dns_dispentry_t **resp); +dns_dispatch_done(dns_dispentry_t **respp); /*%< - * Stops the flow of responses for the provided id and destination. + * Disconnects a dispatch response entry from its dispatch and shuts it + * down. This is called when the dispatch is complete; use + * dns_dispatch_cancel() if it is still pending. * * Requires: *\li "resp" != NULL and "*resp" contain a value previously allocated - * by dns_dispatch_addresponse(); + * by dns_dispatch_add(); + */ + +void +dns_dispatch_cancel(dns_dispentry_t **respp); +/*%< + * Cancel all pending connects and reads in a dispatch entry, + * then call dns_dispatch_done(). This is used when the caller + * cancels a dispatch response before it has completed. + * + * Requires: + *\li "resp" != NULL and "*resp" contain a value previously allocated + * by dns_dispatch_add(); */ isc_result_t diff --git a/lib/dns/request.c b/lib/dns/request.c index 2085575661..0e3619d93a 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -559,10 +559,10 @@ again: } req_attach(request, &rclone); - result = dns_dispatch_addresponse( - request->dispatch, dispopt, request->timeout, destaddr, - req_connected, req_senddone, req_response, request, &id, - &request->dispentry); + result = dns_dispatch_add(request->dispatch, dispopt, request->timeout, + destaddr, req_connected, req_senddone, + req_response, request, &id, + &request->dispentry); if (result != ISC_R_SUCCESS) { if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) { newtcp = true; @@ -722,7 +722,7 @@ use_tcp: } req_attach(request, &rclone); - result = dns_dispatch_addresponse( + result = dns_dispatch_add( request->dispatch, 0, request->timeout, destaddr, req_connected, req_senddone, req_response, request, &id, &request->dispentry); if (result != ISC_R_SUCCESS) { @@ -737,7 +737,7 @@ use_tcp: */ req_detach(&rclone); dns_message_renderreset(message); - dns_dispatch_removeresponse(&request->dispentry); + dns_dispatch_done(&request->dispentry); dns_dispatch_detach(&request->dispatch); options |= DNS_REQUESTOPT_TCP; tcp = true; @@ -901,8 +901,7 @@ request_cancel(dns_request_t *request) { request->flags &= ~DNS_REQUEST_F_CONNECTING; if (request->dispentry != NULL) { - dns_dispatch_cancel(request->dispentry); - dns_dispatch_removeresponse(&request->dispentry); + dns_dispatch_cancel(&request->dispentry); } dns_dispatch_detach(&request->dispatch); @@ -977,8 +976,6 @@ dns_request_destroy(dns_request_t **requestp) { LOCK(&request->requestmgr->lock); LOCK(&request->requestmgr->locks[request->hash]); ISC_LIST_UNLINK(request->requestmgr->requests, request, link); - INSIST(!DNS_REQUEST_CONNECTING(request)); - INSIST(!DNS_REQUEST_SENDING(request)); UNLOCK(&request->requestmgr->locks[request->hash]); UNLOCK(&request->requestmgr->lock); @@ -986,7 +983,6 @@ dns_request_destroy(dns_request_t **requestp) { * These should have been cleaned up before the completion * event was sent. */ - INSIST(!ISC_LINK_LINKED(request, link)); INSIST(request->dispentry == NULL); INSIST(request->dispatch == NULL); @@ -1010,7 +1006,7 @@ req_connected(isc_result_t eresult, isc_region_t *region, void *arg) { request->flags &= ~DNS_REQUEST_F_CONNECTING; if (eresult == ISC_R_TIMEDOUT) { - dns_dispatch_removeresponse(&request->dispentry); + dns_dispatch_done(&request->dispentry); dns_dispatch_detach(&request->dispatch); req_sendevent(request, eresult); } else if (DNS_REQUEST_CANCELED(request)) { @@ -1103,7 +1099,7 @@ done: * Cleanup. */ if (request->dispentry != NULL) { - dns_dispatch_removeresponse(&request->dispentry); + dns_dispatch_done(&request->dispentry); } request_cancel(request); @@ -1192,7 +1188,7 @@ req_destroy(dns_request_t *request) { isc_event_free((isc_event_t **)&request->event); } if (request->dispentry != NULL) { - dns_dispatch_removeresponse(&request->dispentry); + dns_dispatch_done(&request->dispentry); } if (request->dispatch != NULL) { dns_dispatch_detach(&request->dispatch); diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 4ca5d8ab54..02c09aad34 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -1142,7 +1142,7 @@ resquery_destroy(resquery_t *query) { } if (query->dispentry != NULL) { - dns_dispatch_removeresponse(&query->dispentry); + dns_dispatch_done(&query->dispentry); } if (query->dispatch != NULL) { @@ -1229,12 +1229,12 @@ fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response, query = *queryp; fctx = query->fctx; - FCTXTRACE("cancelquery"); - if (RESQUERY_CANCELED(query)) { return; } + FCTXTRACE("cancelquery"); + query->attributes |= RESQUERY_ATTR_CANCELED; /* @@ -1397,8 +1397,7 @@ fctx_cancelquery(resquery_t **queryp, isc_time_t *finish, bool no_response, * exist, cancel them. */ if (query->dispentry != NULL) { - dns_dispatch_cancel(query->dispentry); - dns_dispatch_removeresponse(&query->dispentry); + dns_dispatch_cancel(&query->dispentry); } if (ISC_LINK_LINKED(query, link)) { @@ -2126,7 +2125,7 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, UNLOCK(&res->buckets[fctx->bucketnum].lock); /* Set up the dispatch and set the query ID */ - result = dns_dispatch_addresponse( + result = dns_dispatch_add( query->dispatch, 0, isc_interval_ms(&fctx->interval), &query->addrinfo->sockaddr, resquery_connected, resquery_senddone, resquery_response, query, &query->id, @@ -2733,7 +2732,7 @@ cleanup_message: /* * Stop the dispatcher from listening. */ - dns_dispatch_removeresponse(&query->dispentry); + dns_dispatch_done(&query->dispentry); cleanup_temps: if (qname != NULL) { @@ -4390,7 +4389,7 @@ fctx_doshutdown(isc_task_t *task, isc_event_t *event) { fctx_cleanup(fctx); LOCK(&res->buckets[bucketnum].lock); - fctx_decreference(fctx); + RUNTIME_CHECK(!fctx_decreference(fctx)); FCTX_ATTR_SET(fctx, FCTX_ATTR_SHUTTINGDOWN); diff --git a/lib/dns/tests/dispatch_test.c b/lib/dns/tests/dispatch_test.c index 8b64c82ce0..93eac9f5ae 100644 --- a/lib/dns/tests/dispatch_test.c +++ b/lib/dns/tests/dispatch_test.c @@ -456,10 +456,10 @@ dispatch_timeout_tcp_connect(void **state) { region.base = rbuf; region.length = sizeof(rbuf); - result = dns_dispatch_addresponse(dispatch, 0, T_CLIENT_CONNECT, - &tcp_server_addr, timeout_connected, - client_senddone, response, ®ion, - &id, &dispentry); + result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT, + &tcp_server_addr, timeout_connected, + client_senddone, response, ®ion, &id, + &dispentry); assert_int_equal(result, ISC_R_SUCCESS); memset(message, 0, sizeof(message)); @@ -473,7 +473,7 @@ dispatch_timeout_tcp_connect(void **state) { uv_sem_wait(&sem); - dns_dispatch_removeresponse(&dispentry); + dns_dispatch_done(&dispentry); dns_dispatch_detach(&dispatch); dns_dispatchmgr_detach(&dispatchmgr); @@ -517,9 +517,9 @@ dispatch_timeout_tcp_response(void **state __attribute__((unused))) { region.base = rbuf; region.length = sizeof(rbuf); - result = dns_dispatch_addresponse( - dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected, - client_senddone, response_timeout, ®ion, &id, &dispentry); + result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT, + &tcp_server_addr, connected, client_senddone, + response_timeout, ®ion, &id, &dispentry); assert_int_equal(result, ISC_R_SUCCESS); memset(message, 0, sizeof(message)); @@ -539,7 +539,7 @@ dispatch_timeout_tcp_response(void **state __attribute__((unused))) { isc_nmsocket_close(&sock); assert_null(sock); - dns_dispatch_removeresponse(&dispentry); + dns_dispatch_done(&dispentry); dns_dispatch_detach(&dispatch); dns_dispatchmgr_detach(&dispatchmgr); @@ -573,9 +573,9 @@ dispatch_tcp_response(void **state __attribute__((unused))) { region.base = rbuf; region.length = sizeof(rbuf); - result = dns_dispatch_addresponse( - dispatch, 0, T_CLIENT_CONNECT, &tcp_server_addr, connected, - client_senddone, response, ®ion, &id, &dispentry); + result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT, + &tcp_server_addr, connected, client_senddone, + response, ®ion, &id, &dispentry); assert_int_equal(result, ISC_R_SUCCESS); memset(message, 0, sizeof(message)); @@ -598,7 +598,7 @@ dispatch_tcp_response(void **state __attribute__((unused))) { isc_nmsocket_close(&sock); assert_null(sock); - dns_dispatch_removeresponse(&dispentry); + dns_dispatch_done(&dispentry); dns_dispatch_detach(&dispatch); dns_dispatchmgr_detach(&dispatchmgr); @@ -632,9 +632,9 @@ dispatch_timeout_udp_response(void **state __attribute__((unused))) { region.base = rbuf; region.length = sizeof(rbuf); - result = dns_dispatch_addresponse( - dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected, - client_senddone, response_timeout, ®ion, &id, &dispentry); + result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT, + &udp_server_addr, connected, client_senddone, + response_timeout, ®ion, &id, &dispentry); assert_int_equal(result, ISC_R_SUCCESS); memset(message, 0, sizeof(message)); @@ -654,7 +654,7 @@ dispatch_timeout_udp_response(void **state __attribute__((unused))) { isc_nmsocket_close(&sock); assert_null(sock); - dns_dispatch_removeresponse(&dispentry); + dns_dispatch_done(&dispentry); dns_dispatch_detach(&dispatch); dns_dispatchmgr_detach(&dispatchmgr); @@ -688,9 +688,9 @@ dispatch_getnext(void **state) { region.base = rbuf; region.length = sizeof(rbuf); - result = dns_dispatch_addresponse( - dispatch, 0, T_CLIENT_CONNECT, &udp_server_addr, connected, - client_senddone, response_getnext, ®ion, &id, &dispentry); + result = dns_dispatch_add(dispatch, 0, T_CLIENT_CONNECT, + &udp_server_addr, connected, client_senddone, + response_getnext, ®ion, &id, &dispentry); assert_int_equal(result, ISC_R_SUCCESS); memset(message, 0, sizeof(message)); @@ -711,7 +711,7 @@ dispatch_getnext(void **state) { isc_nmsocket_close(&sock); assert_null(sock); - dns_dispatch_removeresponse(&dispentry); + dns_dispatch_done(&dispentry); dns_dispatch_detach(&dispatch); dns_dispatchmgr_detach(&dispatchmgr); } From 436424c458f9f199e300c7834711d5e62dc37692 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 4 Oct 2021 17:07:22 -0700 Subject: [PATCH 4/4] increment fctx references while waiting for validator We need to ensure the fctx isn't freed while the validator is pending. --- lib/dns/resolver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 02c09aad34..828f732c47 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -916,6 +916,7 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo, INSIST(fctx->validator == NULL); fctx->validator = validator; } + fctx_increference(fctx); ISC_LIST_APPEND(fctx->validators, validator, link); } else { dns_message_detach(&valarg->message); @@ -5222,6 +5223,8 @@ validated(isc_task_t *task, isc_event_t *event) { LOCK(&res->buckets[bucketnum].lock); sentresponse = ((fctx->options & DNS_FETCHOPT_NOVALIDATE) != 0); + RUNTIME_CHECK(!fctx_decreference(fctx)); + /* * If shutting down, ignore the results. Check to see if we're * done waiting for validator completions and ADB pending @@ -5334,8 +5337,10 @@ validated(isc_task_t *task, isc_event_t *event) { result = fctx->vresult; add_bad(fctx, message, addrinfo, result, badns_validation); isc_event_free(&event); + UNLOCK(&res->buckets[bucketnum].lock); INSIST(fctx->validator == NULL); + fctx->validator = ISC_LIST_HEAD(fctx->validators); if (fctx->validator != NULL) { dns_validator_send(fctx->validator);