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

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 32f6e357386f31218d659355b95882b8a38e2272..4b5974b264ba5819bde60877a81c4ca02194fb6a 100644 (file)
--- 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 52602e2181d6caefb208ae03696e68732dff4bd1..54fe97a14d6efd3825f562fb6d9d9745e13c54a5 100644 (file)
--- 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 */