From 78007a118f7c6f5c500298c9e3ea39c0313f7e99 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Wed, 29 Jul 2020 09:38:38 +0200 Subject: [PATCH] dnsdist: Prevent a copy of a pool's backends when selecting a server --- pdns/dnsdist-lbpolicies.hh | 2 +- pdns/dnsdist-lua.cc | 6 +++-- pdns/dnsdist.cc | 4 ++-- pdns/dnsdist.hh | 31 +++++++++++++++++--------- pdns/dnsdistdist/dnsdist-lbpolicies.cc | 2 +- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/pdns/dnsdist-lbpolicies.hh b/pdns/dnsdist-lbpolicies.hh index ec2cad6c60..7ad5f25cff 100644 --- 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 server); void removeServerFromPool(pools_t& pools, const string& poolName, std::shared_ptr server); -ServerPolicy::NumberedServerVector getDownstreamCandidates(const map>& pools, const std::string& poolName); +const std::shared_ptr getDownstreamCandidates(const map>& pools, const std::string& poolName); std::shared_ptr firstAvailable(const ServerPolicy::NumberedServerVector& servers, const DNSQuestion* dq); diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index 39d2e595ad..8c4995e01b 100644 --- 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 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 00526b167a..9346c386e1 100644 --- 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 1ffb3aeb74..f0c98a94cf 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -912,9 +912,10 @@ public: struct ServerPool { - ServerPool() + ServerPool(): d_servers(std::make_shared()) { } + ~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 getServers() { - ServerPolicy::NumberedServerVector result; + std::shared_ptr result; { ReadLock rl(&d_lock); result = d_servers; @@ -959,25 +960,32 @@ struct ServerPool void addServer(shared_ptr& 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(d_servers->size()); + auto newServers = std::make_shared(*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 >& a, const std::pair >& b) { + std::stable_sort(newServers->begin(), newServers->end(), [](const std::pair >& a, const std::pair >& 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& 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(*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 d_servers; ReadWriteLock d_lock; bool d_useECS{false}; }; diff --git a/pdns/dnsdistdist/dnsdist-lbpolicies.cc b/pdns/dnsdistdist/dnsdist-lbpolicies.cc index a36cc16afd..0dfc7e0c56 100644 --- a/pdns/dnsdistdist/dnsdist-lbpolicies.cc +++ b/pdns/dnsdistdist/dnsdist-lbpolicies.cc @@ -218,7 +218,7 @@ shared_ptr 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 getDownstreamCandidates(const pools_t& pools, const std::string& poolName) { std::shared_ptr pool = getPool(pools, poolName); return pool->getServers(); -- 2.47.2