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>

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc
index 9b9e6f9b70f2220352f186037c9230c8a75fa60e..b099993e1ce76f35a9d2a63fe4a490cbedfb6229 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2.cc
@@ -363,6 +363,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.cc b/pdns/dnsdistdist/dnsdist-tcp.cc
index 4f78f49d89c64565f1653778ea54d3453747b046..827ca6859a276faf4527ceae1b1cdcbdb9a1be82 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.cc
+++ b/pdns/dnsdistdist/dnsdist-tcp.cc
@@ -1197,26 +1197,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:
@@ -1228,7 +1208,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-tcp.hh b/pdns/dnsdistdist/dnsdist-tcp.hh
index 382f1a6187df00d724e90c165b4f1b425da43579..c450d5f24c9ecdf00c363290988919e8a00a96a7 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp.hh
@@ -306,6 +306,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 60e0fca09981adb0029080718750d55f880e4cae..a58b738611e38c671c746fb7a5bca0077008f651 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc
@@ -189,9 +189,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)
@@ -202,18 +199,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:
@@ -276,7 +272,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 */
@@ -639,7 +635,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()},
@@ -723,7 +719,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()},
@@ -823,7 +819,7 @@ BOOST_FIXTURE_TEST_CASE(test_ConnectionReuse, TestFixture)
     /* later the backend sends a go away frame */
     {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc) {
        (void)desc;
-       s_connectionBuffers.at(desc)->submitGoAway();
+       s_connectionBuffers.at(desc)->submitGoAway(true);
      }},
     {ExpectedStep::ExpectedRequest::closeBackend, IOState::Done},
   };
@@ -923,7 +919,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},
   };