dnsdist: Fix reentry issue in TCP downstream I/O on macOS/BSD

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc
index 1e289901f3ecdad75aad3fe9a95657a01021a3c9..c5be3a1c883a92b9d329d5636256856509b3dee5 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2.cc
@@ -379,7 +379,6 @@ void DoHConnectionToBackend::handleReadableIOCallback(int fd, FDMultiplexer::fun
   if (conn->d_inIOCallback) {
     return;
   }
-  conn->d_inIOCallback = true;
   dnsdist::tcp::HandlingIOGuard handlingIOGuard(conn->d_inIOCallback);
   IOStateGuard ioGuard(conn->d_ioState);
   do {

diff --git a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
index 1c91bba13753a623b9d079c3230ad78245f68a12..0e126bcf87e9c133fb3d5114cc5f394065a5a20a 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp-downstream.cc
@@ -279,6 +279,7 @@ IOState TCPConnectionToBackend::sendQuery(std::shared_ptr<TCPConnectionToBackend
   IOState state = conn->d_handler->tryWrite(conn->d_currentQuery.d_query.d_buffer, conn->d_currentPos, conn->d_currentQuery.d_query.d_buffer.size());
 
   if (state != IOState::Done) {
+    conn->d_lastIOBlocked = true;
     return state;
   }
 
@@ -310,6 +311,12 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
     throw std::runtime_error("No downstream socket in " + std::string(__PRETTY_FUNCTION__) + "!");
   }
 
+  if (conn->d_handlingIO) {
+    return;
+  }
+  conn->d_handlingIO = true;
+  dnsdist::tcp::HandlingIOGuard reentryGuard(conn->d_handlingIO);
+
   bool connectionDied = false;
   IOState iostate = IOState::Done;
   IOStateGuard ioGuard(conn->d_ioState);
@@ -317,6 +324,7 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
 
   do {
     reconnected = false;
+    conn->d_lastIOBlocked = false;
 
     try {
       if (conn->d_state == State::sendingQueryToBackend) {
@@ -352,8 +360,11 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
           conn->d_currentPos = 0;
           conn->d_lastDataReceivedTime = now;
         }
-        else if (conn->d_state == State::waitingForResponseFromBackend && conn->d_currentPos > 0) {
-          conn->d_state = State::readingResponseSizeFromBackend;
+        else {
+          conn->d_lastIOBlocked = true;
+          if (conn->d_state == State::waitingForResponseFromBackend && conn->d_currentPos > 0) {
+            conn->d_state = State::readingResponseSizeFromBackend;
+          }
         }
       }
 
@@ -373,6 +384,9 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
             return;
           }
         }
+        else {
+          conn->d_lastIOBlocked = true;
+        }
       }
 
       if (conn->d_state != State::idle &&
@@ -506,7 +520,7 @@ void TCPConnectionToBackend::handleIO(std::shared_ptr<TCPConnectionToBackend>& c
       }
     }
   }
-  while (reconnected);
+  while (reconnected || (iostate != IOState::Done && !conn->d_connectionDied && !conn->d_lastIOBlocked));
 
   ioGuard.release();
 }
@@ -760,16 +774,19 @@ IOState TCPConnectionToBackend::handleResponse(std::shared_ptr<TCPConnectionToBa
     DEBUGLOG("still have some queries to send");
     return queueNextQuery(shared);
   }
-  else if (!d_pendingResponses.empty()) {
+  if (d_state == State::sendingQueryToBackend) {
+    DEBUGLOG("still have a query to send");
+    return IOState::NeedWrite;
+  }
+  if (!d_pendingResponses.empty()) {
     DEBUGLOG("still have some responses to read");
     return IOState::NeedRead;
   }
-  else {
-    DEBUGLOG("nothing to do, waiting for a new query");
-    d_state = State::idle;
-    t_downstreamTCPConnectionsManager.moveToIdle(conn);
-    return IOState::Done;
-  }
+
+  DEBUGLOG("nothing to do, waiting for a new query");
+  d_state = State::idle;
+  t_downstreamTCPConnectionsManager.moveToIdle(conn);
+  return IOState::Done;
 }
 
 uint16_t TCPConnectionToBackend::getQueryIdFromResponse() const

diff --git a/pdns/dnsdistdist/dnsdist-tcp-downstream.hh b/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
index d2447e8b9cf8de4a88b7944cc760a263843af213..ae46bd6b9e9d630130f073fdf645dda1de3bf285 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp-downstream.hh
@@ -302,6 +302,8 @@ private:
   size_t d_currentPos{0};
   uint16_t d_responseSize{0};
   State d_state{State::idle};
+  bool d_handlingIO{false};
+  bool d_lastIOBlocked{false};
 };
 
 void setTCPDownstreamMaxIdleConnectionsPerBackend(uint64_t max);

