Http/2 - handle PADDED flag correctly

Previously, when deciding where the actual data is, Frame was calling
padding() to test if offset is needed. A curious case with a DATA frame
containing compressed body and having 'PADDED' flag set with a padding
equal to ... 0, ended in a decompression error (and assert in 6.2 code).

Fixes: QTBUG-97179
Change-Id: I9341a4d68510aa4c26f4972afdcd09a530d5a367
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit dd57605b9ef4e12805868962efce586e57e342b6)

2 files changed, 87 insertions, 2 deletions

diff --git a/src/network/access/http2/http2frames.cpp b/src/network/access/http2/http2frames.cpp
index 3ea3536280..6f1171fb02 100644
--- a/src/network/access/http2/http2frames.cpp
+++ b/src/network/access/http2/http2frames.cpp
@@ -233,7 +233,8 @@ quint32 Frame::dataSize() const
     Q_ASSERT(validatePayload() == FrameStatus::goodFrame);
 
     quint32 size = payloadSize();
-    if (const uchar pad = padding()) {
+    if (flags().testFlag(FrameFlag::PADDED)) {
+        const uchar pad = padding();
         // + 1 one for a byte with padding number itself:
         size -= pad + 1;
     }
@@ -269,7 +270,7 @@ const uchar *Frame::dataBegin() const
         return nullptr;
 
     const uchar *src = &buffer[0] + frameHeaderSize;
-    if (padding())
+    if (flags().testFlag(FrameFlag::PADDED))
         ++src;
 
     if (priority())
diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp
index 58affda4a4..0610d0a301 100644
--- a/tests/auto/network/access/http2/tst_http2.cpp
+++ b/tests/auto/network/access/http2/tst_http2.cpp
@@ -110,6 +110,7 @@ private slots:
     void connectToHost_data();
     void connectToHost();
     void maxFrameSize();
+    void http2DATAFrames();
 
     void authenticationRequired_data();
     void authenticationRequired();
@@ -780,6 +781,89 @@ void tst_Http2::maxFrameSize()
     QVERIFY(serverGotSettingsACK);
 }
 
+void tst_Http2::http2DATAFrames()
+{
+    using namespace Http2;
+
+    {
+        // 0. DATA frame with payload, no padding.
+
+        FrameWriter writer(FrameType::DATA, FrameFlag::EMPTY, 1);
+        writer.append(uchar(1));
+        writer.append(uchar(2));
+        writer.append(uchar(3));
+
+        const Frame frame = writer.outboundFrame();
+        const auto &buffer = frame.buffer;
+        // Frame's header is 9 bytes + 3 bytes of payload
+        // (+ 0 bytes of padding and no padding length):
+        QCOMPARE(int(buffer.size()), 12);
+
+        QVERIFY(!frame.padding());
+        QCOMPARE(int(frame.payloadSize()), 3);
+        QCOMPARE(int(frame.dataSize()), 3);
+        QCOMPARE(frame.dataBegin() - buffer.data(), 9);
+        QCOMPARE(char(*frame.dataBegin()), uchar(1));
+    }
+
+    {
+        // 1. DATA with padding.
+
+        const int padLength = 10;
+        FrameWriter writer(FrameType::DATA, FrameFlag::END_STREAM | FrameFlag::PADDED, 1);
+        writer.append(uchar(padLength)); // The length of padding is 1 byte long.
+        writer.append(uchar(1));
+        for (int i = 0; i < padLength; ++i)
+            writer.append(uchar(0));
+
+        const Frame frame = writer.outboundFrame();
+        const auto &buffer = frame.buffer;
+        // Frame's header is 9 bytes + 1 byte for padding length
+        // + 1 byte of data + 10 bytes of padding:
+        QCOMPARE(int(buffer.size()), 21);
+
+        QCOMPARE(frame.padding(), padLength);
+        QCOMPARE(int(frame.payloadSize()), 12); // Includes padding, its length + data.
+        QCOMPARE(int(frame.dataSize()), 1);
+
+        // Skipping 9 bytes long header and padding length:
+        QCOMPARE(frame.dataBegin() - buffer.data(), 10);
+
+        QCOMPARE(char(frame.dataBegin()[0]), uchar(1));
+        QCOMPARE(char(frame.dataBegin()[1]), uchar(0));
+
+        QVERIFY(frame.flags().testFlag(FrameFlag::END_STREAM));
+        QVERIFY(frame.flags().testFlag(FrameFlag::PADDED));
+    }
+    {
+        // 2. DATA with PADDED flag, but 0 as padding length.
+
+        FrameWriter writer(FrameType::DATA, FrameFlag::END_STREAM | FrameFlag::PADDED, 1);
+
+        writer.append(uchar(0)); // Number of padding bytes is 1 byte long.
+        writer.append(uchar(1));
+
+        const Frame frame = writer.outboundFrame();
+        const auto &buffer = frame.buffer;
+
+        // Frame's header is 9 bytes + 1 byte for padding length + 1 byte of data
+        // + 0 bytes of padding:
+        QCOMPARE(int(buffer.size()), 11);
+
+        QCOMPARE(frame.padding(), 0);
+        QCOMPARE(int(frame.payloadSize()), 2); // Includes padding (0 bytes), its length + data.
+        QCOMPARE(int(frame.dataSize()), 1);
+
+        // Skipping 9 bytes long header and padding length:
+        QCOMPARE(frame.dataBegin() - buffer.data(), 10);
+
+        QCOMPARE(char(*frame.dataBegin()), uchar(1));
+
+        QVERIFY(frame.flags().testFlag(FrameFlag::END_STREAM));
+        QVERIFY(frame.flags().testFlag(FrameFlag::PADDED));
+    }
+}
+
 void tst_Http2::authenticationRequired_data()
 {
     QTest::addColumn<bool>("success");