You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2016/07/28 23:32:28 UTC
mesos git commit: Fixed use-after-close bug when using libevent and
SSL.
Repository: mesos
Updated Branches:
refs/heads/master 219158e28 -> 9094974c6
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/9094974c
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9094974c
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9094974c
Branch: refs/heads/master
Commit: 9094974c6916a1cbf5989ec3d956cf0a20af32d7
Parents: 219158e
Author: Benjamin Mahler <bm...@apache.org>
Authored: Tue Jul 26 17:46:55 2016 -0700
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Thu Jul 28 16:32:13 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/9094974c/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/9094974c/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 f4c0b0b..97af3c2 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/9094974c/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 {