From b6d40b3c4e4452cee6f7f06d889e4e8d113e9588 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 9 Feb 2022 14:56:04 -0800 Subject: [PATCH] correct TCP error handling in dispatch and resolver - certain TCP result codes, including ISC_R_EOF and ISC_R_CONNECTIONRESET, were being mapped to ISC_R_SHUTTINGDOWN before calling the response handler in tcp_recv_cancelall(). the result codes should be passed through to the response handler without being changed. - the response handlers, resquery_response() and req_response(), had code to return immediately if encountering ISC_R_EOF, but this is not the correct behavior; that should only happen in the case of ISC_R_CANCELED when it was the caller that canceled the operation - ISC_R_CONNECTIONRESET was not being caught in rctx_dispfail(). - removed code in rctx_dispfail() to retry queries without EDNS when receiving ISC_R_EOF; this is now treated the same as any other connection failure. --- lib/dns/dispatch.c | 7 +++--- lib/dns/request.c | 2 +- lib/dns/resolver.c | 61 +++++++++++++++++++++------------------------- 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index c13d91d064..81e2f0658d 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -713,13 +713,14 @@ tcp_recv_done(dns_dispentry_t *resp, isc_result_t eresult, } static void -tcp_recv_cancelall(dns_displist_t *resps, isc_region_t *region) { +tcp_recv_cancelall(dns_displist_t *resps, isc_region_t *region, + isc_result_t result) { dns_dispentry_t *resp = NULL, *next = NULL; for (resp = ISC_LIST_HEAD(*resps); resp != NULL; resp = next) { next = ISC_LIST_NEXT(resp, rlink); ISC_LIST_UNLINK(*resps, resp, rlink); - resp->response(ISC_R_SHUTTINGDOWN, region, resp->arg); + resp->response(result, region, resp->arg); dispentry_detach(&resp); } } @@ -831,7 +832,7 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, break; default: /* We're being shut down; cancel all outstanding resps. */ - tcp_recv_cancelall(&resps, region); + tcp_recv_cancelall(&resps, region, result); } dns_dispatch_detach(&disp); diff --git a/lib/dns/request.c b/lib/dns/request.c index b4348e469c..bd268a7717 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -1058,7 +1058,7 @@ req_response(isc_result_t result, isc_region_t *region, void *arg) { req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request, isc_result_totext(result)); - if (result == ISC_R_CANCELED || result == ISC_R_EOF) { + if (result == ISC_R_CANCELED) { return; } diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index b558485157..9610608fb8 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -7411,7 +7411,7 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { fetchctx_t *fctx = NULL; respctx_t rctx; - if (eresult == ISC_R_CANCELED || eresult == ISC_R_EOF) { + if (eresult == ISC_R_CANCELED) { return; } @@ -7886,45 +7886,40 @@ rctx_answer_init(respctx_t *rctx) { static isc_result_t rctx_dispfail(respctx_t *rctx) { fetchctx_t *fctx = rctx->fctx; - resquery_t *query = rctx->query; if (rctx->result == ISC_R_SUCCESS) { return (ISC_R_SUCCESS); } - if (rctx->result == ISC_R_EOF && - (rctx->retryopts & DNS_FETCHOPT_NOEDNS0) == 0) { - /* - * The problem might be that they don't understand - * EDNS0. Turn it off and try again. - */ - rctx->retryopts |= DNS_FETCHOPT_NOEDNS0; - rctx->resend = true; - add_bad_edns(fctx, &query->addrinfo->sockaddr); - } else { - /* - * There's no hope for this response. - */ - rctx->next_server = true; + /* + * There's no hope for this response. + */ + rctx->next_server = true; - /* - * If this is a network error, mark the server as bad so - * that we won't try it for this fetch again. Also - * adjust finish and no_response so that we penalize - * this address in SRTT adjustment later. - */ - if (rctx->result == ISC_R_HOSTUNREACH || - rctx->result == ISC_R_NETUNREACH || - rctx->result == ISC_R_CONNREFUSED || - rctx->result == ISC_R_CANCELED || - rctx->result == ISC_R_SHUTTINGDOWN) - { - rctx->broken_server = rctx->result; - rctx->broken_type = badns_unreachable; - rctx->finish = NULL; - rctx->no_response = true; - } + /* + * If this is a network failure, the operation is cancelled, + * or the network manager is being shut down, we mark the server + * as bad so that we won't try it for this fetch again. Also + * adjust finish and no_response so that we penalize this + * address in SRTT adjustments later. + */ + switch (rctx->result) { + case ISC_R_EOF: + case ISC_R_HOSTUNREACH: + case ISC_R_NETUNREACH: + case ISC_R_CONNREFUSED: + case ISC_R_CONNECTIONRESET: + case ISC_R_CANCELED: + case ISC_R_SHUTTINGDOWN: + rctx->broken_server = rctx->result; + rctx->broken_type = badns_unreachable; + rctx->finish = NULL; + rctx->no_response = true; + break; + default: + break; } + FCTXTRACE3("dispatcher failure", rctx->result); rctx_done(rctx, ISC_R_SUCCESS); return (ISC_R_COMPLETE);