Skip to content

Commit

Permalink
Do not manually reset RocketServerConnection::socket_
Browse files Browse the repository at this point in the history
Summary:
`AsyncTransport::UniquePtr` is a resource managing type that manages the lifetime
of the underlying socket. It is declared as a member variable of `RocketServerConnection`,
which means that the implicit object destructor, which runs after the user defined `RocketServerConnection::~RocketServerConnection`,
will take care of destroying the object.

Manually destroying the object in the user defined destructor is not only unnecessary, it is incorrect. In particular, `ThriftRocketServerHandler::frameHandler_` borrows the socket_ via a raw pointer. `frameHandler_` is declared after the `socket_`, so under normal destruction ordering this borrow is valid. However, due to the fact that `~RocketServerConnection` manually destroys this, it is possible to end up with a use after free:

```
(gdb) bt
#0  0x00007f8c1f6574b8 in folly::DecoratedAsyncTransportWrapper<folly::AsyncTransport>::getPeerCertificate() const () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#1  0x00007f8c11146b4e in apache::thrift::Cpp2ConnContext::getPeerCommonName[abi:cxx11]() const () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/CppServerWrapper.so
#2  0x00007f8c1114690b in apache::thrift::CppContextData::copyContextContents(apache::thrift::Cpp2ConnContext*) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/CppServerWrapper.so
#3  0x00007f8c1114c2d1 in CppServerEventHandler::callPythonHandler(apache::thrift::server::TConnectionContext*, char const*) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/CppServerWrapper.so
#4  0x00007f8c1f6684b5 in apache::thrift::rocket::ThriftRocketServerHandler::~ThriftRocketServerHandler() () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#5  0x00007f8c1f972937 in non-virtual thunk to apache::thrift::rocket::RocketServerConnection::~RocketServerConnection() () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#6  0x00007f8c1f97a4fe in apache::thrift::rocket::RocketServerConnection::WriteBatcher::runLoopCallback() () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#7  0x00007f8c202d6f45 in folly::EventBase::loopBody(int, bool) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#8  0x00007f8c202d81ed in folly::EventBase::loopForever() () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#9  0x00007f8c2026abe4 in folly::IOThreadPoolExecutor::threadRun(std::shared_ptr<folly::ThreadPoolExecutor::Thread>) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#10 0x00007f8c202d2c74 in void std::_Bind<void (folly::ThreadPoolExecutor::*(folly::ThreadPoolExecutor*, std::shared_ptr<folly::ThreadPoolExecutor::Thread>))(std::shared_ptr<folly::ThreadPoolExecutor::Thread>)>::__call<void, , 0ul, 1ul>(std::tuple<>&&, std::_Index_tuple<0ul, 1ul>) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#11 0x00007f8c202d2bc4 in void folly::detail::function::FunctionTraits<void ()>::callBig<std::_Bind<void (folly::ThreadPoolExecutor::*(folly::ThreadPoolExecutor*, std::shared_ptr<folly::ThreadPoolExecutor::Thread>))(std::shared_ptr<folly::ThreadPoolExecutor::Thread>)> >(folly::detail::function::Data&) () from /data/users/mingtao/tmp/thrift_getx509_coredump/par_unpack/sos/libomnibus.so
#12 0x00007f8c84002801 in std::execute_native_thread_routine (__p=0x7f8bfd03aee0) at ../../../.././libstdc++-v3/src/c++11/thread.cc:80
#13 0x00007f8c844ac20c in start_thread (arg=0x7f8bd6dff700) at pthread_create.c:479
#14 0x00007f8c8428881f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
```

This diff removes the manual socket reset in `RocketServerHandler` and replaces it with an explicit call to `socket->closeNow()`.

Reviewed By: andriigrynenko

Differential Revision: D30074941

fbshipit-source-id: af9ea2386cf864683eb843a8c1ca0128d7d8e467
  • Loading branch information
mingtaoy authored and facebook-github-bot committed Aug 5, 2021
1 parent 6ac23dd commit 1d30e47
Showing 1 changed file with 7 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,13 @@ RocketServerConnection::~RocketServerConnection() {
if (rawSocket_) {
rawSocket_->setBufferCallback(nullptr);
}
socket_.reset();

// Subtle: Close the socket, which will fail all outstanding writes and
// unsubscribe the read callback, but do not destroy the object itself, since
// other member variables of RocketServerConnection may be borrowing the
// object.
socket_->closeNow();

// reclaim any memory in use by pending writes
if (egressBufferSize_) {
egressMemoryTracker_.decrement(egressBufferSize_);
Expand Down

0 comments on commit 1d30e47

Please sign in to comment.