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