dnsdist: Fix a memory access violation in the nghttp2 unit tests

Calling `nghttp2_session_send` from a callback does not work well
when ``nghttp2_session_send`` ends up closing the current stream,
triggering a use-after-free.
It's not clear from the API documentation, but it is mentioned in
the programmers' guide's remarks:

> Do not call `nghttp2_session_send()`, `nghttp2_session_mem_send2()`,
`nghttp2_session_recv()` or `nghttp2_session_mem_recv2()` from the
nghttp2 callback functions directly or indirectly. It will lead to the
crash. You can submit requests or frames in the callbacks then call
these functions outside the callbacks.

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>
(cherry picked from commit 346d37abe3beedcec8c856ea4421311de4df1d24)

diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc
index bbe703760111c45b251d0db862eb2b12ad164493..fe1317b2f2039c91eb8d45c2450873b5ae201762 100644 (file)
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -1088,26 +1088,6 @@ bool IncomingTCPConnectionState::readIncomingQuery(const timeval& now, IOState&
   return false;
 }
 
-class HandlingIOGuard
-{
-public:
-  HandlingIOGuard(bool& handlingIO) :
-    d_handlingIO(handlingIO)
-  {
-  }
-  HandlingIOGuard(const HandlingIOGuard&) = delete;
-  HandlingIOGuard(HandlingIOGuard&&) = delete;
-  HandlingIOGuard& operator=(const HandlingIOGuard& rhs) = delete;
-  HandlingIOGuard& operator=(HandlingIOGuard&&) = delete;
-  ~HandlingIOGuard()
-  {
-    d_handlingIO = false;
-  }
-
-private:
-  bool& d_handlingIO;
-};
-
 void IncomingTCPConnectionState::handleIO()
 {
   // let's make sure we are not already in handleIO() below in the stack:
@@ -1119,7 +1099,7 @@ void IncomingTCPConnectionState::handleIO()
     return;
   }
   d_handlingIO = true;
-  HandlingIOGuard reentryGuard(d_handlingIO);
+  dnsdist::tcp::HandlingIOGuard reentryGuard(d_handlingIO);
 
   // why do we loop? Because the TLS layer does buffering, and thus can have data ready to read
   // even though the underlying socket is not ready, so we need to actually ask for the data first

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc
index bf1666f84246652abe004b7d63db05745891dab3..eef56a8b92f798c6623e0c056d61290aea24bfa2 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2.cc
@@ -118,6 +118,7 @@ private:
   size_t d_inPos{0};
   bool d_healthCheckQuery{false};
   bool d_firstWrite{true};
+  bool d_inIOCallback{false};
 };
 
 using DownstreamDoHConnectionsManager = DownstreamConnectionsManager<DoHConnectionToBackend>;
@@ -356,6 +357,7 @@ void DoHConnectionToBackend::handleReadableIOCallback(int fd, FDMultiplexer::fun
     throw std::runtime_error("Unexpected socket descriptor " + std::to_string(fd) + " received in " + std::string(__PRETTY_FUNCTION__) + ", expected " + std::to_string(conn->getHandle()));
   }
 
+  dnsdist::tcp::HandlingIOGuard handlingIOGuard(conn->d_inIOCallback);
   IOStateGuard ioGuard(conn->d_ioState);
   do {
     conn->d_inPos = 0;

diff --git a/pdns/dnsdistdist/dnsdist-tcp.hh b/pdns/dnsdistdist/dnsdist-tcp.hh
index f3d827ebf3d30364ce44013b237723332942bbb3..9c4f4ac9892113f44f91459b9f6f8b6fb3dc446e 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp.hh
@@ -305,6 +305,29 @@ private:
   const uint64_t d_maxthreads{0};
 };
 
+namespace dnsdist::tcp
+{
+class HandlingIOGuard
+{
+public:
+  HandlingIOGuard(bool& handlingIO) :
+    d_handlingIO(handlingIO)
+  {
+  }
+  HandlingIOGuard(const HandlingIOGuard&) = delete;
+  HandlingIOGuard(HandlingIOGuard&&) = delete;
+  HandlingIOGuard& operator=(const HandlingIOGuard& rhs) = delete;
+  HandlingIOGuard& operator=(HandlingIOGuard&&) = delete;
+  ~HandlingIOGuard()
+  {
+    d_handlingIO = false;
+  }
+
+private:
+  bool& d_handlingIO;
+};
+}
+
 extern std::unique_ptr<TCPClientCollection> g_tcpclientthreads;
 
 std::unique_ptr<CrossProtocolQuery> getTCPCrossProtocolQueryFromDQ(DNSQuestion& dnsQuestion);

