mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bmah...@apache.org
Subject [1/2] mesos git commit: Fixed use-after-close bug when using libevent and SSL.
Date Fri, 29 Jul 2016 20:23:33 GMT
Repository: mesos
Updated Branches:
  refs/heads/1.0.x 64adf8961 -> f74bad3c4


Fixed use-after-close bug when using libevent and SSL.

The deferred call to SSL_shutdown within ~LibeventSSLSocketImpl
can occur after the socket fd has been closed by Socket::~Impl.

This can lead to a TLS Alert message being sent on any fd if
it the fd is re-used between the close and the SSL_shutdown!

A fix for this issue was put in place only for connected
sockets in ca3667f4e97e11ad30811753fdb52bc02854113f, but this
did not address accepted sockets. This patch is an attempt to
have a simpler and more efficient fix (avoid dup()).

Thanks to Jan-Philip Gehrcke for reporting the issue.

Review: https://reviews.apache.org/r/50477


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d34e7f61
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d34e7f61
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d34e7f61

Branch: refs/heads/1.0.x
Commit: d34e7f619ec0e05ba5a27d9fb10eea60c5e890c7
Parents: 64adf89
Author: Benjamin Mahler <bmahler@apache.org>
Authored: Tue Jul 26 17:46:55 2016 -0700
Committer: Benjamin Mahler <bmahler@apache.org>
Committed: Fri Jul 29 13:19:07 2016 -0700

----------------------------------------------------------------------
 3rdparty/libprocess/include/process/socket.hpp  | 21 ++++++---
 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 47 +++++---------------
 3rdparty/libprocess/src/libevent_ssl_socket.hpp |  6 ---
 3 files changed, 26 insertions(+), 48 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/d34e7f61/3rdparty/libprocess/include/process/socket.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/socket.hpp b/3rdparty/libprocess/include/process/socket.hpp
