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

Hpack - fix the static lookup

Browse Source 
'accept' breaks the order, making the static table unsorted and thus
std::lower_bound cannot find it and we always index it in a dynamic
table. Also, make this static table accessible to auto-test.
Plus fix some warnings quite annoyingly visible in qt-creator.

Fixes: QTBUG-74161
Change-Id: I47410f2ef974ac92797c9804aa55cb5c36a436c4
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
This commit is contained in:
Timur Pocheptsov 2019-03-01 16:17:57 +01:00
parent 47bb74fd15
commit ef5aefb062
3 changed files with 134 additions and 135 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

209
src/network/access/http2/hpacktable.cpp
View File

@ -42,6 +42,7 @@
#include <QtCore/qdebug.h>
#include <algorithm>
#include <cstddef>
#include <cstring>
#include <limits>
@ -61,7 +62,7 @@ HeaderSize entry_size(const QByteArray &name, const QByteArray &value)
// for counting the number of references to the name and value would have
// 32 octets of overhead."
const unsigned sum = unsigned(name.size()) + value.size();
const unsigned sum = unsigned(name.size() + value.size());
if (std::numeric_limits<unsigned>::max() - 32 < sum)
return HeaderSize();
return HeaderSize(true, quint32(sum + 32));
@ -75,7 +76,7 @@ int compare(const QByteArray &lhs, const QByteArray &rhs)
if (const int minLen = std::min(lhs.size(), rhs.size())) {
// We use memcmp, since strings in headers are allowed
// to contain '\0'.
const int cmp = std::memcmp(lhs.constData(), rhs.constData(), minLen);
const int cmp = std::memcmp(lhs.constData(), rhs.constData(), std::size_t(minLen));
if (cmp)
return cmp;
}
@ -138,82 +139,6 @@ bool FieldLookupTable::SearchEntry::operator < (const SearchEntry &rhs)const
return offset > rhs.offset;
}
// This data is from HPACK's specs and it's quite
// conveniently sorted == works with binary search as it is.
// Later this can probably change and instead of simple
// vector we'll just reuse FieldLookupTable.
// TODO: it makes sense to generate this table while ...
// configuring/building Qt (some script downloading/parsing/generating
// would be quite handy).
const std::vector<HeaderField> &staticTable()
{
static std::vector<HeaderField> table = {
{":authority", ""},
{":method", "GET"},
{":method", "POST"},
{":path", "/"},
{":path", "/index.html"},
{":scheme", "http"},
{":scheme", "https"},
{":status", "200"},
{":status", "204"},
{":status", "206"},
{":status", "304"},
{":status", "400"},
{":status", "404"},
{":status", "500"},
{"accept-charset", ""},
{"accept-encoding", "gzip, deflate"},
{"accept-language", ""},
{"accept-ranges", ""},
{"accept", ""},
{"access-control-allow-origin", ""},
{"age", ""},
{"allow", ""},
{"authorization", ""},
{"cache-control", ""},
{"content-disposition", ""},
{"content-encoding", ""},
{"content-language", ""},
{"content-length", ""},
{"content-location", ""},
{"content-range", ""},
{"content-type", ""},
{"cookie", ""},
{"date", ""},
{"etag", ""},
{"expect", ""},
{"expires", ""},
{"from", ""},
{"host", ""},
{"if-match", ""},
{"if-modified-since", ""},
{"if-none-match", ""},
{"if-range", ""},
{"if-unmodified-since", ""},
{"last-modified", ""},
{"link", ""},
{"location", ""},
{"max-forwards", ""},
{"proxy-authenticate", ""},
{"proxy-authorization", ""},
{"range", ""},
{"referer", ""},
{"refresh", ""},
{"retry-after", ""},
{"server", ""},
{"set-cookie", ""},
{"strict-transport-security", ""},
{"transfer-encoding", ""},
{"user-agent", ""},
{"vary", ""},
{"via", ""},
{"www-authenticate", ""}
};
return table;
}
FieldLookupTable::FieldLookupTable(quint32 maxSize, bool use)
: maxTableSize(maxSize),
tableCapacity(maxSize),
@ -296,12 +221,12 @@ void FieldLookupTable::evictEntry()
quint32 FieldLookupTable::numberOfEntries() const
{
return quint32(staticTable().size()) + nDynamic;
return quint32(staticPart().size()) + nDynamic;
}
quint32 FieldLookupTable::numberOfStaticEntries() const
{
return quint32(staticTable().size());
return quint32(staticPart().size());
}
quint32 FieldLookupTable::numberOfDynamicEntries() const
@ -326,24 +251,18 @@ void FieldLookupTable::clearDynamicTable()
bool FieldLookupTable::indexIsValid(quint32 index) const
{
return index && index <= staticTable().size() + nDynamic;
return index && index <= staticPart().size() + nDynamic;
}
quint32 FieldLookupTable::indexOf(const QByteArray &name, const QByteArray &value)const
{
// Start from the static part first:
const auto &table = staticTable();
const auto &table = staticPart();
const HeaderField field(name, value);
const auto staticPos = std::lower_bound(table.begin(), table.end(), field,
[](const HeaderField &lhs, const HeaderField &rhs) {
int cmp = compare(lhs.name, rhs.name);
if (cmp)
return cmp < 0;
return compare(lhs.value, rhs.value) < 0;
});
const auto staticPos = findInStaticPart(field, CompareMode::nameAndValue);
if (staticPos != table.end()) {
if (staticPos->name == name && staticPos->value == value)
return staticPos - table.begin() + 1;
return quint32(staticPos - table.begin() + 1);
}
// Now we have to lookup in our dynamic part ...
@ -366,15 +285,12 @@ quint32 FieldLookupTable::indexOf(const QByteArray &name, const QByteArray &valu
quint32 FieldLookupTable::indexOf(const QByteArray &name) const
{
// Start from the static part first:
const auto &table = staticTable();
const auto &table = staticPart();
const HeaderField field(name, QByteArray());
const auto staticPos = std::lower_bound(table.begin(), table.end(), field,
[](const HeaderField &lhs, const HeaderField &rhs) {
return compare(lhs.name, rhs.name) < 0;
});
const auto staticPos = findInStaticPart(field, CompareMode::nameOnly);
if (staticPos != table.end()) {
if (staticPos->name == name)
return staticPos - table.begin() + 1;
return quint32(staticPos - table.begin() + 1);
}
// Now we have to lookup in our dynamic part ...
@ -402,7 +318,7 @@ bool FieldLookupTable::field(quint32 index, QByteArray *name, QByteArray *value)
if (!indexIsValid(index))
return false;
const auto &table = staticTable();
const auto &table = staticPart();
if (index - 1 < table.size()) {
*name = table[index - 1].name;
*value = table[index - 1].value;
@ -477,7 +393,7 @@ quint32 FieldLookupTable::keyToIndex(const SearchEntry &key) const
Q_ASSERT(offset < ChunkSize);
Q_ASSERT(chunkIndex || offset >= begin);
return quint32(offset + chunkIndex * ChunkSize - begin + 1 + staticTable().size());
return quint32(offset + chunkIndex * ChunkSize - begin + 1 + staticPart().size());
}
FieldLookupTable::SearchEntry FieldLookupTable::frontKey() const
@ -526,6 +442,103 @@ void FieldLookupTable::setMaxDynamicTableSize(quint32 size)
updateDynamicTableSize(size);
}
// This data is from the HPACK's specs and it's quite conveniently sorted,
// except ... 'accept' is in the wrong position, see how we handle it below.
const std::vector<HeaderField> &FieldLookupTable::staticPart()
{
static std::vector<HeaderField> table = {
{":authority", ""},
{":method", "GET"},
{":method", "POST"},
{":path", "/"},
{":path", "/index.html"},
{":scheme", "http"},
{":scheme", "https"},
{":status", "200"},
{":status", "204"},
{":status", "206"},
{":status", "304"},
{":status", "400"},
{":status", "404"},
{":status", "500"},
{"accept-charset", ""},
{"accept-encoding", "gzip, deflate"},
{"accept-language", ""},
{"accept-ranges", ""},
{"accept", ""},
{"access-control-allow-origin", ""},
{"age", ""},
{"allow", ""},
{"authorization", ""},
{"cache-control", ""},
{"content-disposition", ""},
{"content-encoding", ""},
{"content-language", ""},
{"content-length", ""},
{"content-location", ""},
{"content-range", ""},
{"content-type", ""},
{"cookie", ""},
{"date", ""},
{"etag", ""},
{"expect", ""},
{"expires", ""},
{"from", ""},
{"host", ""},
{"if-match", ""},
{"if-modified-since", ""},
{"if-none-match", ""},
{"if-range", ""},
{"if-unmodified-since", ""},
{"last-modified", ""},
{"link", ""},
{"location", ""},
{"max-forwards", ""},
{"proxy-authenticate", ""},
{"proxy-authorization", ""},
{"range", ""},
{"referer", ""},
{"refresh", ""},
{"retry-after", ""},
{"server", ""},
{"set-cookie", ""},
{"strict-transport-security", ""},
{"transfer-encoding", ""},
{"user-agent", ""},
{"vary", ""},
{"via", ""},
{"www-authenticate", ""}
};
return table;
}
std::vector<HeaderField>::const_iterator FieldLookupTable::findInStaticPart(const HeaderField &field, CompareMode mode)
{
const auto &table = staticPart();
const auto acceptPos = table.begin() + 18;
if (field.name == "accept") {
if (mode == CompareMode::nameAndValue && field.value != "")
return table.end();
return acceptPos;
}
auto predicate = [mode](const HeaderField &lhs, const HeaderField &rhs) {
const int cmp = compare(lhs.name, rhs.name);
if (cmp)
return cmp < 0;
else if (mode == CompareMode::nameAndValue)
return compare(lhs.value, rhs.value) < 0;
return false;
};
const auto staticPos = std::lower_bound(table.begin(), acceptPos, field, predicate);
if (staticPos != acceptPos)
return staticPos;
return std::lower_bound(acceptPos + 1, table.end(), field, predicate);
}
}
QT_END_NAMESPACE

11
src/network/access/http2/hpacktable_p.h
View File

@ -173,6 +173,8 @@ public:
bool updateDynamicTableSize(quint32 size);
void setMaxDynamicTableSize(quint32 size);
static const std::vector<HeaderField> &staticPart();
private:
// Table's maximum size is controlled
// by SETTINGS_HEADER_TABLE_SIZE (HTTP/2, 6.5.2).
@ -225,9 +227,16 @@ private:
quint32 indexOfChunk(const Chunk *chunk) const;
quint32 keyToIndex(const SearchEntry &key) const;
enum class CompareMode {
nameOnly,
nameAndValue
};
static std::vector<HeaderField>::const_iterator findInStaticPart(const HeaderField &field, CompareMode mode);
mutable QByteArray dummyDst;
Q_DISABLE_COPY(FieldLookupTable);
Q_DISABLE_COPY(FieldLookupTable)
};
}

