Windows: Made `libevent` use `int_fd` correctly. Due to the refactoring of `int_fd`, we have two corrections to make.
The first is an edge case where `libevent`, a third-party library, requires a CRT integer file descriptor. Thus we duplicate the `int_fd` and then explicitly allocate via `crt()`, which requires that we also manually close it via `_close()`. The second is an edge case where `libevent` uses its own type to represent a `SOCKET` on Windows, in this case, `evutil_socket_t` (which is actually just an `intptr_t`). While `int_fd` has a constructor for this type, it is marked `explicit`, and unfortunately also has an implicit constructor which takes an `int`. This is a problem because the `intptr_t` can be silently converted to an `int`, causing the `int_fd` abstraction to take on the wrong form (a `HANDLE` instead of a `SOCKET`). So to avoid this implicit conversion, we call the `intptr_t` constructor explicitly. The alternative is to make the `intptr_t` constructor implicit, which we wish to avoid, or to make the `int` constructor explicit, which we can't do because we need to support `int` semantics such as `int_fd fd = -1`. Review: https://reviews.apache.org/r/66438 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9326f8f5 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9326f8f5 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9326f8f5 Branch: refs/heads/master Commit: 9326f8f598606f514135b0d00f4daea52b8872b6 Parents: b061fb1 Author: Andrew Schwartzmeyer <[email protected]> Authored: Tue Mar 20 22:26:01 2018 -0700 Committer: Andrew Schwartzmeyer <[email protected]> Committed: Tue May 1 18:36:04 2018 -0700 ---------------------------------------------------------------------- 3rdparty/libprocess/src/libevent_ssl_socket.cpp | 35 ++++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/9326f8f5/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 4de161d..436b389 100644 --- a/3rdparty/libprocess/src/libevent_ssl_socket.cpp +++ b/3rdparty/libprocess/src/libevent_ssl_socket.cpp @@ -813,7 +813,15 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( return Failure(dup.error()); } - int_fd owned_fd = dup.get(); + // NOTE: This is *not* an `int_fd` because `libevent` requires a CRT + // integer file descriptor, which we allocate and then use + // exclusively here. +#ifdef __WINDOWS__ + int owned_fd = dup->crt(); + // The `os::cloexec` and `os::nonblock` functions do nothing on + // Windows, and cannot be called because they take `int_fd`. +#else + int owned_fd = dup.get(); // Set the close-on-exec flag. Try<Nothing> cloexec = os::cloexec(owned_fd); @@ -832,6 +840,7 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( "Failed to make duplicated file descriptor non-blocking: " + nonblock.error()); } +#endif // __WINDOWS__ // Extend the life-time of 'this' through the execution of the // lambda in the event loop. Note: The 'self' needs to be explicitly @@ -861,16 +870,18 @@ Future<size_t> LibeventSSLSocketImpl::sendfile( // descriptor and close it after it has finished reading it. int result = evbuffer_add_file( bufferevent_get_output(self->bev), -#ifdef __WINDOWS__ - owned_fd.crt(), -#else owned_fd, -#endif // __WINDOWS__ offset, size); CHECK_EQ(0, result); } else { +#ifdef __WINDOWS__ + // NOTE: `os::close()` on Windows is not compatible with CRT + // file descriptors, only `HANDLE` and `SOCKET` types. + ::_close(owned_fd); +#else os::close(owned_fd); +#endif // __WINDOWS__ } }, DISALLOW_SHORT_CIRCUIT); @@ -905,6 +916,7 @@ Try<Nothing> LibeventSSLSocketImpl::listen(int backlog) std::shared_ptr<LibeventSSLSocketImpl> impl(handle->lock()); +#ifndef __WINDOWS__ // NOTE: Passing the flag `LEV_OPT_CLOSE_ON_EXEC` into // `evconnlistener_new` would atomically set `SOCK_CLOEXEC` // on the accepted socket. However, this flag is not supported @@ -922,6 +934,7 @@ Try<Nothing> LibeventSSLSocketImpl::listen(int backlog) os::close(socket); return; } +#endif // __WINDOWS__ if (impl != nullptr) { Try<net::IP> ip = net::IP::create(*addr); @@ -934,7 +947,11 @@ Try<Nothing> LibeventSSLSocketImpl::listen(int backlog) // is set. AcceptRequest* request = new AcceptRequest( - socket, + // NOTE: The `int_fd` must be explicitly constructed + // to avoid the `intptr_t` being casted to an `int`, + // resulting in a `HANDLE` instead of a `SOCKET` on + // Windows. + int_fd(socket), listener, ip.isSome() ? Option<net::IP>(ip.get()) : None()); @@ -1032,7 +1049,11 @@ void LibeventSSLSocketImpl::peek_callback( accept_SSL_callback(request); } else { // Downgrade to a non-SSL socket implementation. - Try<std::shared_ptr<SocketImpl>> impl = PollSocketImpl::create(fd); + // + // NOTE: The `int_fd` must be explicitly constructed to avoid the + // `intptr_t` being casted to an `int`, resulting in a `HANDLE` + // instead of a `SOCKET` on Windows. + Try<std::shared_ptr<SocketImpl>> impl = PollSocketImpl::create(int_fd(fd)); if (impl.isError()) { request->promise.fail(impl.error()); } else {
