Skip to content

Validate returned stream identifiers - #3485

Open
wasphin wants to merge 2 commits into
apache:masterfrom
wasphin:fix-stream-returned-ids
Open

Validate returned stream identifiers#3485
wasphin wants to merge 2 commits into
apache:masterfrom
wasphin:fix-stream-returned-ids

Conversation

@wasphin

@wasphin wasphin commented Aug 24, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

When a client creates multiple streams in one RPC, it expects the server response to return exactly one extra_stream_id for every stream after the first. The response path indexed this list using the number of locally created streams without first validating its size. A malformed response could therefore read past the returned identifiers, while surplus identifiers had no valid client-stream mapping.

What is changed and the side effects?

Changed:

  • Require the number of returned extra_stream_ids to exactly match the number of additional client streams.
  • Fail the RPC with ERESPONSE and use the existing stream cleanup path when the counts differ.
  • Add full client/server regression coverage for both missing and surplus returned stream identifiers.

Side effects:

  • Performance effects: One integer comparison is added when completing a multi-stream RPC.

  • Breaking backward compatibility: Malformed responses with missing or surplus stream identifiers are now rejected instead of being partially processed.


Check List:

@wasphin
wasphin requested a lite review from Copilot August 24, 2026 14:53

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

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.

This PR hardens multi-stream RPC handling by validating that the server returns exactly one extra_stream_id per additional client stream, and adds regression coverage for both missing and surplus returned identifiers.

Changes:

  • Add a client-side validation that extra_stream_ids_size == stream_num - 1 and fail with ERESPONSE on mismatch.
  • Introduce a test service that intentionally returns mismatched extra_stream_ids.
  • Add a unit test asserting the mismatch is rejected and that streams are cleaned up.

Reviewed changes

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

File Description
test/brpc_streaming_rpc_unittest.cpp Adds regression tests for missing/surplus extra_stream_ids using a custom test service.
src/brpc/controller.cpp Adds validation of returned extra_stream_ids count and fails the RPC on mismatch.

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

Comment thread src/brpc/controller.cpp Outdated
Comment thread test/brpc_streaming_rpc_unittest.cpp Outdated
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