index 881b44b..67551a9 100644
--- a/3rdparty/libprocess/include/process/socket.hpp
+++ b/3rdparty/libprocess/include/process/socket.hpp
@@ -106,11 +106,9 @@ public:
   public:
     virtual ~Impl()
     {
-      CHECK(s >= 0);
-      Try<Nothing> close = os::close(s);
-      if (close.isError()) {
-        ABORT("Failed to close socket " +
-              stringify(s) + ": " + close.error());
+      // Don't close if the socket was released.
+      if (s >= 0) {
+        CHECK_SOME(os::close(s)) << "Failed to close socket";
       }
     }
 
@@ -229,6 +227,19 @@ public:
     explicit Impl(int _s) : s(_s) { CHECK(s >= 0); }
 
     /**
+     * Releases ownership of the file descriptor. Not exposed
+     * via the `Socket` interface as this is only intended to
+     * support `Socket::Impl` implementations that need to
+     * override the file descriptor ownership.
+     */
+    int release()
+    {
+      int released = s;
+      s = -1;
+      return released;
+    }
+
+    /**
      * Construct a `Socket` wrapper from this implementation.
      */
     Socket socket() { return Socket(shared_from_this()); }

http://git-wip-us.apache.org/repos/asf/mesos/blob/d34e7f61/3rdparty/libprocess/src/libevent_ssl_socket.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.cpp b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
index 2e7f332..9212b19 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp
@@ -97,19 +97,18 @@ LibeventSSLSocketImpl::~LibeventSSLSocketImpl()
   // calls and structures. This is a safety against the socket being
   // destroyed before existing event loop calls have completed since
   // they require valid data structures (the weak pointer).
+  //
+  // Release ownership of the file descriptor so that
+  // we can defer closing the socket.
+  int fd = release();
+  CHECK(fd >= 0);
 
-  // Copy the members that we are interested in. This is necessary
-  // because 'this' points to memory that may be re-allocated and
-  // invalidate any reference to 'this->XXX'. We want to manipulate
-  // or use these data structures within the finalization lambda
-  // below.
   evconnlistener* _listener = listener;
   bufferevent* _bev = bev;
   std::weak_ptr<LibeventSSLSocketImpl>* _event_loop_handle = event_loop_handle;
-  int ssl_socket_fd = ssl_connect_fd;
 
   run_in_event_loop(
-      [_listener, _bev, _event_loop_handle, ssl_socket_fd]() {
+      [_listener, _bev, _event_loop_handle, fd]() {
         // Once this lambda is called, it should not be possible for
         // more event loop callbacks to be triggered with 'this->bev'.
         // This is important because we delete event_loop_handle which
@@ -133,22 +132,11 @@ LibeventSSLSocketImpl::~LibeventSSLSocketImpl()
           // NOTE: Removes all future callbacks using 'this->bev'.
           bufferevent_disable(_bev, EV_READ | EV_WRITE);
 
-          // Clean up the ssl object.
           SSL_free(ssl);
-
-          // Clean up the buffer event. Since we don't set
-          // 'BEV_OPT_CLOSE_ON_FREE' we rely on the base class
-          // 'Socket::Impl' to clean up the fd.
           bufferevent_free(_bev);
         }
 
-        if (ssl_socket_fd >= 0) {
-          Try<Nothing> close = os::close(ssl_socket_fd);
-          if (close.isError()) {
-            LOG(WARNING) << "Failed to close socket "
-                         << stringify(ssl_socket_fd) << ": " << close.error();
-          }
-        }
+        CHECK_SOME(os::close(fd)) << "Failed to close socket";
 
         delete _event_loop_handle;
       },
@@ -447,8 +435,7 @@ LibeventSSLSocketImpl::LibeventSSLSocketImpl(int _s)
     recv_request(nullptr),
     send_request(nullptr),
     connect_request(nullptr),
-    event_loop_handle(nullptr),
-    ssl_connect_fd(-1) {}
+    event_loop_handle(nullptr) {}
 
 
 LibeventSSLSocketImpl::LibeventSSLSocketImpl(
@@ -462,8 +449,7 @@ LibeventSSLSocketImpl::LibeventSSLSocketImpl(
     send_request(nullptr),
     connect_request(nullptr),
     event_loop_handle(nullptr),
-    peer_hostname(std::move(_peer_hostname)),
-    ssl_connect_fd(-1) {}
+    peer_hostname(std::move(_peer_hostname)) {}
 
 
 Future<Nothing> LibeventSSLSocketImpl::connect(const Address& address)
@@ -481,26 +467,13 @@ Future<Nothing> LibeventSSLSocketImpl::connect(const Address&
address)
     return Failure("Failed to connect: SSL_new");
   }
 
-  ssl_connect_fd = ::dup(get());
-  if (ssl_connect_fd < 0) {
-    return Failure("Failed to 'dup' socket for new openssl socket handle");
-  }
-
-  // Reapply FD_CLOEXEC on the duplicate file descriptor.
-  Try<Nothing> closeexec = os::cloexec(ssl_connect_fd);
-  if (closeexec.isError()) {
-    return Failure(
-        "Failed to set FD_CLOEXEC flag for the dup'ed openssl socket handle: " +
-        closeexec.error());
-  }
-
   // Construct the bufferevent in the connecting state.
   // We set 'BEV_OPT_DEFER_CALLBACKS' to avoid calling the
   // 'event_callback' before 'bufferevent_socket_connect' returns.
   CHECK(bev == nullptr);
   bev = bufferevent_openssl_socket_new(
       base,
-      ssl_connect_fd,
+      s,
       ssl,
       BUFFEREVENT_SSL_CONNECTING,
       BEV_OPT_THREADSAFE | BEV_OPT_DEFER_CALLBACKS);

http://git-wip-us.apache.org/repos/asf/mesos/blob/d34e7f61/3rdparty/libprocess/src/libevent_ssl_socket.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/libevent_ssl_socket.hpp b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
index 4d376d8..acb00d4 100644
--- a/3rdparty/libprocess/src/libevent_ssl_socket.hpp
+++ b/3rdparty/libprocess/src/libevent_ssl_socket.hpp
@@ -179,12 +179,6 @@ private:
 
   Option<std::string> peer_hostname;
   Option<net::IP> peer_ip;
-
-  // Socket descriptor/handle used by libevent_ssl.
-  // Ownership semantics:
-  //  This class owns this handle and is responsible for creating (via dup)
-  //  and closing it.
-  int ssl_connect_fd;
 };
 
 } // namespace network {


Mime
View raw message