QPACK: free arena strings in reverse allocation order - #13636
Conversation
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.
There was a problem hiding this comment.
🟢 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
valuebeforenamein_decode_literal_header_field_without_name_ref()so both allocations are reclaimed. - Free
valuebeforenamein the “Insert Without Name Ref” branch of_on_encoder_stream_read_ready()to reclaim both strings (and avoid leavingvalueoutstanding). - On
xpack_decode_string()failure when decodingvalue, free any partially-allocatedvalue(Huffman path) before freeingname.
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.
| 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 |
There was a problem hiding this comment.
It looks like HPACK has the same issue. We may want to change xpack_decode_string?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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.
fc1229c to
834443e
Compare
|
copilot is getting quite nit and stubborn, hope I am not getting into a rat-hole 🤣 .. lets see |
There was a problem hiding this comment.
🟡 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
Motivation
Arena::free()(src/tscore/Arena.cc:129) only rewinds when the freed rangeends exactly at the block's water level:
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
namethenvalueand releasednamefirst. Withvaluestill outstanding, thenamefree never reached thewater level, so only
valuewas reclaimed:_decode_literal_header_field_without_name_ref()(QPACK.cc:822)_on_encoder_stream_read_ready()(
QPACK.cc:1175), which also never freedvalueat allFailure path.
xpack_decode_string()allocates the huffman temporary areabefore calling
huffman_decode(), and on a decode failure returned withoutreleasing it. That is shared code, so the same thing happened at every caller:
HPACK.cc:622and648, andQPACK.cc:770,802,809and901.Change
Both QPACK sites now free
valuebeforename, so each release lands on thewater 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. Thatcovers HPACK with no change to
HPACK.cc, and it is the only option forQPACK.cc:770and901, where the output pointer is uninitialised on failureand 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
*stroncehuffman_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_lenbounds the encoded length: a huffman-coded string may decodeto more than the limit, and the existing
limit above encoded length allowstest depends on that.
QPACK.cc:809additionally freesnameon the value-error path.namecamefrom an earlier successful decode, so the callee cannot release it.
Tests
test_XPACK.cc:huffman input 100 times and checks the arena did not grow.
Arenahas nowater-level accessor; the address
str_alloc()returns is the proxy. Failsagainst master at
test_XPACK.cc:191.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 beinghanded back.
test_arena.ccgains three cases (129 assertions) pinning the invariant thefree-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.ccgains a case decoding a Literal Header Field Without NameReference, the representation whose name and value come from the arena, 200
times and asserting the decoded field each time (804 assertions).
test_qpackpreviously ran zero assertions: both existing cases return early when the QIF
fixture directories are absent, and Catch2 reports them as passed.
Full unit suite passes (85/85 excluding the
verify_*plugin-load tests, whichneed an install root). Built without quiche.
Note on Arena::free
Recording rather than fixing here:
Arena::free()walks the block list withwhile (b->next), so it never inspects the last block. While an arena holds asingle 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_SIZEis 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,1552and1559guard with
xpack_decode_string(...) < 0 && tmp > N.tmpis only written onsuccess, 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.