From: Nathan Bronson <ngbronson@fb.com>
Date: Fri, 9 Dec 2016 20:42:28 +0000 (-0800)
Subject: folly/io/async/tests: always detach event base in tests, fixes UBSAN tests
X-Git-Tag: v2016.12.12.00~2
X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=c80831a5b097f4b6c0e64fdc36efec5f0d676a11;p=folly.git

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
---

diff --git a/folly/io/async/test/AsyncSSLSocketTest.cpp b/folly/io/async/test/AsyncSSLSocketTest.cpp
index bc7c4f48..f2bd909f 100644
--- a/folly/io/async/test/AsyncSSLSocketTest.cpp
+++ b/folly/io/async/test/AsyncSSLSocketTest.cpp
@@ -915,7 +915,6 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) {
   // Start listening on a local port
   WriteCallbackBase writeCallback;
   ReadCallback readCallback(&writeCallback);
-  EmptyReadCallback clientReadCallback;
   HandshakeCallback handshakeCallback(&readCallback);
   SSLServerAcceptCallback acceptCallback(&handshakeCallback, 50);
   TestSSLAsyncCacheServer server(&acceptCallback);
@@ -925,6 +924,8 @@ TEST(AsyncSSLSocketTest, SSLServerTimeoutTest) {
   // only do a TCP connect
   std::shared_ptr<AsyncSocket> sock = AsyncSocket::newSocket(&eventBase);
   sock->connect(nullptr, server.getAddress());
+
+  EmptyReadCallback clientReadCallback;
   clientReadCallback.tcpSocket_ = sock;
   sock->setReadCB(&clientReadCallback);
 
diff --git a/folly/io/async/test/AsyncSSLSocketTest.h b/folly/io/async/test/AsyncSSLSocketTest.h
index cc09b106..d66f5088 100644
--- a/folly/io/async/test/AsyncSSLSocketTest.h
+++ b/folly/io/async/test/AsyncSSLSocketTest.h
@@ -83,7 +83,6 @@ public:
     this->bytesWritten = nBytesWritten;
     exception = ex;
     socket_->close();
-    socket_->detachEventBase();
   }
 
   std::shared_ptr<AsyncSSLSocket> socket_;
@@ -119,14 +118,12 @@ public AsyncTransportWrapper::ReadCallback {
     std::cerr << "readError " << ex.what() << std::endl;
     state = STATE_FAILED;
     socket_->close();
-    socket_->detachEventBase();
   }
 
   void readEOF() noexcept override {
     std::cerr << "readEOF" << std::endl;
 
     socket_->close();
-    socket_->detachEventBase();
   }
 
   std::shared_ptr<AsyncSSLSocket> socket_;
@@ -288,15 +285,16 @@ public:
   void readErr(const AsyncSocketException& ex) noexcept override {
     std::cerr << "readError " << ex.what() << std::endl;
     state = STATE_FAILED;
-    tcpSocket_->close();
-    tcpSocket_->detachEventBase();
+    if (tcpSocket_) {
+      tcpSocket_->close();
+    }
   }
 
   void readEOF() noexcept override {
     std::cerr << "readEOF" << std::endl;
-
-    tcpSocket_->close();
-    tcpSocket_->detachEventBase();
+    if (tcpSocket_) {
+      tcpSocket_->close();
+    }
     state = STATE_SUCCEEDED;
   }
 
@@ -395,12 +393,14 @@ public:
 
   void connectionAccepted(
       int fd, const folly::SocketAddress& /* clientAddr */) noexcept override {
+    if (socket_) {
+      socket_->detachEventBase();
+    }
     printf("Connection accepted\n");
-    std::shared_ptr<AsyncSSLSocket> sslSock;
     try {
       // Create a AsyncSSLSocket object with the fd. The socket should be
       // 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) {
       LOG(ERROR) << "Exception %s caught while creating a AsyncSSLSocket "
         "object with socket " << e.what() << fd;
@@ -409,15 +409,20 @@ public:
       return;
     }
 
-    connAccepted(sslSock);
+    connAccepted(socket_);
   }
 
   virtual void connAccepted(
     const std::shared_ptr<folly::AsyncSSLSocket> &s) = 0;
 
+  void detach() {
+    socket_->detachEventBase();
+  }
+
   StateEnum state;
   HandshakeCallback *hcb_;
   std::shared_ptr<folly::SSLContext> ctx_;
+  std::shared_ptr<AsyncSSLSocket> socket_;
   folly::EventBase* base_;
 };
 
@@ -551,8 +556,6 @@ public:
     EXPECT_EQ(hcb_->state, STATE_FAILED);
     EXPECT_EQ(callback2.state, STATE_FAILED);
 
-    sock->detachEventBase();
-
     state = STATE_SUCCEEDED;
     hcb_->setState(STATE_SUCCEEDED);
     callback2.setState(STATE_SUCCEEDED);
@@ -623,6 +626,7 @@ class TestSSLServer {
   static void *Main(void *ctx) {
     TestSSLServer *self = static_cast<TestSSLServer*>(ctx);
     self->evb_.loop();
+    self->acb_->detach();
     std::cerr << "Server thread exited event loop" << std::endl;
     return nullptr;
   }