Skip to content

Commit

Permalink
Close RocketClientChannel in resetClient()
Browse files Browse the repository at this point in the history
Summary:
D47382374 added some guards to prevent prematurely destroying a RocketClient object while there are still outstanding operations.  However now McRouter is running into use-after-free issues because the ThriftTransport callback object getting passed to RocketClientChannel (and RocketClient/AsyncSSLSocket underneath) is getting destroyed as part of the ProxyDestination's destruction.  The socket is outliving the callbacks object and causing segfaults when trying to access it.

This should fix the issue by explicitly closing the channel (and the client/socket underneath) in resetClient().  This will prevent future callbacks on the object after it is destroyed.

Notes:
- The race condition seems to be when the socket timeout coincides with destroying the ProxyDestination.  Because the timeout (or some other callback) is keeping the socket alive, it outlives the ThriftTransport object and its reference to that object becomes invalid.
- The the ownership model for RocketClientChannel seems suspicious.  The channel *could* potentially outlive the ThriftTransport because it's a shared_ptr and ThriftTransport hands out Thrift clients (which hold owning references to the RocketClientChannel).  I'm not sure how McRouter ensures there are no more active clients, but again this should ensure they no longer attempt to do any operations on the channel.

Reviewed By: alikhtarov

Differential Revision: D51030792

fbshipit-source-id: e1b0c4a4ac5d8900081a4b7fdd7ce0823f83dd59
  • Loading branch information
Rob Lyerly authored and facebook-github-bot committed Nov 7, 2023
1 parent 6f73c0e commit dcdb32b
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion mcrouter/lib/network/ThriftTransport.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ class ThriftTransport : public ThriftTransportBase {
"Router ", RouterInfo::name, " does not support thrift transport"));
}

void resetClient() override final {}
void resetClient() override final {
channel_->closeNow();
}

void setFlushList(FlushList* /* flushList */) override final {}
};
Expand Down

0 comments on commit dcdb32b

Please sign in to comment.