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.
xpack_decode_string() allocates the huffman temporary area out of the arena before calling huffman_decode(), and returned the error without releasing it. Freeing it in the callee rather than at each call site covers HPACK and the two QPACK sites that leave the pointer uninitialised, where the caller cannot do it at all. The pointer is deliberately left set, not nulled: the encoder stream paths guarded by `< 0 && tmp > 0xFFFF` fall through on failure and read it, and those guards need fixing first.
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.
The three encoder stream guards read `xpack_decode_string(...) < 0 && tmp > 0xFFFF`, but tmp is only written on success, so a failed decode could fall through on an indeterminate read and go on to use the string. Each is now split so a decode failure returns, and the length check frees what it rejects. With no path left that uses the output after a failure, xpack_decode_string() can also clear it, which the previous commit could not safely do.
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.
|
|
||
| len = huffman_decode(*str, str_len, p, encoded_string_len); | ||
| if (len < 0) { | ||
| // Release the temporary area, otherwise it is left outstanding until the arena dies. | ||
| arena.str_free(*str); | ||
| *str = nullptr; | ||
| str_length = 0; | ||
| return XPACK_ERROR_COMPRESSION_ERROR; | ||
| } |
| uint8_t bad_huffman[] = "\x88\xff\xff\xff\xff\xff\xff\xff\xff"; | ||
| int bad_huffman_len = 9; |
| if (tmp > 0xFF) { | ||
| arena.str_free(*value); | ||
| return -1; | ||
| } |
| if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0) { | ||
| arena.str_free(*name); | ||
| return -1; | ||
| } | ||
| if (tmp > 0xFFFF) { | ||
| arena.str_free(*value); | ||
| arena.str_free(*name); | ||
| return -1; | ||
| } |
Arena::free()(src/tscore/Arena.cc:129) only rewinds when the freed rangeends exactly at the block's water level:
So releasing an earlier allocation while a later one is still outstanding is a
silent no-op, and that space is not reclaimed for the lifetime of the arena.
For HPACK and QPACK that arena lives as long as the connection.
Free order
Two QPACK sites got the order wrong:
_decode_literal_header_field_without_name_ref()(QPACK.cc:822) allocatedname, thenvalue, then freednamefirst. Thenamefree did nothing andonly
valuewas reclaimed._on_encoder_stream_read_ready()(
QPACK.cc:1175) freednameand never freedvalueat all.Both now free
valuebeforename, so each free lands at the water level andboth entries rewind.
Failure path
xpack_decode_string()allocates the huffman temporary area before callinghuffman_decode(), and returnedXPACK_ERROR_COMPRESSION_ERRORwithoutreleasing it when that decode failed. Malformed huffman input is client
controlled, so this is reachable per header field.
The first revision handled it at the one QPACK call site. Following @maskit's
review it is fixed in
xpack_decode_string()instead, which covers everycaller:
HPACK.cc:622,HPACK.cc:648HTTP2_ERROR_COMPRESSION_ERRORQPACK.cc:770,QPACK.cc:901valueis uninitialised at both, so a caller-side free is not possibleQPACK.cc:809QPACK.cc:809still freesnameitself, since that came from an earliersuccessful call. LIFO still holds there because
valuehas already beenrewound by the callee.
No caller frees on an error return, so there is no double free. On failure the
output is also cleared,
*strtonullptrandstr_lengthto 0, so a callercannot pick up a stale pointer or an indeterminate length.
That clearing is only safe because of the guard fix below. It has to land with
it, not before it.
Test
test_XPACK.ccgains a section that decodes malformed huffman input 100 timesand checks the arena has not grown.
Arenaexposes no water level accessor,but the address
str_alloc()hands back is a usable proxy for it, so thisneeds no new API. The existing huffman cases in that file all trip the
max_string_lencheck, which returns before anything is allocated, so none ofthem covered this path.
Verified in both directions:
All tests passed (338 assertions in 3 test cases)XPACK.cchunk reversed:test_XPACK.cc:191: FAILED: REQUIRE( static_cast<void *>(arena.str_alloc(1)) == static_cast<void *>(baseline) )The full unit suite passes, 126/126,
test_qpackandtest_http3included.Built without quiche, so that configuration is not covered here.
The free-order fix above is still not covered by a test.
test_qpackdoesdrive a real
QPACKthroughdecode(), butQPACK::_arenais private, soasserting on the water level would mean widening that for the test alone.
The autests were run against the first revision only, 4/4 pass:
h3_proxy_verifier,h3_python_client,h3_stream_lifetime,h3_flow_control.Note on Arena::free
Recording this rather than fixing it here:
Arena::free()walks the block listwith
while (b->next), so it never inspects the last block. While an arenastill holds a single block every free is a no-op, including the ones in this
change. Both fixes take effect once the arena holds two or more blocks
(
DEFAULT_BLOCK_SIZEis 1000 bytes), which is where the unbounded growth was,but "the free is reclaimed" is not true from the first allocation. The new test
has to allocate past the first block before it can observe anything, which is
why that loop is there.
Insert instruction guards
The three encoder stream call sites were guarded by
xpack_decode_string(...) < 0 && tmp > 0xFFFF.tmpis only written onsuccess, so on a failed decode that second operand is an indeterminate read:
when it happens to be false, control falls through and the caller goes on to
use a string that was never decoded.
_read_insert_without_name_ref()thenallocates the value over the space the name occupied, so the two alias.
Each is now split, so a decode failure returns and the length check frees what
it rejects:
A plain
&&to||swap would not do:ret >= 0 && tmp > 0xFFFFreturns -1with the string allocated and nobody freeing it.
The first revision left these alone on the grounds that they belonged to a
separate change. That reasoning does not survive
xpack_decode_string()clearing its output, which turns an indeterminate read into a reliably false
one.
Note on scope
Seven more guards share the same shape and are not touched here:
QPACK.cc:287,927,936,1521,1594,1616and1638. All of themcall
xpack_decode_integer(), which allocates nothing, so none leak or alias;they are bounds checks that do not work.
936additionally has the comparisoninverted,
delta_base_index < 0xFFFF.QPACK.cc:1528caps the value length at0xFFwhere every sibling uses0xFFFF. That looks like a typo, but correcting it changes which instructionsare rejected, so it is left as it is.