rec: dedup results from auths and results constructed ourselves

diff --git a/pdns/recursordist/Makefile.am b/pdns/recursordist/Makefile.am
index 8d2210f6cfeff1408ae80c578f0c673fbc4558cf..7f86c657505b60ff5372ba1085b0e6d1031ed897 100644 (file)
--- 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 8ea9286171303b6d61ad327f33e40ac8187a4e3e..cdcb1f71f13eee06b8dd42f6b773c953d1a0618b 100644 (file)
--- 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 9a318dfc46ba8a2a2a3276e2796f1ec39137f429..cb4f9a6ab09b81da5564ac9a6db885e66082a2fc 100644 (file)
--- 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<DNSRecord>& newRecords, unsigned int depth, const string& prefix)

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 49d593503dbfd91119c24e4c7f5e97c5f2afdf4f..3e4644bce45d077e0585ea67d32cb99a39d89524 100644 (file)
--- 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<DNSName, std::pair<DNSSECPrivateKey, DSRecordContent>> testkeysset_t;
 
-bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, bool broken, boost::optional<uint8_t> algo, boost::optional<DNSName> wildcard, boost::optional<time_t> now)
+bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, std::variant<bool, int> broken, boost::optional<uint8_t> algo, boost::optional<DNSName> wildcard, boost::optional<time_t> now)
 {
   if (records.empty()) {
     return false;
@@ -364,9 +364,12 @@ bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& 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<bool>(&broken); bval != nullptr && *bval) {
     rrc.d_signature[0] ^= 42;
   }
+  else if (auto *ival = std::get_if<int>(&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 8978fcffb52fed22d29d856bc412aea5e3120bfc..1e0e22590c08c47691394b67dd41caed6591b82c 100644 (file)
--- 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<DNSName, std::pair<DNSSECPrivateKey, DSRecordContent>> testkeysset_t;
 
-bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, bool broken = false, boost::optional<uint8_t> algo = boost::none, boost::optional<DNSName> wildcard = boost::none, boost::optional<time_t> now = boost::none);
+bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, std::variant<bool, int> broken = false, boost::optional<uint8_t> algo = boost::none, boost::optional<DNSName> wildcard = boost::none, boost::optional<time_t> now = boost::none);
 
 void addDNSKEY(const testkeysset_t& keys, const DNSName& signer, uint32_t ttl, std::vector<DNSRecord>& records);
 

diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc
index 7e5db3c57b3d1a6fb998593c45ffc8867dfee272..77ac227a31caf0f96a47025dc48fccf2c471fb9f 100644 (file)
--- 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 38e979e43a449675dca4ab434e212a6c3d0cc6a3..e0eb6a4d1ae3ea4770b53e6816dbfcf23d34ce0d 100644 (file)
--- 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 0e507a76d54d3fb101f94dde0d4f527d8b019cc1..43417426fbc8fc9f5de1adee27bcb3e4a6449da3 100644 (file)
--- a/pdns/shuffle.cc
+++ b/pdns/shuffle.cc
@@ -140,3 +140,46 @@ void pdns::orderAndShuffle(vector<DNSRecord>& rrs, bool includingAdditionals)
   });
   shuffle(rrs, includingAdditionals);
 }
+
+void pdns::dedup(vector<DNSRecord>& 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<std::string> seen;
+  std::vector<bool> 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<DNSRecord> 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 54a04ecd9043beef4f5c679c911d7c1f6f48c5d5..f21ec73f4de2e30165baf339afe0341b2e65e6d5 100644 (file)
--- a/pdns/shuffle.hh
+++ b/pdns/shuffle.hh
@@ -29,4 +29,5 @@ namespace pdns
 {
 void shuffle(std::vector<DNSZoneRecord>& rrs);
 void orderAndShuffle(std::vector<DNSRecord>& rrs, bool includingAdditionals);
+void dedup(std::vector<DNSRecord>& rrs);
 }