Fixed potential FD double close in the libevent socket. `evbuffer_add_file` will take ownership of the file descriptor passed into it. Normally, this file descriptor is owned by the `FileEncoder` in the libprocess's `SocketManager`. Since there are two owners, one of the owners may close the file descriptor when it has been re-used.
In this case, when multiple threads access the master's web UI at once with SSL enabled, the master may CHECK-fail due to a bad (closed) file descriptor. Review: https://reviews.apache.org/r/51509/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/96d73c78 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/96d73c78 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/96d73c78 Branch: refs/heads/0.28.x Commit: 96d73c7874e850794527c9d07f3aa9e0de7d4585 Parents: 237f316 Author: Joseph Wu <jos...@mesosphere.io> Authored: Mon Sep 12 12:40:09 2016 -0700 Committer: Joseph Wu <josep...@apache.org> Committed: Thu Sep 15 10:18:44 2016 -0700 ---------------------------------------------------------------------- 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 38 ++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/96d73c78/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 77af7b3..c0fd1d6 100644 --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp @@ -737,6 +737,36 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( std::swap(request, send_request); } + // Duplicate the file descriptor because Libevent will take ownership + // and control the lifecycle separately. + // + // TODO(josephw): We can avoid duplicating the file descriptor in + // future versions of Libevent. In Libevent versions 2.1.2 and later, + // we may use `evbuffer_file_segment_new` and `evbuffer_add_file_segment` + // instead of `evbuffer_add_file`. + int owned_fd = dup(fd); + if (fd == -1) { + return Failure(ErrnoError("Failed to duplicate file descriptor")); + } + + // Set the close-on-exec flag. + Try<Nothing> cloexec = os::cloexec(owned_fd); + if (cloexec.isError()) { + os::close(owned_fd); + return Failure( + "Failed to set close-on-exec on duplicated file descriptor: " + + cloexec.error()); + } + + // Make the file descriptor non-blocking. + Try<Nothing> nonblock = os::nonblock(owned_fd); + if (nonblock.isError()) { + os::close(owned_fd); + return Failure( + "Failed to make duplicated file descriptor non-blocking: " + + nonblock.error()); + } + // 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 @@ -745,7 +775,7 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( auto self = shared(this); run_in_event_loop( - [self, fd, offset, size]() { + [self, owned_fd, offset, size]() { CHECK(__in_event_loop__); CHECK(self); @@ -761,12 +791,16 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( } if (write) { + // NOTE: `evbuffer_add_file` will take ownership of the file + // descriptor and close it after it has finished reading it. int result = evbuffer_add_file( bufferevent_get_output(self->bev), - fd, + owned_fd, offset, size); CHECK_EQ(0, result); + } else { + os::close(owned_fd); } }, DISALLOW_SHORT_CIRCUIT);