49
tests/auto/network/access/hpack/tst_hpack.cpp
View File

@ -57,7 +57,6 @@ private Q_SLOTS:
void lookupTableConstructor();
void lookupTableStatic_data();
void lookupTableStatic();
void lookupTableDynamic();
@ -126,7 +125,7 @@ void tst_Hpack::bitstreamConstruction()
// 'Read' some data back:
for (int i = 0; i < size; ++i) {
uchar bitPattern = 0;
const auto bitsRead = in.peekBits(i * 8, 8, &bitPattern);
const auto bitsRead = in.peekBits(quint64(i * 8), 8, &bitPattern);
QVERIFY(bitsRead == 8);
QVERIFY(bitPattern == bytes[i]);
}
@ -282,7 +281,7 @@ void tst_Hpack::bitstreamCompression()
const auto start = QRandomGenerator::global()->bounded(uint(bytes.length()) / 2);
auto end = start * 2;
if (!end)
end = bytes.length() / 2;
end = unsigned(bytes.length() / 2);
strings.push_back(bytes.substr(start, end - start));
const auto &s = strings.back();
totalStringBytes += s.size();
@ -384,43 +383,21 @@ void tst_Hpack::lookupTableConstructor()
}
}
void tst_Hpack::lookupTableStatic_data()
{
QTest::addColumn<QByteArray>("expectedName");
QTest::addColumn<QByteArray>("expectedValue");
// Some predefined fields to find
// (they are always defined/required by HPACK).
QTest::newRow(":authority|") << QByteArray(":authority") << QByteArray("");
QTest::newRow(":method|GET") << QByteArray(":method") << QByteArray("GET");
QTest::newRow(":method|POST") << QByteArray(":method") << QByteArray("POST");
QTest::newRow(":path|/") << QByteArray(":path") << QByteArray("/");
QTest::newRow(":path|/index.html") << QByteArray(":path") << QByteArray("/index.html");
QTest::newRow(":scheme|http") << QByteArray(":scheme") << QByteArray("http");
QTest::newRow(":scheme|https") << QByteArray(":scheme") << QByteArray("https");
QTest::newRow(":status|200") << QByteArray(":status") << QByteArray("200");
QTest::newRow(":status|204") << QByteArray(":status") << QByteArray("204");
QTest::newRow(":status|206") << QByteArray(":status") << QByteArray("206");
QTest::newRow(":status|304") << QByteArray(":status") << QByteArray("304");
QTest::newRow(":status|400") << QByteArray(":status") << QByteArray("400");
QTest::newRow(":status|404") << QByteArray(":status") << QByteArray("404");
QTest::newRow(":status|500") << QByteArray(":status") << QByteArray("500");
}
void tst_Hpack::lookupTableStatic()
{
const FieldLookupTable table(0, false /*all static, no need in 'search index'*/);
QFETCH(QByteArray, expectedName);
QFETCH(QByteArray, expectedValue);
const quint32 index = table.indexOf(expectedName, expectedValue);
QVERIFY(index != 0);
const auto &staticTable = FieldLookupTable::staticPart();
QByteArray name, value;
QVERIFY(table.field(index, &name, &value));
QCOMPARE(name, expectedName);
QCOMPARE(value, expectedValue);
quint32 currentIndex = 1; // HPACK is indexing starting from 1.
for (const HeaderField &field : staticTable) {
const quint32 index = table.indexOf(field.name, field.value);
QVERIFY(index != 0);
QCOMPARE(index, currentIndex);
QVERIFY(table.field(index, &name, &value));
QCOMPARE(name, field.name);
QCOMPARE(value, field.value);
++currentIndex;
}
}
void tst_Hpack::lookupTableDynamic()