From b23d88947a78b84bacdbadf964635c96751db752 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 3 Aug 2022 22:21:46 +0000 Subject: [PATCH 1/2] DiG: fix lookup reference counting bug When DiG finishes its work with a lookup (due to success or error), it calls the clear_current_lookup() function, which decreases the lookup's reference count. That decrease action is the counterpart of the initial creation of the reference counter, so this function was designed in such a way that it should decrease the reference count only once, when there are no more active queries in the lookup. The way it checks whether there are any active queries is by looking at the queries list of the lookup object - if it's NULL then there are no active queries. But that is not always true - the cancel_lookup() function, when canceling the queries one by one, also removes them from the lookup's list, but in NSSEARCH mode, when the queries are working in parallel, some of those queries can be still active. And when their recv_done() callback gets called, it sees that the lookup has been canceled, calls clear_current_lookup(), which decreases the reference count every time for each query that was still active (because ISC_LIST_HEAD(lookup->q) is NULL) and results in a reference counting error. Fix the issue by introducing a new "cleared" property for the lookup, which will ensure that the clear_current_lookup() function does its job only once per lookup. (cherry picked from commit 08ba2732e061710d3622ae181de33bbaaf63f63c) --- bin/dig/dighost.c | 8 ++++++++ bin/dig/dighost.h | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index a207b91a2c..2131aa18ff 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -1828,11 +1828,19 @@ clear_current_lookup(void) { return; } + if (lookup->cleared) { + debug("current_lookup is already cleared"); + return; + } + if (ISC_LIST_HEAD(lookup->q) != NULL) { debug("still have a worker"); return; } + lookup->cleared = true; + debug("lookup cleared"); + lookup_detach(&lookup); } diff --git a/bin/dig/dighost.h b/bin/dig/dighost.h index e9da8f618e..00158ac6f9 100644 --- a/bin/dig/dighost.h +++ b/bin/dig/dighost.h @@ -105,7 +105,7 @@ typedef struct dig_searchlist dig_searchlist_t; struct dig_lookup { unsigned int magic; isc_refcount_t references; - bool aaonly, adflag, badcookie, besteffort, cdflag, comments, + bool aaonly, adflag, badcookie, besteffort, cdflag, cleared, comments, dns64prefix, dnssec, doing_xfr, done_as_is, ednsneg, expandaaaa, expire, header_only, identify, /*%< Append an "on server " message */ From b83ff9cb2098c65c6f66738a1d93a86e2f981e47 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 4 Aug 2022 11:47:03 +0000 Subject: [PATCH 2/2] Add CHANGES note for [GL #3478] (cherry picked from commit 8eea655053e17def61815965d1bbb13bf03d3b53) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 70466f220c..3536565c8a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5935. [bug] Fix DiG lookup reference counting bug, which could + be observed in NSSEARCH mode. [GL #3478] + 5934. [func] Improve fetches-per-zone fetch limit logging to log the final allowed and spilled values of the fetch counters before the counter object gets destroyed.