diff --git a/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc b/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc
index 98c37243e3343b2208ef2b8d7356614c3a12e683..1a57c9cea8c17a725fc53d11d7f27d59a46bf69e 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc
@@ -186,9 +186,6 @@ public:
     int rv = nghttp2_submit_response(d_session.get(), streamId, hdrs, sizeof(hdrs) / sizeof(*hdrs), &dataProvider);
     // cerr<<"Submitting response for stream ID "<<streamId<<": "<<rv<<endl;
     BOOST_CHECK_EQUAL(rv, 0);
-    /* just in case, see if we have anything to send */
-    rv = nghttp2_session_send(d_session.get());
-    BOOST_CHECK_EQUAL(rv, 0);
   }
 
   void submitError(uint32_t streamId, uint16_t status, const std::string& msg)
@@ -198,18 +195,17 @@ public:
 
     int rv = nghttp2_submit_response(d_session.get(), streamId, hdrs, sizeof(hdrs) / sizeof(*hdrs), nullptr);
     BOOST_CHECK_EQUAL(rv, 0);
-    /* just in case, see if we have anything to send */
-    rv = nghttp2_session_send(d_session.get());
-    BOOST_CHECK_EQUAL(rv, 0);
   }
 
-  void submitGoAway()
+  void submitGoAway(bool flush) const
   {
     int rv = nghttp2_submit_goaway(d_session.get(), NGHTTP2_FLAG_NONE, 0, NGHTTP2_INTERNAL_ERROR, nullptr, 0);
     BOOST_CHECK_EQUAL(rv, 0);
-    /* just in case, see if we have anything to send */
-    rv = nghttp2_session_send(d_session.get());
-    BOOST_CHECK_EQUAL(rv, 0);
+    if (flush) {
+      /* just in case, see if we have anything to send */
+      rv = nghttp2_session_send(d_session.get());
+      BOOST_CHECK_EQUAL(rv, 0);
+    }
   }
 
 private:
@@ -269,7 +265,7 @@ private:
 
       DNSName qname(reinterpret_cast<const char*>(query.data()), query.size(), sizeof(dnsheader), false);
       if (qname == DNSName("goaway.powerdns.com.")) {
-        conn->submitGoAway();
+        conn->submitGoAway(false);
       }
       else if (qname == DNSName("500.powerdns.com.") && (id % 2) == 0) {
         /* we return a 500 on the first query only */
@@ -613,7 +609,7 @@ BOOST_FIXTURE_TEST_CASE(test_SingleQuery, TestFixture)
      }},
     /* acknowledge settings */
     {ExpectedStep::ExpectedRequest::writeToBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc) {
-       s_connectionBuffers.at(desc)->submitGoAway();
+       s_connectionBuffers.at(desc)->submitGoAway(true);
        dynamic_cast<MockupFDMultiplexer*>(s_mplexer.get())->setReady(desc);
      }},
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max()},
@@ -697,7 +693,7 @@ BOOST_FIXTURE_TEST_CASE(test_ConcurrentQueries, TestFixture)
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max()},
     /* acknowledge settings */
     {ExpectedStep::ExpectedRequest::writeToBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc) {
-       s_connectionBuffers.at(desc)->submitGoAway();
+       s_connectionBuffers.at(desc)->submitGoAway(true);
        dynamic_cast<MockupFDMultiplexer*>(s_mplexer.get())->setReady(desc);
      }},
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max()},
@@ -794,7 +790,8 @@ BOOST_FIXTURE_TEST_CASE(test_ConnectionReuse, TestFixture)
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max()},
     /* later the backend sends a go away frame */
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc) {
-       s_connectionBuffers.at(desc)->submitGoAway();
+       (void)desc;
+       s_connectionBuffers.at(desc)->submitGoAway(true);
      }},
     {ExpectedStep::ExpectedRequest::closeBackend, IOState::Done},
   };
@@ -893,7 +890,7 @@ BOOST_FIXTURE_TEST_CASE(test_InvalidDNSAnswer, TestFixture)
     {ExpectedStep::ExpectedRequest::writeToBackend, IOState::Done, std::numeric_limits<size_t>::max()},
     /* try to read, the backend says to go away */
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc) {
-       s_connectionBuffers.at(desc)->submitGoAway();
+       s_connectionBuffers.at(desc)->submitGoAway(true);
      }},
     {ExpectedStep::ExpectedRequest::closeBackend, IOState::Done},
   };