From 8d5db24c14d5c7ea8a2abf55e4fd88472dd9ca8b 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. Pick-to: 6.10 6.9 Change-Id: I345448efd7da92f4f74033b03a5c040b5db9d271 Reviewed-by: Timur Pocheptsov --- 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 6a1e5d69230..a4bf29815f7 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -311,7 +311,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(); @@ -394,6 +395,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" } };