You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "ljcjclljc (via GitHub)" <gi...@apache.org> on 2026/06/24 16:23:58 UTC
[PR] Fix StreamCreate failure handling (brpc)
ljcjclljc opened a new pull request, #3357:
URL: https://github.com/apache/brpc/pull/3357
### 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:
- [x] Please make sure your changes are compilable.
- [x] When providing us with a new feature, it is best to add related tests.
- [x] Please follow `https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md` .
Fixes#3356
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3357:
URL: https://github.com/apache/brpc/pull/3357#discussion_r3471444161
##########
test/brpc_stream_create_unittest.cpp:
##########
@@ -0,0 +1,34 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "brpc/controller.h"
+#include "brpc/stream.h"
+
+class StreamCreateBugTest : public testing::Test {};
+
+TEST_F(StreamCreateBugTest, create_twice_on_same_controller_returns_error) {
+ brpc::Controller cntl;
+
+ brpc::StreamId first_stream = brpc::INVALID_STREAM_ID;
+ ASSERT_EQ(0, brpc::StreamCreate(&first_stream, cntl, NULL));
+
Review Comment:
The test creates a stream but never closes it. Other stream-related tests use `brpc::ScopedStream` to ensure streams are closed even if assertions fail; leaving the stream open can leak sockets/resources and can trip leak/ASAN checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3357:
URL: https://github.com/apache/brpc/pull/3357#discussion_r3545474321
##########
test/brpc_stream_create_unittest.cpp:
##########
@@ -0,0 +1,35 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "brpc/controller.h"
+#include "brpc/stream.h"
+
+class StreamCreateBugTest : public testing::Test {};
+
+TEST_F(StreamCreateBugTest, create_twice_on_same_controller_returns_error) {
Review Comment:
Please add the UT in brpc_streaming_rpc_unittest.cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3357:
URL: https://github.com/apache/brpc/pull/3357#issuecomment-4824745699
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "ljcjclljc (via GitHub)" <gi...@apache.org>.
ljcjclljc commented on PR #3357:
URL: https://github.com/apache/brpc/pull/3357#issuecomment-4934012674
@chenBright The changes in this PR pass locally, and the full
`brpc_streaming_rpc_unittest` suite also passes.
The CI failure comes from `brpc_socket_unittest`
(`SocketTest.keepalive_input_message`).
Could you please help check what might be causing this failure?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3357:
URL: https://github.com/apache/brpc/pull/3357#discussion_r3487434781
##########
test/brpc_stream_create_unittest.cpp:
##########
@@ -0,0 +1,34 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "brpc/controller.h"
+#include "brpc/stream.h"
+
+class StreamCreateBugTest : public testing::Test {};
+
+TEST_F(StreamCreateBugTest, create_twice_on_same_controller_returns_error) {
+ brpc::Controller cntl;
+
+ brpc::StreamId first_stream = brpc::INVALID_STREAM_ID;
+ ASSERT_EQ(0, brpc::StreamCreate(&first_stream, cntl, NULL));
+
Review Comment:
@ljcjclljc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "ljcjclljc (via GitHub)" <gi...@apache.org>.
ljcjclljc commented on code in PR #3357:
URL: https://github.com/apache/brpc/pull/3357#discussion_r3544145082
##########
test/brpc_stream_create_unittest.cpp:
##########
@@ -0,0 +1,34 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "brpc/controller.h"
+#include "brpc/stream.h"
+
+class StreamCreateBugTest : public testing::Test {};
+
+TEST_F(StreamCreateBugTest, create_twice_on_same_controller_returns_error) {
+ brpc::Controller cntl;
+
+ brpc::StreamId first_stream = brpc::INVALID_STREAM_ID;
+ ASSERT_EQ(0, brpc::StreamCreate(&first_stream, cntl, NULL));
+
Review Comment:
Thanks for pointing this out.
I updated the test to use `brpc::ScopedStream` so the created stream is closed
automatically even if an assertion fails. This avoids leaking stream/socket
resources and keeps the test consistent with other stream-related tests.
I also reran the regression test after the change, and it still passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix StreamCreate failure handling (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3357:
URL: https://github.com/apache/brpc/pull/3357
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org