Skip to content

consume refused HEADERS block before RST_STREAM in h2 - #3542

Open
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:h2-refused-stream-drain
Open

ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:h2-refused-stream-drain

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve N/A

Problem Summary:

When a HEADERS frame opens a stream past max_concurrent_streams, H2Context::OnHeaders returns RST_STREAM(REFUSED_STREAM) but never consumes that frame's header-block payload. REFUSED_STREAM is a stream error, so the connection stays open and ParseH2Message keeps parsing; the leftover header-block bytes are then read by ConsumeFrameHead as the next frame head, letting a peer smuggle frames past the h2 framing layer, and the HPACK decoder is left desynced because the refused block was never decoded. OnData and the client-side unknown-stream branch right below already drain the payload before returning a stream error; only the refused-stream path skipped it.

What is changed and the side effects?

Changed:

Decode and consume the refused stream's header block (sctx->OnHeaders) before deleting the context and returning REFUSED_STREAM, mirroring the two sibling paths. This keeps both the frame parser and the HPACK decoder in sync. Added a regression test in test/brpc_http_rpc_protocol_unittest.cpp that fails before the change (only the 9-byte frame head was consumed, leaving the header block to be misparsed) and passes after.

Side effects:

  • Performance effects: one extra HPACK decode of a header block that is then discarded, only on refused streams.

  • Breaking backward compatibility: none.


Check List:

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.

🟡 Changes recommended

The refused-stream path ignores header-decoding failures, potentially leaving framing and HPACK state desynchronized.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes HTTP/2 refused-stream HEADERS handling and adds regression coverage.

Changes:

  • Consume refused HEADERS payloads to preserve framing and HPACK state.
  • Add a regression test for frame-buffer consumption.
File summaries
File Summary
test/brpc_http_rpc_protocol_unittest.cpp Adds refused-stream consumption regression coverage.
src/brpc/policy/http2_rpc_protocol.cpp Handles refused HEADERS payloads before stream cleanup.
Review details

Suppressed comments (1)

src/brpc/policy/http2_rpc_protocol.cpp:665

  • This also loses the continuation state for a fragmented header block. When END_HEADERS is absent, OnHeaders stores the partial block in sctx; deleting it here makes the required following CONTINUATION enter the server-side unknown-stream path and produce a connection PROTOCOL_ERROR/GOAWAY, even though REFUSED_STREAM is intended to leave the connection usable. Keep discard state for refused streams and decode/drain their CONTINUATION frames through END_HEADERS.
            sctx->OnHeaders(it, frame_head, frag_size, pad_length);
            delete sctx;
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +664 to +665
sctx->OnHeaders(it, frame_head, frag_size, pad_length);
delete sctx;
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