From: Yang Chi <yangchi@fb.com>
Date: Fri, 27 May 2016 22:33:40 +0000 (-0700)
Subject: Set bufferCallback_ back to nullptr upon HTTPSession shutdown
X-Git-Tag: 2016.07.26~194
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=5451eeae6eff35f3b839d532d4939bd587b15ba5;p=folly.git

Set bufferCallback_ back to nullptr upon HTTPSession shutdown

Summary: AsyncSocket::handleWrite may trigger HTTPSession::onWriteSuccess which may totally delete the HTTPSession. AsyncSocket::handleWrite also calls bufferCallback_->onEgressBufferCleared(). That bufferCallback_ is the HTTPSession. So in this diff resets bufferCallback_ to nullptr upon HTTPSession shutdown. afrind had a suggestion of adding a DestructorGuard in AsyncSocket::handleWrite. But i don't know DestructorGuard that well. I think it will keep AsyncSocket from being deleted but not the HTTPSession? Or maybe I can DestructorGuard dg(bufferCallback_) ?

Reviewed By: afrind

Differential Revision: D3311058

fbshipit-source-id: cdb5fcbd53837a3effbc096eab87fca4e5d17291
---

diff --git a/folly/io/async/AsyncSocket.cpp b/folly/io/async/AsyncSocket.cpp
index 848cf5c4..3d9f91c7 100644
--- a/folly/io/async/AsyncSocket.cpp
+++ b/folly/io/async/AsyncSocket.cpp
@@ -1430,6 +1430,8 @@ void AsyncSocket::handleRead() noexcept {
 void AsyncSocket::handleWrite() noexcept {
   VLOG(5) << "AsyncSocket::handleWrite() this=" << this << ", fd=" << fd_
           << ", state=" << state_;
+  DestructorGuard dg(this);
+
   if (state_ == StateEnum::CONNECTING) {
     handleConnect();
     return;
diff --git a/folly/io/async/test/AsyncSocketTest2.cpp b/folly/io/async/test/AsyncSocketTest2.cpp
index 4eecf3a6..4f9bd3a2 100644
--- a/folly/io/async/test/AsyncSocketTest2.cpp
+++ b/folly/io/async/test/AsyncSocketTest2.cpp
@@ -2280,3 +2280,34 @@ TEST(AsyncSocketTest, BufferTest) {
   ASSERT_TRUE(socket->isClosedBySelf());
   ASSERT_FALSE(socket->isClosedByPeer());
 }
+
+TEST(AsyncSocketTest, BufferCallbackKill) {
+  TestServer server;
+  EventBase evb;
+  AsyncSocket::OptionMap option{{{SOL_SOCKET, SO_SNDBUF}, 128}};
+  std::shared_ptr<AsyncSocket> socket = AsyncSocket::newSocket(&evb);
+  ConnCallback ccb;
+  socket->connect(&ccb, server.getAddress(), 30, option);
+  evb.loopOnce();
+
+  char buf[100 * 1024];
+  memset(buf, 'c', sizeof(buf));
+  BufferCallback* bcb = new BufferCallback;
+  socket->setBufferCallback(bcb);
+  WriteCallback wcb;
+  wcb.successCallback = [&] {
+    ASSERT_TRUE(socket.unique());
+    socket.reset();
+  };
+
+  // This will trigger AsyncSocket::handleWrite,
+  // which calls WriteCallback::writeSuccess,
+  // which calls wcb.successCallback above,
+  // which tries to delete socket
+  // Then, the socket will also try to use this BufferCallback
+  // And that should crash us, if there is no DestructorGuard on the stack
+  socket->write(&wcb, buf, sizeof(buf), WriteFlags::NONE);
+
+  evb.loop();
+  CHECK_EQ(ccb.state, STATE_SUCCEEDED);
+}