iframe-proxy

wwbmmm · 2026-10-20T07:48:16Z

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

H2UnsentRequest::AppendAndDestroySelf() HPACK-encoded the request headers with the connection's encoder before calling H2Context::TryToInsertClientStream(). Encoding adds header fields to the encoder's dynamic table, so when the insert failed (ELIMIT once the open streams reach the server's SETTINGS_MAX_CONCURRENT_STREAMS, EOVERCROWDED when unsent DATA exceeds -socket_max_unwritten_bytes, or ELOGOFF after a GOAWAY), Socket::WriteRequest::Setup dropped the appended HEADERS frame while the encoder's dynamic table kept the new entries. The peer's decoder then lags the encoder, so later requests on the same connection are decoded with header fields of other requests (e.g. another :path or another authorization value), or fail with a compression error. ELIMIT and EOVERCROWDED are retried by the default RetryPolicy, and h2 only uses CONNECTION_TYPE_SINGLE, so a retried request itself can be decoded with the wrong fields.

What is changed and the side effects?

Changed:

Insert the stream before encoding the headers (as before 9aa41f7), so a refused request no longer mutates HPACK state. The DATA frames allowed by the current flow-control windows are collected separately and appended after the HEADERS, so frames reach the wire in the same order as before.
Added H2UnsentMessage.append_and_destroy_self_keeps_hpack_state_on_refused_insert, which decodes what the client writes with a separate HPacker standing for the server, and made request_does_not_fail_when_body_exceeds_window check that HEADERS are written before DATA. Without the source change the new test fails on every run.

Side effects:

Performance effects: none, the emitted frames and their order are unchanged.
Breaking backward compatibility: none.

Tests run:

brpc_h2_unsent_message_unittest (all 6 cases pass)
brpc_grpc_protocol_unittest (all 8 cases pass)
brpc_http_rpc_protocol_unittest (61/62 pass; HttpTest.parse_http_address fails in the local sandbox because it resolves www.baidu.com, unrelated to this change)

🤖 This PR was automatically created by brpc-oncall

H2UnsentRequest::AppendAndDestroySelf encoded the request headers before H2Context::TryToInsertClientStream. Encoding adds the headers to the dynamic table of the connection, so when the insert failed (ELIMIT at the server's max_concurrent_streams, EOVERCROWDED, ELOGOFF) Socket::WriteRequest::Setup dropped the HEADERS while the table kept the entries. The server's decoder then lags the client's encoder, and later requests on the connection are decoded with the header fields of other requests (another :path, another authorization) or fail with a compression error. ELIMIT and EOVERCROWDED are retried by default on the same connection, so the retried request itself is decoded wrong. Insert the stream first, as before 9aa41f7, and encode the headers only once the request will be sent. The DATA frames the windows allow are collected on the side and appended after the HEADERS.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open issues

DecodeHeaderBlock appends the entire HEADERS payload as if it were a pure HPACK block fragment.… · New
This test hard-codes stream IDs (1/3/5), which makes it brittle to internal stream-id allocation… · New
The 0x1 flag mask is a magic number for END_STREAM. Using the existing named constant (e.g.,… · New
ctx is heap-allocated in the test and never deleted, which introduces a persistent leak across… · New

What changed in this PR

Fixes a HTTP/2 HPACK state desynchronization when a client stream insert is refused, preventing later requests on the same connection from being decoded with incorrect headers.

Changes:

Insert client stream before HPACK header encoding and separately buffer DATA frames so refused inserts don’t mutate the connection’s dynamic table.
Add a unit test that decodes client-emitted header blocks with an independent HPACK decoder to validate encoder/decoder state remains aligned.
Strengthen an existing test to assert HEADERS are written before DATA.

_{Review effort: Lite}

wwbmmm · 2026-10-21T02:31:45Z

+bool DecodeHeaderBlock(brpc::HPacker* peer, butil::IOBuf* buf,
+                       std::map<std::string, std::string>* headers) {
+    butil::IOBuf block;
+    std::string payload;
+    while (!buf->empty()) {
+        const brpc::policy::H2FrameHead frame = PopFrame(buf, &payload);
+        if (frame.type == brpc::policy::H2_FRAME_HEADERS ||
+            frame.type == brpc::policy::H2_FRAME_CONTINUATION) {
+            block.append(payload);
+        }
+    }


[replied by brpc-oncall robot] The frames decoded here are produced by brpc's own PackH2Headers(), which only sets H2_FLAGS_END_STREAM / H2_FLAGS_END_HEADERS and never PADDED or PRIORITY, so there is no padding or priority prefix to strip. Draining the whole buffer is intentional: one header block may span a HEADERS frame plus CONTINUATION frames, and the same buffer also carries DATA frames that DecodeHeaderBlock must skip.

wwbmmm · 2026-10-21T02:31:48Z

+    delete ctx->RemoveStreamAndDeferWU(1);
+
+    ASSERT_TRUE(AppendRequest(sock.get(), "http://example.com/user",
+                              "Bearer alice", &out).ok());
+    ASSERT_TRUE(DecodeHeaderBlock(&peer, &out, &headers));
+    EXPECT_EQ("/user", headers[":path"]);
+    EXPECT_EQ("Bearer alice", headers["authorization"]);
+
+    // Stream 3 takes the only stream the server allows, so this request is
+    // refused and Socket::WriteRequest::Setup drops what it appended.
+    const butil::Status refused = AppendRequest(
+        sock.get(), "http://example.com/user", "Bearer bob", &out);
+    EXPECT_EQ(brpc::ELIMIT, refused.error_code());
+    EXPECT_EQ(nullptr, ctx->FindStream(5));


[replied by brpc-oncall robot] The stream IDs are deterministic in this setup: AllocateClientStreamId() continues from _last_sent_stream_id = 1 in steps of 2, so the three requests are 1, 3 and 5. Keeping them explicit makes the max_concurrent_streams = 1 refusal scenario easy to follow, and the other tests in this file (e.g. clear_pending_data_releases_overcrowded_buffer) also use fixed IDs 1 and 3.

wwbmmm · 2026-10-21T02:31:51Z

+    brpc::policy::H2FrameHead frame = PopFrame(&out, &payload);
+    EXPECT_EQ(brpc::policy::H2_FRAME_HEADERS, frame.type);
+    EXPECT_EQ(0, frame.flags & 0x1);
+    while (frame.type != brpc::policy::H2_FRAME_DATA && !out.empty()) {
+        frame = PopFrame(&out, &payload);
+    }
+    EXPECT_EQ(brpc::policy::H2_FRAME_DATA, frame.type);
+    EXPECT_EQ(0, frame.flags & 0x1);


[replied by brpc-oncall robot] H2_FLAGS_END_STREAM is a file-static constant defined in http2_rpc_protocol.cpp, not in the header, so the test cannot use it, and the other assertions in this file already use frame.flags & 0x1. Moving the constant to the public header is out of scope for this fix.

wwbmmm · 2026-10-21T02:31:55Z

+    brpc::policy::H2Context* ctx =
+        new brpc::policy::H2Context(sock.get(), nullptr);
+    ASSERT_EQ(0, ctx->Init());
+    sock->initialize_parsing_context(&ctx);
+    ctx->_last_sent_stream_id = 1;


[replied by brpc-oncall robot] ctx is not leaked: after sock->initialize_parsing_context(&ctx) the H2Context (a Destroyable) is owned by the socket's parsing context and released by the socket lifecycle, exactly like the other tests in this file. The test must not delete it, since the socket still holds the pointer.

wwbmmm · 2026-10-21T02:32:03Z

wwbmmm requested a balanced review from Copilot October 20, 2026 07:48

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 20, 2026 09:01
The job was not acquired by a runner even after multiple attempts.

File	Description
test/brpc_h2_unsent_message_unittest.cpp	Adds HPACK decode helper and new regression test covering refused stream insertion + ordering checks.
src/brpc/policy/http2_rpc_protocol.cpp	Reorders stream insertion vs. HPACK encoding and appends buffered DATA frames after HEADERS.

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Keep h2 client HPACK state in sync when a stream is refused - #3630

Keep h2 client HPACK state in sync when a stream is refused#3630
wwbmmm wants to merge 1 commit into
apache:masterfrom
wwbmmm:oncall/req-20261020-052625

wwbmmm commented Oct 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

wwbmmm Oct 21, 2026

Uh oh!

wwbmmm Oct 21, 2026

Uh oh!

wwbmmm Oct 21, 2026

Uh oh!

wwbmmm Oct 21, 2026

Uh oh!

wwbmmm commented Oct 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

wwbmmm commented Oct 20, 2026

What problem does this PR solve?

What is changed and the side effects?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

wwbmmm Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm commented Oct 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants