Copilot commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4236943429
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,6 +201,12 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
+ // sub_chan is not attached to any Socket, recycle it directly.
+ // sub_channel is deleted only when OWNS_CHANNEL, consistent with the
+ // ownership contract.
+ if (sub_chan->ownership == OWNS_CHANNEL) {
+ delete sub_chan->chan;
+ }
Review Comment:
This can access and delete an already-recycled `SubChannel`.
`Socket::OnCreated` assigns `options.user` before several later failure points
(`socket.cpp:749-840`), and `VersionedRefWithId::Create` responds to those
failures by calling `SetFailed()`, which can synchronously run
`Socket::BeforeRecycled` and `SubChannel::BeforeRecycle`. For `OWNS_CHANNEL`,
that callback has already deleted both objects before `Socket::Create` returns,
so these lines cause a use-after-free/double-free. Cleanup needs one
authoritative owner across both failures before `_user` is attached and
failures after it is attached.
##########
test/brpc_channel_unittest.cpp:
##########
@@ -3740,4 +3740,97 @@ TEST_F(RateLimitedBackupPolicyTest,
OnRPCEndDrivesRatioDownAndReAllows) {
ASSERT_TRUE(p->DoBackup(nullptr));
}
+// Issue #1408: SelectiveChannel::AddChannel must take over (or not take over)
+// the sub channel consistently w.r.t. option.ownership, even on failure, so
+// that callers don't leak or double-free.
+class CountingSubChannel : public brpc::Channel {
+public:
+ static int s_alive;
+ CountingSubChannel() { ++s_alive; }
+ ~CountingSubChannel() override { --s_alive; }
+};
+int CountingSubChannel::s_alive = 0;
+
+TEST_F(ChannelTest, selective_channel_add_failure_ownership) {
Review Comment:
The added test only exercises duplicate rejection (which exits before
creating a `SubChannel`) and successful registration; it does not exercise any
of the newly changed `Socket::Create`, address, health-check, or `AddServer`
failure cleanup. Consequently it cannot detect regressions such as
double-freeing an owned channel during creation failure. Please add a
deterministic failure-injection seam and cover at least the creation-failure
ownership handoff for both ownership modes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
