From a06745426b4df4d3946c36cd3429a5c8db9a8cd0 Mon Sep 17 00:00:00 2001
From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Wed, 3 Jun 2020 09:07:56 +0200
Subject: [PATCH] Correct depth increments.

With the introduction of qname minimization, a function
doResolveNoQNameMinimization() was introduced. This function is
called by doResolve() with depth incremented. Due to the recursive
nature of the resursor algortihm (Nomen est Omen) we end up
incrementing the depth too much. This prompted a review of the other
places depth was incremented, and I believe it should only be done
when calling doResolve(). Especially the case "+ 2" in the getAddrs()
call looks strange to me, as the doResolve() calls in getAddrs()
already call doResolve() with depth + 1.

This fixes #9184 and likely other cases of deep recursion caused
by long CNAME chains.
---
 pdns/recursordist/test-syncres_cc2.cc |  2 +-
 pdns/syncres.cc                       | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index 2e19ce8a82..618fb271c3 100644
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -56,7 +56,7 @@ static void do_test_referral_depth(bool limited)
 
   if (limited) {
     /* Set the maximum depth low */
-    SyncRes::s_maxdepth = 10;
+    SyncRes::s_maxdepth = 4;
     try {
       vector<DNSRecord> ret;
       sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 3fde0ef257..45f44e4550 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -671,7 +671,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
   vector<DNSRecord> retq;
   bool old = setCacheOnly(true);
   bool fromCache = false;
-  int res = doResolveNoQNameMinimization(qname, qtype, retq, depth + 1, beenthere, state, &fromCache);
+  int res = doResolveNoQNameMinimization(qname, qtype, retq, depth, beenthere, state, &fromCache);
   setCacheOnly(old);
   if (fromCache) {
     QLOG("Step0 Found in cache");
@@ -695,7 +695,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
     for (int tries = 0; tries < 2 && bestns.empty(); ++tries) {
       bool flawedNSSet = false;
       set<GetBestNSAnswer> beenthereIgnored;
-      getBestNSFromCache(nsdomain, qtype, bestns, &flawedNSSet, depth + 1, beenthereIgnored);
+      getBestNSFromCache(nsdomain, qtype, bestns, &flawedNSSet, depth, beenthereIgnored);
     }
 
     if (bestns.size() == 0) {
@@ -723,7 +723,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
       // Step 3 resolve
       if (child == qname) {
         QLOG("Step3 Going to do final resolve");
-        res = doResolveNoQNameMinimization(qname, qtype, ret, depth + 1, beenthere, state);
+        res = doResolveNoQNameMinimization(qname, qtype, ret, depth, beenthere, state);
         QLOG("Step3 Final resolve: " << RCode::to_s(res) << "/" << ret.size());
         return res;
       }
@@ -732,7 +732,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
       QLOG("Step4 Resolve A for child");
       retq.resize(0);
       StopAtDelegation stopAtDelegation = Stop;
-      res = doResolveNoQNameMinimization(child, QType::A, retq, depth + 1, beenthere, state, NULL, &stopAtDelegation);
+      res = doResolveNoQNameMinimization(child, QType::A, retq, depth, beenthere, state, NULL, &stopAtDelegation);
       QLOG("Step4 Resolve A result is " << RCode::to_s(res) << "/" << retq.size() << "/" << stopAtDelegation);
       if (stopAtDelegation == Stopped) {
         QLOG("Delegation seen, continue at step 1");
@@ -743,7 +743,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
         // Case 5: unexpected answer
         QLOG("Step5: other rcode, last effort final resolve");
         setQNameMinimization(false);
-        res = doResolveNoQNameMinimization(qname, qtype, ret, depth + 1, beenthere, state);
+        res = doResolveNoQNameMinimization(qname, qtype, ret, depth, beenthere, state);
 
         if(res == RCode::NoError) {
           s_qnameminfallbacksuccess++;
@@ -1866,7 +1866,7 @@ vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix,
 
   if(!tns->first.empty()) {
     LOG(prefix<<qname<<": Trying to resolve NS '"<<tns->first<< "' ("<<1+tns-rnameservers.begin()<<"/"<<(unsigned int)rnameservers.size()<<")"<<endl);
-    result = getAddrs(tns->first, depth+2, beenthere, cacheOnly, retrieveAddressesForNS);
+    result = getAddrs(tns->first, depth, beenthere, cacheOnly, retrieveAddressesForNS);
     pierceDontQuery=false;
   }
   else {
@@ -2228,7 +2228,7 @@ void SyncRes::computeZoneCuts(const DNSName& begin, const DNSName& end, unsigned
     /* temporarily mark as Indeterminate, so that we won't enter an endless loop
        trying to determine that zone cut again. */
     d_cutStates[qname] = newState;
-    bool foundCut = lookForCut(qname, depth + 1, cutState, newState);
+    bool foundCut = lookForCut(qname, depth, cutState, newState);
     if (foundCut) {
       LOG(d_prefix<<": - Found cut at "<<qname<<endl);
       if (newState != Indeterminate) {
-- 
2.47.2