From: Otto Moerbeek Date: Tue, 23 Jul 2024 12:20:30 +0000 (+0200) Subject: rec: dedup results from auths and results constructed ourselves X-Git-Tag: dnsdist-2.0.0-alpha1~182^2~9 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=31767d5834f127dead8ddc08d18ad0a1b30f883c;p=thirdparty%2Fpdns.git rec: dedup results from auths and results constructed ourselves --- diff --git a/pdns/recursordist/Makefile.am b/pdns/recursordist/Makefile.am index 8d2210f6cf..7f86c65750 100644 --- a/pdns/recursordist/Makefile.am +++ b/pdns/recursordist/Makefile.am @@ -338,6 +338,7 @@ testrunner_SOURCES = \ rpzloader.cc rpzloader.hh \ secpoll.cc \ settings/cxxsupport.cc \ + shuffle.cc shuffle.hh \ sholder.hh \ sillyrecords.cc \ sortlist.cc sortlist.hh \ diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 8ea9286171..cdcb1f71f1 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -1527,6 +1527,7 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi } if (!ret.empty()) { + pdns::dedup(ret); pdns::orderAndShuffle(ret, false); if (auto listToSort = luaconfsLocal->sortlist.getOrderCmp(comboWriter->d_source)) { stable_sort(ret.begin(), ret.end(), *listToSort); diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9a318dfc46..cb4f9a6ab0 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -39,6 +39,7 @@ #include "dnsseckeeper.hh" #include "validate-recursor.hh" #include "rec-taskqueue.hh" +#include "shuffle.hh" rec::GlobalCounters g_Counters; thread_local rec::TCounters t_Counters(g_Counters); @@ -4445,6 +4446,7 @@ void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, con } lwr.d_records = std::move(vec); } + pdns::dedup(lwr.d_records); } void SyncRes::rememberParentSetIfNeeded(const DNSName& domain, const vector& newRecords, unsigned int depth, const string& prefix) diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index 49d593503d..3e4644bce4 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -322,7 +322,7 @@ void computeRRSIG(const DNSSECPrivateKey& dpk, const DNSName& signer, const DNSN typedef std::unordered_map> testkeysset_t; -bool addRRSIG(const testkeysset_t& keys, std::vector& records, const DNSName& signer, uint32_t sigValidity, bool broken, boost::optional algo, boost::optional wildcard, boost::optional now) +bool addRRSIG(const testkeysset_t& keys, std::vector& records, const DNSName& signer, uint32_t sigValidity, std::variant broken, boost::optional algo, boost::optional wildcard, boost::optional now) { if (records.empty()) { return false; @@ -364,9 +364,12 @@ bool addRRSIG(const testkeysset_t& keys, std::vector& records, const RRSIGRecordContent rrc; computeRRSIG(it->second.first, signer, wildcard ? *wildcard : name, type, ttl, sigValidity, rrc, recordcontents, algo, boost::none, now); - if (broken) { + if (auto* bval = std::get_if(&broken); bval != nullptr && *bval) { rrc.d_signature[0] ^= 42; } + else if (auto *ival = std::get_if(&broken)) { + rrc.d_signature[0] ^= *ival; // NOLINT(*-narrowing-conversions) + } DNSRecord rec; rec.d_type = QType::RRSIG; diff --git a/pdns/recursordist/test-syncres_cc.hh b/pdns/recursordist/test-syncres_cc.hh index 8978fcffb5..1e0e22590c 100644 --- a/pdns/recursordist/test-syncres_cc.hh +++ b/pdns/recursordist/test-syncres_cc.hh @@ -48,7 +48,7 @@ void computeRRSIG(const DNSSECPrivateKey& dpk, const DNSName& signer, const DNSN typedef std::unordered_map> testkeysset_t; -bool addRRSIG(const testkeysset_t& keys, std::vector& records, const DNSName& signer, uint32_t sigValidity, bool broken = false, boost::optional algo = boost::none, boost::optional wildcard = boost::none, boost::optional now = boost::none); +bool addRRSIG(const testkeysset_t& keys, std::vector& records, const DNSName& signer, uint32_t sigValidity, std::variant broken = false, boost::optional algo = boost::none, boost::optional wildcard = boost::none, boost::optional now = boost::none); void addDNSKEY(const testkeysset_t& keys, const DNSName& signer, uint32_t ttl, std::vector& records); diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc index 7e5db3c57b..77ac227a31 100644 --- a/pdns/recursordist/test-syncres_cc4.cc +++ b/pdns/recursordist/test-syncres_cc4.cc @@ -1729,9 +1729,9 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_too_many_sigs) addRecordToLW(res, domain, QType::NS, std::string(addr), DNSResourceRecord::ANSWER, 3600); } - addRRSIG(keys, res->d_records, domain, 300, true, boost::none, boost::none, fixedNow); - addRRSIG(keys, res->d_records, domain, 300, true, boost::none, boost::none, fixedNow); - addRRSIG(keys, res->d_records, domain, 300, false, boost::none, boost::none, fixedNow); + addRRSIG(keys, res->d_records, domain, 300, 1, boost::none, boost::none, fixedNow); + addRRSIG(keys, res->d_records, domain, 300, 2, boost::none, boost::none, fixedNow); + addRRSIG(keys, res->d_records, domain, 300, 0, boost::none, boost::none, fixedNow); addRecordToLW(res, "a.root-servers.net.", QType::A, "198.41.0.4", DNSResourceRecord::ADDITIONAL, 3600); addRecordToLW(res, "a.root-servers.net.", QType::AAAA, "2001:503:ba3e::2:30", DNSResourceRecord::ADDITIONAL, 3600); diff --git a/pdns/recursordist/test-syncres_cc5.cc b/pdns/recursordist/test-syncres_cc5.cc index 38e979e43a..e0eb6a4d1a 100644 --- a/pdns/recursordist/test-syncres_cc5.cc +++ b/pdns/recursordist/test-syncres_cc5.cc @@ -1603,8 +1603,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec3_nodata_nowildcard_duplicated_n int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure); - /* because we pass along the duplicated NSEC3 */ - BOOST_REQUIRE_EQUAL(ret.size(), 9U); + /* the duplicated NSEC3 should have been dedupped */ + BOOST_REQUIRE_EQUAL(ret.size(), 8U); BOOST_CHECK_EQUAL(queriesCount, 4U); /* again, to test the cache */ @@ -1612,8 +1612,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec3_nodata_nowildcard_duplicated_n res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure); - /* because we pass along the duplicated NSEC3 */ - BOOST_REQUIRE_EQUAL(ret.size(), 9U); + /* the duplicated NSEC3 should have been dedupped */ + BOOST_REQUIRE_EQUAL(ret.size(), 8U); BOOST_CHECK_EQUAL(queriesCount, 4U); } diff --git a/pdns/shuffle.cc b/pdns/shuffle.cc index 0e507a76d5..43417426fb 100644 --- a/pdns/shuffle.cc +++ b/pdns/shuffle.cc @@ -140,3 +140,46 @@ void pdns::orderAndShuffle(vector& rrs, bool includingAdditionals) }); shuffle(rrs, includingAdditionals); } + +void pdns::dedup(vector& rrs) +{ + // This functino tries to avoid unneccesary work + // First a vector with zero or one element does not need dedupping + if (rrs.size() <= 1) { + return; + } + + // If we have a larger vector, first check if we actually have duplicates. + // We assume the most common case is: no + std::unordered_set seen; + std::vector unique(rrs.size(), false); + + unsigned int counter = 0; + unsigned int numUnique = 0; + + for (const auto& rec : rrs) { + // This ignores class, ttl and place + if (seen.emplace(rec.getContent()->serialize(rec.d_name, true, true)).second) { + unique[counter] = true; + numUnique++; + } + ++counter; + } + + if (numUnique == rrs.size()) { + // Original is fine as-is. + return; + } + + // We avoid calling erase, as it calls a lot of move constructors. This can hurt, especially if + // you call it on a large vector muliple times. + // So we just take the elements that are unique + std::vector ret; + ret.reserve(numUnique); + for (counter = 0; counter < rrs.size(); ++counter) { + if (unique[counter]) { + ret.emplace_back(std::move(rrs[counter])); + } + } + rrs = std::move(ret); +} diff --git a/pdns/shuffle.hh b/pdns/shuffle.hh index 54a04ecd90..f21ec73f4d 100644 --- a/pdns/shuffle.hh +++ b/pdns/shuffle.hh @@ -29,4 +29,5 @@ namespace pdns { void shuffle(std::vector& rrs); void orderAndShuffle(std::vector& rrs, bool includingAdditionals); +void dedup(std::vector& rrs); }