shared ptr vector sockets

Summary: promote the sockets vector to a shared_ptr, since both ServerWorkerPool and ServerBootstrap use it.  Otherwise there are destruction order issues between ServerBootstrap and any IOThreadPoolExecutor you use

Test Plan: Saw use after free in D1942242, gone after this.

Reviewed By: yfeldblum@fb.com

Subscribers: chalfant, doug, fugalh, folly-diffs@, jsedgwick, yfeldblum

FB internal diff: D1947553

Signature: t1:1947553:1427484417:5b78f5c9c70d244d3f52a6f71b6d1fab7b29d106

diff --git a/folly/wangle/bootstrap/ServerBootstrap-inl.h b/folly/wangle/bootstrap/ServerBootstrap-inl.h
index a6ffd26884b94f79085b9405ee90ba1aefed2a98..ecc1372169d5b5a67dfc540d198337fc2f80bb12 100644 (file)
--- a/folly/wangle/bootstrap/ServerBootstrap-inl.h
+++ b/folly/wangle/bootstrap/ServerBootstrap-inl.h
@@ -140,7 +140,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer {
   explicit ServerWorkerPool(
     std::shared_ptr<AcceptorFactory> acceptorFactory,
     folly::wangle::IOThreadPoolExecutor* exec,
-    std::vector<std::shared_ptr<folly::AsyncSocketBase>>* sockets,
+    std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets,
     std::shared_ptr<ServerSocketFactory> socketFactory)
       : acceptorFactory_(acceptorFactory)
       , exec_(exec)
@@ -170,7 +170,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer {
            std::shared_ptr<Acceptor>> workers_;
   std::shared_ptr<AcceptorFactory> acceptorFactory_;
   folly::wangle::IOThreadPoolExecutor* exec_{nullptr};
-  std::vector<std::shared_ptr<folly::AsyncSocketBase>>* sockets_;
+  std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets_;
   std::shared_ptr<ServerSocketFactory> socketFactory_;
 };
 

diff --git a/folly/wangle/bootstrap/ServerBootstrap.cpp b/folly/wangle/bootstrap/ServerBootstrap.cpp
index 3f5592a6160b08161b82579b0bfaf23890718b65..e59ed657560102b122febdbf7fa730702e35e6cb 100644 (file)
--- a/folly/wangle/bootstrap/ServerBootstrap.cpp
+++ b/folly/wangle/bootstrap/ServerBootstrap.cpp
@@ -39,7 +39,7 @@ void ServerWorkerPool::threadStopped(
   auto worker = workers_.find(h);
   CHECK(worker != workers_.end());
 
-  for (auto& socket : *sockets_) {
+  for (auto socket : *sockets_) {
     socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait(
       [&]() {
         socketFactory_->removeAcceptCB(

diff --git a/folly/wangle/bootstrap/ServerBootstrap.h b/folly/wangle/bootstrap/ServerBootstrap.h
index 98890b43a11a81dc4148a022ca86d75795194134..3520b0f51232e2d0ec62f5ea6231664a29504f88 100644 (file)
--- a/folly/wangle/bootstrap/ServerBootstrap.h
+++ b/folly/wangle/bootstrap/ServerBootstrap.h
@@ -132,13 +132,13 @@ class ServerBootstrap {
 
     if (acceptorFactory_) {
       workerFactory_ = std::make_shared<ServerWorkerPool>(
-        acceptorFactory_, io_group.get(), &sockets_, socketFactory_);
+        acceptorFactory_, io_group.get(), sockets_, socketFactory_);
     } else {
       workerFactory_ = std::make_shared<ServerWorkerPool>(
         std::make_shared<ServerAcceptorFactory<Pipeline>>(
           childPipelineFactory_,
           pipeline_),
-        io_group.get(), &sockets_, socketFactory_);
+        io_group.get(), sockets_, socketFactory_);
     }
 
     io_group->addObserver(workerFactory_);
@@ -183,7 +183,7 @@ class ServerBootstrap {
       });
     });
 
-    sockets_.push_back(socket);
+    sockets_->push_back(socket);
   }
 
   void bind(folly::SocketAddress& address) {
@@ -268,7 +268,7 @@ class ServerBootstrap {
         });
       });
 
-      sockets_.push_back(socket);
+      sockets_->push_back(socket);
     }
   }
 
@@ -276,13 +276,16 @@ class ServerBootstrap {
    * Stop listening on all sockets.
    */
   void stop() {
-    for (auto socket : sockets_) {
-      socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait(
-        [&]() mutable {
-          socketFactory_->stopSocket(socket);
-      });
+    // sockets_ may be null if ServerBootstrap has been std::move'd
+    if (sockets_) {
+      for (auto socket : *sockets_) {
+        socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait(
+          [&]() mutable {
+            socketFactory_->stopSocket(socket);
+          });
+      }
+      sockets_->clear();
     }
-    sockets_.clear();
   }
 
   void join() {
@@ -299,7 +302,7 @@ class ServerBootstrap {
    */
   const std::vector<std::shared_ptr<folly::AsyncSocketBase>>&
   getSockets() const {
-    return sockets_;
+    return *sockets_;
   }
 
   std::shared_ptr<wangle::IOThreadPoolExecutor> getIOGroup() const {
@@ -318,7 +321,8 @@ class ServerBootstrap {
   std::shared_ptr<wangle::IOThreadPoolExecutor> io_group_;
 
   std::shared_ptr<ServerWorkerPool> workerFactory_;
-  std::vector<std::shared_ptr<folly::AsyncSocketBase>> sockets_;
+  std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets_{
+    std::make_shared<std::vector<std::shared_ptr<folly::AsyncSocketBase>>>()};
 
   std::shared_ptr<AcceptorFactory> acceptorFactory_;
   std::shared_ptr<PipelineFactory<Pipeline>> childPipelineFactory_;