QPACK: free arena strings in reverse allocation order

[brbzull0]

Motivation

Arena::free() (src/tscore/Arena.cc:129) only rewinds when the freed range
ends exactly at the block's water level:

if (b->m_water_level == (static_cast<char *>(mem) + size)) {
  b->m_water_level = static_cast<char *>(mem);
  return;
}

Anything else is a silent no-op: the space stays outstanding until the arena is
destroyed. HPACK and QPACK each keep one arena per connection for the strings
they decode, so every release that does not land on the water level makes that
connection's arena a little larger, for as long as the connection lives. Two
things in the header decode paths did exactly that.

Free order. Two QPACK sites allocated name then value and released
name first. With value still outstanding, the name free never reached the
water level, so only value was reclaimed:

• _decode_literal_header_field_without_name_ref() (QPACK.cc:822)
• the Insert Without Name Ref branch of _on_encoder_stream_read_ready()
  (QPACK.cc:1175), which also never freed value at all

Failure path. xpack_decode_string() allocates the huffman temporary area
before calling huffman_decode(), and on a decode failure returned without
releasing it. That is shared code, so the same thing happened at every caller:
HPACK.cc:622 and 648, and QPACK.cc:770, 802, 809 and 901.

Change

Both QPACK sites now free value before name, so each release lands on the
water level and both rewind.

Following @maskit's review, the failure-path release moved into
xpack_decode_string() rather than being repeated at each call site. That
covers HPACK with no change to HPACK.cc, and it is the only option for
QPACK.cc:770 and 901, where the output pointer is uninitialised on failure
and the caller has nothing it could release.

While there, the function got a clearer contract. The temporary is decoded
through a local and only assigned to *str once huffman_decode() succeeds,
so on any error return neither output is written and the released pointer never
reaches a caller. The header now says so, and also records that
max_string_len bounds the encoded length: a huffman-coded string may decode
to more than the limit, and the existing limit above encoded length allows
test depends on that.

QPACK.cc:809 additionally frees name on the value-error path. name came
from an earlier successful decode, so the callee cannot release it.

Tests

test_XPACK.cc:

• failed huffman decoding releases the temporary area — decodes malformed
  huffman input 100 times and checks the arena did not grow. Arena has no
  water-level accessor; the address str_alloc() returns is the proxy. Fails
  against master at test_XPACK.cc:191.
• outputs are written only on success — a sentinel pointer and length survive
  both a post-allocation failure (bad huffman) and a pre-allocation failure
  (truncated literal). Fails against the first revision of this PR with
  actual == <arena address>, i.e. it catches the released temporary being
  handed back.

test_arena.cc gains three cases (129 assertions) pinning the invariant the
free-order fix relies on: the most recent allocation rewinds, two allocations
freed in reverse order both come back, two freed in allocation order do not.
Swapping the free order in the test fails it.

test_QPACK.cc gains a case decoding a Literal Header Field Without Name
Reference, the representation whose name and value come from the arena, 200
times and asserting the decoded field each time. decode() schedules its
completion event on an event thread, so the handler counts deliveries and the
test waits on that count after every decode rather than sleeping; that also
checks the completion event arrives (1006 assertions). test_qpack previously
ran zero assertions: both existing cases return early when the QIF fixture
directories are absent, and Catch2 reports them as passed.

That case cannot observe the arena, since QPACK::_arena is private. It shows
the path decodes correctly; reverting the call-site free order is caught by the
test_arena.cc order case, not by test_qpack. Catching it through QPACK
would need a test-only accessor, which this PR does not add.

Full unit suite passes (85/85 excluding the verify_* plugin-load tests, which
need an install root). Built without quiche.

Note on Arena::free

Recording rather than fixing here: Arena::free() walks the block list with
while (b->next), so it never inspects the last block. While an arena holds a
single block every free is a no-op, including the ones in this change. Both
fixes take effect once the arena has two or more blocks (DEFAULT_BLOCK_SIZE
is 1000 bytes), which is where the unbounded growth was. The tests pad the arena
past its first block for this reason.

Sequencing with #13655

The three encoder-stream call sites at QPACK.cc:1528, 1552 and 1559
guard with xpack_decode_string(...) < 0 && tmp > N. tmp is only written on
success, so those guards read an indeterminate value after a failure and can
fall through to use the outputs. #13655 corrects them. This PR does not touch
those guards and should merge after #13655; the two branches merge cleanly.

Two things follow from that and belong in #13655 rather than here:

• Once its guards return on a failed decode, _read_insert_without_name_ref()
  can return -1 with name already allocated by the preceding successful
  decode, and nothing releases it. On master that path falls through instead,
  so the release is only reachable there. The line sits inside the guard body
  QPACK: use || in the varint decode range guards #13655 rewrites.
• A regression test for the Insert Without Name Ref branch has to feed the
  encoder stream. On master a failed decode on that path re-reads the same
  bytes indefinitely, so such a test does not terminate until QPACK: use || in the varint decode range guards #13655 is in.
  test_QPACK.cc already has the stream harness for it.

[Copilot]

🟢 Approval recommended

The changes are localized, align with Arena::free()’s rewind semantics, and add necessary failure-path cleanup without altering QPACK decode/encode logic.

Pull request overview

This PR fixes long-lived per-connection arena growth in the HTTP/3 QPACK implementation by ensuring QPACK-decoded header name/value strings are freed in strict LIFO order so Arena::free() can actually rewind the block water level.

Changes:

• Free value before name in _decode_literal_header_field_without_name_ref() so both allocations are reclaimed.
• Free value before name in the “Insert Without Name Ref” branch of _on_encoder_stream_read_ready() to reclaim both strings (and avoid leaving value outstanding).
• On xpack_decode_string() failure when decoding value, free any partially-allocated value (Huffman path) before freeing name.

File summaries

File	Description
src/proxy/http3/QPACK.cc	Fixes QPACK arena string frees to follow reverse allocation order and cleans up partial allocations on decode failure.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[maskit]

It looks like HPACK has the same issue. We may want to change xpack_decode_string?

[brbzull0]

Done — the free is now in xpack_decode_string() rather than at the call site.

You were right about HPACK: HPACK.cc:622 and 648 both returned without
releasing the huffman temporary area. Fixing it in the callee means neither
needs a change of its own. It also picks up QPACK.cc:770 and 901, where
value is uninitialised on the error return, so a caller-side free was not
possible there at all.

One difference worth noting: in HPACK the compression error tears the
connection down, so the arena goes with it. QPACK swallows the decode failure
at Http3HeaderVIOAdaptor.cc:97 under // FIXME: handle error, so there the
same leak repeats on a live connection.

[brbzull0]

Done. The release is in xpack_decode_string() now rather than at the call
site, reshaped slightly so the released temporary never reaches a caller: the
huffman output is decoded into a local, freed on failure, and assigned to the
outputs only once huffman_decode() succeeds. The header states that contract,
and that max_string_len bounds the encoded length (the existing "limit above
encoded length allows" test depends on that).

You were right about HPACK: HPACK.cc:622 and 648 both returned without
releasing the temporary. Fixing it in the callee means neither needs a change.
It also picks up QPACK.cc:770 and 901, where value is uninitialised on
failure, so a caller-side free was not possible there at all.

One thing I left alone deliberately: the encoder stream call sites at
QPACK.cc:1528, 1552, 1559 read tmp after a failed decode because their
guards are < 0 && tmp > N. #13655 fixes those, so this should merge after it.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

[brbzull0]

copilot is getting quite nit and stubborn, hope I am not getting into a rat-hole 🤣 .. lets see

[Copilot]

🟡 Changes recommended

There are still concrete correctness/robustness issues to address (notably a length-narrowing hazard in xpack_decode_string() and a unit-test heap allocation leak).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved findings require encoder-stream coverage, value-error cleanup, and an arena-reuse assertion.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/proxy/http3/QPACK.cc:1177

• Once #13655 makes the helper checks return on xpack_decode_string() failure, _read_insert_without_name_ref() can fail after the name decode has already allocated name but before the value decode succeeds. The caller then takes this < 0 path and skips the success-only cleanup here, leaving that name allocation in _arena until connection teardown for malformed encoder input. Release the already-decoded name on the helper's value-error path as well.

src/proxy/http3/QPACK.cc:1177

• The added test only invokes QPACK::decode() for a header block; it never opens or writes the encoder stream, so this Insert Without Name Reference branch—including the newly added value free—remains untested. The existing fixture-driven decoder test reaches it only when external QIF data is present and otherwise returns before making assertions. Add a focused encoder-stream regression test that feeds this instruction repeatedly and verifies arena reuse, so removing these frees is detected.

      // Free in reverse allocation order so Arena can rewind both entries.
      this->_arena.str_free(value);
      this->_arena.str_free(name);

src/proxy/http3/test/test_QPACK.cc:551

• The repeated-decode assertions only inspect the copied HTTPHdr field. With the pre-change str_free(name); str_free(value); order, _attach_header() still copies abc/xyz, so all 200 iterations pass while the arena continues to retain each name allocation. Add an observable arena-reuse/growth assertion (or a test-only hook) so reverting this call-site order makes the test fail.

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

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[brbzull0]

Copilot's latest review left three suppressed findings rather than inline comments, so answering them here. None change this PR; two belong in #13655.

• _read_insert_without_name_ref() does not release name when the value decode fails. Correct, and only reachable once the guards in QPACK: use || in the varint decode range guards #13655 return on failure; on master that path falls through instead. The one-line release sits inside the guard body QPACK: use || in the varint decode range guards #13655 rewrites, so it goes there, not here, to keep the two branches merging cleanly.
• No encoder-stream test for the Insert Without Name Ref branch. Correct. It also has to wait for QPACK: use || in the varint decode range guards #13655: on master a failed decode on that path re-reads the same bytes indefinitely, so a regression test would not terminate. test_QPACK.cc already has the stream harness for it.
• The repeated-decode test does not detect a reverted free order. Correct, and the description says so. QPACK::_arena is private, so that test shows the path decodes correctly; the free-order invariant is asserted in test_arena.cc, where swapping the order fails it. Catching it through QPACK would need a test-only accessor, which this PR does not add.