dnsdist: Prevent a copy of a pool's backends when selecting a server

diff --git a/pdns/dnsdist-lbpolicies.hh b/pdns/dnsdist-lbpolicies.hh
index ec2cad6c6029d4577274da8ad9782a15c6e62cd7..7ad5f25cff1f94e0236373aca0cd100199176f05 100644 (file)
--- a/pdns/dnsdist-lbpolicies.hh
+++ b/pdns/dnsdist-lbpolicies.hh
@@ -64,7 +64,7 @@ void setPoolPolicy(pools_t& pools, const string& poolName, std::shared_ptr<Serve
 void addServerToPool(pools_t& pools, const string& poolName, std::shared_ptr<DownstreamState> server);
 void removeServerFromPool(pools_t& pools, const string& poolName, std::shared_ptr<DownstreamState> server);
 
-ServerPolicy::NumberedServerVector getDownstreamCandidates(const map<std::string,std::shared_ptr<ServerPool>>& pools, const std::string& poolName);
+const std::shared_ptr<ServerPolicy::NumberedServerVector> getDownstreamCandidates(const map<std::string,std::shared_ptr<ServerPool>>& pools, const std::string& poolName);
 
 std::shared_ptr<DownstreamState> firstAvailable(const ServerPolicy::NumberedServerVector& servers, const DNSQuestion* dq);
 

diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc
index 39d2e595ad94d6206e3447098856f7e5ef3ef8a7..8c4995e01b1321f80c3a845a8778b9b4b2164b9d 100644 (file)
--- a/pdns/dnsdist-lua.cc
+++ b/pdns/dnsdist-lua.cc
@@ -758,7 +758,8 @@ static void setupLuaConfig(bool client, bool configCheck)
     });
 
   g_lua.writeFunction("getPoolServers", [](string pool) {
-      return getDownstreamCandidates(g_pools.getCopy(), pool);
+      const auto poolServers = getDownstreamCandidates(g_pools.getCopy(), pool);
+      return *poolServers;
     });
 
   g_lua.writeFunction("getServer", [client](boost::variant<unsigned int, std::string> i) {
@@ -1437,7 +1438,8 @@ static void setupLuaConfig(bool client, bool configCheck)
           }
           string servers;
 
-          for (const auto& server: pool->getServers()) {
+          const auto poolServers = pool->getServers();
+          for (const auto& server: *poolServers) {
             if (!servers.empty()) {
               servers += ", ";
             }

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 00526b167a34a7261e9e9db135e0209b85a2a4cc..9346c386e1aa24e23e8c928803872db3720860ed 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -1193,8 +1193,8 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders&
     if (serverPool->policy != nullptr) {
       policy = *(serverPool->policy);
     }
-    auto servers = serverPool->getServers();
-    selectedBackend = getSelectedBackendFromPolicy(policy, servers, dq);
+    const auto servers = serverPool->getServers();
+    selectedBackend = getSelectedBackendFromPolicy(policy, *servers, dq);
 
     uint16_t cachedResponseSize = dq.size;
     uint32_t allowExpired = selectedBackend ? 0 : g_staleCacheEntriesTTL;

diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh
index 1ffb3aeb7454981277997cba7aae49cc897a24f6..f0c98a94cfdbb09fcbe975cffe57f49907a663f0 100644 (file)
--- a/pdns/dnsdist.hh
+++ b/pdns/dnsdist.hh
@@ -912,9 +912,10 @@ public:
 
 struct ServerPool
 {
-  ServerPool()
+  ServerPool(): d_servers(std::make_shared<ServerPolicy::NumberedServerVector>())
   {
   }
+
   ~ServerPool()
   {
   }
@@ -938,7 +939,7 @@ struct ServerPool
   {
     size_t count = 0;
     ReadLock rl(&d_lock);
-    for (const auto& server : d_servers) {
+    for (const auto& server : *d_servers) {
       if (!upOnly || std::get<1>(server)->isUp() ) {
         count++;
       }
@@ -946,9 +947,9 @@ struct ServerPool
     return count;
   }
 
-  ServerPolicy::NumberedServerVector getServers()
+  const std::shared_ptr<ServerPolicy::NumberedServerVector> getServers()
   {
-    ServerPolicy::NumberedServerVector result;
+    std::shared_ptr<ServerPolicy::NumberedServerVector> result;
     {
       ReadLock rl(&d_lock);
       result = d_servers;
@@ -959,25 +960,32 @@ struct ServerPool
   void addServer(shared_ptr<DownstreamState>& server)
   {
     WriteLock wl(&d_lock);
-    unsigned int count = (unsigned int) d_servers.size();
-    d_servers.push_back(make_pair(++count, server));
+    /* we can't update the content of the shared pointer directly even when holding the lock,
+       as other threads might hold a copy. We can however update the pointer as long as we hold the lock. */
+    unsigned int count = static_cast<unsigned int>(d_servers->size());
+    auto newServers = std::make_shared<ServerPolicy::NumberedServerVector>(*d_servers);
+    newServers->push_back(make_pair(++count, server));
     /* we need to reorder based on the server 'order' */
-    std::stable_sort(d_servers.begin(), d_servers.end(), [](const std::pair<unsigned int,std::shared_ptr<DownstreamState> >& a, const std::pair<unsigned int,std::shared_ptr<DownstreamState> >& b) {
+    std::stable_sort(newServers->begin(), newServers->end(), [](const std::pair<unsigned int,std::shared_ptr<DownstreamState> >& a, const std::pair<unsigned int,std::shared_ptr<DownstreamState> >& b) {
       return a.second->order < b.second->order;
     });
     /* and now we need to renumber for Lua (custom policies) */
     size_t idx = 1;
-    for (auto& serv : d_servers) {
+    for (auto& serv : *newServers) {
       serv.first = idx++;
     }
+    d_servers = newServers;
   }
 
   void removeServer(shared_ptr<DownstreamState>& server)
   {
     WriteLock wl(&d_lock);
+    /* we can't update the content of the shared pointer directly even when holding the lock,
+       as other threads might hold a copy. We can however update the pointer as long as we hold the lock. */
+    auto newServers = std::make_shared<ServerPolicy::NumberedServerVector>(*d_servers);
     size_t idx = 1;
     bool found = false;
-    for (auto it = d_servers.begin(); it != d_servers.end();) {
+    for (auto it = newServers->begin(); it != newServers->end();) {
       if (found) {
         /* we need to renumber the servers placed
            after the removed one, for Lua (custom policies) */
@@ -985,17 +993,18 @@ struct ServerPool
         it++;
       }
       else if (it->second == server) {
-        it = d_servers.erase(it);
+        it = newServers->erase(it);
         found = true;
       } else {
         idx++;
         it++;
       }
     }
+    d_servers = newServers;
   }
 
 private:
-  ServerPolicy::NumberedServerVector d_servers;
+  std::shared_ptr<ServerPolicy::NumberedServerVector> d_servers;
   ReadWriteLock d_lock;
   bool d_useECS{false};
 };

diff --git a/pdns/dnsdistdist/dnsdist-lbpolicies.cc b/pdns/dnsdistdist/dnsdist-lbpolicies.cc
index a36cc16afd783c3d556becd68e465e8563857921..0dfc7e0c562f66cdefa001d259a1398a817b2976 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-lbpolicies.cc
+++ b/pdns/dnsdistdist/dnsdist-lbpolicies.cc
@@ -218,7 +218,7 @@ shared_ptr<DownstreamState> roundrobin(const ServerPolicy::NumberedServerVector&
   return (*res)[(counter++) % res->size()].second;
 }
 
-ServerPolicy::NumberedServerVector getDownstreamCandidates(const pools_t& pools, const std::string& poolName)
+const std::shared_ptr<ServerPolicy::NumberedServerVector> getDownstreamCandidates(const pools_t& pools, const std::string& poolName)
 {
   std::shared_ptr<ServerPool> pool = getPool(pools, poolName);
   return pool->getServers();