From: Otto Date: Mon, 25 Jan 2021 12:44:42 +0000 (+0100) Subject: More elaborate unit tests for recursor cache and fix implemantation. X-Git-Tag: dnsdist-1.6.0-alpha1~9^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=98c615a865dc5eadf19b9c9f5158def991f336ba;p=thirdparty%2Fpdns.git 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 4b2d04380a..37c0b2a372 100644 --- a/pdns/recursor_cache.cc +++ b/pdns/recursor_cache.cc @@ -112,10 +112,10 @@ static void updateDNSSECValidationStateFromCache(boost::optional& state, } } -int32_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* fromAuthZone) +time_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& 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(entry->d_ttd); + dr.d_ttl = static_cast(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(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(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* res, const ComboAddress& who, bool refresh, const OptTag& routingTag, vector>* signatures, std::vector>* 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* res, const ComboAddress& who, bool refresh, const OptTag& routingTag, vector>* signatures, std::vector>* authorityRecs, bool* variable, vState* state, bool* wasAuth, DNSName* fromAuthZone) { boost::optional 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(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 df46c728d1..c2446f4c5e 100644 --- a/pdns/recursor_cache.hh +++ b/pdns/recursor_cache.hh @@ -57,7 +57,7 @@ public: typedef boost::optional OptTag; -int32_t get(time_t, const DNSName &qname, const QType& qt, bool requireAuth, vector* res, const ComboAddress& who, bool refresh = false, const OptTag& routingTag = boost::none, vector>* signatures=nullptr, std::vector>* 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* res, const ComboAddress& who, bool refresh = false, const OptTag& routingTag = boost::none, vector>* signatures=nullptr, std::vector>* 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& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask=boost::none, const OptTag& routingTag = boost::none, vState state=vState::Indeterminate, boost::optional 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* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* authZone); + time_t handleHit(MapCombo& map, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& 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 2f25b08450..7a8b3532af 100644 --- 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> authRecords; std::vector> 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(dr0Content); - dr0.d_ttl = static_cast(ttd); + dr0.d_ttl = static_cast(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(dr1Content); - dr1.d_ttl = static_cast(ttd); + dr1.d_ttl = static_cast(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(dr2Content); - dr2.d_ttl = static_cast(ttd); + dr2.d_ttl = static_cast(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(dr3Content); - dr3.d_ttl = static_cast(ttd + 100); + dr3.d_ttl = static_cast(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(dr4Content); - dr4.d_ttl = static_cast(ttd); + dr4.d_ttl = static_cast(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(ns1Content); - ns1.d_ttl = static_cast(ttd); + ns1.d_ttl = static_cast(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(ttd + 3600); + ns1.d_ttl = static_cast(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(dr1Content); - dr1.d_ttl = static_cast(ttd); + dr1.d_ttl = static_cast(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(dr2Content); - dr2.d_ttl = static_cast(ttd); + dr2.d_ttl = static_cast(ttd); // XXX truncation dr2.d_place = DNSResourceRecord::ANSWER; /* insert both entries */