brbzull0 opened a new pull request, #13636:
URL: https://github.com/apache/trafficserver/pull/13636
`Arena::free()` (`src/tscore/Arena.cc:129`) only rewinds when the freed range
ends exactly at the block's water level:
```c
if (b->m_water_level == (static_cast<char *>(mem) + size)) {
b->m_water_level = static_cast<char *>(mem);
return;
}
```
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.
Two QPACK sites got the order wrong:
- `_decode_literal_header_field_without_name_ref()` (`QPACK.cc:827`)
allocated
`name`, then `value`, then freed `name` first. The `name` free did nothing
and
only `value` was reclaimed.
- The Insert Without Name Ref branch of `_on_encoder_stream_read_ready()`
(`QPACK.cc:1180`) freed `name` and never freed `value` at all.
Because the QPACK arena lives as long as the connection, both cases
accumulate
per decoded field rather than per request.
### Change
Free `value` before `name` at both sites, so each free lands at the water
level
and both entries rewind. Also release whatever `xpack_decode_string()`
allocated
before returning failure -- it can allocate on the Huffman path and then
fail,
so the error return needs the same treatment (`value` is initialised to
`nullptr` so the guard is meaningful).
### Test
No test. This is worth being explicit about rather than glossing:
`Arena` exposes no water-level or used-bytes accessor, so there is nothing to
assert against from a unit test. And the leak is invisible to ASan and LSan
because `Arena::~Arena()` calls `reset()`, which frees every block at
connection
close -- so nothing is leaking at process exit, the arena just grows while
the
connection is alive.
Making this testable would mean adding an accessor to `Arena` for the sake of
the test, which seemed worse than submitting it with the reasoning above and
the
`Arena::free()` behaviour cited directly.
Run as a no-regression check, 4/4 pass: `h3_proxy_verifier`,
`h3_python_client`,
`h3_stream_lifetime`, `h3_flow_control` -- these drive real QPACK header
exchange through both the decode path and the encoder stream.
### Note on scope
The original change also touched `_read_insert_without_name_ref()`. That
hunk is
omitted here: its frees sit inside an
`if (xpack_decode_string(...) < 0 && tmp > 0xFFFF)` body, and with that `&&`
the
body is unreachable, so the code would be dead on arrival. It belongs with a
change that corrects those guards, not with this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
