From be477819ac17850632bca6ce59ee2c8ef11191cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Wed, 18 Dec 2024 12:38:41 +0100 Subject: [PATCH] HPack: test and fix Dynamic Table Size Update Expose some new API to make it easier to test too. Will be used in follow-up patches for a http2 bugfix. The setMaxDynamicTableSize() method no longer sets capacity or evicts entries. This can then be used to set the new maximum size which we will accept later when we receive a Dynamic Table Size Update. This flow makes sense with both encoder and decoder. The decoder would usually be set by us when we send our SETTINGS frame, and then the Dynamic Table Size Update would be sent by the peer. For the encoder we would set the maximum size when we receive the SETTINGS frame, and then we must defer the actual resizing until we can send the Dynamic Table Size Update ourselves, usually along with the next HEADERS frame. Pick-to: 6.9 6.8 6.5 Task-number: QTBUG-132277 Change-Id: I959f5006eb09427d130735efe136da8c04453fa2 Reviewed-by: Timur Pocheptsov --- src/network/access/http2/hpack.cpp | 25 ++++++++++++++++++ src/network/access/http2/hpack_p.h | 4 +++ src/network/access/http2/hpacktable.cpp | 17 +++++++++--- src/network/access/http2/hpacktable_p.h | 2 ++ tests/auto/network/access/hpack/tst_hpack.cpp | 26 +++++++++++++++++++ 5 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/network/access/http2/hpack.cpp b/src/network/access/http2/hpack.cpp index d859affd7c3..f6c7c9d69e1 100644 --- a/src/network/access/http2/hpack.cpp +++ b/src/network/access/http2/hpack.cpp @@ -111,6 +111,16 @@ quint32 Encoder::dynamicTableSize() const return lookupTable.dynamicDataSize(); } +quint32 Encoder::dynamicTableCapacity() const +{ + return lookupTable.dynamicDataCapacity(); +} + +quint32 Encoder::maxDynamicTableCapacity() const +{ + return lookupTable.maxDynamicDataCapacity(); +} + bool Encoder::encodeRequest(BitOStream &outputStream, const HttpHeader &header) { if (!header.size()) { @@ -408,6 +418,16 @@ quint32 Decoder::dynamicTableSize() const return lookupTable.dynamicDataSize(); } +quint32 Decoder::dynamicTableCapacity() const +{ + return lookupTable.dynamicDataCapacity(); +} + +quint32 Decoder::maxDynamicTableCapacity() const +{ + return lookupTable.maxDynamicDataCapacity(); +} + void Decoder::setMaxDynamicTableSize(quint32 size) { // Up to a caller (HTTP2 protocol handler) @@ -491,6 +511,11 @@ bool Decoder::processDecodedField(BitPattern fieldType, return false; } + if (lookupTable.maxDynamicDataCapacity() < lookupTable.dynamicDataCapacity()) { + qDebug("about to add a new field, but expected a Dynamic Table Size Update"); + return false; // We expected a dynamic table size update. + } + header.push_back(HeaderField(name, value)); return true; } diff --git a/src/network/access/http2/hpack_p.h b/src/network/access/http2/hpack_p.h index aecaf5467e4..56162e340ac 100644 --- a/src/network/access/http2/hpack_p.h +++ b/src/network/access/http2/hpack_p.h @@ -38,6 +38,8 @@ public: Encoder(quint32 maxTableSize, bool compressStrings); quint32 dynamicTableSize() const; + quint32 dynamicTableCapacity() const; + quint32 maxDynamicTableCapacity() const; bool encodeRequest(class BitOStream &outputStream, const HttpHeader &header); @@ -93,6 +95,8 @@ public: } quint32 dynamicTableSize() const; + quint32 dynamicTableCapacity() const; + quint32 maxDynamicTableCapacity() const; void setMaxDynamicTableSize(quint32 size); diff --git a/src/network/access/http2/hpacktable.cpp b/src/network/access/http2/hpacktable.cpp index e06da4e7ddb..73f63b722d4 100644 --- a/src/network/access/http2/hpacktable.cpp +++ b/src/network/access/http2/hpacktable.cpp @@ -208,6 +208,16 @@ quint32 FieldLookupTable::dynamicDataSize() const return dataSize; } +quint32 FieldLookupTable::dynamicDataCapacity() const +{ + return tableCapacity; +} + +quint32 FieldLookupTable::maxDynamicDataCapacity() const +{ + return maxTableSize; +} + void FieldLookupTable::clearDynamicTable() { searchIndex.clear(); @@ -386,6 +396,7 @@ bool FieldLookupTable::updateDynamicTableSize(quint32 size) { if (!size) { clearDynamicTable(); + tableCapacity = 0; return true; } @@ -404,10 +415,10 @@ void FieldLookupTable::setMaxDynamicTableSize(quint32 size) // This is for an external user, for example, HTTP2 protocol // layer that can receive SETTINGS frame from its peer. // No validity checks here, up to this external user. - // We update max size and capacity (this can also result in - // items evicted or even dynamic table completely cleared). + // We update max size only, the capacity will be updated + // later through the Dynamic Table Size Update mechanism + // in HPack. maxTableSize = size; - updateDynamicTableSize(size); } // This data is from the HPACK's specs and it's quite conveniently sorted, diff --git a/src/network/access/http2/hpacktable_p.h b/src/network/access/http2/hpacktable_p.h index 320ba133362..4c91aa3fe20 100644 --- a/src/network/access/http2/hpacktable_p.h +++ b/src/network/access/http2/hpacktable_p.h @@ -124,6 +124,8 @@ public: quint32 numberOfStaticEntries() const; quint32 numberOfDynamicEntries() const; quint32 dynamicDataSize() const; + quint32 dynamicDataCapacity() const; + quint32 maxDynamicDataCapacity() const; void clearDynamicTable(); bool indexIsValid(quint32 index) const; diff --git a/tests/auto/network/access/hpack/tst_hpack.cpp b/tests/auto/network/access/hpack/tst_hpack.cpp index e6b43eaed45..0092d586803 100644 --- a/tests/auto/network/access/hpack/tst_hpack.cpp +++ b/tests/auto/network/access/hpack/tst_hpack.cpp @@ -36,6 +36,8 @@ private Q_SLOTS: void lookupTableStatic(); void lookupTableDynamic(); + void dynamicTableSizeUpdate(); + void hpackEncodeRequest_data(); void hpackEncodeRequest(); void hpackDecodeRequest_data(); @@ -469,6 +471,30 @@ void tst_Hpack::lookupTableDynamic() QVERIFY(table.indexOf("name1") == 0); } +void tst_Hpack::dynamicTableSizeUpdate() +{ + std::vector buffer; + buffer.reserve(4096); + + BitOStream out(buffer); + + HPack::Encoder encoder(4096u, true); + + QVERIFY(encoder.encodeSizeUpdate(out, 2048u)); + QVERIFY(encoder.encodeRequest(out, {{":method", "GET"}, {":path", "/"}, {":scheme", "https"}})); + + BitIStream in(buffer.data(), buffer.data() + buffer.size()); + HPack::Decoder decoder(4096u); + QCOMPARE(decoder.dynamicTableCapacity(), 4096u); + QCOMPARE(decoder.maxDynamicTableCapacity(), 4096u); + + QVERIFY(decoder.decodeHeaderFields(in)); + QCOMPARE(decoder.dynamicTableCapacity(), 2048u); + // The max is adjusted out-of-band, so it's not changed here: + QCOMPARE(decoder.maxDynamicTableCapacity(), 4096u); +} + + void tst_Hpack::hpackEncodeRequest_data() { QTest::addColumn("compression");