From f5eb24d5b8767521e821b00aed87ab87615800e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Wed, 11 Jun 2025 18:03:12 +0200 Subject: [PATCH] Http2ProtocolHandler: fix logic error with potential use-after-free We previously asserted that the reply was not nullptr, except in some special circumstance. But then we proceeded to dereference it anyway. This was then recently changed to be an if-check, but that just highlighted the logic-flaw (and made static analyzers warn about it...) What we want to assert is that the stream object is valid and conditionally return early if the reply is nullptr, which it is for promised streams, since no request has been made yet so no reply is created. At the same time, update the logic in the QHttp2Stream to not store or emit header-related signals for a stream that has been reset. Change-Id: I55d69bbedc027893f6ad125c29468a34e7fb406f Reviewed-by: Edward Welbourne (cherry picked from commit 3af5e42bdd6ffcf6d9ca386583add2329372056f) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 43e249cba8af8ab34e39fa63adbbbef29251d673) --- src/network/access/qhttp2connection.cpp | 4 ++-- src/network/access/qhttp2protocolhandler.cpp | 3 ++- .../network/access/qhttp2connection/tst_qhttp2connection.cpp | 4 ++++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/network/access/qhttp2connection.cpp b/src/network/access/qhttp2connection.cpp index bec6867b760..6f8a318dae0 100644 --- a/src/network/access/qhttp2connection.cpp +++ b/src/network/access/qhttp2connection.cpp @@ -1930,8 +1930,8 @@ void QHttp2Connection::handleContinuedHEADERS() return connectionError(FRAME_SIZE_ERROR, "HEADERS frame too large"); } - if (streamIt == m_streams.cend()) // No more processing without a stream from here on. - return; + if (streamWasResetLocally(streamID) || streamIt == m_streams.cend()) + return; // No more processing without a stream from here on. switch (firstFrameType) { case FrameType::HEADERS: diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index a4bf29815f7..3697a003aad 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -308,10 +308,11 @@ bool QHttp2ProtocolHandler::sendDATA(QHttp2Stream *stream, QHttpNetworkReply *re void QHttp2ProtocolHandler::handleHeadersReceived(const HPack::HttpHeader &headers, bool endStream) { QHttp2Stream *stream = qobject_cast(sender()); + Q_ASSERT(stream); auto &requestPair = requestReplyPairs[stream]; auto *httpReply = requestPair.second; auto &httpRequest = requestPair.first; - if (!httpReply && (!stream || stream->state() != QHttp2Stream::State::ReservedRemote)) + if (!httpReply) return; auto *httpReplyPrivate = httpReply->d_func(); diff --git a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp index f4cbff26db5..8e8c90e14de 100644 --- a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp +++ b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp @@ -745,6 +745,7 @@ void tst_QHttp2Connection::headerFrameAfterRSTOutgoing() QHttp2Stream *clientStream = connection->createStream().unwrap(); QVERIFY(clientStream); + QSignalSpy client1HeadersSpy{ clientStream, &QHttp2Stream::headersReceived}; QVERIFY(waitForSettingsExchange(connection, serverConnection)); QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; @@ -774,6 +775,9 @@ void tst_QHttp2Connection::headerFrameAfterRSTOutgoing() // causing an error on Qt's side. QVERIFY(serverRSTReceivedSpy.wait()); + // We don't emit any headers for a reset stream + QVERIFY(!client1HeadersSpy.count()); + // Create a new stream then send and handle a new request! QHttp2Stream *clientStream2 = connection->createStream().unwrap(); QSignalSpy client2HeaderReceivedSpy{ clientStream2, &QHttp2Stream::headersReceived };