More elaborate unit tests for recursor cache and fix implemantation.

There remain cases where a time_t is assigned to a uint32_t. This is wrong
but cannot be avoided atm due to swicthing back and forth between ttl and ttd
in records. This will cause trouble when time causes uint32_t to wrap.

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 4b2d04380a2824a3c147ce3e83a60ab70e315e3f..37c0b2a372e43f20f4cc55d263dd2fb7c401b80a 100644 (file)
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -112,10 +112,10 @@ static void updateDNSSECValidationStateFromCache(boost::optional<vState>& state,
   }
 }
 
-int32_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector<DNSRecord>* res, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, boost::optional<vState>& state, bool* wasAuth, DNSName* fromAuthZone)
+time_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector<DNSRecord>* res, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, boost::optional<vState>& state, bool* wasAuth, DNSName* fromAuthZone)
 {
   // MUTEX SHOULD BE ACQUIRED
-  int32_t ttd = entry->d_ttd;
+  time_t ttd = entry->d_ttd;
   origTTL = entry->d_orig_ttl;
 
   if (variable && (!entry->d_netmask.empty() || entry->d_rtag)) {
@@ -131,7 +131,7 @@ int32_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagI
       dr.d_type = entry->d_qtype;
       dr.d_class = QClass::IN;
       dr.d_content = k;
-      dr.d_ttl = static_cast<uint32_t>(entry->d_ttd);
+      dr.d_ttl = static_cast<uint32_t>(entry->d_ttd); // XXX truncation
       dr.d_place = DNSResourceRecord::ANSWER;
       res->push_back(std::move(dr));
     }
@@ -250,9 +250,9 @@ bool MemRecursorCache::entryMatches(MemRecursorCache::OrderedTagIterator_t& entr
 }
 
 // Fake a cache miss if more than refreshTTLPerc of the original TTL has passed
-int32_t MemRecursorCache::fakeTTD(MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint16_t qtype, int32_t ret, time_t now, uint32_t origTTL, bool refresh)
+time_t MemRecursorCache::fakeTTD(MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint16_t qtype, time_t ret, time_t now, uint32_t origTTL, bool refresh)
 {
-  int32_t ttl = static_cast<int32_t>(ret - now);
+  time_t ttl = ret - now;
   if (ttl > 0 && SyncRes::s_refresh_ttlperc > 0) {
     const uint32_t deadline = origTTL * SyncRes::s_refresh_ttlperc / 100;
     const bool almostExpired = static_cast<uint32_t>(ttl) <= deadline;
@@ -270,7 +270,7 @@ int32_t MemRecursorCache::fakeTTD(MemRecursorCache::OrderedTagIterator_t& entry,
   return ttl;
 }
 // returns -1 for no hits
-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)
+time_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};
   uint32_t origTTL;
@@ -292,7 +292,7 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
      to be able to use the nice d_cachecache hack. */
   if (qtype != QType::ANY && !map.d_ecsIndex.empty() && !routingTag) {
     if (qtype == QType::ADDR) {
-      int32_t ret = -1;
+      time_t ret = -1;
 
       auto entryA = getEntryUsingECSIndex(map, now, qname, QType::A, requireAuth, who);
       if (entryA != map.d_map.end()) {
@@ -300,7 +300,7 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
       }
       auto entryAAAA = getEntryUsingECSIndex(map, now, qname, QType::AAAA, requireAuth, who);
       if (entryAAAA != map.d_map.end()) {
-        int32_t ttdAAAA = handleHit(map, entryAAAA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
+        time_t ttdAAAA = handleHit(map, entryAAAA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
         if (ret > 0) {
           ret = std::min(ret, ttdAAAA);
         } else {
@@ -312,12 +312,12 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
         *state = *cachedState;
       }
 
-      return ret > 0 ? static_cast<int32_t>(ret-now) : ret;
+      return ret > 0 ? (ret - now) : ret;
     }
     else {
       auto entry = getEntryUsingECSIndex(map, now, qname, qtype, requireAuth, who);
       if (entry != map.d_map.end()) {
-        int32_t ret = handleHit(map, entry, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
+        time_t ret = handleHit(map, entry, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone);
         if (state && cachedState) {
           *state = *cachedState;
         }

diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh
index df46c728d14c59adc1025c23826a963988050b1e..c2446f4c5ecb3fda6832103bc9dd54b8d0220852 100644 (file)
--- a/pdns/recursor_cache.hh
+++ b/pdns/recursor_cache.hh
@@ -57,7 +57,7 @@ public:
 
   typedef boost::optional<std::string> OptTag;
 
-int32_t get(time_t, const DNSName &qname, const QType& qt, bool requireAuth, vector<DNSRecord>* res, const ComboAddress& who, bool refresh = false, const OptTag& routingTag = boost::none, vector<std::shared_ptr<RRSIGRecordContent>>* signatures=nullptr, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs=nullptr, bool* variable=nullptr, vState* state=nullptr, bool* wasAuth=nullptr, DNSName* fromAuthZone=nullptr);
+time_t get(time_t, const DNSName &qname, const QType& qt, bool requireAuth, vector<DNSRecord>* res, const ComboAddress& who, bool refresh = false, const OptTag& routingTag = boost::none, vector<std::shared_ptr<RRSIGRecordContent>>* signatures=nullptr, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs=nullptr, bool* variable=nullptr, vState* state=nullptr, bool* wasAuth=nullptr, DNSName* fromAuthZone=nullptr);
 
   void replace(time_t, const DNSName &qname, const QType& qt,  const vector<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask=boost::none, const OptTag& routingTag = boost::none, vState state=vState::Indeterminate, boost::optional<ComboAddress> from=boost::none);
 
@@ -231,13 +231,13 @@ private:
     return d_maps[qname.hash() % d_maps.size()];
   }
 
-  static int32_t fakeTTD(OrderedTagIterator_t& entry, const DNSName& qname, uint16_t qtype, int32_t ret, time_t now, uint32_t origTTL, bool refresh);
+  static time_t fakeTTD(OrderedTagIterator_t& entry, const DNSName& qname, uint16_t qtype, time_t ret, time_t now, uint32_t origTTL, bool refresh);
 
   bool entryMatches(OrderedTagIterator_t& entry, uint16_t qt, bool requireAuth, const ComboAddress& who);
   Entries getEntries(MapCombo& map, const DNSName &qname, const QType& qt, const OptTag& rtag);
   cache_t::const_iterator getEntryUsingECSIndex(MapCombo& map, time_t now, const DNSName &qname, uint16_t qtype, bool requireAuth, const ComboAddress& who);
 
-  int32_t handleHit(MapCombo& map, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector<DNSRecord>* res, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, boost::optional<vState>& state, bool* wasAuth, DNSName* authZone);
+  time_t handleHit(MapCombo& map, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector<DNSRecord>* res, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, boost::optional<vState>& state, bool* wasAuth, DNSName* authZone);
 
 public:
   struct lock {

diff --git a/pdns/recursordist/test-recursorcache_cc.cc b/pdns/recursordist/test-recursorcache_cc.cc
index 2f25b084509ad92ea9b96d6cbe6322317a7a3ab8..7a8b3532afc7868e684a8d4c859808a449246dd9 100644 (file)
--- a/pdns/recursordist/test-recursorcache_cc.cc
+++ b/pdns/recursordist/test-recursorcache_cc.cc
@@ -11,7 +11,7 @@
 
 BOOST_AUTO_TEST_SUITE(recursorcache_cc)
 
-BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
+static void simple(time_t now)
 {
   MemRecursorCache MRC;
 
@@ -19,7 +19,6 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
   std::vector<std::shared_ptr<DNSRecord>> authRecords;
   std::vector<std::shared_ptr<RRSIGRecordContent>> signatures;
   const DNSName authZone(".");
-  time_t now = INT_MAX - 15;
 
   time_t ttd = now + 30;
   DNSName power("powerdns.com.");
@@ -29,7 +28,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
   dr0.d_type = QType::AAAA;
   dr0.d_class = QClass::IN;
   dr0.d_content = std::make_shared<AAAARecordContent>(dr0Content);
-  dr0.d_ttl = static_cast<uint32_t>(ttd);
+  dr0.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
   dr0.d_place = DNSResourceRecord::ANSWER;
 
   records.push_back(dr0);
@@ -87,7 +86,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     dr1.d_type = QType::AAAA;
     dr1.d_class = QClass::IN;
     dr1.d_content = std::make_shared<AAAARecordContent>(dr1Content);
-    dr1.d_ttl = static_cast<uint32_t>(ttd);
+    dr1.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
     dr1.d_place = DNSResourceRecord::ANSWER;
 
     DNSRecord dr2;
@@ -96,7 +95,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     dr2.d_type = QType::A;
     dr2.d_class = QClass::IN;
     dr2.d_content = std::make_shared<ARecordContent>(dr2Content);
-    dr2.d_ttl = static_cast<uint32_t>(ttd);
+    dr2.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
     // the place should not matter to the cache
     dr2.d_place = DNSResourceRecord::AUTHORITY;
 
@@ -235,7 +234,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     dr3.d_type = QType::A;
     dr3.d_class = QClass::IN;
     dr3.d_content = std::make_shared<ARecordContent>(dr3Content);
-    dr3.d_ttl = static_cast<uint32_t>(ttd + 100);
+    dr3.d_ttl = static_cast<uint32_t>(ttd + 100); // XXX truncation
     // the place should not matter to the cache
     dr3.d_place = DNSResourceRecord::AUTHORITY;
 
@@ -315,7 +314,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     dr4.d_type = QType::A;
     dr4.d_class = QClass::IN;
     dr4.d_content = std::make_shared<ARecordContent>(dr4Content);
-    dr4.d_ttl = static_cast<uint32_t>(ttd);
+    dr4.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
     dr4.d_place = DNSResourceRecord::AUTHORITY;
 
     // insert another entry but for 192.168.0.1/31
@@ -363,6 +362,29 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
+{
+  simple(time(nullptr));
+}
+
+BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple2038)
+{
+  simple(INT_MAX - 15);
+}
+
+BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple2038bis)
+{
+  simple(time_t(INT_MAX) + 10000);
+}
+
+#if 0
+BOOST_AUTO_TEST_CASE(test_RecursorCacheSimpleDistantFuture)
+{
+  // Fails due to using 32-bit ttl values in records for absolute time, see handleHit()
+  simple(2 * time_t(INT_MAX));
+}
+#endif
+
 BOOST_AUTO_TEST_CASE(test_RecursorCacheGhost)
 {
   MemRecursorCache MRC;
@@ -383,7 +405,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheGhost)
   ns1.d_type = QType::NS;
   ns1.d_class = QClass::IN;
   ns1.d_content = std::make_shared<NSRecordContent>(ns1Content);
-  ns1.d_ttl = static_cast<uint32_t>(ttd);
+  ns1.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
   ns1.d_place = DNSResourceRecord::ANSWER;
   records.push_back(ns1);
   MRC.replace(now, ns1.d_name, QType(ns1.d_type), records, signatures, authRecords, true, DNSName("powerdns.com."), boost::none);
@@ -392,7 +414,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheGhost)
   /* try to raise the TTL, simulating the delegated authoritative server
      raising the TTL so the zone stays alive */
   records.clear();
-  ns1.d_ttl = static_cast<uint32_t>(ttd + 3600);
+  ns1.d_ttl = static_cast<uint32_t>(ttd + 3600); // XXX truncation
   records.push_back(ns1);
   MRC.replace(now, ns1.d_name, QType(ns1.d_type), records, signatures, authRecords, true, DNSName("ghost.powerdns.com."), boost::none);
   BOOST_CHECK_EQUAL(MRC.size(), 1U);
@@ -427,7 +449,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCache_ExpungingExpiredEntries)
   dr1.d_type = QType::AAAA;
   dr1.d_class = QClass::IN;
   dr1.d_content = std::make_shared<AAAARecordContent>(dr1Content);
-  dr1.d_ttl = static_cast<uint32_t>(ttd);
+  dr1.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
   dr1.d_place = DNSResourceRecord::ANSWER;
 
   /* entry for power1, which expired 30 ago too */
@@ -437,7 +459,7 @@ BOOST_AUTO_TEST_CASE(test_RecursorCache_ExpungingExpiredEntries)
   dr2.d_type = QType::AAAA;
   dr2.d_class = QClass::IN;
   dr2.d_content = std::make_shared<AAAARecordContent>(dr2Content);
-  dr2.d_ttl = static_cast<uint32_t>(ttd);
+  dr2.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
   dr2.d_place = DNSResourceRecord::ANSWER;
 
   /* insert both entries */