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-Url: http://git.ipfire.org/?a=commitdiff_plain;h=346d37abe3beedcec8c856ea4421311de4df1d24;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 --- diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc index 9b9e6f9b7..b099993e1 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 4f78f49d8..827ca6859 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 382f1a618..c450d5f24 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 60e0fca09..a58b73861 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}, };