You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2026/09/03 12:06:32 UTC
[PR] QPACK: free arena strings in reverse allocation order [trafficserver]
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3969481980
##########
src/proxy/http3/QPACK.cc:
##########
@@ -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 rewinds both entries.
Review Comment:
Fixed, both occurrences now read "so Arena can rewind both entries".
The description covers why the stronger claim does not hold: `Arena::free()`
walks the block list with `while (b->next)`, so it never inspects the last
block, and on a single-block arena the free is a no-op.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3987169396
##########
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();
+
Review Comment:
You were right and I was wrong on both counts. Rocky's LeakSanitizer run
reported exactly these two allocations, and the "same as `test_encode` /
`test_decode`" defence was empty: those never reach their `new` calls because
the QIF directories are absent, so their leaks never execute. This test does.
Fixed in 08ec6abf26. Both objects are RAII now, the handler on the stack and
`QPACK` in a `unique_ptr`. The lifetime concern was real, so instead of a fixed
sleep the handler counts deliveries and each decode waits on that count,
bounded, before the header and the handler go out of scope. That also makes the
test check that the completion event arrives at all.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3981508570
##########
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) {
Review Comment:
Not reachable: `max_string_len` comes from `proxy.config.http.header_field_max_size`,
which is `[0-65535]` in `RecordsConfig.cc:582` and additionally clamped to
`UINT16_MAX` in `HttpConfig.cc:1289`, so `encoded_string_len * 2` is at most
131070. The `uint32_t` on that line predates this PR, which only renamed the
pointer in that hunk. Leaving it.
##########
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();
+
Review Comment:
Same shape as `test_encode` and `test_decode` in this file (lines 297-299,
355-359), and for the same reason: `decode()` schedules events on
`all_ethreads[0]` that target `event_handler`, so a stack object would be torn
down with events for it still queued. The file's convention is to let these test
objects live rather than race the event thread. Happy to clean up the whole file
in a follow-up; not doing only the new case here.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3969647087
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -135,6 +135,10 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_
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;
}
Review Comment:
`xpack_decode_string()` now clears outputs on Huffman decode failure, but other early error returns (e.g., bounds/max-length failures before allocation) still return without deterministically setting `*str` / `str_length`. If a caller passes non-null/stale values, an error return can leave them unchanged, which is inconsistent with the PR description (“On failure the output is also cleared”) and can lead to misuse later. Consider initializing `*str = nullptr` and `str_length = 0` at function entry, and ensuring every error return leaves outputs in that same state.
##########
src/proxy/hdrs/unit_tests/test_XPACK.cc:
##########
@@ -155,6 +155,42 @@ 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;
Review Comment:
Initializing a `uint8_t[]` from a string literal implicitly includes a trailing `'\0'` byte (even though you manually set `bad_huffman_len = 9`), and can also trigger signedness/encoding warnings depending on compiler flags. To keep the test data unambiguous and warning-free, use an explicit byte initializer list (or `std::array<uint8_t, 9>`) that contains exactly the intended 9 bytes.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1521,8 +1525,11 @@ QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint1
read_len += ret;
// Value
- if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
- tmp > 0xFF) {
+ if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0) {
+ return -1;
+ }
+ if (tmp > 0xFF) {
+ arena.str_free(*value);
return -1;
}
Review Comment:
`_read_insert_with_name_ref()` enforces `tmp > 0xFF` while nearby/similar insert paths use `0xFFFF`. Since this looks surprising and is easy for future readers to “fix” incorrectly, add a short comment explaining why this instruction’s value-length limit is intentionally 8-bit (if it is), or reference the relevant spec/constraint.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1545,15 +1552,24 @@ QPACK::_read_insert_without_name_ref(IOBufferReader &reader, Arena &arena, char
// Name
uint64_t tmp;
- if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 && tmp > 0xFFFF) {
+ if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0) {
+ return -1;
+ }
+ if (tmp > 0xFFFF) {
+ arena.str_free(*name);
return -1;
}
name_len = tmp;
read_len += ret;
// Value
- if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
- tmp > 0xFFFF) {
+ 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;
}
Review Comment:
After freeing `*name` / `*value` on error paths, consider setting the corresponding pointers to `nullptr` (and optionally lengths to 0) before returning. This reduces the chance of accidental reuse/double-free if this function is later refactored (e.g., adding shared cleanup code) or if callers start doing error-path cleanup in the future.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3969480830
##########
src/proxy/http3/QPACK.cc:
##########
@@ -804,9 +804,15 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t
}
read_len += ret;
- char *value;
+ 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
Review 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.
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -135,6 +135,8 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_
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);
return XPACK_ERROR_COMPRESSION_ERROR;
}
Review Comment:
Cleared the output on failure, `*str` to `nullptr` and `str_length` to 0.
On the mechanism: `Arena::str_free()` only moves the block's water level back.
Nothing is unmapped and nothing goes back to the allocator, so the pointer is
stale rather than dangling in the usual sense. The real hazard was narrower and
worse than a stale read: in `_read_insert_without_name_ref()` the value
allocation lands on the space the name just gave up, so the two alias.
Decoding into a local and assigning only on success does not fix that. The
caller declares `char *name;` uninitialised, so leaving `*str` untouched on
failure hands it an indeterminate pointer instead of a stale one.
What removes the hazard is the guard. Those paths read
`xpack_decode_string(...) < 0 && tmp > 0xFFFF`, and `tmp` is only written on
success, so a failed decode could fall through on an indeterminate read. That
is fixed in the same push, and it is what makes clearing the output safe.
##########
src/proxy/hdrs/unit_tests/test_XPACK.cc:
##########
@@ -155,6 +155,42 @@ 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;
Review Comment:
Left as is: this is the pattern the file already uses. `huff_encoded` at line
236 and `empty_encoded` at 264 both declare a string literal with the length
written out separately. Changing only the new section would make it the odd one
out. Worth doing across the file, but not in this change.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -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 rewinds both entries.
+ this->_arena.str_free(value);
this->_arena.str_free(name);
Review Comment:
Agreed it is uncovered, and the description says so.
The fixture side is not the obstacle — `test_qpack` already drives a real
`QPACK` through `decode()`. `QPACK::_arena` is private, so the baseline-pointer
probe would mean widening the class's surface for the test alone. Leaving that
to a follow-up.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#issuecomment-5631688367
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 #13655 return on failure; on master that path falls through instead. The one-line release sits inside the guard body #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 #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.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3967814266
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -135,6 +135,8 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_
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);
return XPACK_ERROR_COMPRESSION_ERROR;
}
Review Comment:
On the Huffman failure path, `*str` is freed but left pointing to freed arena memory (dangling). Given the note in the PR description that some call paths may incorrectly dereference this pointer after an error, this change can turn a leak into an input-triggerable use-after-free. Prefer decoding into a local temporary pointer and only assigning `*str` on success, or at minimum set `*str = nullptr` (and consider `str_length = 0`) after `arena.str_free(*str)` before returning.
##########
src/proxy/hdrs/unit_tests/test_XPACK.cc:
##########
@@ -155,6 +155,42 @@ 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;
Review Comment:
Initializing a `uint8_t[]` from a string literal implicitly appends a NUL terminator, while `bad_huffman_len` is hard-coded to 9. This is correct today but brittle (easy to desync if the literal changes). Consider switching to an explicit byte initializer list and deriving the length from `sizeof(bad_huffman)` to keep the buffer and length consistent.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -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 rewinds both entries.
+ this->_arena.str_free(value);
this->_arena.str_free(name);
Review Comment:
The QPACK free-order fix is not covered by an automated test (the new coverage focuses on XPACK’s failure path). If feasible, add a focused regression test that exercises these QPACK paths repeatedly and asserts the arena does not grow (similar to the new XPACK test’s 'baseline pointer' approach), to prevent future reordering regressions.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -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 rewinds both entries.
Review Comment:
The comment is phrased as a guaranteed outcome, but per the PR description `Arena::free()` may not rewind in some cases (e.g., when only a single block exists / last block not inspected). Consider softening the wording to reflect intent (e.g., '...so Arena can rewind...') to avoid overpromising behavior.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3981485803
##########
src/proxy/http3/QPACK.cc:
##########
@@ -804,9 +804,15 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t
}
read_len += ret;
- char *value;
+ 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
Review 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.
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -135,6 +135,8 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_
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);
return XPACK_ERROR_COMPRESSION_ERROR;
}
Review Comment:
Correcting my earlier reply here. I had said the guard fix was in this push; it
is not. That work overlapped #13655 and has been taken out of this PR, and the
`*str = nullptr` clearing went with it because it is only safe once those
guards return on failure.
What is here instead: the temporary is decoded through a local and assigned to
`*str` only on success, so the freed pointer never escapes the function and
there is nothing left dangling. `SECTION("outputs are written only on
success")` asserts a sentinel pointer and length survive both a huffman failure
and a truncated literal.
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -135,6 +135,10 @@ xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_
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;
}
Review Comment:
Addressed by contract rather than by clearing. The outputs are now never written
on any error return: the huffman result goes through a local and is assigned to
`*str` and `str_length` only after `huffman_decode()` succeeds, and the four
earlier returns never touched them. The header documents that. `SECTION("outputs
are written only on success")` checks a sentinel pointer and length survive both
a post-allocation failure and a pre-allocation one.
Clearing to `nullptr`/`0` specifically has to wait for #13655: three encoder
stream call sites guard with `< 0 && tmp > N`, and a zero `str_length` makes
that fall through deterministically into `str_free(nullptr)`. That is why the
earlier attempt at it in this PR was reverted.
##########
src/proxy/hdrs/unit_tests/test_XPACK.cc:
##########
@@ -155,6 +155,42 @@ 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;
Review Comment:
Leaving as is, same reason as before: `huff_encoded` at line 236 and
`empty_encoded` at 264 in this file use the same string-literal form with a
separate length. Changing only the new sections makes them the odd ones out.
The full build is warning-free on this file.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1521,8 +1525,11 @@ QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint1
read_len += ret;
// Value
- if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
- tmp > 0xFF) {
+ if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0) {
+ return -1;
+ }
+ if (tmp > 0xFF) {
+ arena.str_free(*value);
return -1;
}
Review Comment:
This code is no longer in the PR. The guard change it was part of overlapped
#13655, which fixes all of these guards together and also raises this one bound
from `0xFF` to `0xFFFF` to match its siblings. Nothing here touches that line
now.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -1545,15 +1552,24 @@ QPACK::_read_insert_without_name_ref(IOBufferReader &reader, Arena &arena, char
// Name
uint64_t tmp;
- if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 && tmp > 0xFFFF) {
+ if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0) {
+ return -1;
+ }
+ if (tmp > 0xFFFF) {
+ arena.str_free(*name);
return -1;
}
name_len = tmp;
read_len += ret;
// Value
- if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 &&
- tmp > 0xFFFF) {
+ 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;
}
Review Comment:
This code is no longer in the PR; the guard split it belonged to moved to
#13655. The remaining error-path free at `QPACK.cc:810` releases `name` and
returns immediately, and the caller does not read it after a `-1`.
##########
src/proxy/http3/QPACK.cc:
##########
@@ -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 rewinds both entries.
+ this->_arena.str_free(value);
this->_arena.str_free(name);
Review Comment:
Now covered, at the level where it is testable. `test_arena.cc` pins the
invariant directly: the most recent allocation rewinds, two allocations freed
in reverse order both come back, two freed in allocation order do not (129
assertions; swapping the order in the test fails it). `test_QPACK.cc` decodes a
Literal Header Field Without Name Reference, the path with the reordered
frees, 200 times through `decode()` and asserts the field each time.
`QPACK::_arena` is private, so the arena-growth assertion lives against `Arena`
rather than through `QPACK`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3981463853
##########
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();
+
Review Comment:
This test allocates `QPACK` and `TestQPACKEventHandler` with `new` and never releases them, which can produce LSAN/ASAN failures even when the test assertions pass. Prefer stack objects (or other RAII) and keep pointers only as non-owning aliases.
##########
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) {
Review Comment:
In the Huffman branch, `encoded_string_len` is a `uint64_t` but it is implicitly narrowed to `uint32_t` (both via `str_len` and the `huffman_decode` `src_len` parameter). If `max_string_len` is ever configured above `UINT32_MAX/2`, this can truncate/wrap the lengths, allocate a too-small buffer, and pass inconsistent sizes into the decoder. Add an explicit bound check and casts so the function fails safely on oversized inputs.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#issuecomment-6303748545
The description says the release of `name` in `_read_insert_without_name_ref()` when the value decode fails belongs in #13655. #13655 has merged (`bfe88e104a`) and only changed the guards and added a test; it did not add that release. So it still needs to be done, and it fits in this PR.
When the value decode fails, `name` was already allocated by the preceding successful decode and nothing frees it. Something like:
```cpp
// Value
if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 ||
tmp > 0xFFFF) {
arena.str_free(*name);
return -1;
}
```
`name` is the last allocation still outstanding at that point, so the free rewinds. The Insert Without Name Ref regression test mentioned in the description should also be possible now that the guards return on failure.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#issuecomment-5622186393
copilot is getting quite nit and stubborn, hope I am not getting into a rat-hole 🤣 .. lets see
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] QPACK: free arena strings in reverse allocation order [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13636:
URL: https://github.com/apache/trafficserver/pull/13636#discussion_r3960047257
##########
src/proxy/http3/QPACK.cc:
##########
@@ -804,9 +804,15 @@ QPACK::_decode_literal_header_field_without_name_ref(const uint8_t *buf, size_t
}
read_len += ret;
- char *value;
+ 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
Review Comment:
It looks like HPACK has the same issue. We may want to change xpack_decode_string?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org