Fix race in SSL 'Socket::connect()'. Assign callback and run 'bufferevent_socket_connect' from in event loop.
Review: https://reviews.apache.org/r/36148 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/a42f871e Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/a42f871e Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/a42f871e Branch: refs/heads/master Commit: a42f871e6172d69adc2193850d1f3fac883e2fc6 Parents: b9da875 Author: Joris Van Remoortere <[email protected]> Authored: Thu Jul 2 14:57:57 2015 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Thu Jul 2 14:57:58 2015 -0700 ---------------------------------------------------------------------- 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 61 ++++++++++++++------ 1 file changed, 44 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/a42f871e/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 583526b..5ccbdb8 100644 --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp @@ -473,14 +473,6 @@ Future<Nothing> LibeventSSLSocketImpl::connect(const Address& address) // From this point on, as long as 'bev' is freed properly, it will // free 'ssl' along with it due to the BEV_OPT_CLOSE_ON_FREE' flag. - // Assign the callbacks for the bufferevent. - bufferevent_setcb( - bev, - &LibeventSSLSocketImpl::recv_callback, - &LibeventSSLSocketImpl::send_callback, - &LibeventSSLSocketImpl::event_callback, - CHECK_NOTNULL(event_loop_handle)); - // Try and determine the 'peer_hostname' from the address we're // connecting to in order to properly verify the SSL connection later. const Try<string> hostname = address.hostname(); @@ -506,16 +498,51 @@ Future<Nothing> LibeventSSLSocketImpl::connect(const Address& address) std::swap(request, connect_request); } - sockaddr_storage addr = net::createSockaddrStorage(address.ip, address.port); + // Extend the life-time of 'this' through the execution of the + // lambda in the event loop. Note: The 'self' needs to be explicitly + // captured because we're not using it in the body of the lambda. We + // can use a 'shared_ptr' because run_in_event_loop is guaranteed to + // execute. + auto self = shared(this); - if (bufferevent_socket_connect( - bev, - reinterpret_cast<sockaddr*>(&addr), - sizeof(addr)) < 0) { - bufferevent_free(bev); - bev = NULL; - return Failure("Failed to connect: bufferevent_socket_connect"); - } + run_in_event_loop( + [self, address]() { + sockaddr_storage addr = + net::createSockaddrStorage(address.ip, address.port); + + // Assign the callbacks for the bufferevent. We do this + // before the 'bufferevent_socket_connect()' call to avoid + // any race on the underlying buffer events becoming ready. + bufferevent_setcb( + self->bev, + &LibeventSSLSocketImpl::recv_callback, + &LibeventSSLSocketImpl::send_callback, + &LibeventSSLSocketImpl::event_callback, + CHECK_NOTNULL(self->event_loop_handle)); + + if (bufferevent_socket_connect( + self->bev, + reinterpret_cast<sockaddr*>(&addr), + sizeof(addr)) < 0) { + bufferevent_free(self->bev); + self->bev = NULL; + + Owned<ConnectRequest> request; + + // Swap out the 'connect_request' so we can destroy it + // outside of the lock. + synchronized (self->lock) { + std::swap(request, self->connect_request); + } + + CHECK_NOTNULL(request.get()); + + // Fail the promise since we failed to connect. + request->promise.fail( + "Failed to connect: bufferevent_socket_connect"); + } + }, + DISALLOW_SHORT_CIRCUIT); return future; }
