Skip to content

Fix StreamCreate failure handling#3357

Open
ljcjclljc wants to merge 1 commit into
apache:masterfrom
ljcjclljc:master
Open

Fix StreamCreate failure handling#3357
ljcjclljc wants to merge 1 commit into
apache:masterfrom
ljcjclljc:master

Conversation

@ljcjclljc

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: resolve #3356

Problem Summary:

StreamCreate(StreamId*, Controller&, const StreamOptions*) ignored the return
value of the batched overload. When the same Controller was reused to create a
request stream twice, the batched overload correctly returned -1, but the
single-stream wrapper still unconditionally accessed request_streams[0].
This could cause out-of-bounds access and crash the process.

What is changed and the side effects?

Changed:

  • Check the return value of the batched StreamCreate in the single-stream wrapper
  • Return the error code immediately when the inner call fails
  • Add a regression test to verify that the second call on the same Controller
    returns -1 instead of crashing

Side effects:

  • Performance effects:

    • None
  • Breaking backward compatibility:

    • No API change
    • The behavior changes from crashing on invalid repeated use to returning -1

Check List:

  • Please make sure your changes are compilable.
  • When providing us with a new feature, it is best to add related tests.
  • Please follow https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md .

Fixes#3356

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

Fixes a crash in StreamCreate(StreamId*, Controller&, ...) by correctly propagating failures from the batched StreamCreate(StreamIds&, ...) overload, preventing out-of-bounds access when a Controller is reused.

Changes:

  • Check and return the inner (batched) StreamCreate return code in the single-stream wrapper before indexing request_streams[0].
  • Add a regression unit test ensuring a second StreamCreate call on the same Controller returns -1 (no crash).

Reviewed changes

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

File Description
src/brpc/stream.cpp Fixes the single-stream wrapper to respect the batched overload’s failure return and avoid indexing an empty vector.
test/brpc_stream_create_unittest.cpp Adds a regression test covering repeated StreamCreate on the same Controller.

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

Comment on lines +28 to +30
brpc::StreamId first_stream = brpc::INVALID_STREAM_ID;
ASSERT_EQ(0, brpc::StreamCreate(&first_stream, cntl, NULL));

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.

StreamCreate(StreamId*, Controller&, ...) crashes when called twice on the same Controller

2 participants