From 0cb38f0fced6aa1f4597c57227efe913e636becf Mon Sep 17 00:00:00 2001
From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Fri, 5 Jun 2020 10:19:08 +0200
Subject: [PATCH] Use seperate function to test for loop; empty result vector
 on loop detection (like other resolvers I tested  do).

---
 pdns/recursordist/test-syncres_cc1.cc |  2 +-
 pdns/syncres.cc                       | 38 ++++++++++++++++-----------
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc
index 3bb11de100..1e048cd32d 100644
--- a/pdns/recursordist/test-syncres_cc1.cc
+++ b/pdns/recursordist/test-syncres_cc1.cc
@@ -1125,7 +1125,7 @@ BOOST_AUTO_TEST_CASE(test_cname_loop)
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::ServFail);
-  BOOST_CHECK_GT(ret.size(), 0U);
+  BOOST_CHECK_EQUAL(ret.size(), 0U);
   BOOST_CHECK_EQUAL(count, 2U);
 }
 
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index b6a3bf9d85..c01851dfe0 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1172,6 +1172,18 @@ void SyncRes::updateValidationStatusInCache(const DNSName &qname, const QType& q
   }
 }
 
+static bool scanForCNAMELoop(const DNSName& name, const vector<DNSRecord>& records)
+{
+  for (const auto record: records) {
+    if (record.d_type == QType::CNAME && record.d_place == DNSResourceRecord::ANSWER) {
+      if (name == record.d_name) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 bool SyncRes::doCNAMECacheCheck(const DNSName &qname, const QType &qtype, vector<DNSRecord>& ret, unsigned int depth, int &res, vState& state, bool wasAuthZone, bool wasForwardRecurse)
 {
   string prefix;
@@ -1337,14 +1349,10 @@ bool SyncRes::doCNAMECacheCheck(const DNSName &qname, const QType &qtype, vector
       }
 
       // Check to see if we already have seen the new target as a previous target
-      for (const auto &rec: ret) {
-        if (rec.d_type == QType::CNAME && rec.d_place == DNSResourceRecord::ANSWER) {
-          if (newTarget == rec.d_name) {
-            string msg = "got a CNAME referral (from cache) that causes a loop";
-            LOG(prefix<<qname<<": status="<<msg<<endl);
-            throw ImmediateServFailException(msg);
-          }
-        }
+      if (scanForCNAMELoop(newTarget, ret)) {
+        string msg = "got a CNAME referral (from cache) that causes a loop";
+        LOG(prefix<<qname<<": status="<<msg<<endl);
+        throw ImmediateServFailException(msg);
       }
 
       set<GetBestNSAnswer>beenthere;
@@ -3380,6 +3388,7 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn
   if(!newtarget.empty()) {
     if(newtarget == qname) {
       LOG(prefix<<qname<<": status=got a CNAME referral to self, returning SERVFAIL"<<endl);
+      ret.clear();
       *rcode = RCode::ServFail;
       return true;
     }
@@ -3391,14 +3400,11 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn
     }
 
     // Check to see if we already have seen the new target as a previous target
-    for (const auto &record: ret) {
-      if (record.d_type == QType::CNAME && record.d_place == DNSResourceRecord::ANSWER) {
-        if (newtarget == record.d_name) {
-          LOG(prefix<<qname<<": status=got a CNAME referral that causes a loop, returning SERVFAIL"<<endl);
-          *rcode = RCode::ServFail;
-          return true;
-        }
-      }
+    if (scanForCNAMELoop(newtarget, ret)) {
+      LOG(prefix<<qname<<": status=got a CNAME referral that causes a loop, returning SERVFAIL"<<endl);
+      ret.clear();
+      *rcode = RCode::ServFail;
+      return true;
     }
 
     if (qtype == QType::DS) {
-- 
2.47.2