From ccee3507c26b5727f6e6c5788f9e7d124dc16b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH] Check recursions at the end of request processing ns_client_endrequest() currently contains code that looks for outstanding quota references and cleans them up if necessary. This approach masks programming errors because ns_client_endrequest() is only called from ns__client_reset_cb(), which in turn is only called when all references to the client's netmgr handle are released, which in turn only happens after all recursion completion callbacks are invoked (because isc_nmhandle_attach() is called before every call to dns_resolver_createfetch() in lib/ns/query.c and the completion callback is expected to detach from the handle), which in turn is expected to happen for all recursions attempts, even those that get canceled. Furthermore, declaring the prototype of ns_client_endrequest() at the top of lib/ns/client.c is redundant because the definition of that function is placed before its first use in that file. Remove the redundant function prototype. Finally, remove INSIST assertions ensuring quota pointers are NULL in ns__client_reset_cb() because the latter calls ns_client_endrequest() a few lines earlier. --- lib/ns/client.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/lib/ns/client.c b/lib/ns/client.c index ebfcc77ba9..c6b268b974 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -124,8 +124,6 @@ clientmgr_detach(ns_clientmgr_t **mp); static void clientmgr_destroy(ns_clientmgr_t *manager); static void -ns_client_endrequest(ns_client_t *client); -static void ns_client_dumpmessage(ns_client_t *client, const char *reason); static void compute_cookie(ns_client_t *client, uint32_t when, uint32_t nonce, @@ -263,16 +261,10 @@ ns_client_endrequest(ns_client_t *client) { dns_message_reset(client->message, DNS_MESSAGE_INTENTPARSE); /* - * Clean up from recursion - normally this would be done in - * fetch_callback(), but if we're shutting down and canceling then - * it might not have happened. + * Ensure there are no recursions that still need to be cleaned up. */ for (int i = 0; i < RECTYPE_COUNT; i++) { - if (client->query.recursions[i].quota != NULL) { - isc_quota_detach(&client->query.recursions[i].quota); - ns_stats_decrement(client->manager->sctx->nsstats, - ns_statscounter_recursclients); - } + INSIST(client->query.recursions[i].quota == NULL); } /* @@ -1646,9 +1638,6 @@ ns__client_reset_cb(void *client0) { } client->state = NS_CLIENTSTATE_READY; - for (int i = 0; i < RECTYPE_COUNT; i++) { - INSIST(client->query.recursions[i].quota == NULL); - } #ifdef WANT_SINGLETRACE isc_log_setforcelog(false);