QPACK: free arena strings in reverse allocation order

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);
 

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);

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<void *>(arena.str_alloc(1)) == static_cast<void *>(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<void *>(actual) == static_cast<void *>(&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<void *>(actual) == static_cast<void *>(&sentinel));
+    CHECK(actual_len == 42);
+  }
+
   SECTION("max_string_len enforcement")
   {
     // "custom-key" (10 bytes), non-huffman encoded: length byte 0x0a + raw string

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<uint16_t>(name_len), name,
              static_cast<uint16_t>(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<int>(name_len), name,
                  static_cast<int>(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;

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 <Name Length (3+)>, no never-index, no huffman, name of
+  // 3 bytes. Then 0x03: H <Value Length (7+)>, 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();
+    }
+  }
+}

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<void *>(arena.str_alloc(size)) == static_cast<void *>(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<void *>(name) == static_cast<void *>(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<void *>(arena.str_alloc(20)) != static_cast<void *>(name));
+}