From 346d37abe3beedcec8c856ea4421311de4df1d24 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 22 Aug 2025 09:57:57 +0200 Subject: [PATCH] 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 --- pdns/dnsdistdist/dnsdist-nghttp2.cc | 1 + pdns/dnsdistdist/dnsdist-tcp.cc | 22 +----------------- pdns/dnsdistdist/dnsdist-tcp.hh | 23 +++++++++++++++++++ pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc | 26 +++++++++------------- 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc index 9b9e6f9b70..b099993e1c 100644 --- 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 4f78f49d89..827ca6859a 100644 --- 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 382f1a6187..c450d5f24c 100644 --- 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 g_tcpclientthreads; std::unique_ptr getTCPCrossProtocolQueryFromDQ(DNSQuestion& dnsQuestion); diff --git a/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc b/pdns/dnsdistdist/test-dnsdistnghttp2_cc.cc index 60e0fca099..a58b738611 100644 --- 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 "<(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::max(), [](int desc) { - s_connectionBuffers.at(desc)->submitGoAway(); + s_connectionBuffers.at(desc)->submitGoAway(true); dynamic_cast(s_mplexer.get())->setReady(desc); }}, {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::max()}, @@ -723,7 +719,7 @@ BOOST_FIXTURE_TEST_CASE(test_ConcurrentQueries, TestFixture) {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::max()}, /* acknowledge settings */ {ExpectedStep::ExpectedRequest::writeToBackend, IOState::Done, std::numeric_limits::max(), [](int desc) { - s_connectionBuffers.at(desc)->submitGoAway(); + s_connectionBuffers.at(desc)->submitGoAway(true); dynamic_cast(s_mplexer.get())->setReady(desc); }}, {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::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::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::max()}, /* try to read, the backend says to go away */ {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::max(), [](int desc) { - s_connectionBuffers.at(desc)->submitGoAway(); + s_connectionBuffers.at(desc)->submitGoAway(true); }}, {ExpectedStep::ExpectedRequest::closeBackend, IOState::Done}, }; -- 2.47.3