From: bert hubert <bert.hubert@netherlabs.nl>
Date: Fri, 6 Nov 2015 11:23:18 +0000 (+0100)
Subject: fix up PacketCache misuse if empty DNSNames by moving it to native DNSName canonical... 
X-Git-Tag: dnsdist-1.0.0-alpha1~239^2~12
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=478748c30528625c38b3e804fcdf9bf569209f6d;p=pdns

fix up PacketCache misuse if empty DNSNames by moving it to native DNSName canonical ordering. Speed up non-range delete. Make it possible to wipe .
---

diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc
index 4d17129f7..65ca46e79 100644
--- a/pdns/dnsname.cc
+++ b/pdns/dnsname.cc
@@ -265,7 +265,7 @@ bool DNSName::operator==(const DNSName& rhs) const
 size_t hash_value(DNSName const& d)
 {
   boost::hash<string> hasher;
-  return hasher(toLower(d.toString())); // FIXME400 HACK
+  return hasher(toLower(d.toString())); // FIXME400 HACK - we rely on this lowercasing though in packetcache.hh
 }
 
 string DNSName::escapeLabel(const std::string& label)
diff --git a/pdns/dynhandler.cc b/pdns/dynhandler.cc
index e581ac79d..d8701a2e8 100644
--- a/pdns/dynhandler.cc
+++ b/pdns/dynhandler.cc
@@ -129,7 +129,10 @@ string DLPurgeHandler(const vector<string>&parts, Utility::pid_t ppid)
   if(parts.size()>1) {
     for (vector<string>::const_iterator i=++parts.begin();i<parts.end();++i) {
       ret+=PC.purge(*i);
-      dk.clearCaches(DNSName(*i));
+      if(!boost::ends_with(*i, "$"))
+	dk.clearCaches(DNSName(*i));
+      else
+	dk.clearAllCaches(); // at least we do what we promise.. and a bit more!
     }
   }
   else {
diff --git a/pdns/packetcache.cc b/pdns/packetcache.cc
index 8cde6b515..ac6027bc4 100644
--- a/pdns/packetcache.cc
+++ b/pdns/packetcache.cc
@@ -98,7 +98,7 @@ int PacketCache::get(DNSPacket *p, DNSPacket *cached, bool recursive)
   string value;
   bool haveSomething;
   {
-    MapCombo& mc=getMap(pcReverse(p->qdomain));
+    MapCombo& mc=getMap(p->qdomain);
     TryReadLock l(&mc.d_mut); // take a readlock here
     if(!l.gotIt()) {
       S.inc("deferred-cache-lookup");
@@ -175,7 +175,7 @@ void PacketCache::insert(const DNSName &qname, const QType& qtype, CacheEntryTyp
   CacheEntry val;
   val.created=time(0);
   val.ttd=val.created+ttl;
-  val.qname=pcReverse(qname);
+  val.qname=qname;
   val.qtype=qtype.getCode();
   val.value=value;
   val.ctype=cet;
@@ -216,36 +216,35 @@ int PacketCache::purge()
 int PacketCache::purge(const string &match)
 {
   int delcount=0;
-
-  BOOST_FOREACH(MapCombo& mc, d_maps) {
-    WriteLock l(&mc.d_mut);
-    
-    if(ends_with(match, "$")) {
-      string prefix(match);
-      prefix.resize(prefix.size()-1);
-
-      string zone = pcReverse(DNSName(prefix));
-
-      cmap_t::const_iterator iter = mc.d_map.lower_bound(tie(zone));
-      cmap_t::const_iterator start=iter;
+  if(ends_with(match, "$")) {
+    string prefix(match);
+    prefix.resize(prefix.size()-1);
+    DNSName dprefix(prefix);
+    for(MapCombo& mc : d_maps) {
+      WriteLock l(&mc.d_mut);
+      cmap_t::const_iterator iter = mc.d_map.lower_bound(tie(dprefix));
+      auto start=iter;
 
       for(; iter != mc.d_map.end(); ++iter) {
-	if(iter->qname.compare(0, zone.size(), zone) != 0) {
+	if(!iter->qname.isPartOf(dprefix)) {
 	  break;
 	}
 	delcount++;
       }
       mc.d_map.erase(start, iter);
     }
-  
-    else {
-      string qname = pcReverse(DNSName(match));
-      
-      delcount+=mc.d_map.count(tie(qname));
-      pair<cmap_t::iterator, cmap_t::iterator> range = mc.d_map.equal_range(tie(qname));
+  }
+  else {
+    DNSName dn(match);
+    auto mc = getMap(dn);
+    WriteLock l(&mc.d_mut);
+    auto range = mc.d_map.equal_range(tie(dn));
+    if(range.first != range.second) {
+      delcount+=distance(range.first, range.second) - 1;
       mc.d_map.erase(range.first, range.second);
     }
   }
+
   *d_statnumentries-=delcount; // XXX FIXME NEEDS TO BE ADJUSTED
   return delcount;
 }
@@ -260,7 +259,7 @@ bool PacketCache::getEntry(const DNSName &qname, const QType& qtype, CacheEntryT
     cleanup();
   }
 
-  MapCombo& mc=getMap(pcReverse(qname));
+  MapCombo& mc=getMap(qname);
 
   TryReadLock l(&mc.d_mut); // take a readlock here
   if(!l.gotIt()) {
@@ -277,9 +276,8 @@ bool PacketCache::getEntryLocked(const DNSName &qname, const QType& qtype, Cache
 {
   uint16_t qt = qtype.getCode();
   //cerr<<"Lookup for maxReplyLen: "<<maxReplyLen<<endl;
-  string pcqname = pcReverse(qname);
-  MapCombo& mc=getMap(pcqname);
-  cmap_t::const_iterator i=mc.d_map.find(tie(pcqname, qt, cet, zoneID, meritsRecursion, maxReplyLen, dnssecOK, hasEDNS, *age));
+  MapCombo& mc=getMap(qname);
+  cmap_t::const_iterator i=mc.d_map.find(tie(qname, qt, cet, zoneID, meritsRecursion, maxReplyLen, dnssecOK, hasEDNS, *age));
   time_t now=time(0);
   bool ret=(i!=mc.d_map.end() && i->ttd > now);
   if(ret) {
@@ -292,12 +290,6 @@ bool PacketCache::getEntryLocked(const DNSName &qname, const QType& qtype, Cache
 }
 
 
-string PacketCache::pcReverse(DNSName content)
-{
-  string ret=content.labelReverse().toDNSString();
-  return ret.substr(0, ret.size()-1);
-}
-
 map<char,int> PacketCache::getCounts()
 {
   int recursivePackets=0, nonRecursivePackets=0, queryCacheEntries=0, negQueryCacheEntries=0;
diff --git a/pdns/packetcache.hh b/pdns/packetcache.hh
index 7f98c75f8..1a28689be 100644
--- a/pdns/packetcache.hh
+++ b/pdns/packetcache.hh
@@ -72,12 +72,12 @@ public:
 private:
   bool getEntryLocked(const DNSName &content, const QType& qtype, CacheEntryType cet, string& entry, int zoneID=-1,
     bool meritsRecursion=false, unsigned int maxReplyLen=512, bool dnssecOk=false, bool hasEDNS=false, unsigned int *age=0);
-  string pcReverse(DNSName content);
+
   struct CacheEntry
   {
     CacheEntry() { qtype = ctype = 0; zoneID = -1; meritsRecursion=false; dnssecOk=false; hasEDNS=false; created=0; ttd=0; maxReplyLen=512;}
 
-    string qname;
+    DNSName qname;
     string value;
     time_t created;
     time_t ttd;
@@ -100,7 +100,7 @@ private:
                 ordered_unique<
                       composite_key< 
                         CacheEntry,
-                        member<CacheEntry,string,&CacheEntry::qname>,
+                        member<CacheEntry,DNSName,&CacheEntry::qname>,
                         member<CacheEntry,uint16_t,&CacheEntry::qtype>,
                         member<CacheEntry,uint16_t, &CacheEntry::ctype>,
                         member<CacheEntry,int, &CacheEntry::zoneID>,
@@ -109,7 +109,7 @@ private:
                         member<CacheEntry,bool, &CacheEntry::dnssecOk>,
                         member<CacheEntry,bool, &CacheEntry::hasEDNS>
                         >,
-                        composite_key_compare<std::less<string>, std::less<uint16_t>, std::less<uint16_t>, std::less<int>, std::less<bool>, 
+		       composite_key_compare<CanonDNSNameCompare, std::less<uint16_t>, std::less<uint16_t>, std::less<int>, std::less<bool>, 
                           std::less<unsigned int>, std::less<bool>, std::less<bool> >
                             >,
                            sequenced<>
@@ -124,9 +124,9 @@ private:
   };
 
   vector<MapCombo> d_maps;
-  MapCombo& getMap(const std::string& qname) 
+  MapCombo& getMap(const DNSName& qname) 
   {
-    return d_maps[burtle((const unsigned char*)qname.c_str(), qname.length(), 0) % d_maps.size()];
+    return d_maps[hash_value(qname) % d_maps.size()];
   }
 
   AtomicCounter d_ops;