dnsdist: Fix a crash in the Downstream TCP handler

when we are looking for an existing TCP connection to a backend to
reuse, we routinely (every 60s by default) clean up existing
connections from the cache.
7b5f590ee72fecf54c0c40b24e98ba03a406af53 removes a connection
from the cache more aggressively when it has failed, but I did not
notice that the same function might be called from the cache cleaning
algorithm. It caused the cache cleanup function to call this function
which in turns tried to remove the connection from the same cache,
invalidating the iterator of the cache algorithm, and causing a crash
when the function returned.

diff --git a/pdns/dnsdistdist/dnsdist-downstream-connection.hh b/pdns/dnsdistdist/dnsdist-downstream-connection.hh
index f13cbcff224a90d2fcf3567ad5014cd222f5b76e..8d50de369f2efa8f4cd5d3b0e837a591b39e1130 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-downstream-connection.hh
+++ b/pdns/dnsdistdist/dnsdist-downstream-connection.hh
@@ -203,7 +203,7 @@ public:
 
     if (backendIt->second.d_idles.size() >= s_maxIdleConnectionsPerDownstream) {
       auto old = backendIt->second.d_idles.template get<SequencedTag>().back();
-      old->release();
+      old->release(false);
       backendIt->second.d_idles.template get<SequencedTag>().pop_back();
     }
 
@@ -231,7 +231,7 @@ protected:
 
       if (entry->isIdle() && entry->getLastDataReceivedTime() < idleCutOff) {
         /* idle for too long */
-        (*connIt)->release();
+        (*connIt)->release(false);
         connIt = sidx.erase(connIt);
         continue;
       }
@@ -242,7 +242,7 @@ protected:
       }
 
       if (entry->isIdle()) {
-        (*connIt)->release();
+        (*connIt)->release(false);
       }
       connIt = sidx.erase(connIt);
     }

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc
index bf1666f84246652abe004b7d63db05745891dab3..68d4f92962e2fbfb016997fb5b3b0b7ba73e35f5 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2.cc
@@ -70,8 +70,9 @@ public:
   bool reachedMaxConcurrentQueries() const override;
   bool reachedMaxStreamID() const override;
   bool isIdle() const override;
-  void release() override
+  void release(bool removeFromCache) override
   {
+    (void)removeFromCache;
   }
 
 private:

diff --git a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
index cf02303e9eeccbcfbac994ff8914b198faac2570..0e828939d5758e33c4f992775b543d2086cca02f 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
@@ -137,7 +137,8 @@ TCPConnectionToBackend::~TCPConnectionToBackend()
   }
 }
 
-void TCPConnectionToBackend::release(){
+void TCPConnectionToBackend::release(bool removeFromCache)
+{
   d_ds->outstanding -= d_pendingResponses.size();
 
   d_pendingResponses.clear();
@@ -147,8 +148,8 @@ void TCPConnectionToBackend::release(){
     d_ioState.reset();
   }
 
-  auto shared = std::dynamic_pointer_cast<TCPConnectionToBackend>(shared_from_this());
-  if (!willBeReusable(true)) {
+  if (removeFromCache && !willBeReusable(true)) {
+    auto shared = std::dynamic_pointer_cast<TCPConnectionToBackend>(shared_from_this());
     /* remove ourselves from the connection cache, this might mean that our
        reference count drops to zero after that, so we need to be careful */
     t_downstreamTCPConnectionsManager.removeDownstreamConnection(shared);
@@ -371,7 +372,7 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
           catch (const std::exception& e) {
             vinfolog("Got an exception while handling TCP response from %s (client is %s): %s", conn->d_ds ? conn->d_ds->getNameWithAddr() : "unknown", conn->d_currentQuery.d_query.d_idstate.origRemote.toStringWithPort(), e.what());
             ioGuard.release();
-            conn->release();
+            conn->release(true);
             return;
           }
         }
@@ -584,7 +585,7 @@ void TCPConnectionToBackend::handleTimeout(const struct timeval& now, bool write
     vinfolog("Got exception while notifying a timeout");
   }
 
-  release();
+  release(true);
 }
 
 void TCPConnectionToBackend::notifyAllQueriesFailed(const struct timeval& now, FailureReason reason)
@@ -647,7 +648,7 @@ void TCPConnectionToBackend::notifyAllQueriesFailed(const struct timeval& now, F
     vinfolog("Got exception while notifying");
   }
 
-  release();
+  release(true);
 }
 
 IOState TCPConnectionToBackend::handleResponse(std::shared_ptr<TCPConnectionToBackend>& conn, const struct timeval& now)

diff --git a/pdns/dnsdistdist/dnsdist-tcp-downstream.hh b/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
index a165dc18cb1282dcff0d5fc0c3e0e6d0c223040d..d2447e8b9cf8de4a88b7944cc760a263843af213 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
@@ -119,7 +119,7 @@ public:
   virtual bool reachedMaxStreamID() const = 0;
   virtual bool reachedMaxConcurrentQueries() const = 0;
   virtual bool isIdle() const = 0;
-  virtual void release() = 0;
+  virtual void release(bool removeFromCache) = 0;
   virtual void stopIO()
   {
   }
@@ -256,7 +256,7 @@ public:
 
   void queueQuery(std::shared_ptr<TCPQuerySender>& sender, TCPQuery&& query) override;
   void handleTimeout(const struct timeval& now, bool write) override;
-  void release() override;
+  void release(bool removeFromCache) override;
 
   std::string toString() const override
   {

diff --git a/pdns/dnsdistdist/dnsdist-tcp.cc b/pdns/dnsdistdist/dnsdist-tcp.cc
index da7e4e22325026dc4ec51c9a616e49fe1678215e..27b2a116e0dbab566ddc7659c390d47dd212d618 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp.cc
@@ -342,7 +342,7 @@ void IncomingTCPConnectionState::terminateClientConnection()
      we don't care about the ones still waiting for responses */
   for (auto& backend : d_ownedConnectionsToBackend) {
     for (auto& conn : backend.second) {
-      conn->release();
+      conn->release(true);
     }
   }
   d_ownedConnectionsToBackend.clear();
@@ -475,7 +475,7 @@ void IncomingTCPConnectionState::handleResponse(const struct timeval& now, TCPRe
         for (auto it = list.begin(); it != list.end(); ++it) {
           if (*it == response.d_connection) {
             try {
-              response.d_connection->release();
+              response.d_connection->release(true);
             }
             catch (const std::exception& e) {
               vinfolog("Error releasing connection: %s", e.what());

diff --git a/pdns/dnsdistdist/test-dnsdist-connections-cache.cc b/pdns/dnsdistdist/test-dnsdist-connections-cache.cc
index b6f86b6525ee0a210f403ec913ccbca3b0ac0589..fa113aa83a0f0e8e73242695f0741ad518169541 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdist-connections-cache.cc
+++ b/pdns/dnsdistdist/test-dnsdist-connections-cache.cc
@@ -71,7 +71,7 @@ public:
   {
   }
 
-  void release()
+  void release(bool removeFomCache)
   {
   }