From 0ad88475ba4248ae18a64b768a05fbc9bfe8fd74 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 3 Sep 2026 14:04:37 +0200 Subject: [PATCH 1/4] QPACK: free arena strings in reverse allocation order Arena::free only rewinds when the freed range ends at the block's water level, so releasing an earlier allocation before a later one is a silent no-op and its space is never reclaimed. _decode_literal_header_field_without_name_ref() freed name before value, so the name never came back, and the Insert Without Name Ref branch of _on_encoder_stream_read_ready() never freed value at all. Free value then name at both sites, and release whatever xpack_decode_string allocated before failing on the Huffman path. --- src/proxy/http3/QPACK.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 38aa3a5630f..de92da8fe34 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -804,9 +804,15 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t } read_len += ret; - char *value; + char *value = nullptr; uint64_t value_len; if ((ret = xpack_decode_string(this->_arena, &value, value_len, buf + read_len, buf + buf_len, _header_field_max_size, 7)) < 0) { + // xpack_decode_string may allocate before returning failure (Huffman + // path). Free value first when present, then name, to preserve LIFO. + if (value != nullptr) { + this->_arena.str_free(value); + } + this->_arena.str_free(name); return -1; } read_len += ret; @@ -818,8 +824,9 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t QPACKDebug("Decoded Literal Header Field Without Name Ref: name=%.*s, value=%.*s", static_cast(name_len), name, static_cast(value_len), value); - this->_arena.str_free(name); + // Free in reverse allocation order so Arena rewinds both entries. this->_arena.str_free(value); + this->_arena.str_free(name); return read_len; } @@ -1170,6 +1177,8 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader &reader) QPACKDebug("Received Insert Without Name Ref: name=%.*s, value=%.*s", static_cast(name_len), name, static_cast(value_len), value); this->_dynamic_table.insert_entry(name, name_len, value, value_len); + // Free in reverse allocation order so Arena rewinds both entries. + this->_arena.str_free(value); this->_arena.str_free(name); } else if (buf & 0x20) { // Dynamic Table Size Update uint16_t max_size; From 4d0cc371ca52f1fd124d36bac954e65637770ace Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 10 Sep 2026 18:38:07 +0200 Subject: [PATCH 2/4] XPACK: release huffman temp area on decode failure xpack_decode_string() allocates the huffman temporary area out of the arena before calling huffman_decode(), and on a decode failure returned without releasing it. Every caller's arena lives for the connection, so each malformed huffman string left a little more of it permanently outstanding until the connection closed. Freeing in the callee rather than at each call site covers HPACK, whose two sites had the same leak, and the two QPACK sites where the output pointer is uninitialised on failure and a caller-side free is not possible. The temporary is decoded through a local and only assigned to the output on success, so a failure writes neither output and the released pointer never reaches a caller. The header now states that contract, and that max_string_len bounds the encoded length: a huffman string may decode to more than the limit, and the existing test relies on that. QPACK's literal-without-name-ref path also frees name on the value error path; name came from an earlier successful decode, so the callee cannot release it. --- include/proxy/hdrs/XPACK.h | 7 +++ src/proxy/hdrs/XPACK.cc | 6 ++- src/proxy/hdrs/unit_tests/test_XPACK.cc | 62 +++++++++++++++++++++++++ src/proxy/http3/QPACK.cc | 7 +-- 4 files changed, 74 insertions(+), 8 deletions(-) diff --git a/include/proxy/hdrs/XPACK.h b/include/proxy/hdrs/XPACK.h index 5829a4ac953..98d111d5e84 100644 --- a/include/proxy/hdrs/XPACK.h +++ b/include/proxy/hdrs/XPACK.h @@ -34,6 +34,13 @@ const static int XPACK_ERROR_SIZE_EXCEEDED_ERROR = -2; int64_t xpack_encode_integer(uint8_t *buf_start, const uint8_t *buf_end, uint64_t value, uint8_t n); int64_t xpack_decode_integer(uint64_t &dst, const uint8_t *buf_start, const uint8_t *buf_end, uint8_t n); int64_t xpack_encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, uint64_t value_len, uint8_t n = 7); + +/** Decode a string literal ([RFC 7541] 5.2) into @a arena. + * + * @a max_string_len bounds the encoded length, so a huffman-coded string may decode to more than that. + * @a str and @a str_length are written only on success, and the caller releases @a str with Arena::str_free(). A failure + * writes neither and leaves the caller nothing to release. + */ int64_t xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_t *buf_start, const uint8_t *buf_end, uint64_t max_string_len, uint8_t n = 7); diff --git a/src/proxy/hdrs/XPACK.cc b/src/proxy/hdrs/XPACK.cc index a45d2185b55..26019b1bf9e 100644 --- a/src/proxy/hdrs/XPACK.cc +++ b/src/proxy/hdrs/XPACK.cc @@ -131,12 +131,14 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_ if (isHuffman) { // Allocate temporary area twice the size of before decoded data uint32_t const str_len = encoded_string_len * 2; - *str = arena.str_alloc(str_len); + char *decoded = arena.str_alloc(str_len); - len = huffman_decode(*str, str_len, p, encoded_string_len); + len = huffman_decode(decoded, str_len, p, encoded_string_len); if (len < 0) { + arena.str_free(decoded); return XPACK_ERROR_COMPRESSION_ERROR; } + *str = decoded; str_length = len; } else { *str = arena.str_alloc(encoded_string_len); diff --git a/src/proxy/hdrs/unit_tests/test_XPACK.cc b/src/proxy/hdrs/unit_tests/test_XPACK.cc index 78911fc8b37..84e2511770d 100644 --- a/src/proxy/hdrs/unit_tests/test_XPACK.cc +++ b/src/proxy/hdrs/unit_tests/test_XPACK.cc @@ -155,6 +155,68 @@ TEST_CASE("XPACK_String", "[xpack]") } } + SECTION("failed huffman decoding releases the temporary area") + { + // 0x88 is the huffman flag plus a length of 8. An all-ones payload is not a + // decodable huffman sequence, so huffman_decode() fails after the temporary + // area has already been allocated out of the arena. + uint8_t bad_huffman[] = "\x88\xff\xff\xff\xff\xff\xff\xff\xff"; + int bad_huffman_len = 9; + + Arena arena; + + // Arena::free() walks the block list with `while (b->next)`, so it never + // inspects the last block. Put the arena past its first block, otherwise + // nothing can be observed to rewind at all. + for (int i = 0; i < 40; ++i) { + arena.str_alloc(64); + } + + // Arena has no water level accessor, so use the address str_alloc() hands + // back as the proxy for it. + char *baseline = arena.str_alloc(1); + arena.str_free(baseline); + + for (int i = 0; i < 100; ++i) { + char *actual = nullptr; + uint64_t actual_len = 0; + int len = xpack_decode_string(arena, &actual, actual_len, bad_huffman, bad_huffman + bad_huffman_len, MAX_FIELD_SIZE); + + REQUIRE(len == XPACK_ERROR_COMPRESSION_ERROR); + } + + // Had the failed decodes left their temporary areas outstanding, the water + // level would have advanced once per failure and this would not match. + // Compared as void * so a mismatch is reported as an address. + REQUIRE(static_cast(arena.str_alloc(1)) == static_cast(baseline)); + } + + SECTION("outputs are written only on success") + { + uint8_t bad_huffman[] = "\x88\xff\xff\xff\xff\xff\xff\xff\xff"; + int bad_huffman_len = 9; + // The length prefix announces ten octets but only four follow. + uint8_t truncated[] = {0x0a, 'c', 'u', 's', 't'}; + int truncated_len = 5; + + Arena arena; + char sentinel = '\0'; + char *actual = &sentinel; + uint64_t actual_len = 42; + + // Fails after the temporary area was allocated out of the arena ... + REQUIRE(xpack_decode_string(arena, &actual, actual_len, bad_huffman, bad_huffman + bad_huffman_len, MAX_FIELD_SIZE) == + XPACK_ERROR_COMPRESSION_ERROR); + CHECK(static_cast(actual) == static_cast(&sentinel)); + CHECK(actual_len == 42); + + // ... and before anything was allocated. + REQUIRE(xpack_decode_string(arena, &actual, actual_len, truncated, truncated + truncated_len, MAX_FIELD_SIZE) == + XPACK_ERROR_COMPRESSION_ERROR); + CHECK(static_cast(actual) == static_cast(&sentinel)); + CHECK(actual_len == 42); + } + SECTION("max_string_len enforcement") { // "custom-key" (10 bytes), non-huffman encoded: length byte 0x0a + raw string diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index de92da8fe34..3d140906385 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -804,14 +804,9 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t } read_len += ret; - char *value = nullptr; + char *value; uint64_t value_len; if ((ret = xpack_decode_string(this->_arena, &value, value_len, buf + read_len, buf + buf_len, _header_field_max_size, 7)) < 0) { - // xpack_decode_string may allocate before returning failure (Huffman - // path). Free value first when present, then name, to preserve LIFO. - if (value != nullptr) { - this->_arena.str_free(value); - } this->_arena.str_free(name); return -1; } From 834443e088b712a69d82f22841fe46c015fca22c Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 10 Sep 2026 18:38:08 +0200 Subject: [PATCH 3/4] QPACK: cover the arena free order with tests Arena::free() only rewinds when the freed range ends at the block's water level, so releasing in the wrong order silently keeps a per-connection arena growing. Nothing asserted that, and test_qpack ran no assertions at all because both of its cases bail out when the QIF directories are absent. test_arena covers the rewind and both free orders directly. test_qpack decodes a literal header field without a name reference, which is the representation whose name and value come from the arena. Also softens two comments: on a single-block arena Arena::free() never inspects the only block, so the release is not guaranteed to rewind. --- src/proxy/http3/QPACK.cc | 4 +- src/proxy/http3/test/test_QPACK.cc | 62 ++++++++++++++++++++++ src/tscore/unit_tests/test_arena.cc | 80 +++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 3d140906385..86eaa162375 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -819,7 +819,7 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t QPACKDebug("Decoded Literal Header Field Without Name Ref: name=%.*s, value=%.*s", static_cast(name_len), name, static_cast(value_len), value); - // Free in reverse allocation order so Arena rewinds both entries. + // Free in reverse allocation order so Arena can rewind both entries. this->_arena.str_free(value); this->_arena.str_free(name); @@ -1172,7 +1172,7 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader &reader) QPACKDebug("Received Insert Without Name Ref: name=%.*s, value=%.*s", static_cast(name_len), name, static_cast(value_len), value); this->_dynamic_table.insert_entry(name, name_len, value, value_len); - // Free in reverse allocation order so Arena rewinds both entries. + // Free in reverse allocation order so Arena can rewind both entries. this->_arena.str_free(value); this->_arena.str_free(name); } else if (buf & 0x20) { // Dynamic Table Size Update diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index 7059c773f94..e89ef5a9592 100644 --- a/src/proxy/http3/test/test_QPACK.cc +++ b/src/proxy/http3/test/test_QPACK.cc @@ -472,3 +472,65 @@ TEST_CASE("Decoding", "[qpack-decode]") } } } + +// Decodes one Literal Header Field Without Name Reference. That is the field +// representation whose name and value are both allocated out of QPACK's +// per-connection arena and released again once the header is attached, so it is +// the path that cares about the order those releases happen in. +TEST_CASE("Decoding a literal header field without name reference", "[qpack-literal-decode]") +{ + QUICApplicationDriver driver; + QPACK *qpack = new QPACK(driver.get_connection(), UINT32_MAX, 0, 0, MAX_FIELD_SIZE); + TestQPACKEventHandler *event_handler = new TestQPACKEventHandler(); + + // Header Data Prefix: Required Insert Count 0, Delta Base 0. + // Then 0x23: 001 N H , no never-index, no huffman, name of + // 3 bytes. Then 0x03: H , no huffman, value of 3 bytes. + // clang-format off + const uint8_t header_block[] = { + 0x00, 0x00, + 0x23, 'a', 'b', 'c', + 0x03, 'x', 'y', 'z', + }; + // clang-format on + + SECTION("the name and value survive the decode") + { + HTTPHdr hdr; + hdr.create(HTTPType::REQUEST); + + REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, event_handler, eventProcessor.all_ethreads[0]) == 0); + + MIMEField *field = hdr.field_find("abc"); + + REQUIRE(field != nullptr); + + auto value = field->value_get(); + + CHECK(value.length() == 3); + CHECK(memcmp(value.data(), "xyz", 3) == 0); + + hdr.destroy(); + } + + SECTION("repeated decodes keep returning the same field") + { + for (int i = 0; i < 200; i++) { + HTTPHdr hdr; + hdr.create(HTTPType::REQUEST); + + REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, event_handler, eventProcessor.all_ethreads[0]) == 0); + + MIMEField *field = hdr.field_find("abc"); + + REQUIRE(field != nullptr); + + auto value = field->value_get(); + + REQUIRE(value.length() == 3); + REQUIRE(memcmp(value.data(), "xyz", 3) == 0); + + hdr.destroy(); + } + } +} diff --git a/src/tscore/unit_tests/test_arena.cc b/src/tscore/unit_tests/test_arena.cc index cf472598bc4..ca5150cb99a 100644 --- a/src/tscore/unit_tests/test_arena.cc +++ b/src/tscore/unit_tests/test_arena.cc @@ -87,3 +87,83 @@ TEST_CASE("test arena", "[libts][arena]") a->reset(); } } + +// Arena::free() only rewinds when the freed range ends exactly at the block's +// water level, so the order in which allocations are released decides whether +// the space comes back. Callers that free out of order silently keep the arena +// growing, which matters for the long-lived per-connection arenas in HPACK and +// QPACK. +// +// Arena has no water level accessor. The address str_alloc() returns is the +// observable proxy: if a release rewound the block, the next allocation of the +// same size comes back at the same address. +// +// Note the warm-up loops below. Arena::free() walks the block list with +// `while (b->next)` and so never inspects the last block, which means nothing +// rewinds while the arena still holds a single block. The loops push the arena +// past that first block so the behaviour under test is reachable at all. + +TEST_CASE("arena releases the most recent allocation", "[libts][arena]") +{ + auto sizes = {size_t{1}, size_t{16}, size_t{127}, size_t{128}, size_t{129}, size_t{1000}, size_t{4096}, size_t{65535}}; + + for (auto size : sizes) { + Arena arena; + + for (int i = 0; i < 40; i++) { + arena.str_alloc(64); + } + + char *first = arena.str_alloc(size); + + REQUIRE(arena.str_length(first) == size); + + arena.str_free(first); + + REQUIRE(static_cast(arena.str_alloc(size)) == static_cast(first)); + } +} + +TEST_CASE("arena reclaims two allocations freed in reverse order", "[libts][arena]") +{ + Arena arena; + + for (int i = 0; i < 40; i++) { + arena.str_alloc(64); + } + + char *name_first = nullptr; + + for (int i = 0; i < 100; i++) { + char *name = arena.str_alloc(20); + char *value = arena.str_alloc(60); + + if (i == 0) { + name_first = name; + } + + arena.str_free(value); + arena.str_free(name); + + REQUIRE(static_cast(name) == static_cast(name_first)); + } +} + +TEST_CASE("arena does not reclaim allocations freed in allocation order", "[libts][arena]") +{ + Arena arena; + + for (int i = 0; i < 40; i++) { + arena.str_alloc(64); + } + + char *name = arena.str_alloc(20); + char *value = arena.str_alloc(60); + + // name does not end at the water level while value is still outstanding, so + // this free is a no-op and only value's space comes back. + arena.str_free(name); + arena.str_free(value); + + CHECK(static_cast(arena.str_alloc(20)) != static_cast(name)); +} From 08ec6abf261887fe2dc558a655f46ba99d6b4f46 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 11 Sep 2026 10:07:57 +0200 Subject: [PATCH 4/4] test_QPACK: own the objects the literal decode test creates The test allocated QPACK and its event handler with new and never released them, which LeakSanitizer reports. They could not simply be stack objects: decode() schedules its completion event on an event thread and hands it the handler, so the handler has to outlive that delivery. The handler now counts deliveries and the test waits on that count after every decode, bounded, before the header the event points at and the handler itself go out of scope. That also turns the wait into a check that the completion event arrives at all. --- src/proxy/http3/test/test_QPACK.cc | 41 +++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index e89ef5a9592..fbd01650866 100644 --- a/src/proxy/http3/test/test_QPACK.cc +++ b/src/proxy/http3/test/test_QPACK.cc @@ -22,10 +22,14 @@ */ #include +#include +#include #include #include #include #include +#include +#include #include "proxy/hdrs/XPACK.h" #include "proxy/http3/QPACK.h" #include "proxy/hdrs/HTTP.h" @@ -101,6 +105,7 @@ class TestQPACKEventHandler : public Continuation event_handler(int event, Event * /* data ATS_UNUSED */) { this->_event = event; + ++this->_events_seen; return 0; } @@ -110,8 +115,26 @@ class TestQPACKEventHandler : public Continuation return this->_event; } + // Delivered on an event thread, so wait for this to move rather than sleeping + // a fixed time when the handler must outlive the events QPACK schedules. + int + events_seen() + { + return this->_events_seen.load(); + } + + bool + wait_for_events(int expected) + { + for (int i = 0; i < 5000 && this->_events_seen.load() < expected; ++i) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + return this->_events_seen.load() >= expected; + } + private: - int _event = 0; + int _event = 0; + std::atomic _events_seen{0}; }; static int @@ -479,9 +502,12 @@ TEST_CASE("Decoding", "[qpack-decode]") // the path that cares about the order those releases happen in. TEST_CASE("Decoding a literal header field without name reference", "[qpack-literal-decode]") { - QUICApplicationDriver driver; - QPACK *qpack = new QPACK(driver.get_connection(), UINT32_MAX, 0, 0, MAX_FIELD_SIZE); - TestQPACKEventHandler *event_handler = new TestQPACKEventHandler(); + QUICApplicationDriver driver; + // decode() schedules its completion event on an event thread and hands it + // this handler, so every decode below is waited on before anything it can + // reach goes out of scope. + TestQPACKEventHandler event_handler; + auto qpack = std::make_unique(driver.get_connection(), UINT32_MAX, 0, 0, MAX_FIELD_SIZE); // Header Data Prefix: Required Insert Count 0, Delta Base 0. // Then 0x23: 001 N H , no never-index, no huffman, name of @@ -499,7 +525,9 @@ TEST_CASE("Decoding a literal header field without name reference", "[qpack-lite HTTPHdr hdr; hdr.create(HTTPType::REQUEST); - REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, event_handler, eventProcessor.all_ethreads[0]) == 0); + REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, &event_handler, eventProcessor.all_ethreads[0]) == 0); + REQUIRE(event_handler.wait_for_events(1)); + CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_COMPLETE); MIMEField *field = hdr.field_find("abc"); @@ -519,7 +547,8 @@ TEST_CASE("Decoding a literal header field without name reference", "[qpack-lite HTTPHdr hdr; hdr.create(HTTPType::REQUEST); - REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, event_handler, eventProcessor.all_ethreads[0]) == 0); + REQUIRE(qpack->decode(1, header_block, sizeof(header_block), hdr, &event_handler, eventProcessor.all_ethreads[0]) == 0); + REQUIRE(event_handler.wait_for_events(i + 1)); MIMEField *field = hdr.field_find("abc");