diff --git a/src/brpc/details/hpack.cpp b/src/brpc/details/hpack.cpp index e627e81d25..d17191781a 100644 --- a/src/brpc/details/hpack.cpp +++ b/src/brpc/details/hpack.cpp @@ -223,11 +223,23 @@ int IndexTable::Init(const IndexTableOptions& options) { num_headers = options.static_table_size; _max_size = UINT_MAX; } else { - num_headers = options.max_size / (32 + 2); + num_headers = options.max_size / (32 + 1); // ^ - // name and value both have at least one byte in. + // AddHeader() only requires a non-empty name; the value may be empty, + // so the smallest possible entry is name(1) + value(0) + 32 = 33 bytes + // (rfc7541 section 4.1). Sizing the queue for 34-byte entries under- + // provisions it and lets a peer sending many 33-byte entries fill the + // queue before eviction triggers, tripping CHECK(!full()) in AddHeader. _max_size = options.max_size; } + // A dynamic table smaller than the 33-byte minimum entry (including the + // valid max_size == 0 case that disables it) yields num_headers == 0. + // malloc(0) may return NULL and make Init fail on some platforms, so keep + // at least one slot. No entry can actually be stored since entry_size > + // _max_size still holds in AddHeader(). + if (num_headers == 0) { + num_headers = 1; + } void *header_queue_storage = malloc(num_headers * sizeof(Header)); if (!header_queue_storage) { LOG(ERROR) << "Fail to malloc space for " << num_headers << " headers"; diff --git a/test/brpc_hpack_unittest.cpp b/test/brpc_hpack_unittest.cpp index 48d3ae52eb..91c53e0369 100644 --- a/test/brpc_hpack_unittest.cpp +++ b/test/brpc_hpack_unittest.cpp @@ -639,3 +639,37 @@ TEST_F(HPackTest, responses_with_huffman) { } ASSERT_TRUE(buf.buf().empty()); } + +TEST_F(HPackTest, many_small_indexed_headers) { + // Each entry below is a "literal header field with incremental indexing" + // (0x40) with a 1-byte name ("a") and an empty value, costing + // name(1) + value(0) + 32 = 33 bytes in the dynamic table (rfc7541 4.1). + // 121 of them stay under the default 4096-byte table (121*33 = 3993) so + // none are evicted and all must decode. The decode table's ring buffer was + // previously sized for 34-byte entries (4096/34 = 120), so the 121st + // AddHeader() tripped CHECK(!full()) and aborted the process. + brpc::HPacker p; + ASSERT_EQ(0, p.Init(4096)); + + const int num_headers = 121; + butil::IOBuf buf; + for (int i = 0; i < num_headers; ++i) { + const uint8_t entry[] = {0x40, 0x01, (uint8_t)'a', 0x00}; + buf.append(entry, sizeof(entry)); + } + for (int i = 0; i < num_headers; ++i) { + brpc::HPacker::Header h; + ASSERT_GT(p.Decode(&buf, &h), 0) << "failed at header " << i; + ASSERT_EQ("a", h.name); + ASSERT_TRUE(h.value.empty()); + } + ASSERT_TRUE(buf.empty()); +} + +TEST_F(HPackTest, zero_size_dynamic_table) { + // max_size == 0 disables the dynamic table. num_headers then rounds down to + // 0, and malloc(0) may return NULL, so Init must still keep one slot and + // succeed. No entry is ever stored because entry_size > max_size. + brpc::HPacker p; + ASSERT_EQ(0, p.Init(0)); +}