Commit c80831a5 authored by Nathan Bronson's avatar Nathan Bronson Committed by Facebook Github Bot

folly/io/async/tests: always detach event base in tests, fixes UBSAN tests

Summary:
In AsyncSSLSocket tests the shared_ptr structure means that the
AsyncSSLSocket-s outlive the stack-allocated EventBase on which they were
created.  Previously there were scattered calls to detachEventBase on the
last interesting callback, but several calls were missing.  This diff
switches to a model where the SSLServerAcceptCallbackBase is responsible
for detaching the sockets.

This diff also fixes a low-firing race between shutdown
of the TestSSLAsyncCacheServer Main thread and a call to
EmptyReadCallback::readEOF.  Most uses of EmptyReadCallback don't attach
the TCP socket, so they can't actually tolerate a call to readError
or readEOF.

These use-after-destruction problems were discovered by UBSAN.

Reviewed By: djwatson

Differential Revision: D4301416

fbshipit-source-id: 127fb5e506d0694c5ca81d7a38c704d69f4ab3eb
parent 10847386
...@@ -915,7 +915,6 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) { ...@@ -915,7 +915,6 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) {
// Start listening on a local port // Start listening on a local port
WriteCallbackBase writeCallback; WriteCallbackBase writeCallback;
ReadCallback readCallback(&writeCallback); ReadCallback readCallback(&writeCallback);
EmptyReadCallback clientReadCallback;
HandshakeCallback handshakeCallback(&readCallback); HandshakeCallback handshakeCallback(&readCallback);
SSLServerAcceptCallback acceptCallback(&handshakeCallback, 50); SSLServerAcceptCallback acceptCallback(&handshakeCallback, 50);
TestSSLAsyncCacheServer server(&acceptCallback); TestSSLAsyncCacheServer server(&acceptCallback);
...@@ -925,6 +924,8 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) { ...@@ -925,6 +924,8 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) {
// only do a TCP connect // only do a TCP connect
std::shared_ptr<AsyncSocket> sock = AsyncSocket::newSocket(&eventBase); std::shared_ptr<AsyncSocket> sock = AsyncSocket::newSocket(&eventBase);
sock->connect(nullptr, server.getAddress()); sock->connect(nullptr, server.getAddress());
EmptyReadCallback clientReadCallback;
clientReadCallback.tcpSocket_ = sock; clientReadCallback.tcpSocket_ = sock;
sock->setReadCB(&clientReadCallback); sock->setReadCB(&clientReadCallback);
......
...@@ -83,7 +83,6 @@ public: ...@@ -83,7 +83,6 @@ public:
this->bytesWritten = nBytesWritten; this->bytesWritten = nBytesWritten;
exception = ex; exception = ex;
socket_->close(); socket_->close();
socket_->detachEventBase();
} }
std::shared_ptr<AsyncSSLSocket> socket_; std::shared_ptr<AsyncSSLSocket> socket_;
...@@ -119,14 +118,12 @@ public AsyncTransportWrapper::ReadCallback { ...@@ -119,14 +118,12 @@ public AsyncTransportWrapper::ReadCallback {
std::cerr << "readError " << ex.what() << std::endl; std::cerr << "readError " << ex.what() << std::endl;
state = STATE_FAILED; state = STATE_FAILED;
socket_->close(); socket_->close();
socket_->detachEventBase();
} }
void readEOF() noexcept override { void readEOF() noexcept override {
std::cerr << "readEOF" << std::endl; std::cerr << "readEOF" << std::endl;
socket_->close(); socket_->close();
socket_->detachEventBase();
} }
std::shared_ptr<AsyncSSLSocket> socket_; std::shared_ptr<AsyncSSLSocket> socket_;
...@@ -288,15 +285,16 @@ public: ...@@ -288,15 +285,16 @@ public:
void readErr(const AsyncSocketException& ex) noexcept override { void readErr(const AsyncSocketException& ex) noexcept override {
std::cerr << "readError " << ex.what() << std::endl; std::cerr << "readError " << ex.what() << std::endl;
state = STATE_FAILED; state = STATE_FAILED;
tcpSocket_->close(); if (tcpSocket_) {
tcpSocket_->detachEventBase(); tcpSocket_->close();
}
} }
void readEOF() noexcept override { void readEOF() noexcept override {
std::cerr << "readEOF" << std::endl; std::cerr << "readEOF" << std::endl;
if (tcpSocket_) {
tcpSocket_->close(); tcpSocket_->close();
tcpSocket_->detachEventBase(); }
state = STATE_SUCCEEDED; state = STATE_SUCCEEDED;
} }
...@@ -395,12 +393,14 @@ public: ...@@ -395,12 +393,14 @@ public:
void connectionAccepted( void connectionAccepted(
int fd, const folly::SocketAddress& /* clientAddr */) noexcept override { int fd, const folly::SocketAddress& /* clientAddr */) noexcept override {
if (socket_) {
socket_->detachEventBase();
}
printf("Connection accepted\n"); printf("Connection accepted\n");
std::shared_ptr<AsyncSSLSocket> sslSock;
try { try {
// Create a AsyncSSLSocket object with the fd. The socket should be // Create a AsyncSSLSocket object with the fd. The socket should be
// added to the event base and in the state of accepting SSL connection. // added to the event base and in the state of accepting SSL connection.
sslSock = AsyncSSLSocket::newSocket(ctx_, base_, fd); socket_ = AsyncSSLSocket::newSocket(ctx_, base_, fd);
} catch (const std::exception &e) { } catch (const std::exception &e) {
LOG(ERROR) << "Exception %s caught while creating a AsyncSSLSocket " LOG(ERROR) << "Exception %s caught while creating a AsyncSSLSocket "
"object with socket " << e.what() << fd; "object with socket " << e.what() << fd;
...@@ -409,15 +409,20 @@ public: ...@@ -409,15 +409,20 @@ public:
return; return;
} }
connAccepted(sslSock); connAccepted(socket_);
} }
virtual void connAccepted( virtual void connAccepted(
const std::shared_ptr<folly::AsyncSSLSocket> &s) = 0; const std::shared_ptr<folly::AsyncSSLSocket> &s) = 0;
void detach() {
socket_->detachEventBase();
}
StateEnum state; StateEnum state;
HandshakeCallback *hcb_; HandshakeCallback *hcb_;
std::shared_ptr<folly::SSLContext> ctx_; std::shared_ptr<folly::SSLContext> ctx_;
std::shared_ptr<AsyncSSLSocket> socket_;
folly::EventBase* base_; folly::EventBase* base_;
}; };
...@@ -551,8 +556,6 @@ public: ...@@ -551,8 +556,6 @@ public:
EXPECT_EQ(hcb_->state, STATE_FAILED); EXPECT_EQ(hcb_->state, STATE_FAILED);
EXPECT_EQ(callback2.state, STATE_FAILED); EXPECT_EQ(callback2.state, STATE_FAILED);
sock->detachEventBase();
state = STATE_SUCCEEDED; state = STATE_SUCCEEDED;
hcb_->setState(STATE_SUCCEEDED); hcb_->setState(STATE_SUCCEEDED);
callback2.setState(STATE_SUCCEEDED); callback2.setState(STATE_SUCCEEDED);
...@@ -623,6 +626,7 @@ class TestSSLServer { ...@@ -623,6 +626,7 @@ class TestSSLServer {
static void *Main(void *ctx) { static void *Main(void *ctx) {
TestSSLServer *self = static_cast<TestSSLServer*>(ctx); TestSSLServer *self = static_cast<TestSSLServer*>(ctx);
self->evb_.loop(); self->evb_.loop();
self->acb_->detach();
std::cerr << "Server thread exited event loop" << std::endl; std::cerr << "Server thread exited event loop" << std::endl;
return nullptr; return nullptr;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment