From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Wed, 3 Jun 2020 07:07:56 +0000 (+0200)
Subject: Correct depth increments.
X-Git-Tag: rec-4.3.2~6^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F9247%2Fhead;p=thirdparty%2Fpdns.git

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.

(cherry picked from commit a06745426b4df4d3946c36cd3429a5c8db9a8cd0)
---

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index 19d2b70961..d1e48e3fe4 100644
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -5,7 +5,7 @@
 
 BOOST_AUTO_TEST_SUITE(syncres_cc2)
 
-BOOST_AUTO_TEST_CASE(test_referral_depth)
+static void do_test_referral_depth(bool limited)
 {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
@@ -35,36 +35,66 @@ BOOST_AUTO_TEST_CASE(test_referral_depth)
       }
       else if (domain == DNSName("ns3.powerdns.org.")) {
         addRecordToLW(res, domain, QType::NS, "ns4.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800);
-      }
-      else if (domain == DNSName("ns4.powerdns.org.")) {
-        addRecordToLW(res, domain, QType::NS, "ns5.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800);
-        addRecordToLW(res, domain, QType::A, "192.0.2.1", DNSResourceRecord::AUTHORITY, 172800);
+        addRecordToLW(res, "ns4.powerdns.org.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
       }
 
       return 1;
     }
     else if (ip == ComboAddress("192.0.2.1:53")) {
-
       setLWResult(res, 0, true, false, false);
-      addRecordToLW(res, domain, QType::A, "192.0.2.2");
+      if (domain == DNSName("www.powerdns.com.")) {
+        addRecordToLW(res, domain, QType::A, "192.0.2.2");
+      }
+      else {
+        addRecordToLW(res, domain, QType::A, "192.0.2.1");
+      }
       return 1;
     }
 
     return 0;
   });
 
-  /* Set the maximum depth low */
-  SyncRes::s_maxdepth = 10;
-
-  try {
-    vector<DNSRecord> ret;
-    sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
-    BOOST_CHECK(false);
+  if (limited) {
+    /* Set the maximum depth low */
+    SyncRes::s_maxdepth = 4;
+    try {
+      vector<DNSRecord> ret;
+      sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+      BOOST_CHECK(false);
+    }
+    catch (const ImmediateServFailException& e) {
+      BOOST_CHECK(e.reason.find("max-recursion-depth") != string::npos);
+    }
   }
-  catch (const ImmediateServFailException& e) {
+  else {
+    // Check if the setup with high limit is OK.
+    SyncRes::s_maxdepth = 50;
+    try {
+      vector<DNSRecord> ret;
+      int rcode = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+      BOOST_CHECK_EQUAL(rcode, RCode::NoError);
+      BOOST_REQUIRE_EQUAL(ret.size(), 1U);
+      BOOST_CHECK_EQUAL(ret[0].d_name, target);
+      BOOST_REQUIRE(ret[0].d_type == QType::A);
+      BOOST_CHECK(getRR<ARecordContent>(ret[0])->getCA() == ComboAddress("192.0.2.2"));
+    }
+    catch (const ImmediateServFailException& e) {
+      BOOST_CHECK(false);
+    }
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_referral_depth)
+{
+  // Test with limit
+  do_test_referral_depth(true);
+}
+BOOST_AUTO_TEST_CASE(test_referral_depth_ok)
+{
+  // Test with default limit
+  do_test_referral_depth(false);
+}
+
 BOOST_AUTO_TEST_CASE(test_cname_qperq)
 {
   std::unique_ptr<SyncRes> sr;
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 3a27b2ace3..8f48490f02 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");
@@ -691,7 +691,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(qname, qtype, bestns, &flawedNSSet, depth + 1, beenthereIgnored);
+      getBestNSFromCache(qname, qtype, bestns, &flawedNSSet, depth, beenthereIgnored);
     }
 
     if (bestns.size() == 0) {
@@ -719,7 +719,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;
       }
@@ -728,7 +728,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");
@@ -739,7 +739,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++;
@@ -1851,7 +1851,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 {
@@ -2213,7 +2213,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) {