Skip to content

read GOAWAY fields before Additional Debug Data in OnGoAway#3395

Open
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:h2-goaway-field-order
Open

read GOAWAY fields before Additional Debug Data in OnGoAway#3395
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:h2-goaway-field-order

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

  1. RFC 7540 section 6.8 puts Last-Stream-ID and Error Code at the front of the GOAWAY payload with the Additional Debug Data after them, but OnGoAway skips payload_size - 8 bytes first and then reads the trailing 8 bytes, so a peer that attaches debug data gets to choose the Last-Stream-ID we act on.
  2. It also keeps the reserved high bit, so a 0xff debug byte makes last_stream_id negative, and _goaway_stream_id then never passes the >= 0 test in TryToInsertStream, leaving the client free to open new streams on a connection the peer has already abandoned.

What is changed and the side effects?

Changed: moved the two reads ahead of the forward and masked the reserved bit, the way ConsumeFrameHead and OnWindowUpdate already treat theirs. The new http2_goaway_with_debug_data case sends a GOAWAY announcing stream 1 with 8 bytes of debug data and checks _goaway_stream_id ends up as 1 instead of -1.

Side effects:

  • Performance effects: none, same number of bytes consumed from the iterator.

  • Breaking backward compatibility: no. A GOAWAY carrying no debug data parses exactly as before.

Signed-off-by: ubeddulla khan <ubed@bugqore.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes HTTP/2 GOAWAY frame parsing in brpc::policy::H2Context::OnGoAway to follow RFC 7540 layout and prevent peers from influencing Last-Stream-ID via Additional Debug Data. It also masks off the reserved high bit of Last-Stream-ID to avoid negative values affecting stream admission logic.

Changes:

  • Read Last-Stream-ID and Error Code from the beginning of the GOAWAY payload (before skipping debug data).
  • Mask off the reserved bit in Last-Stream-ID on receipt (& 0x7FFFFFFF).
  • Add a unit test covering GOAWAY with Additional Debug Data and reserved-bit handling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/brpc/policy/http2_rpc_protocol.cpp Fix GOAWAY parsing order and reserved-bit handling to prevent malformed/malicious debug data from affecting Last-Stream-ID.
test/brpc_http_rpc_protocol_unittest.cpp Add regression test for GOAWAY frames that include debug data and reserved-bit set in Last-Stream-ID.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants