1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Http2: fix handling incoming frames on locally reset stream

Browse Source 
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 <timur.pocheptsov@qt.io>
This commit is contained in:
Mårten Nordheim 2025-06-05 10:57:07 +02:00
parent 904aec2f37
commit 8d5db24c14
3 changed files with 32 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
src/network/access/qhttp2connection.cpp
View File

@ -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.

5
src/network/access/qhttp2protocolhandler.cpp
View File

@ -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<QHttp2Stream *>(sender());
auto &httpPair = requestReplyPairs[stream];
auto *httpReply = httpPair.second;
if (!httpReply)
return;
Q_ASSERT(!stream->isPromisedStream());
if (!data.isEmpty() && !httpPair.first.d->needResendWithCredentials) {

11
tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp
View File

@ -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<bool>("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" } };