From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Fri, 14 Feb 2020 09:22:12 +0000 (+0100)
Subject: Avoid copying policies around by passing a Policy& that gets modified
X-Git-Tag: auth-4.3.0-beta2~11^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2996400c6411b2cbd0b0d8f5d811f7daecad5282;p=thirdparty%2Fpdns.git

Avoid copying policies around by passing a Policy& that gets modified
if a match is found.
---

diff --git a/pdns/filterpo.cc b/pdns/filterpo.cc
index 273cd7cb71..95f7b70d64 100644
--- a/pdns/filterpo.cc
+++ b/pdns/filterpo.cc
@@ -115,7 +115,7 @@ bool DNSFilterEngine::Zone::findExactNamedPolicy(const std::unordered_map<DNSNam
   return false;
 }
 
-DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const DNSName& qname, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const
+bool DNSFilterEngine::getProcessingPolicy(const DNSName& qname, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& pol) const
 {
   // cout<<"Got question for nameserver name "<<qname<<endl;
   std::vector<bool> zoneEnabled(d_zones.size());
@@ -124,7 +124,7 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const DNSName& qnam
   for (const auto& z : d_zones) {
     bool enabled = true;
     const auto zoneName = z->getName();
-    if (z->getPriority() >= maxPriority) {
+    if (z->getPriority() >= pol.d_priority) {
       enabled = false;
     }
     else if (zoneName && discardedPolicies.find(*zoneName) != discardedPolicies.end()) {
@@ -143,9 +143,8 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const DNSName& qnam
     ++count;
   }
 
-  Policy pol;
   if (allEmpty) {
-    return pol;
+    return false;
   }
 
   /* prepare the wildcard-based names */
@@ -164,27 +163,26 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const DNSName& qnam
     }
     if (z->findExactNSPolicy(qname, pol)) {
       // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-      return pol;
+      return true;
     }
 
     for (const auto& wc : wcNames) {
       if (z->findExactNSPolicy(wc, pol)) {
         // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-        return pol;
+        return true;
       }
     }
     ++count;
   }
 
-  return pol;
+  return false;
 }
 
-DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const ComboAddress& address, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const
+bool DNSFilterEngine::getProcessingPolicy(const ComboAddress& address, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& pol) const
 {
-  Policy pol;
   //  cout<<"Got question for nameserver IP "<<address.toString()<<endl;
   for(const auto& z : d_zones) {
-    if (z->getPriority() >= maxPriority) {
+    if (z->getPriority() >= pol.d_priority) {
       break;
     }
     const auto zoneName = z->getName();
@@ -194,13 +192,13 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const ComboAddress&
 
     if(z->findNSIPPolicy(address, pol)) {
       //      cerr<<"Had a hit on the nameserver ("<<address.toString()<<") used to process the query"<<endl;
-      return pol;
+      return true;
     }
   }
-  return pol;
+  return false;
 }
 
-DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, const ComboAddress& ca, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const
+bool DNSFilterEngine::getQueryPolicy(const DNSName& qname, const ComboAddress& ca, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& pol) const
 {
   // cout<<"Got question for "<<qname<<" from "<<ca.toString()<<endl;
   std::vector<bool> zoneEnabled(d_zones.size());
@@ -208,7 +206,7 @@ DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, co
   bool allEmpty = true;
   for (const auto& z : d_zones) {
     bool enabled = true;
-    if (z->getPriority() >= maxPriority) {
+    if (z->getPriority() >= pol.d_priority) {
       enabled = false;
     } else {
       const auto zoneName = z->getName();
@@ -229,9 +227,8 @@ DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, co
     ++count;
   }
 
-  Policy pol;
   if (allEmpty) {
-    return pol;
+    return false;
   }
 
   /* prepare the wildcard-based names */
@@ -251,30 +248,29 @@ DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, co
 
     if (z->findClientPolicy(ca, pol)) {
       // cerr<<"Had a hit on the IP address ("<<ca.toString()<<") of the client"<<endl;
-      return pol;
+      return true;
     }
 
     if (z->findExactQNamePolicy(qname, pol)) {
       // cerr<<"Had a hit on the name of the query"<<endl;
-      return pol;
+      return true;
     }
 
     for (const auto& wc : wcNames) {
       if (z->findExactQNamePolicy(wc, pol)) {
         // cerr<<"Had a hit on the name of the query"<<endl;
-        return pol;
+        return true;
       }
     }
 
     ++count;
   }
 
-  return pol;
+  return false;
 }
 
-DNSFilterEngine::Policy DNSFilterEngine::getPostPolicy(const vector<DNSRecord>& records, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const
+bool DNSFilterEngine::getPostPolicy(const vector<DNSRecord>& records, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& pol) const
 {
-  Policy pol;
   ComboAddress ca;
   for (const auto& r : records) {
     if (r.d_place != DNSResourceRecord::ANSWER)
@@ -293,7 +289,7 @@ DNSFilterEngine::Policy DNSFilterEngine::getPostPolicy(const vector<DNSRecord>&
       continue;
 
     for (const auto& z : d_zones) {
-      if (z->getPriority() >= maxPriority) {
+      if (z->getPriority() >= pol.d_priority) {
         break;
       }
       const auto zoneName = z->getName();
@@ -302,11 +298,11 @@ DNSFilterEngine::Policy DNSFilterEngine::getPostPolicy(const vector<DNSRecord>&
       }
 
       if (z->findResponsePolicy(ca, pol)) {
-	return pol;
+	return true;
       }
     }
   }
-  return pol;
+  return false;
 }
 
 void DNSFilterEngine::assureZones(size_t zone)
diff --git a/pdns/filterpo.hh b/pdns/filterpo.hh
index d0ef6fb26f..aea360658a 100644
--- a/pdns/filterpo.hh
+++ b/pdns/filterpo.hh
@@ -282,10 +282,39 @@ public:
     }
   }
 
-  Policy getQueryPolicy(const DNSName& qname, const ComboAddress& nm, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const;
-  Policy getProcessingPolicy(const DNSName& qname, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const;
-  Policy getProcessingPolicy(const ComboAddress& address, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const;
-  Policy getPostPolicy(const vector<DNSRecord>& records, const std::unordered_map<std::string,bool>& discardedPolicies, Priority maxPriority) const;
+  bool getQueryPolicy(const DNSName& qname, const ComboAddress& nm, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& policy) const;
+  bool getProcessingPolicy(const DNSName& qname, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& policy) const;
+  bool getProcessingPolicy(const ComboAddress& address, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& policy) const;
+  bool getPostPolicy(const vector<DNSRecord>& records, const std::unordered_map<std::string,bool>& discardedPolicies, Policy& policy) const;
+
+  // A few convenience methods for the unit test code
+  Policy getQueryPolicy(const DNSName& qname, const ComboAddress& nm, const std::unordered_map<std::string,bool>& discardedPolicies, Priority p) const {
+    Policy policy;
+    policy.d_priority = p;
+    getQueryPolicy(qname, nm, discardedPolicies, policy);
+    return policy;
+  }
+
+  Policy getProcessingPolicy(const DNSName& qname, const std::unordered_map<std::string,bool>& discardedPolicies, Priority p) const {
+    Policy policy;
+    policy.d_priority = p;
+    getProcessingPolicy(qname, discardedPolicies, policy);
+    return policy;
+  }
+
+  Policy getProcessingPolicy(const ComboAddress& address, const std::unordered_map<std::string,bool>& discardedPolicies, Priority p) const {
+    Policy policy;
+    policy.d_priority = p;
+    getProcessingPolicy(address, discardedPolicies, policy);
+    return policy;
+  }
+
+  Policy getPostPolicy(const vector<DNSRecord>& records, const std::unordered_map<std::string,bool>& discardedPolicies, Priority p) const {
+    Policy policy;
+    policy.d_priority = p;
+    getPostPolicy(records, discardedPolicies, policy);
+    return policy;
+  }
 
   size_t size() const {
     return d_zones.size();
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 001d6f2ada..7349643eb3 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -1327,7 +1327,7 @@ static void startDoResolve(void *p)
 
     // Check if the query has a policy attached to it
     if (wantsRPZ && (appliedPolicy.d_type == DNSFilterEngine::PolicyType::None || appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction)) {
-      appliedPolicy = luaconfsLocal->dfe.getQueryPolicy(dc->d_mdp.d_qname, dc->d_source, sr.d_discardedPolicies, appliedPolicy.d_priority);
+      luaconfsLocal->dfe.getQueryPolicy(dc->d_mdp.d_qname, dc->d_source, sr.d_discardedPolicies, appliedPolicy);
     }
 
     // if there is a RecursorLua active, and it 'took' the query in preResolve, we don't launch beginResolve
@@ -1386,6 +1386,7 @@ static void startDoResolve(void *p)
         res = -2;
       }
       dq.validationState = sr.getValidationState();
+      appliedPolicy = sr.d_appliedPolicy;
 
       // During lookup, an NSDNAME or NSIP trigger was hit in RPZ
       if (res == -2) { // XXX This block should be macro'd, it is repeated post-resolve.
@@ -1429,7 +1430,7 @@ static void startDoResolve(void *p)
       }
 
       if (wantsRPZ && (appliedPolicy.d_type == DNSFilterEngine::PolicyType::None || appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction)) {
-        appliedPolicy = luaconfsLocal->dfe.getPostPolicy(ret, sr.d_discardedPolicies, appliedPolicy.d_priority);
+        luaconfsLocal->dfe.getPostPolicy(ret, sr.d_discardedPolicies, appliedPolicy);
       }
 
       if(t_pdl) {
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 9c62b55485..095a2b1bbf 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1806,16 +1806,16 @@ bool SyncRes::nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& n
   */
   if (d_wantsRPZ && (d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None || d_appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction)) {
     for (auto const &ns : nameservers) {
-      d_appliedPolicy = dfe.getProcessingPolicy(ns.first, d_discardedPolicies, d_appliedPolicy.d_priority);
-      if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
+      bool match = dfe.getProcessingPolicy(ns.first, d_discardedPolicies, d_appliedPolicy);
+      if (match && d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
         LOG(", however nameserver "<<ns.first<<" was blocked by RPZ policy '"<<(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")<<"'"<<endl);
         return true;
       }
 
       // Traverse all IP addresses for this NS to see if they have an RPN NSIP policy
       for (auto const &address : ns.second.first) {
-        d_appliedPolicy = dfe.getProcessingPolicy(address, d_discardedPolicies, d_appliedPolicy.d_priority);
-        if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
+        match = dfe.getProcessingPolicy(address, d_discardedPolicies, d_appliedPolicy);
+        if (match && d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
           LOG(", however nameserver "<<ns.first<<" IP address "<<address.toString()<<" was blocked by RPZ policy '"<<(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")<<"'"<<endl);
           return true;
         }
@@ -1834,8 +1834,8 @@ bool SyncRes::nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAd
      process any further RPZ rules.
   */
   if (d_wantsRPZ && (d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None || d_appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction)) {
-    d_appliedPolicy = dfe.getProcessingPolicy(remoteIP, d_discardedPolicies, d_appliedPolicy.d_priority);
-    if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) {
+    bool match = dfe.getProcessingPolicy(remoteIP, d_discardedPolicies, d_appliedPolicy);
+    if (match && d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) {
       LOG(" (blocked by RPZ policy '"+(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")+"')");
       return true;
     }
@@ -3406,8 +3406,8 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn
     nameservers.clear();
     for (auto const &nameserver : nsset) {
       if (d_wantsRPZ && (d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None || d_appliedPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction)) {
-        d_appliedPolicy = dfe.getProcessingPolicy(nameserver, d_discardedPolicies, d_appliedPolicy.d_priority);
-        if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
+        bool match = dfe.getProcessingPolicy(nameserver, d_discardedPolicies, d_appliedPolicy);
+        if (match && d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
           LOG("however "<<nameserver<<" was blocked by RPZ policy '"<<(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")<<"'"<<endl);
           throw PolicyHitException();
         }