Copilot commented on code in PR #3690:
URL: https://github.com/apache/brpc/pull/3690#discussion_r4421643595
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -155,19 +144,13 @@ static int WriteAck(Socket* s, const void* data, size_t
n) {
butil::IOBuf sendbuf;
sendbuf.append(data, n);
Socket::WriteOptions wopt;
- // These writes historically ignored EOVERCROWDED so control replies
- // could always be queued, but that defeats the explicit memory bound of
- // -socket_max_unwritten_bytes: a peer flooding PING/SETTINGS frames
- // while withholding TCP reads makes this process buffer ack frames
- // without limit. Respect the bound by default; callers treat a failed
- // ack write as a connection error and close the overcrowded connection.
- wopt.ignore_eovercrowded = FLAGS_h2_ack_ignore_eovercrowded;
+ wopt.ignore_eovercrowded = true;
Review Comment:
Forced `ignore_eovercrowded = true` for HTTP/2 control-plane acks
(PING/SETTINGS/RST/GOAWAY/WINDOW_UPDATE) bypasses the
`-socket_max_unwritten_bytes` memory bound and can reintroduce unbounded
buffering under a peer that floods control frames while not reading. Consider
restoring the prior behavior (respect the bound by default) and/or keeping a
guarded flag, but defaulting to bounded behavior.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -590,11 +594,7 @@ ParseResult H2Context::Consume(
LOG(WARNING) << "Fail to send GOAWAY to " << *_socket;
return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
}
- // https://tools.ietf.org/html/rfc7540#section-5.4.1
- // A connection error is unrecoverable: close the connection
- // after sending GOAWAY instead of continuing to parse (and
- // buffer) whatever the misbehaving peer keeps sending.
- return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
+ return MakeMessage(NULL);
Review Comment:
After sending a connection-level GOAWAY, the code now returns
`MakeMessage(NULL)` (i.e., keeps parsing/accepting more frames) instead of
forcing connection teardown. For protocol violations, continuing to parse input
from a misbehaving peer can become an unbounded buffering/CPU sink. Consider
restoring the previous behavior: treat a connection error as unrecoverable and
stop parsing/close the connection after GOAWAY.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -797,26 +760,18 @@ H2ParseResult H2StreamContext::OnContinuation(
<< ", stream_id=" << frame_head.stream_id;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
- // See the same check in H2StreamContext::OnHeaders().
- if (_rejected_error != H2_NO_ERROR) {
- return MakeH2Error(_rejected_error, stream_id());
- }
if (_stream_ended) {
return OnEndStream();
}
}
- return MakeH2Message(nullptr);
+ return MakeH2Message(NULL);
}
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
uint32_t frag_size = frame_head.payload_size;
uint8_t pad_length = 0;
if (frame_head.flags & H2_FLAGS_PADDED) {
Review Comment:
This regresses validation for PADDED DATA frames: if `payload_size == 0` and
PADDED is set, `--frag_size` underflows (uint32_t) and can make subsequent size
checks behave incorrectly. Restore the explicit `payload_size == 0` (or
`frag_size == 0`) guard before decrementing.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1076,12 +1045,10 @@ H2ParseResult H2Context::OnGoAway(
LOG(ERROR) << "Invalid flags=" << h.flags;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
- // Last-Stream-ID and Error Code precede the Additional Debug Data.
- // The reserved bit of Last-Stream-ID is ignored on receipt.
- const int last_stream_id = static_cast<int>(LoadUint32(it) & 0x7FFFFFFF);
- const H2Error ALLOW_UNUSED h2_error = static_cast<H2Error>(LoadUint32(it));
// Skip Additional Debug Data
it.forward(h.payload_size - 8);
+ const int last_stream_id = static_cast<int>(LoadUint32(it));
+ const H2Error ALLOW_UNUSED h2_error = static_cast<H2Error>(LoadUint32(it));
Review Comment:
GOAWAY parsing is incorrect: the first 8 bytes of the payload are
`Last-Stream-ID` and `Error Code`, followed by optional debug data. Forwarding
`(payload_size - 8)` before reading causes the code to read from the debug-data
tail (or past the payload) rather than the fixed header fields. Read the two
uint32s first (masking the reserved bit of Last-Stream-ID with `0x7FFFFFFF`),
then forward the remaining debug data.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1222,10 +1189,6 @@ void H2Context::DeferWindowUpdate(int64_t size) {
SaveUint32(winbuf + FRAME_HEAD_SIZE, conn_wu);
if (WriteAck(_socket, winbuf, sizeof(winbuf)) != 0) {
LOG(WARNING) << "Fail to send WINDOW_UPDATE";
- // Retry on a later DATA frame instead of silently losing
- // the window bytes (the peer would stall otherwise).
- _deferred_window_update.fetch_add(
- conn_wu, butil::memory_order_relaxed);
}
Review Comment:
On WINDOW_UPDATE write failure, the deferred window bytes are now dropped.
That can permanently stall the peer (it will never regain window it expects),
causing hung streams/connections. Either restore re-adding the bytes back to
`_deferred_window_update` on write failure, or treat a failed WINDOW_UPDATE
write as a connection error that closes the socket so callers don't hang.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1338,13 +1300,6 @@ void H2StreamContext::SetState(H2StreamState state) {
int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
HPacker& hpacker = _conn_ctx->hpacker();
HttpHeader& h = header();
- // https://tools.ietf.org/html/rfc7540#section-10.5.1
- // Bound the cumulative decoded size of the header list. Without this
- // check nothing limits header bytes (-max_body_size covers DATA only)
- // and 1-byte HPACK indexed references to a large dynamic-table entry
- // amplify a small HEADERS/CONTINUATION frame ~4000x ("HPACK bomb").
- const uint32_t max_header_list_size =
- _conn_ctx->_unack_local_settings.max_header_list_size;
while (it) {
Review Comment:
This change removes multiple HTTP/2 header safety limits (e.g., max decoded
header list size / \"HPACK bomb\" mitigation, max header count enforcement, and
the prior stream-level rejection mechanism that continued decoding to keep the
dynamic table in sync). The new behavior can allow a single stream to drive
unbounded memory/cpu via excessive headers or oversized header blocks. Please
reintroduce: (1) a decoded header-list-size bound (RFC 7540/9113 10.5.1), (2)
header count limiting, and (3) stream-level rejection that keeps decoding but
avoids accumulating headers, so one bad stream doesn't force a connection-level
failure.
##########
src/brpc/details/hpack.h:
##########
@@ -108,6 +108,14 @@ class HPacker : public Describable {
void Encode(butil::IOBufAppender* out, const Header& header)
{ return Encode(out, header, HPackOptions()); }
+ // Encode header as a literal without consulting or mutating the dynamic
+ // table (RFC 7541 section 6.2.2 / 6.2.3). Safe to use for trailers whose
+ // transmission may be deferred behind flow-controlled DATA frames while
+ // other streams continue to encode HEADERS on the same connection.
Review Comment:
The comment says \"encode header as a literal\", but the implementation in
`EncodeWithoutDynamicTable` may emit an indexed representation (0x80) when the
header matches the static table. That's still correct for the goal (no dynamic
table interaction), but the wording is inaccurate. Update the comment to
reflect that it encodes without consulting/mutating the dynamic table and may
use static-table indexed encoding when possible.
--
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]