diff --git a/pdns/dnsdistdist/dnsdist-tcp.cc b/pdns/dnsdistdist/dnsdist-tcp.cc
index 29bcc518a8d3898bdd8341e6bdce28bd7ad372bf..cb0186bc40e475eb011462ebf66d32fa0070a275 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp.cc
@@ -1207,7 +1207,6 @@ void IncomingTCPConnectionState::handleIO()
   if (d_handlingIO) {
     return;
   }
-  d_handlingIO = true;
   dnsdist::tcp::HandlingIOGuard reentryGuard(d_handlingIO);
 
   // why do we loop? Because the TLS layer does buffering, and thus can have data ready to read

diff --git a/pdns/dnsdistdist/dnsdist-tcp.hh b/pdns/dnsdistdist/dnsdist-tcp.hh
index c450d5f24c9ecdf00c363290988919e8a00a96a7..948a90a3afa0aa8902f2fc2004cd5876311344e0 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp.hh
@@ -314,6 +314,7 @@ public:
   HandlingIOGuard(bool& handlingIO) :
     d_handlingIO(handlingIO)
   {
+    d_handlingIO = true;
   }
   HandlingIOGuard(const HandlingIOGuard&) = delete;
   HandlingIOGuard(HandlingIOGuard&&) = delete;

diff --git a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
index 9b6e262988589aed79abad91e51085b68c996cf6..53c8b1110a51f824952e58539ee1c55f62e5dfa2 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc
@@ -672,7 +672,6 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture)
   }
 
   {
-#if 1
     TEST_INIT("=> 10k self-generated pipelined on the same connection");
 
     /* 10k self-generated REFUSED pipelined on the same connection */
@@ -699,7 +698,6 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture)
     auto state = std::make_shared<IncomingTCPConnectionState>(ConnectionInfo(&localCS, getBackendAddress("84", 4242)), threadData, now);
     state->handleIO();
     BOOST_CHECK_EQUAL(s_writeBuffer.size(), query.size() * count);
-#endif
   }
 
   {
@@ -1819,7 +1817,6 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_BackendNoOOOR, TestFixture)
   }
 
   {
-#if 1
     /* 101 queries on the same connection, check that the maximum number of queries kicks in */
     TEST_INIT("=> 101 queries on the same connection");
 
@@ -1886,7 +1883,6 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_BackendNoOOOR, TestFixture)
     dnsdist::configuration::updateRuntimeConfiguration([](dnsdist::configuration::RuntimeConfiguration& config) {
       config.d_maxTCPQueriesPerConn = 0;
     });
-#endif
   }
 
   {
@@ -2206,8 +2202,6 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
         /* set the client descriptor as NOT ready */
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(desc);
       } },
-      /* reading from the client (not ready) */
-      { ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 0 },
       /* reading a response from the backend (5) */
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(4).size() - 2 },
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(4).size() },
@@ -2220,6 +2214,8 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(desc);
         timeout = true;
       } },
+      /* reading from the client (not ready) */
+      { ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 0 },
 
       /* A timeout occurs */
 
@@ -3964,10 +3960,14 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, 2 },
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(0).size() - 2 },
       /* sending it to the client */
-      { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, responses.at(0).size(), [&threadData](int desc) {
+      { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, responses.at(0).size(), [&threadData,&backend1Desc](int desc) {
         /* client becomes readable */
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setReady(desc);
+        /* first backend is no longer readable */
+        dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(backend1Desc);
       } },
+      /* no response ready from the backend yet */
+      { ExpectedStep::ExpectedRequest::readFromBackend, IOState::NeedRead, 0 },
       /* reading a query from the client (5) */
       { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 2 },
       { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, queries.at(4).size() - 2, [&threadData](int desc) {
@@ -3993,6 +3993,8 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(backend1Desc);
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setReady(backend2Desc);
       } },
+      /* no more response ready yet from backend 1 */
+      { ExpectedStep::ExpectedRequest::readFromBackend, IOState::NeedRead, 0 },
       /* reading response (3) from the second backend (2) */
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, 2 },
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(2).size() - 2 },
@@ -4003,6 +4005,8 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(backend2Desc);
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setReady(backend1Desc);
       } },
+      /* no more response ready yet from backend 2 */
+      { ExpectedStep::ExpectedRequest::readFromBackend, IOState::NeedRead, 0 },
       /* reading response (5) from the first backend (1) */
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, 2 },
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(4).size() - 2 },
@@ -4013,6 +4017,8 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnectionOOOR_BackendOOOR, TestFixture)
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setNotReady(backend1Desc);
         dynamic_cast<MockupFDMultiplexer*>(threadData.mplexer.get())->setReady(backend2Desc);
       } },
+      /* no more response ready yet from backend 1 */
+      { ExpectedStep::ExpectedRequest::readFromBackend, IOState::NeedRead, 0 },
       /* reading response (4) from the second backend (2) */
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, 2 },
       { ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, responses.at(3).size() - 2 },