From 2ae5c4a674ef2d43c145c8486becc92aa26fff58 Mon Sep 17 00:00:00 2001
From: Aram Sargsyan <aram@isc.org>
Date: Sat, 27 May 2023 11:01:28 +0000
Subject: [PATCH] Fix a clients-per-query miscalculation bug

The number of clients per query is calculated using the pending
fetch responses in the list. The dns_resolver_createfetch() function
includes every item in the list when deciding whether the limit is
reached (i.e. fctx->spilled is true). Then, when the limit is reached,
there is another calculation in fctx_sendevents(), when deciding
whether it is needed to increase the limit, but this time the TRYSTALE
responses are not included in the calculation (because of early break
from the loop), and because of that the limit is never increased.

A single client can have more than one associated response/event in the
list (currently max. two), and calculating them as separate "clients"
is unexpected. E.g. if 'stale-answer-enable' is enabled and
'stale-answer-client-timeout' is enabled and is larger than 0, then
each client will have two events, which will effectively halve the
clients-per-query limit.

Fix the dns_resolver_createfetch() function to calculate only the
regular FETCHDONE responses/events.

Change the fctx_sendevents() function to also calculate only FETCHDONE
responses/events. Currently, this second change doesn't have any impact,
because the TRYSTALE events were already skipped, but having the same
condition in both places will help prevent similar bugs in the future
if a new type of response/event is ever added.
---
 lib/dns/resolver.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index ce0d7bc389..e7e3c3914d 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -1553,6 +1553,16 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {
 	for (resp = ISC_LIST_HEAD(fctx->resps); resp != NULL; resp = next) {
 		next = ISC_LIST_NEXT(resp, link);
 		ISC_LIST_UNLINK(fctx->resps, resp, link);
+
+		/*
+		 * Only the regular fetch events should be counted for the
+		 * clients-per-query limit, in case if there are multiple events
+		 * registered for a single client.
+		 */
+		if (resp->type == FETCHDONE) {
+			count++;
+		}
+
 		if (resp->type == TRYSTALE) {
 			/*
 			 * Not applicable to TRYSTALE resps; this function is
@@ -1586,7 +1596,6 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {
 
 		FCTXTRACE("post response event");
 		isc_async_run(resp->loop, resp->cb, resp);
-		count++;
 	}
 	UNLOCK(&fctx->lock);
 
@@ -10391,7 +10400,16 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 					result = DNS_R_DUPLICATE;
 					goto unlock;
 				}
-				count++;
+
+				/*
+				 * Only the regular fetch events should be
+				 * counted for the clients-per-query limit, in
+				 * case if there are multiple events registered
+				 * for a single client.
+				 */
+				if (resp->type == FETCHDONE) {
+					count++;
+				}
 			}
 		}
 		if (count >= spillatmin && spillatmin != 0) {