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 38aa3a5630f..86eaa162375 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -807,6 +807,7 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t 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) { + this->_arena.str_free(name); return -1; } read_len += ret; @@ -818,8 +819,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 can rewind both entries. this->_arena.str_free(value); + this->_arena.str_free(name); return read_len; } @@ -1170,6 +1172,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 can rewind both entries. + this->_arena.str_free(value); this->_arena.str_free(name); } else if (buf & 0x20) { // Dynamic Table Size Update uint16_t max_size; diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index 7059c773f94..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 @@ -472,3 +495,71 @@ 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; + // 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 + // 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); + REQUIRE(event_handler.wait_for_events(1)); + CHECK(event_handler.last_event() == QPACK_EVENT_DECODE_COMPLETE); + + 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); + REQUIRE(event_handler.wait_for_events(i + 1)); + + 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)); +}