From: Otto <otto.moerbeek@open-xchange.com>
Date: Wed, 13 Jan 2021 10:07:45 +0000 (+0100)
Subject: Solution that only sets *state and calls fakeTTD if the loop actually found a non... 
X-Git-Tag: rec-4.5.0-alpha1^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=017d02afa74d5bc3f0f362fad49492493f6946c6;p=thirdparty%2Fpdns.git

Solution that only sets *state and calls fakeTTD if the loop actually found a non-expired match.
---

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 32f6e35738..4b5974b264 100644
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -273,7 +273,6 @@ int32_t MemRecursorCache::fakeTTD(MemRecursorCache::OrderedTagIterator_t& entry,
 int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt, bool requireAuth, vector<DNSRecord>* res, const ComboAddress& who, bool refresh, const OptTag& routingTag, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, vState* state, bool* wasAuth, DNSName* fromAuthZone)
 {
   boost::optional<vState> cachedState{boost::none};
-  time_t ttd=0;
   uint32_t origTTL;
 
   if(res) {
@@ -330,13 +329,14 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
 
   if (routingTag) {
     auto entries = getEntries(map, qname, qt, routingTag);
+    bool found = false;
+    time_t ttd;
 
     if (entries.first != entries.second) {
       OrderedTagIterator_t firstIndexIterator;
       for (auto i=entries.first; i != entries.second; ++i) {
-
         firstIndexIterator = map.d_map.project<OrderedTag>(i);
-        origTTL = firstIndexIterator->d_orig_ttl;
+
         if (i->d_ttd <= now) {
           moveCacheItemToFront<SequencedTag>(map.d_map, firstIndexIterator);
           continue;
@@ -345,17 +345,19 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
         if (!entryMatches(firstIndexIterator, qtype, requireAuth, who)) {
           continue;
         }
-
+        found = true;
         ttd = handleHit(map, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
 
         if (qt.getCode() != QType::ANY && qt.getCode() != QType::ADDR) { // normally if we have a hit, we are done
           break;
         }
       }
-      if (state && cachedState) {
-        *state = *cachedState;
+      if (found) {
+        if (state && cachedState) {
+          *state = *cachedState;
+        }
+        return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh);
       }
-      return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh);
     }
   }
   // Try (again) without tag
@@ -363,10 +365,12 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
 
   if (entries.first != entries.second) {
     OrderedTagIterator_t firstIndexIterator;
-    for (auto i=entries.first; i != entries.second; ++i) {
+    bool found = false;
+    time_t ttd;
 
+    for (auto i=entries.first; i != entries.second; ++i) {
       firstIndexIterator = map.d_map.project<OrderedTag>(i);
-      origTTL = firstIndexIterator->d_orig_ttl;
+
       if (i->d_ttd <= now) {
         moveCacheItemToFront<SequencedTag>(map.d_map, firstIndexIterator);
         continue;
@@ -376,16 +380,19 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
         continue;
       }
 
+      found = true;
       ttd = handleHit(map, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
 
       if (qt.getCode() != QType::ANY && qt.getCode() != QType::ADDR) { // normally if we have a hit, we are done
         break;
       }
     }
-    if (state && cachedState) {
-      *state = *cachedState;
+    if (found) {
+      if (state && cachedState) {
+        *state = *cachedState;
+      }
+      return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh);
     }
-    return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh);
   }
   return -1;
 }
diff --git a/pdns/recursordist/test-recursorcache_cc.cc b/pdns/recursordist/test-recursorcache_cc.cc
index 52602e2181..54fe97a14d 100644
--- a/pdns/recursordist/test-recursorcache_cc.cc
+++ b/pdns/recursordist/test-recursorcache_cc.cc
@@ -265,7 +265,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     MRC.replace(now, power, QType(QType::A), records, signatures, authRecords, false, authZone, boost::none);
     BOOST_CHECK_EQUAL(MRC.size(), 1U);
     // let's first check that non-auth is not returned when we need authoritative data
-    BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::A), true, &retrieved, ComboAddress("127.0.0.1")), -now);
+    BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::A), true, &retrieved, ComboAddress("127.0.0.1")), -1);
     BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::A), false, &retrieved, ComboAddress("127.0.0.1")), (ttd - now));
     BOOST_REQUIRE_EQUAL(retrieved.size(), 1U);
     BOOST_CHECK_EQUAL(getRR<ARecordContent>(retrieved.at(0))->getCA().toString(), dr2Content.toString());
@@ -478,7 +478,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCache_ExpungingExpiredEntries)
   BOOST_CHECK_EQUAL(MRC.size(), 2U);
 
   /* trigger a miss (expired) for power2 */
-  BOOST_CHECK_EQUAL(MRC.get(now, power2, QType(dr2.d_type), false, &retrieved, who, 0, boost::none, nullptr), -now);
+  BOOST_CHECK_EQUAL(MRC.get(now, power2, QType(dr2.d_type), false, &retrieved, who, 0, boost::none, nullptr), -1);
 
   /* power2 should have been moved to the front of the expunge
      queue, and should this time be removed first */