wwbmmm commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4244983837
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,19 +201,33 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
- delete sub_chan;
+ // On every creation-failure path Socket recycles synchronously and
runs
+ // SubChannel::BeforeRecycle, which deletes chan iff OWNS_CHANNEL and
+ // then deletes sub_chan. Do NOT delete here: that double-frees.
LOG(ERROR) << "Fail to create fake socket for sub channel";
return -1;
}
SocketUniquePtr ptr;
int rc = Socket::AddressFailedAsWell(sock_id, &ptr);
if (rc < 0) {
LOG(ERROR) << "Fail to address SocketId=" << sock_id;
+ // The fake Socket was created (holding the HC-related reference) but
+ // cannot be addressed. Recycle it so that sub_chan, and sub_channel
+ // when OWNS_CHANNEL, are deleted in SubChannel::BeforeRecycle.
+ SocketUniquePtr fail_ptr;
+ if (Socket::AddressFailedAsWell(sock_id, &fail_ptr) >= 0) {
Review Comment:
[replied by brpc-oncall robot] This second `AddressFailedAsWell` cannot
succeed and the block is a no-op: `Socket::Create` just returned 0 for this id
(the socket still holds the additional reference taken in `Create`), so this
branch is not entered in practice; and once `AddressFailedAsWell` returns -1
the slot is free/version-mismatched, so repeating the call with the same id
also returns -1 and `fail_ptr` is never set. If the branch is believed
reachable, the leak it is meant to fix is not fixed by this code; otherwise
please drop it (and the comment) rather than leaving dead code that looks like
it handles the ownership handoff.
##########
src/brpc/socket.cpp:
##########
@@ -722,7 +722,10 @@ int Socket::Create(const SocketOptions& options, SocketId*
id) {
int Socket::OnCreated(const SocketOptions& options) {
if (_io_event.Init((void*)id()) != 0) {
LOG(ERROR) << "Fail to init IOEvent";
- SetFailed(ENOMEM, "%s", "Fail to init IOEvent");
+ // VersionedRefWithId::Create() performs the failure transition after
+ // OnCreated() returns an error; attach the user before returning so
+ // recycling invokes SocketUser::BeforeRecycle.
+ _user = options.user;
Review Comment:
[replied by brpc-oncall robot] In addition to the double-delete of
`EpollOutRequest`'s request and the `Acceptor::StartAccept()` re-entrant
`_map_mutex` lock, running `BeforeRecycle` in this state is unsafe by itself:
at this point the socket is only half-constructed, so after the callback
`Socket::BeforeRecycled()` continues with `_transport->Release()` on a
still-null `unique_ptr` (the transport is created at line ~734, after this
check) and `bthread_id_list_destroy(&_id_wait_list)` before
`bthread_id_list_init()` has run. So even removing the caller's redundant
`delete req` would not make this failure path work; `_user` should not be
attached here (or `BeforeRecycled` must be made safe for a partially
constructed `Socket`).
--
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]
