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