From dbd308436f83b41033b7fc855d671f9874d1469c Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 2 Oct 2024 16:22:38 +0000 Subject: [PATCH 1/2] Fix error path bugs in the "recursing-clients" list management In two places, after linking the client to the manager's "recursing-clients" list using the check_recursionquota() function, the query.c module fails to unlink it on error paths. Fix the bugs by unlinking the client from the list. Also make sure that unlinking happens before detaching the client's handle, as it is the logically correct order, e.g. in case if it's the last handle and ns__client_reset_cb() can be called because of the detachment. (cherry picked from commit 36c4808903088fd547a09ddd79c35169387d4581) --- lib/ns/query.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 478eb58bd7..7aa1afaf1c 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6569,12 +6569,20 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, 0, NULL, client->manager->loop, fetch_callback, client, rdataset, sigrdataset, &FETCH_RECTYPE_NORMAL(client)); if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&HANDLE_RECTYPE_NORMAL(client)); + recursionquotatype_detach(client); + + LOCK(&client->manager->reclock); + if (ISC_LINK_LINKED(client, rlink)) { + ISC_LIST_UNLINK(client->manager->recursing, client, rlink); + } + UNLOCK(&client->manager->reclock); + ns_client_putrdataset(client, &rdataset); if (sigrdataset != NULL) { ns_client_putrdataset(client, &sigrdataset); } - recursionquotatype_detach(client); + + isc_nmhandle_detach(&HANDLE_RECTYPE_NORMAL(client)); } /* @@ -6965,6 +6973,12 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, cleanup_and_detach_from_quota: recursionquotatype_detach(client); + + LOCK(&client->manager->reclock); + if (ISC_LINK_LINKED(client, rlink)) { + ISC_LIST_UNLINK(client->manager->recursing, client, rlink); + } + UNLOCK(&client->manager->reclock); cleanup: /* * If we fail, send SERVFAIL now. It may be better to let the caller From 3b76aa01ba189fdcade7033acd2a47b7181354a5 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 2 Oct 2024 16:39:13 +0000 Subject: [PATCH 2/2] Refactor the way check_recursionquota() is used Rename check_recursionquota() to acquire_recursionquota(), and implement a new function called release_recursionquota() to reverse the action. It helps with decreasing code duplication. (cherry picked from commit 7bd44a4182eb38592f05380462d7f80eb649ff96) --- lib/ns/query.c | 62 +++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 7aa1afaf1c..a7825eeedc 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -240,6 +240,12 @@ query_addanswer(query_ctx_t *qctx); static isc_result_t query_prepare_delegation_response(query_ctx_t *qctx); +static isc_result_t +acquire_recursionquota(ns_client_t *client); + +static void +release_recursionquota(ns_client_t *client); + /* * Return the hooktable in use with 'qctx', or if there isn't one * set, return the default hooktable. @@ -6350,14 +6356,7 @@ fetch_callback(void *arg) { * We're done recursing, detach from quota and unlink from * the manager's recursing-clients list. */ - - recursionquotatype_detach(client); - - LOCK(&client->manager->reclock); - if (ISC_LINK_LINKED(client, rlink)) { - ISC_LIST_UNLINK(client->manager->recursing, client, rlink); - } - UNLOCK(&client->manager->reclock); + release_recursionquota(client); isc_nmhandle_detach(&HANDLE_RECTYPE_NORMAL(client)); @@ -6459,6 +6458,7 @@ recparam_update(ns_query_recparam_t *param, dns_rdatatype_t qtype, dns_name_copy(qdomain, param->qdomain); } } + static void recursionquota_log(ns_client_t *client, atomic_uint_fast32_t *last_log_time, const char *format, isc_quota_t *quota) { @@ -6476,10 +6476,10 @@ recursionquota_log(ns_client_t *client, atomic_uint_fast32_t *last_log_time, static atomic_uint_fast32_t last_soft, last_hard; /*% - * Check recursion quota before making the current client "recursing". + * Acquire recursion quota before making the current client "recursing". */ static isc_result_t -check_recursionquota(ns_client_t *client) { +acquire_recursionquota(ns_client_t *client) { isc_result_t result; result = recursionquotatype_attach_soft(client); @@ -6509,6 +6509,20 @@ check_recursionquota(ns_client_t *client) { return (ISC_R_SUCCESS); } +/*% + * Release recursion quota and remove the client from the "recursing" list. + */ +static void +release_recursionquota(ns_client_t *client) { + recursionquotatype_detach(client); + + LOCK(&client->manager->reclock); + if (ISC_LINK_LINKED(client, rlink)) { + ISC_LIST_UNLINK(client->manager->recursing, client, rlink); + } + UNLOCK(&client->manager->reclock); +} + isc_result_t ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, dns_name_t *qdomain, dns_rdataset_t *nameservers, @@ -6535,7 +6549,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, inc_stats(client, ns_statscounter_recursion); } - result = check_recursionquota(client); + result = acquire_recursionquota(client); if (result != ISC_R_SUCCESS) { return (result); } @@ -6569,13 +6583,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, 0, NULL, client->manager->loop, fetch_callback, client, rdataset, sigrdataset, &FETCH_RECTYPE_NORMAL(client)); if (result != ISC_R_SUCCESS) { - recursionquotatype_detach(client); - - LOCK(&client->manager->reclock); - if (ISC_LINK_LINKED(client, rlink)) { - ISC_LIST_UNLINK(client->manager->recursing, client, rlink); - } - UNLOCK(&client->manager->reclock); + release_recursionquota(client); ns_client_putrdataset(client, &rdataset); if (sigrdataset != NULL) { @@ -6812,13 +6820,7 @@ query_hookresume(void *arg) { UNLOCK(&client->query.fetchlock); SAVE(hctx, rev->ctx); - recursionquotatype_detach(client); - - LOCK(&client->manager->reclock); - if (ISC_LINK_LINKED(client, rlink)) { - ISC_LIST_UNLINK(client->manager->recursing, client, rlink); - } - UNLOCK(&client->manager->reclock); + release_recursionquota(client); /* * The fetch handle should be detached before resuming query processing @@ -6943,7 +6945,7 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, REQUIRE(client->query.hookactx == NULL); REQUIRE(FETCH_RECTYPE_NORMAL(client) == NULL); - result = check_recursionquota(client); + result = acquire_recursionquota(client); if (result != ISC_R_SUCCESS) { goto cleanup; } @@ -6972,13 +6974,7 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, return (ISC_R_SUCCESS); cleanup_and_detach_from_quota: - recursionquotatype_detach(client); - - LOCK(&client->manager->reclock); - if (ISC_LINK_LINKED(client, rlink)) { - ISC_LIST_UNLINK(client->manager->recursing, client, rlink); - } - UNLOCK(&client->manager->reclock); + release_recursionquota(client); cleanup: /* * If we fail, send SERVFAIL now. It may be better to let the caller