From: Remi Gacogne Date: Fri, 22 Aug 2025 07:57:57 +0000 (+0200) Subject: dnsdist: Fix a memory access violation in the nghttp2 unit tests X-Git-Tag: dnsdist-1.9.11~1^2~5 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f735acc2a70c1e18029683aa4c1b3712aa0d1a60;p=thirdparty%2Fpdns.git 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 (cherry picked from commit 346d37abe3beedcec8c856ea4421311de4df1d24) --- diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc index bbe703760..fe1317b2f 100644 --- 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 bf1666f84..eef56a8b9 100644 --- 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; @@ -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 f3d827ebf..9c4f4ac98 100644 --- 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 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 98c37243e..1a57c9cea 100644 --- 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 "<(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::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()}, @@ -697,7 +693,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()}, @@ -794,7 +790,8 @@ BOOST_FIXTURE_TEST_CASE(test_ConnectionReuse, TestFixture) {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::max()}, /* later the backend sends a go away frame */ {ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits::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::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}, };