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