From: Otto Moerbeek Date: Mon, 23 Oct 2023 09:48:54 +0000 (+0200) Subject: Only assign to the arguments of get if the entry is not expired. X-Git-Tag: rec-5.0.0-beta1~25^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48706cbeac94891578acf14db2764c7a01f6dd10;p=thirdparty%2Fpdns.git Only assign to the arguments of get if the entry is not expired. Callers of get() *must* ensure they onluy look at the args if get() returned > 0 Implements #12612 for the record cache. Negcache fill follow later. --- diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc index 36f327e976..9dbdced2b7 100644 --- a/pdns/recursordist/recursor_cache.cc +++ b/pdns/recursordist/recursor_cache.cc @@ -147,10 +147,15 @@ static void ptrAssign(T* ptr, T value) } } -time_t MemRecursorCache::handleHit(MapCombo::LockedContent& content, 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, ComboAddress* fromAuthIP) +time_t MemRecursorCache::handleHit(time_t now, MapCombo::LockedContent& content, 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, ComboAddress* fromAuthIP) { // MUTEX SHOULD BE ACQUIRED (as indicated by the reference to the content which is protected by a lock) time_t ttd = entry->d_ttd; + if (ttd <= now) { + // Expired, don't bother returning contents. Callers *MUST* check return value of get(), and only look at the entry + // if it returnded > 0 + return ttd; + } origTTL = entry->d_orig_ttl; if (!entry->d_netmask.empty() || entry->d_rtag) { @@ -373,11 +378,11 @@ time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype auto entryA = getEntryUsingECSIndex(*lockedShard, now, qname, QType::A, requireAuth, who, serveStale); if (entryA != lockedShard->d_map.end()) { - ret = handleHit(*lockedShard, entryA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); + ret = handleHit(now, *lockedShard, entryA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); } auto entryAAAA = getEntryUsingECSIndex(*lockedShard, now, qname, QType::AAAA, requireAuth, who, serveStale); if (entryAAAA != lockedShard->d_map.end()) { - time_t ttdAAAA = handleHit(*lockedShard, entryAAAA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); + time_t ttdAAAA = handleHit(now, *lockedShard, entryAAAA, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); if (ret > 0) { ret = std::min(ret, ttdAAAA); } @@ -386,7 +391,7 @@ time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype } } - if (cachedState) { + if (cachedState && ret > 0) { ptrAssign(state, *cachedState); } @@ -394,8 +399,8 @@ time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype } auto entry = getEntryUsingECSIndex(*lockedShard, now, qname, qtype, requireAuth, who, serveStale); if (entry != lockedShard->d_map.end()) { - time_t ret = handleHit(*lockedShard, entry, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); - if (cachedState) { + time_t ret = handleHit(now, *lockedShard, entry, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); + if (cachedState && ret > now) { ptrAssign(state, *cachedState); } return fakeTTD(entry, qname, qtype, ret, now, origTTL, refresh); @@ -426,14 +431,14 @@ time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype handleServeStaleBookkeeping(now, serveStale, firstIndexIterator); - ttd = handleHit(*lockedShard, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); + ttd = handleHit(now, *lockedShard, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); if (qtype != QType::ANY && qtype != QType::ADDR) { // normally if we have a hit, we are done break; } } if (found) { - if (cachedState) { + if (cachedState && ttd > now) { ptrAssign(state, *cachedState); } return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh); @@ -465,14 +470,14 @@ time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype handleServeStaleBookkeeping(now, serveStale, firstIndexIterator); - ttd = handleHit(*lockedShard, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); + ttd = handleHit(now, *lockedShard, firstIndexIterator, qname, origTTL, res, signatures, authorityRecs, variable, cachedState, wasAuth, fromAuthZone, fromAuthIP); if (qtype != QType::ANY && qtype != QType::ADDR) { // normally if we have a hit, we are done break; } } if (found) { - if (cachedState) { + if (cachedState && ttd > now) { ptrAssign(state, *cachedState); } return fakeTTD(firstIndexIterator, qname, qtype, ttd, now, origTTL, refresh); diff --git a/pdns/recursordist/recursor_cache.hh b/pdns/recursordist/recursor_cache.hh index f0d821e7aa..360e97958c 100644 --- a/pdns/recursordist/recursor_cache.hh +++ b/pdns/recursordist/recursor_cache.hh @@ -67,7 +67,7 @@ public: static constexpr Flags Refresh = 1 << 1; static constexpr Flags ServeStale = 1 << 2; - time_t get(time_t, const DNSName& qname, QType qtype, Flags flags, vector* res, const ComboAddress& who, const OptTag& routingTag = boost::none, vector>* signatures = nullptr, std::vector>* authorityRecs = nullptr, bool* variable = nullptr, vState* state = nullptr, bool* wasAuth = nullptr, DNSName* fromAuthZone = nullptr, ComboAddress* fromAuthIP = nullptr); + [[nodiscard]] time_t get(time_t, const DNSName& qname, QType qtype, Flags flags, vector* res, const ComboAddress& who, const OptTag& routingTag = boost::none, vector>* signatures = nullptr, std::vector>* authorityRecs = nullptr, bool* variable = nullptr, vState* state = nullptr, bool* wasAuth = nullptr, DNSName* fromAuthZone = nullptr, ComboAddress* fromAuthIP = nullptr); void replace(time_t, const DNSName& qname, QType qtype, 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, bool refresh = false, time_t ttl_time = time(nullptr)); @@ -333,7 +333,7 @@ private: static Entries getEntries(MapCombo::LockedContent& map, const DNSName& qname, QType qtype, const OptTag& rtag); static cache_t::const_iterator getEntryUsingECSIndex(MapCombo::LockedContent& map, time_t now, const DNSName& qname, QType qtype, bool requireAuth, const ComboAddress& who, bool serveStale); - static time_t handleHit(MapCombo::LockedContent& content, 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, ComboAddress* fromAuthIP); + static time_t handleHit(time_t now, MapCombo::LockedContent& content, 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, ComboAddress* fromAuthIP); static void updateStaleEntry(time_t now, OrderedTagIterator_t& entry); static void handleServeStaleBookkeeping(time_t, bool, OrderedTagIterator_t&); };