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.

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index 2e19ce8a828d491b016496dca826790d36c53b3c..618fb271c3a5608835c18d8fa31f716228954d0e 100644 (file)
--- 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 3fde0ef257060ea7116080ff1eba002e39bd482f..45f44e4550a3adf7a4c0d463649ce254975f809c 100644 (file)
--- 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) {