Copilot commented on code in PR #3474:
URL: https://github.com/apache/brpc/pull/3474#discussion_r3835582001
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1249,6 +1324,12 @@ int
H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
if (rc == 0) {
break;
}
+ _decoded_header_list_size += pair.name.size() + pair.value.size() + 32;
+ if (_decoded_header_list_size > max_header_list_size) {
+ LOG(ERROR) << "Decoded header list exceeds max_header_list_size="
+ << max_header_list_size << ", stream_id=" << _stream_id;
+ return -1;
Review Comment:
_decoded_header_list_size is never reset when a new HEADERS block begins
(e.g. trailers on the same stream). As written, the size accumulates across
multiple header lists on the same stream, which can incorrectly reject valid
trailers once the initial header list consumed most of the budget. Consider
resetting _decoded_header_list_size (and any related per-header-block state) at
the start of H2StreamContext::OnHeaders (i.e. when starting a new header block)
while keeping it cumulative across CONTINUATION decoding for the same block.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -695,6 +745,19 @@ H2ParseResult H2StreamContext::OnContinuation(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
_parsed_length += FRAME_HEAD_SIZE + frame_head.payload_size;
it.append_and_forward(&_remaining_header_fragment,
frame_head.payload_size);
+ // A header block may span many CONTINUATION frames; ConsumeHeaders()
+ // drains complete fields, so the fragment only buffers one incomplete
+ // field, whose wire size never legitimately exceeds the decoded header
+ // list limit. Without this cap a never-completed field (e.g. a huge
+ // declared string length) accumulates unbounded memory.
+ if (_remaining_header_fragment.size() >
+ _conn_ctx->_unack_local_settings.max_header_list_size) {
+ LOG(ERROR) << "Accumulated header fragment exceeds"
+ " max_header_list_size="
+ << _conn_ctx->_unack_local_settings.max_header_list_size
Review Comment:
The fragment-size cap is only applied on CONTINUATION frames. A large
initial HEADERS fragment (bounded by SETTINGS_MAX_FRAME_SIZE, which can be much
larger than max_header_list_size) can already be appended into
_remaining_header_fragment in H2StreamContext::OnHeaders before any
CONTINUATION is processed, allowing a single HEADERS to grow memory beyond the
intended max_header_list_size bound. Consider applying the same
_remaining_header_fragment.size() check after appending leftover bytes in
OnHeaders as well.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
