From eb6fd1d74b66fa2f390ec8b2456141cad3e571b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Thu, 5 Jun 2025 10:57:07 +0200 Subject: [PATCH] Http2: fix handling incoming frames on locally reset stream After some of the RST stream handling was updated to more closely follow the RFC it was accidentally not updating the handleHEADERS function, and the handleDATA function was handled incorrectly leading to a potential nullptr dereference. Amends d17d260948e16549d82f1fdd4dec98d246b0622e. Change-Id: I345448efd7da92f4f74033b03a5c040b5db9d271 Reviewed-by: Timur Pocheptsov (cherry picked from commit 8d5db24c14d5c7ea8a2abf55e4fd88472dd9ca8b) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 4235f0587b144ef90e4736861b4b25371efc49b9) --- src/network/access/qhttp2connection.cpp | 27 ++++++++++++------- src/network/access/qhttp2protocolhandler.cpp | 5 +++- .../qhttp2connection/tst_qhttp2connection.cpp | 11 ++++++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/network/access/qhttp2connection.cpp b/src/network/access/qhttp2connection.cpp index 0f47aa010c2..bec6867b760 100644 --- a/src/network/access/qhttp2connection.cpp +++ b/src/network/access/qhttp2connection.cpp @@ -1384,13 +1384,16 @@ void QHttp2Connection::handleDATA() if (isInvalidStream(streamID)) return connectionError(ENHANCE_YOUR_CALM, "DATA on invalid stream"); - // RFC9113, 6.1: If a DATA frame is received whose stream is not in the "open" or - // "half-closed (local)" state, the recipient MUST respond with a stream error. - auto stream = getStream(streamID); - if (stream->state() == QHttp2Stream::State::HalfClosedRemote - || stream->state() == QHttp2Stream::State::Closed) { - return stream->streamError(Http2Error::STREAM_CLOSED, - QLatin1String("Data on closed stream")); + QHttp2Stream *stream = nullptr; + if (!streamWasResetLocally(streamID)) { + stream = getStream(streamID); + // RFC9113, 6.1: If a DATA frame is received whose stream is not in the "open" or + // "half-closed (local)" state, the recipient MUST respond with a stream error. + if (stream->state() == QHttp2Stream::State::HalfClosedRemote + || stream->state() == QHttp2Stream::State::Closed) { + return stream->streamError(Http2Error::STREAM_CLOSED, + QLatin1String("Data on closed stream")); + } } if (qint32(inboundFrame.payloadSize()) > sessionReceiveWindowSize) { @@ -1403,9 +1406,8 @@ void QHttp2Connection::handleDATA() sessionReceiveWindowSize -= inboundFrame.payloadSize(); - auto it = m_streams.constFind(streamID); - if (it != m_streams.cend() && it.value()) - it.value()->handleDATA(inboundFrame); + if (stream) + stream->handleDATA(inboundFrame); if (inboundFrame.flags().testFlag(FrameFlag::END_STREAM)) emit receivedEND_STREAM(streamID); @@ -1451,6 +1453,11 @@ void QHttp2Connection::handleHEADERS() qCDebug(qHttp2ConnectionLog, "[%p] Created new incoming stream %d", this, streamID); emit newIncomingStream(newStream); + } else if (streamWasResetLocally(streamID)) { + qCDebug(qHttp2ConnectionLog, + "[%p] Received HEADERS on previously locally reset stream %d (must process but ignore)", + this, streamID); + // nop } else if (auto it = m_streams.constFind(streamID); it == m_streams.cend()) { // RFC 9113, 6.2: HEADERS frames MUST be associated with a stream. // A connection error is not required but it seems to be the right thing to do. diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index a99921f5288..84bf5b1a78d 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -310,7 +310,8 @@ void QHttp2ProtocolHandler::handleHeadersReceived(const HPack::HttpHeader &heade auto &requestPair = requestReplyPairs[stream]; auto *httpReply = requestPair.second; auto &httpRequest = requestPair.first; - Q_ASSERT(httpReply || stream->state() == QHttp2Stream::State::ReservedRemote); + if (!httpReply && (!stream || stream->state() != QHttp2Stream::State::ReservedRemote)) + return; auto *httpReplyPrivate = httpReply->d_func(); @@ -393,6 +394,8 @@ void QHttp2ProtocolHandler::handleDataReceived(const QByteArray &data, bool endS QHttp2Stream *stream = qobject_cast(sender()); auto &httpPair = requestReplyPairs[stream]; auto *httpReply = httpPair.second; + if (!httpReply) + return; Q_ASSERT(!stream->isPromisedStream()); if (!data.isEmpty() && !httpPair.first.d->needResendWithCredentials) { diff --git a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp index b7928130e6d..f4cbff26db5 100644 --- a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp +++ b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp @@ -30,6 +30,7 @@ private slots: void testBadFrameSize(); void testDataFrameAfterRSTIncoming(); void testDataFrameAfterRSTOutgoing(); + void headerFrameAfterRSTOutgoing_data(); void headerFrameAfterRSTOutgoing(); void connectToServer(); void WINDOW_UPDATE(); @@ -728,8 +729,16 @@ void tst_QHttp2Connection::testDataFrameAfterRSTOutgoing() QVERIFY(closedServerSpy.wait()); } +void tst_QHttp2Connection::headerFrameAfterRSTOutgoing_data() +{ + QTest::addColumn("deleteStream"); + QTest::addRow("retain-stream") << false; + QTest::addRow("delete-stream") << true; +} + void tst_QHttp2Connection::headerFrameAfterRSTOutgoing() { + QFETCH(const bool, deleteStream); auto [client, server] = makeFakeConnectedSockets(); auto *connection = makeHttp2Connection(client.get(), {}, Client); auto *serverConnection = makeHttp2Connection(server.get(), {}, Server); @@ -753,6 +762,8 @@ void tst_QHttp2Connection::headerFrameAfterRSTOutgoing() // Send an RST frame from the client, but we don't process it yet clientStream->sendRST_STREAM(Http2::CANCEL); + if (deleteStream) + delete std::exchange(clientStream, nullptr); // The server sends a reply, not knowing about the inbound RST frame const HPack::HttpHeader StandardReply{ { ":status", "200" }, { "x-whatever", "some info" } };