From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 20 May 2021 08:51:40 +0000 (+0200)
Subject: auth: Convert the TCPNameserver to LockGuarded
X-Git-Tag: dnsdist-1.7.0-alpha1~11^2~14
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=24fb57748b45171b0d1444d1cd164647c0844a6a;p=thirdparty%2Fpdns.git

auth: Convert the TCPNameserver to LockGuarded
---

diff --git a/pdns/tcpreceiver.cc b/pdns/tcpreceiver.cc
index b9c619eb2d..716236febc 100644
--- a/pdns/tcpreceiver.cc
+++ b/pdns/tcpreceiver.cc
@@ -66,24 +66,22 @@ extern StatBag S;
 \brief This file implements the tcpreceiver that receives and answers questions over TCP/IP
 */
 
-std::mutex TCPNameserver::s_plock;
 std::unique_ptr<Semaphore> TCPNameserver::d_connectionroom_sem{nullptr};
-std::unique_ptr<PacketHandler> TCPNameserver::s_P{nullptr};
+LockGuarded<std::unique_ptr<PacketHandler>> TCPNameserver::s_P{nullptr};
 unsigned int TCPNameserver::d_maxTCPConnections = 0;
 NetmaskGroup TCPNameserver::d_ng;
 size_t TCPNameserver::d_maxTransactionsPerConn;
 size_t TCPNameserver::d_maxConnectionsPerClient;
 unsigned int TCPNameserver::d_idleTimeout;
 unsigned int TCPNameserver::d_maxConnectionDuration;
-std::mutex TCPNameserver::s_clientsCountMutex;
-std::map<ComboAddress,size_t,ComboAddress::addressOnlyLessThan> TCPNameserver::s_clientsCount;
+LockGuarded<std::map<ComboAddress,size_t,ComboAddress::addressOnlyLessThan>> TCPNameserver::s_clientsCount;
 
 void TCPNameserver::go()
 {
   g_log<<Logger::Error<<"Creating backend connection for TCP"<<endl;
-  s_P.reset();
+  s_P.lock()->reset();
   try {
-    s_P=make_unique<PacketHandler>();
+    *(s_P.lock()) = make_unique<PacketHandler>();
   }
   catch(PDNSException &ae) {
     g_log<<Logger::Error<<"TCP server is unable to launch backends - will try again when questions come in: "<<ae.reason<<endl;
@@ -204,10 +202,15 @@ static bool maxConnectionDurationReached(unsigned int maxConnectionDuration, tim
 void TCPNameserver::decrementClientCount(const ComboAddress& remote)
 {
   if (d_maxConnectionsPerClient) {
-    std::lock_guard<std::mutex> lock(s_clientsCountMutex);
-    s_clientsCount[remote]--;
-    if (s_clientsCount[remote] == 0) {
-      s_clientsCount.erase(remote);
+    auto count = s_clientsCount.lock();
+    auto it = count->find(remote);
+    if (it == count->end()) {
+      // this is worrying, but nothing we can do at this point
+      return;
+    }
+    --it->second;
+    if (it->second == 0) {
+      count->erase(it);
     }
   }
 }
@@ -334,13 +337,13 @@ void TCPNameserver::doConnection(int fd)
         }
       }
       {
-        std::lock_guard<std::mutex> l(s_plock);
-        if(!s_P) {
+        auto packetHandler = s_P.lock();
+        if (!*packetHandler) {
           g_log<<Logger::Warning<<"TCP server is without backend connections, launching"<<endl;
-          s_P=make_unique<PacketHandler>();
+          *packetHandler = make_unique<PacketHandler>();
         }
 
-        reply= s_P->doQuestion(*packet); // we really need to ask the backend :-)
+        reply = (*packetHandler)->doQuestion(*packet); // we really need to ask the backend :-)
       }
 
       if(!reply)  // unable to write an answer?
@@ -350,8 +353,7 @@ void TCPNameserver::doConnection(int fd)
     }
   }
   catch(PDNSException &ae) {
-    std::lock_guard<std::mutex> l(s_plock);
-    s_P.reset(); // on next call, backend will be recycled
+    s_P.lock()->reset(); // on next call, backend will be recycled
     g_log<<Logger::Error<<"TCP nameserver had error, cycling backend: "<<ae.reason<<endl;
   }
   catch(NetworkError &e) {
@@ -377,8 +379,7 @@ void TCPNameserver::doConnection(int fd)
 }
 
 
-// call this method with s_plock held!
-bool TCPNameserver::canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR)
+bool TCPNameserver::canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR, std::unique_ptr<PacketHandler>& packetHandler)
 {
   if(::arg().mustDo("disable-axfr"))
     return false;
@@ -389,13 +390,13 @@ bool TCPNameserver::canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR)
     TSIGRecordContent trc;
     DNSName keyname;
     string secret;
