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 <timur.pocheptsov@qt.io>
Reviewed-by: Konrad Kujawa <konrad.kujawa@qt.io>

src/network/access/qhttp2protocolhandler.cpp

@@ -964,7 +964,12 @@ void QHttp2ProtocolHandler::handleContinuedHEADERS()
     }
 
     std::vector<uchar> 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.

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);
 

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();
 };

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<QNetworkReply> 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;