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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
