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/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc
index 36dedd43c8f8ac1d1e8a94f0698e22e9bc71b502..fb597742267835977e2a5a63fbc82cb971ea00fa 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 6397d5a415841066dc4bbf7a7bdb7a8735f6e20d..ce430c4569a251667fe0c6f85ab98717d70eb233 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},
   };