From 7822b89aef30404164bc6f6f790d0cc563e10cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Mon, 6 Mar 2023 16:23:23 +0100 Subject: [PATCH] Http2: Don't skip checking flag for trailing HEADERS frame if PRIORITY If the trailing frame just had PRIORITY we would early-return, though this meant we didn't check if the frame had the END_STREAM flag set, leading some requests to certain servers to hang. Fixes: QTBUG-111417 Pick-to: 6.5 6.4 6.2 Change-Id: Iac174dc5aeca30d5d19fae35f303983de9841847 Reviewed-by: Timur Pocheptsov Reviewed-by: Konrad Kujawa --- src/network/access/qhttp2protocolhandler.cpp | 18 ++++----- tests/auto/network/access/http2/http2srv.cpp | 22 +++++++++-- tests/auto/network/access/http2/http2srv.h | 4 ++ tests/auto/network/access/http2/tst_http2.cpp | 37 +++++++++++++++++++ 4 files changed, 69 insertions(+), 12 deletions(-) diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index f41cde067a..93be8412db 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -964,7 +964,12 @@ void QHttp2ProtocolHandler::handleContinuedHEADERS() } std::vector hpackBlock(assemble_hpack_block(continuedFrames)); - if (!hpackBlock.size()) { + const bool hasHeaderFields = !hpackBlock.empty(); + if (hasHeaderFields) { + HPack::BitIStream inputStream{&hpackBlock[0], &hpackBlock[0] + hpackBlock.size()}; + if (!decoder.decodeHeaderFields(inputStream)) + return connectionError(COMPRESSION_ERROR, "HPACK decompression failed"); + } else if (firstFrameType == FrameType::PUSH_PROMISE) { // It could be a PRIORITY sent in HEADERS - already handled by this // point in handleHEADERS. If it was PUSH_PROMISE (HTTP/2 8.2.1): // "The header fields in PUSH_PROMISE and any subsequent CONTINUATION @@ -973,21 +978,16 @@ void QHttp2ProtocolHandler::handleContinuedHEADERS() // not include a complete and valid set of header fields or the :method // pseudo-header field identifies a method that is not safe, it MUST // respond with a stream error (Section 5.4.2) of type PROTOCOL_ERROR." - if (firstFrameType == FrameType::PUSH_PROMISE) - resetPromisedStream(continuedFrames[0], Http2::PROTOCOL_ERROR); - + resetPromisedStream(continuedFrames[0], Http2::PROTOCOL_ERROR); return; } - HPack::BitIStream inputStream{&hpackBlock[0], &hpackBlock[0] + hpackBlock.size()}; - if (!decoder.decodeHeaderFields(inputStream)) - return connectionError(COMPRESSION_ERROR, "HPACK decompression failed"); - switch (firstFrameType) { case FrameType::HEADERS: if (activeStreams.contains(streamID)) { Stream &stream = activeStreams[streamID]; - updateStream(stream, decoder.decodedHeader()); + if (hasHeaderFields) + updateStream(stream, decoder.decodedHeader()); // Needs to resend the request; we should finish and delete the current stream const bool needResend = stream.request().d->needResendWithCredentials; // No DATA frames. Or needs to resend. diff --git a/tests/auto/network/access/http2/http2srv.cpp b/tests/auto/network/access/http2/http2srv.cpp index bf2625876e..91d57a5caa 100644 --- a/tests/auto/network/access/http2/http2srv.cpp +++ b/tests/auto/network/access/http2/http2srv.cpp @@ -111,6 +111,11 @@ void Http2Server::setRedirect(const QByteArray &url, int count) redirectCount = count; } +void Http2Server::setSendTrailingHEADERS(bool enable) +{ + sendTrailingHEADERS = enable; +} + void Http2Server::emulateGOAWAY(int timeout) { Q_ASSERT(timeout >= 0); @@ -248,9 +253,20 @@ void Http2Server::sendDATA(quint32 streamID, quint32 windowSize) return; if (last) { - writer.start(FrameType::DATA, FrameFlag::END_STREAM, streamID); - writer.setPayloadSize(0); - writer.write(*socket); + if (sendTrailingHEADERS) { + writer.start(FrameType::HEADERS, + FrameFlag::PRIORITY | FrameFlag::END_HEADERS | FrameFlag::END_STREAM, streamID); + const quint32 maxFrameSize(clientSetting(Settings::MAX_FRAME_SIZE_ID, + Http2::maxPayloadSize)); + // 5 bytes for PRIORITY data: + writer.append(quint32(0)); // streamID 0 (32-bit) + writer.append(quint8(0)); // + weight 0 (8-bit) + writer.writeHEADERS(*socket, maxFrameSize); + } else { + writer.start(FrameType::DATA, FrameFlag::END_STREAM, streamID); + writer.setPayloadSize(0); + writer.write(*socket); + } suspendedStreams.erase(it); activeRequests.erase(streamID); diff --git a/tests/auto/network/access/http2/http2srv.h b/tests/auto/network/access/http2/http2srv.h index e42ed8b148..cc5353c855 100644 --- a/tests/auto/network/access/http2/http2srv.h +++ b/tests/auto/network/access/http2/http2srv.h @@ -68,6 +68,8 @@ public: // Set the redirect URL and count. The server will return a redirect response with the url // 'count' amount of times void setRedirect(const QByteArray &redirectUrl, int count); + // Send a trailing HEADERS frame with PRIORITY and END_STREAM flag + void setSendTrailingHEADERS(bool enable); void emulateGOAWAY(int timeout); void redirectOpenStream(quint16 targetPort); @@ -203,6 +205,8 @@ private: QByteArray redirectUrl; int redirectCount = 0; + + bool sendTrailingHEADERS = false; protected slots: void ignoreErrorSlot(); }; diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index 0702a03541..1b0c074a6e 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -99,6 +99,8 @@ private slots: void redirect_data(); void redirect(); + void trailingHEADERS(); + protected slots: // Slots to listen to our in-process server: void serverStarted(quint16 port); @@ -1280,6 +1282,41 @@ void tst_Http2::redirect() QTRY_VERIFY(serverGotSettingsACK); } +void tst_Http2::trailingHEADERS() +{ + clearHTTP2State(); + serverPort = 0; + + ServerPtr targetServer(newServer(defaultServerSettings, defaultConnectionType())); + targetServer->setSendTrailingHEADERS(true); + + QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + + QVERIFY(serverPort != 0); + + nRequests = 1; + + const auto url = requestUrl(defaultConnectionType()); + QNetworkRequest request(url); + // H2C might be used on macOS where SecureTransport doesn't support server-side ALPN + request.setAttribute(QNetworkRequest::Http2CleartextAllowedAttribute, true); + + std::unique_ptr reply{ manager->get(request) }; + connect(reply.get(), &QNetworkReply::finished, this, &tst_Http2::replyFinished); + + // Since we're using self-signed certificates, ignore SSL errors: + reply->ignoreSslErrors(); + + runEventLoop(); + STOP_ON_FAILURE + + QCOMPARE(nRequests, 0); + + QCOMPARE(reply->error(), QNetworkReply::NoError); + QTRY_VERIFY(serverGotSettingsACK); +} + void tst_Http2::serverStarted(quint16 port) { serverPort = port;