-    if(!q->checkForCorrectTSIG(s_P->getBackend(), &keyname, &secret, &trc)) {
+    if(!q->checkForCorrectTSIG(packetHandler->getBackend(), &keyname, &secret, &trc)) {
       return false;
     } else {
       getTSIGHashEnum(trc.d_algoName, q->d_tsig_algo);
     }
 
-    DNSSECKeeper dk(s_P->getBackend());
+    DNSSECKeeper dk(packetHandler->getBackend());
     if(!dk.TSIGGrantsAccess(q->qdomain, keyname)) {
       g_log<<Logger::Warning<<logPrefix<<"denied: key with name '"<<keyname<<"' and algorithm '"<<getTSIGAlgoName(q->d_tsig_algo)<<"' does not grant access"<<endl;
       return false;
@@ -416,10 +417,10 @@ bool TCPNameserver::canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR)
 
   // cerr<<"doing per-zone-axfr-acls"<<endl;
   SOAData sd;
-  if(s_P->getBackend()->getSOAUncached(q->qdomain,sd)) {
+  if(packetHandler->getBackend()->getSOAUncached(q->qdomain,sd)) {
     // cerr<<"got backend and SOA"<<endl;
     vector<string> acl;
-    s_P->getBackend()->getDomainMetadata(q->qdomain, "ALLOW-AXFR-FROM", acl);
+    packetHandler->getBackend()->getDomainMetadata(q->qdomain, "ALLOW-AXFR-FROM", acl);
     for (const auto & i : acl) {
       // cerr<<"matching against "<<*i<<endl;
       if(pdns_iequals(i, "AUTO-NS")) {
@@ -433,7 +434,7 @@ bool TCPNameserver::canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR)
           nsset.insert(DNSName(rr.content));
         }
         for(const auto & j: nsset) {
-          vector<string> nsips=fns.lookup(j, s_P->getBackend());
+          vector<string> nsips=fns.lookup(j, packetHandler->getBackend());
           for(const auto & nsip : nsips) {
             // cerr<<"got "<<*k<<" from AUTO-NS"<<endl;
             if(nsip == q->getRemote().toString())
@@ -502,22 +503,22 @@ int TCPNameserver::doAXFR(const DNSName &target, std::unique_ptr<DNSPacket>& q,
   // determine if zone exists and AXFR is allowed using existing backend before spawning a new backend.
   SOAData sd;
   {
-    std::lock_guard<std::mutex> l(s_plock);
+    auto packetHandler = s_P.lock();
     DLOG(g_log<<logPrefix<<"looking for SOA"<<endl);    // find domain_id via SOA and list complete domain. No SOA, no AXFR
-    if(!s_P) {
+    if(!*packetHandler) {
       g_log<<Logger::Warning<<"TCP server is without backend connections in doAXFR, launching"<<endl;
-      s_P=make_unique<PacketHandler>();
+      *packetHandler = make_unique<PacketHandler>();
     }
 
     // canDoAXFR does all the ACL checks, and has the if(disable-axfr) shortcut, call it first.
-    if (!canDoAXFR(q, true)) {
+    if (!canDoAXFR(q, true, *packetHandler)) {
       g_log<<Logger::Warning<<logPrefix<<"failed: client may not request AXFR"<<endl;
       outpacket->setRcode(RCode::NotAuth);
       sendPacket(outpacket,outsock);
       return 0;
     }
 
-    if(!s_P->getBackend()->getSOAUncached(target, sd)) {
+    if(!(*packetHandler)->getBackend()->getSOAUncached(target, sd)) {
       g_log<<Logger::Warning<<logPrefix<<"failed: not authoritative"<<endl;
       outpacket->setRcode(RCode::NotAuth);
       sendPacket(outpacket,outsock);
@@ -1067,22 +1068,22 @@ int TCPNameserver::doIXFR(std::unique_ptr<DNSPacket>& q, int outsock)
   bool securedZone;
   bool serialPermitsIXFR;
   {
-    std::lock_guard<std::mutex> l(s_plock);
+    auto packetHandler = s_P.lock();
     DLOG(g_log<<logPrefix<<"Looking for SOA"<<endl); // find domain_id via SOA and list complete domain. No SOA, no IXFR
-    if(!s_P) {
+    if(!*packetHandler) {
       g_log<<Logger::Warning<<"TCP server is without backend connections in doIXFR, launching"<<endl;
-      s_P=make_unique<PacketHandler>();
+      *packetHandler = make_unique<PacketHandler>();
     }
 
     // canDoAXFR does all the ACL checks, and has the if(disable-axfr) shortcut, call it first.
-    if(!canDoAXFR(q, false) || !s_P->getBackend()->getSOAUncached(q->qdomain, sd)) {
+    if(!canDoAXFR(q, false, *packetHandler) || !(*packetHandler)->getBackend()->getSOAUncached(q->qdomain, sd)) {
       g_log<<Logger::Warning<<logPrefix<<"failed: not authoritative"<<endl;
       outpacket->setRcode(RCode::NotAuth);
       sendPacket(outpacket,outsock);
       return 0;
     }
 
-    DNSSECKeeper dk(s_P->getBackend());
+    DNSSECKeeper dk((*packetHandler)->getBackend());
     DNSSECKeeper::clearCaches(q->qdomain);
     bool narrow;
     securedZone = dk.isSecuredZone(q->qdomain);
@@ -1260,13 +1261,13 @@ void TCPNameserver::thread()
           }
           else {
             if (d_maxConnectionsPerClient) {
-              std::lock_guard<std::mutex> lock(s_clientsCountMutex);
-              if (s_clientsCount[remote] >= d_maxConnectionsPerClient) {
+              auto clientsCount = s_clientsCount.lock();
+              if ((*clientsCount)[remote] >= d_maxConnectionsPerClient) {
                 g_log<<Logger::Notice<<"Limit of simultaneous TCP connections per client reached for "<< remote<<", dropping"<<endl;
                 close(fd);
                 continue;
               }
-              s_clientsCount[remote]++;
+              (*clientsCount)[remote]++;
             }
 
             d_connectionroom_sem->wait(); // blocks if no connections are available
diff --git a/pdns/tcpreceiver.hh b/pdns/tcpreceiver.hh
index 366921a91e..e996adc1fe 100644
--- a/pdns/tcpreceiver.hh
+++ b/pdns/tcpreceiver.hh
@@ -25,7 +25,6 @@
 #include "dnsbackend.hh"
 #include "packethandler.hh"
 #include <vector>
-#include <mutex>
 #include <poll.h>
 #include <sys/select.h>
 #include <sys/socket.h>
@@ -37,6 +36,7 @@
 #include <sys/uio.h>
 #include <sys/select.h>
 
+#include "lock.hh"
 #include "namespaces.hh"
 
 class TCPNameserver
@@ -53,14 +53,12 @@ private:
   static void getQuestion(int fd, char *mesg, int pktlen, const ComboAddress& remote, unsigned int totalTime);
   static int doAXFR(const DNSName &target, std::unique_ptr<DNSPacket>& q, int outsock);
   static int doIXFR(std::unique_ptr<DNSPacket>& q, int outsock);
-  static bool canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR);
+  static bool canDoAXFR(std::unique_ptr<DNSPacket>& q, bool isAXFR, std::unique_ptr<PacketHandler>& packetHandler);
   static void doConnection(int fd);
   static void decrementClientCount(const ComboAddress& remote);
   void thread(void);
-  static std::mutex s_plock;
-  static std::mutex s_clientsCountMutex;
-  static std::map<ComboAddress,size_t,ComboAddress::addressOnlyLessThan> s_clientsCount;
-  static std::unique_ptr<PacketHandler> s_P;
+  static LockGuarded<std::map<ComboAddress,size_t,ComboAddress::addressOnlyLessThan>> s_clientsCount;
+  static LockGuarded<std::unique_ptr<PacketHandler>> s_P;
   static std::unique_ptr<Semaphore> d_connectionroom_sem;
   static unsigned int d_maxTCPConnections;
   static NetmaskGroup d_ng;