From b402f78926f5f88cc0b5e76b736681973a95d040 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Fri, 14 Feb 2020 16:38:43 -0500 Subject: [PATCH 1/9] Use os_sys_calls in all code paths to replace raw syscalls This PR seeks to address all remaining changes to the linux and os/x code paths, in order to consolidate the necessary patch review requirements. The subsequent PR(s) should not include changes impacting the current working linux and os/x code paths. Primarilly adds usage of os_sys_calls for various network and test sources. Addresses a reoccuring issue that only linux/posix allows for connection -to- an any-address, on Windows and in general, this makes no sense. So several tests are adjusted to explicitly connect to the loopback interface of the "any" if listener. Signed-off-by: Sunjay Bhatia Signed-off-by: William A Rowe Jr Co-authored-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr --- include/envoy/common/platform.h | 32 +++++ source/common/common/byte_order.h | 40 +----- source/common/network/address_impl.cc | 99 +++++++++----- source/common/network/connection_impl.cc | 51 ++++++-- .../common/network/io_socket_handle_impl.cc | 83 ++++++------ source/common/network/raw_buffer_socket.cc | 3 +- .../network/mysql_proxy/mysql_utils.cc | 2 +- test/common/buffer/watermark_buffer_test.cc | 2 +- test/common/event/file_event_impl_test.cc | 58 ++++++--- ...dr_family_aware_socket_option_impl_test.cc | 12 +- test/common/network/address_impl_test.cc | 26 ++-- test/common/network/connection_impl_test.cc | 89 +++++++------ .../common/network/listen_socket_impl_test.cc | 9 +- test/common/network/listener_impl_test.cc | 9 +- test/common/network/socket_option_test.h | 39 +++++- .../proxy_protocol/proxy_protocol_test.cc | 123 ++++++++++++++---- .../tls_inspector/tls_inspector_benchmark.cc | 5 +- .../tls_inspector/tls_inspector_test.cc | 37 +++--- test/test_common/network_utility.cc | 19 ++- test/test_common/network_utility_test.cc | 4 +- 20 files changed, 471 insertions(+), 271 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index f80390cce5eb..5601167f657a 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -29,6 +29,20 @@ #include #include +#define htole16(x) (x) +#define htole32(x) (x) +#define htole64(x) (x) +#define le16toh(x) (x) +#define le32toh(x) (x) +#define le64toh(x) (x) + +#define htobe16(x) htons((x)) +#define htobe32(x) htonl((x)) +#define htobe64(x) htonll((x)) +#define be16toh(x) ntohs((x)) +#define be32toh(x) ntohl((x)) +#define be64toh(x) ntohll((x)) + #define PACKED_STRUCT(definition, ...) \ __pragma(pack(push, 1)) definition, ##__VA_ARGS__; \ __pragma(pack(pop)) @@ -122,6 +136,24 @@ struct msghdr { #include #include +#ifdef __APPLE__ +#include +#define htole16(x) OSSwapHostToLittleInt16((x)) +#define htole32(x) OSSwapHostToLittleInt32((x)) +#define htole64(x) OSSwapHostToLittleInt64((x)) +#define le16toh(x) OSSwapLittleToHostInt16((x)) +#define le32toh(x) OSSwapLittleToHostInt32((x)) +#define le64toh(x) OSSwapLittleToHostInt64((x)) +#define htobe16(x) OSSwapHostToBigInt16((x)) +#define htobe32(x) OSSwapHostToBigInt32((x)) +#define htobe64(x) OSSwapHostToBigInt64((x)) +#define be16toh(x) OSSwapBigToHostInt16((x)) +#define be32toh(x) OSSwapBigToHostInt32((x)) +#define be64toh(x) OSSwapBigToHostInt64((x)) +#else +#include +#endif + #if defined(__linux__) #include #endif diff --git a/source/common/common/byte_order.h b/source/common/common/byte_order.h index 1ca0286399b6..6f4d9e862c2d 100644 --- a/source/common/common/byte_order.h +++ b/source/common/common/byte_order.h @@ -3,47 +3,9 @@ #include #include -// NOLINT(namespace-envoy) - -#ifdef __APPLE__ - -#include - -#define htole16(x) OSSwapHostToLittleInt16((x)) -#define htole32(x) OSSwapHostToLittleInt32((x)) -#define htole64(x) OSSwapHostToLittleInt64((x)) -#define le16toh(x) OSSwapLittleToHostInt16((x)) -#define le32toh(x) OSSwapLittleToHostInt32((x)) -#define le64toh(x) OSSwapLittleToHostInt64((x)) - -#define htobe16(x) OSSwapHostToBigInt16((x)) -#define htobe32(x) OSSwapHostToBigInt32((x)) -#define htobe64(x) OSSwapHostToBigInt64((x)) -#define be16toh(x) OSSwapBigToHostInt16((x)) -#define be32toh(x) OSSwapBigToHostInt32((x)) -#define be64toh(x) OSSwapBigToHostInt64((x)) - -#elif WIN32 - #include "envoy/common/platform.h" -#define htole16(x) (x) -#define htole32(x) (x) -#define htole64(x) (x) -#define le16toh(x) (x) -#define le32toh(x) (x) -#define le64toh(x) (x) - -#define htobe16(x) htons((x)) -#define htobe32(x) htonl((x)) -#define htobe64(x) htonll((x)) -#define be16toh(x) ntohs((x)) -#define be32toh(x) ntohl((x)) -#define be64toh(x) ntohll((x)) - -#else -#include -#endif +// NOLINT(namespace-envoy) enum class ByteOrder { Host, LittleEndian, BigEndian }; diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 5c9c2f2bea65..222f3108e1c2 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -77,6 +77,10 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, #if defined(__APPLE__) struct sockaddr_in sin = { {}, AF_INET, sin6->sin6_port, {sin6->sin6_addr.__u6_addr.__u6_addr32[3]}, {}}; +#elif defined(WIN32) + struct in_addr in_v4 = {}; + in_v4.S_un.S_addr = reinterpret_cast(sin6->sin6_addr.u.Byte)[3]; + struct sockaddr_in sin = {AF_INET, sin6->sin6_port, in_v4, {}}; #else struct sockaddr_in sin = {AF_INET, sin6->sin6_port, {sin6->sin6_addr.s6_addr32[3]}, {}}; #endif @@ -102,38 +106,57 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, InstanceConstSharedPtr addressFromFd(os_fd_t fd) { sockaddr_storage ss; socklen_t ss_len = sizeof ss; - int rc = ::getsockname(fd, reinterpret_cast(&ss), &ss_len); - if (rc != 0) { - throw EnvoyException( - fmt::format("getsockname failed for '{}': ({}) {}", fd, errno, strerror(errno))); + auto& os_sys_calls = Api::OsSysCallsSingleton::get(); + Api::SysCallIntResult result = + os_sys_calls.getsockname(fd, reinterpret_cast(&ss), &ss_len); + if (result.rc_ != 0) { + throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd, result.errno_, + strerror(result.errno_))); } int socket_v6only = 0; if (ss.ss_family == AF_INET6) { socklen_t size_int = sizeof(socket_v6only); - RELEASE_ASSERT(::getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int) == 0, ""); + result = os_sys_calls.getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int); +#ifdef WIN32 + // On Windows, it is possible for this getsockopt() call to fail. + // This can happen if the address we are trying to connect to has nothing + // listening. So we can't use RELEASE_ASSERT and instead must throw an + // exception + if (SOCKET_FAILURE(result.rc_)) { + throw EnvoyException(fmt::format("getsockopt failed for '{}': ({}) {}", fd, result.errno_, + strerror(result.errno_))); + } +#else + RELEASE_ASSERT(result.rc_ == 0, ""); +#endif } - return addressFromSockAddr(ss, ss_len, rc == 0 && socket_v6only); + return addressFromSockAddr(ss, ss_len, socket_v6only); } InstanceConstSharedPtr peerAddressFromFd(os_fd_t fd) { sockaddr_storage ss; socklen_t ss_len = sizeof ss; - const int rc = ::getpeername(fd, reinterpret_cast(&ss), &ss_len); - if (rc != 0) { - throw EnvoyException(fmt::format("getpeername failed for '{}': {}", fd, strerror(errno))); + auto& os_sys_calls = Api::OsSysCallsSingleton::get(); + Api::SysCallIntResult result = + os_sys_calls.getpeername(fd, reinterpret_cast(&ss), &ss_len); + if (result.rc_ != 0) { + throw EnvoyException( + fmt::format("getpeername failed for '{}': {}", fd, strerror(result.errno_))); } #ifdef __APPLE__ - if (ss_len == sizeof(sockaddr) && ss.ss_family == AF_UNIX) { + if (ss_len == sizeof(sockaddr) && ss.ss_family == AF_UNIX) #else - if (ss_len == sizeof(sa_family_t) && ss.ss_family == AF_UNIX) { + if (ss_len == sizeof(sa_family_t) && ss.ss_family == AF_UNIX) #endif + { // For Unix domain sockets, can't find out the peer name, but it should match our own // name for the socket (i.e. the path should match, barring any namespace or other // mechanisms to hide things, of which there are many). ss_len = sizeof ss; - const int rc = ::getsockname(fd, reinterpret_cast(&ss), &ss_len); - if (rc != 0) { - throw EnvoyException(fmt::format("getsockname failed for '{}': {}", fd, strerror(errno))); + result = os_sys_calls.getsockname(fd, reinterpret_cast(&ss), &ss_len); + if (result.rc_ != 0) { + throw EnvoyException( + fmt::format("getsockname failed for '{}': {}", fd, strerror(result.errno_))); } } return addressFromSockAddr(ss, ss_len); @@ -166,14 +189,16 @@ IoHandlePtr InstanceBase::socketFromSocketType(SocketType socket_type) const { domain = AF_UNIX; } - const Api::SysCallSocketResult result = Api::OsSysCallsSingleton::get().socket(domain, flags, 0); + auto os_sys_calls = Api::OsSysCallsSingleton::get(); + const Api::SysCallSocketResult result = os_sys_calls.socket(domain, flags, 0); RELEASE_ASSERT(SOCKET_VALID(result.rc_), fmt::format("socket(2) failed, got error: {}", strerror(result.errno_))); IoHandlePtr io_handle = std::make_unique(result.rc_); #ifdef __APPLE__ // Cannot set SOCK_NONBLOCK as a ::socket flag. - RELEASE_ASSERT(fcntl(io_handle->fd(), F_SETFL, O_NONBLOCK) != -1, ""); + const int rc = os_sys_calls.setsocketblocking(io_handle->fd(), false).rc_; + RELEASE_ASSERT(!SOCKET_FAILURE(rc), ""); #endif return io_handle; @@ -230,8 +255,8 @@ Api::SysCallIntResult Ipv4Instance::bind(os_fd_t fd) const { } Api::SysCallIntResult Ipv4Instance::connect(os_fd_t fd) const { - const int rc = ::connect(fd, sockAddr(), sockAddrLen()); - return {rc, errno}; + return Api::OsSysCallsSingleton::get().connect( + fd, reinterpret_cast(&ip_.ipv4_.address_), sizeof(ip_.ipv4_.address_)); } IoHandlePtr Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } @@ -319,16 +344,18 @@ Api::SysCallIntResult Ipv6Instance::bind(os_fd_t fd) const { } Api::SysCallIntResult Ipv6Instance::connect(os_fd_t fd) const { - const int rc = ::connect(fd, sockAddr(), sockAddrLen()); - return {rc, errno}; + return Api::OsSysCallsSingleton::get().connect( + fd, reinterpret_cast(&ip_.ipv6_.address_), sizeof(ip_.ipv6_.address_)); } IoHandlePtr Ipv6Instance::socket(SocketType type) const { IoHandlePtr io_handle = socketFromSocketType(type); // Setting IPV6_V6ONLY restricts the IPv6 socket to IPv6 connections only. const int v6only = ip_.v6only_; - RELEASE_ASSERT( - ::setsockopt(io_handle->fd(), IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, ""); + const Api::SysCallIntResult result = Api::OsSysCallsSingleton::get().setsockopt( + io_handle->fd(), IPPROTO_IPV6, IPV6_V6ONLY, reinterpret_cast(&v6only), + sizeof(v6only)); + RELEASE_ASSERT(!SOCKET_FAILURE(result.rc_), ""); return io_handle; } @@ -398,16 +425,18 @@ PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode) : Instance bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } Api::SysCallIntResult PipeInstance::bind(os_fd_t fd) const { - if (!abstract_namespace_) { - // Try to unlink an existing filesystem object at the requested path. Ignore - // errors -- it's fine if the path doesn't exist, and if it exists but can't - // be unlinked then `::bind()` will generate a reasonable errno. - unlink(address_.sun_path); + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); + if (abstract_namespace_) { + return os_sys_calls.bind(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_); } - auto& os_syscalls = Api::OsSysCallsSingleton::get(); - auto bind_result = os_syscalls.bind(fd, sockAddr(), sockAddrLen()); - if (mode != 0 && !abstract_namespace_ && bind_result.rc_ == 0) { - auto set_permissions = os_syscalls.chmod(address_.sun_path, mode); + // Try to unlink an existing filesystem object at the requested path. Ignore + // errors -- it's fine if the path doesn't exist, and if it exists but can't + // be unlinked then `::bind()` will generate a reasonable errno. + unlink(address_.sun_path); + auto bind_result = os_sys_calls.bind(fd, sockAddr(), sockAddrLen()); + if (mode != 0 && bind_result.rc_ == 0) { + auto set_permissions = os_sys_calls.chmod(address_.sun_path, mode); if (set_permissions.rc_ != 0) { throw EnvoyException(absl::StrCat("Failed to create socket with mode ", mode)); } @@ -416,8 +445,12 @@ Api::SysCallIntResult PipeInstance::bind(os_fd_t fd) const { } Api::SysCallIntResult PipeInstance::connect(os_fd_t fd) const { - const int rc = ::connect(fd, sockAddr(), sockAddrLen()); - return {rc, errno}; + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); + if (abstract_namespace_) { + return os_sys_calls.connect(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_); + } + return os_sys_calls.connect(fd, reinterpret_cast(&address_), sizeof(address_)); } IoHandlePtr PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 16a9caf32e97..6215a8bf83bc 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -10,6 +10,7 @@ #include "envoy/event/timer.h" #include "envoy/network/filter.h" +#include "common/api/os_sys_calls_impl.h" #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" @@ -54,17 +55,23 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt write_end_stream_(false), current_write_end_stream_(false), dispatch_buffered_data_(false) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. - RELEASE_ASSERT(ioHandle().fd() != -1, ""); + RELEASE_ASSERT(SOCKET_VALID(ioHandle().fd()), ""); if (!connected) { connecting_ = true; } + // Libevent only supports Level trigger on Windows. +#ifdef WIN32 + Event::FileTriggerType trigger = Event::FileTriggerType::Level; +#else + Event::FileTriggerType trigger = Event::FileTriggerType::Edge; +#endif // We never ask for both early close and read at the same time. If we are reading, we want to // consume all available data. file_event_ = dispatcher_.createFileEvent( - ioHandle().fd(), [this](uint32_t events) -> void { onFileEvent(events); }, - Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Write); + ioHandle().fd(), [this](uint32_t events) -> void { onFileEvent(events); }, trigger, + Event::FileReadyType::Read | Event::FileReadyType::Write); transport_socket_->setTransportSocketCallbacks(*this); } @@ -205,6 +212,10 @@ void ConnectionImpl::closeSocket(ConnectionEvent close_type) { connection_stats_.reset(); file_event_.reset(); + + // Shutdown the socket so the other side can read any data in flight. + Api::OsSysCallsSingleton::get().shutdown(ioHandle().fd(), ENVOY_SHUT_WR); + socket_->close(); raiseEvent(close_type); @@ -223,27 +234,39 @@ void ConnectionImpl::noDelay(bool enable) { } // Don't set NODELAY for unix domain sockets - sockaddr addr; + sockaddr_storage addr; socklen_t len = sizeof(addr); - int rc = getsockname(ioHandle().fd(), &addr, &len); - RELEASE_ASSERT(rc == 0, ""); - if (addr.sa_family == AF_UNIX) { + auto os_sys_calls = Api::OsSysCallsSingleton::get(); + Api::SysCallIntResult result = + os_sys_calls.getsockname(ioHandle().fd(), reinterpret_cast(&addr), &len); + + RELEASE_ASSERT(result.rc_ == 0, ""); + + if (addr.ss_family == AF_UNIX) { return; } // Set NODELAY int new_value = enable; - rc = setsockopt(ioHandle().fd(), IPPROTO_TCP, TCP_NODELAY, &new_value, sizeof(new_value)); -#ifdef __APPLE__ - if (-1 == rc && errno == EINVAL) { + result = os_sys_calls.setsockopt(ioHandle().fd(), IPPROTO_TCP, TCP_NODELAY, &new_value, + sizeof(new_value)); +#if defined(__APPLE__) + if (SOCKET_FAILURE(result.rc_) && result.errno_ == EINVAL) { + // Sometimes occurs when the connection is not yet fully formed. Empirically, TCP_NODELAY is + // enabled despite this result. + return; + } +#elif defined(WIN32) + if (SOCKET_FAILURE(result.rc_) && + (result.errno_ == WSAEWOULDBLOCK || result.errno_ == WSAEINVAL)) { // Sometimes occurs when the connection is not yet fully formed. Empirically, TCP_NODELAY is // enabled despite this result. return; } #endif - RELEASE_ASSERT(0 == rc, ""); + RELEASE_ASSERT(result.rc_ == 0, ""); } void ConnectionImpl::onRead(uint64_t read_buffer_size) { @@ -562,8 +585,10 @@ void ConnectionImpl::onWriteReady() { if (connecting_) { int error; socklen_t error_size = sizeof(error); - int rc = getsockopt(ioHandle().fd(), SOL_SOCKET, SO_ERROR, &error, &error_size); - ASSERT(0 == rc); + RELEASE_ASSERT(Api::OsSysCallsSingleton::get() + .getsockopt(ioHandle().fd(), SOL_SOCKET, SO_ERROR, &error, &error_size) + .rc_ == 0, + ""); if (error == 0) { ENVOY_CONN_LOG(debug, "connected", *this); diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 90ea393bbd3e..da936757ffcd 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -19,21 +19,19 @@ namespace Envoy { namespace Network { IoSocketHandleImpl::~IoSocketHandleImpl() { - if (fd_ != -1) { + if (SOCKET_VALID(fd_)) { IoSocketHandleImpl::close(); } } Api::IoCallUint64Result IoSocketHandleImpl::close() { - ASSERT(fd_ != -1); - auto& os_syscalls = Api::OsSysCallsSingleton::get(); - const auto& result = os_syscalls.close(fd_); - fd_ = -1; - return Api::IoCallUint64Result(result.rc_, - Api::IoErrorPtr(nullptr, IoSocketError::deleteIoError)); + ASSERT(SOCKET_VALID(fd_)); + const int rc = Api::OsSysCallsSingleton::get().close(fd_).rc_; + SET_SOCKET_INVALID(fd_); + return Api::IoCallUint64Result(rc, Api::IoErrorPtr(nullptr, IoSocketError::deleteIoError)); } -bool IoSocketHandleImpl::isOpen() const { return fd_ != -1; } +bool IoSocketHandleImpl::isOpen() const { return SOCKET_VALID(fd_); } Api::IoCallUint64Result IoSocketHandleImpl::readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) { @@ -48,10 +46,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::readv(uint64_t max_length, Buffer::R num_bytes_to_read += slice_length; } ASSERT(num_bytes_to_read <= max_length); - auto& os_syscalls = Api::OsSysCallsSingleton::get(); - const Api::SysCallSizeResult result = - os_syscalls.readv(fd_, iov.begin(), static_cast(num_slices_to_read)); - return sysCallResultToIoCallResult(result); + return sysCallResultToIoCallResult(Api::OsSysCallsSingleton::get().readv( + fd_, iov.begin(), static_cast(num_slices_to_read))); } Api::IoCallUint64Result IoSocketHandleImpl::writev(const Buffer::RawSlice* slices, @@ -68,9 +64,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::writev(const Buffer::RawSlice* slice if (num_slices_to_write == 0) { return Api::ioCallUint64ResultNoError(); } - auto& os_syscalls = Api::OsSysCallsSingleton::get(); - const Api::SysCallSizeResult result = os_syscalls.writev(fd_, iov.begin(), num_slices_to_write); - return sysCallResultToIoCallResult(result); + return sysCallResultToIoCallResult( + Api::OsSysCallsSingleton::get().writev(fd_, iov.begin(), num_slices_to_write)); } Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slices, @@ -99,12 +94,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic message.msg_iov = iov.begin(); message.msg_iovlen = num_slices_to_write; message.msg_flags = 0; - auto& os_syscalls = Api::OsSysCallsSingleton::get(); if (self_ip == nullptr) { message.msg_control = nullptr; message.msg_controllen = 0; - const Api::SysCallSizeResult result = os_syscalls.sendmsg(fd_, &message, flags); - return sysCallResultToIoCallResult(result); } else { const size_t space_v6 = CMSG_SPACE(sizeof(in6_pktinfo)); // FreeBSD only needs in_addr size, but allocates more to unify code in two platforms. @@ -126,7 +118,11 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic cmsg->cmsg_type = IP_PKTINFO; auto pktinfo = reinterpret_cast(CMSG_DATA(cmsg)); pktinfo->ipi_ifindex = 0; +#ifdef WIN32 + pktinfo->ipi_addr.s_addr = self_ip->ipv4()->address(); +#else pktinfo->ipi_spec_dst.s_addr = self_ip->ipv4()->address(); +#endif #else cmsg->cmsg_type = IP_SENDSRCADDR; cmsg->cmsg_len = CMSG_LEN(sizeof(in_addr)); @@ -140,9 +136,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic pktinfo->ipi6_ifindex = 0; *(reinterpret_cast(pktinfo->ipi6_addr.s6_addr)) = self_ip->ipv6()->address(); } - const Api::SysCallSizeResult result = os_syscalls.sendmsg(fd_, &message, flags); - return sysCallResultToIoCallResult(result); } + return sysCallResultToIoCallResult(Api::OsSysCallsSingleton::get().sendmsg(fd_, &message, flags)); } Api::IoCallUint64Result @@ -162,7 +157,7 @@ IoSocketHandleImpl::sysCallResultToIoCallResult(const Api::SysCallSizeResult& re : Api::IoErrorPtr(new IoSocketError(result.errno_), IoSocketError::deleteIoError))); } -Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const struct cmsghdr& cmsg, +Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg, uint32_t self_port) { if (cmsg.cmsg_type == IPV6_PKTINFO) { auto info = reinterpret_cast(CMSG_DATA(&cmsg)); @@ -199,12 +194,12 @@ Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const struct cmsghd absl::optional maybeGetPacketsDroppedFromHeader( #ifdef SO_RXQ_OVFL - const struct cmsghdr& cmsg) { + const cmsghdr& cmsg) { if (cmsg.cmsg_type == SO_RXQ_OVFL) { return *reinterpret_cast(CMSG_DATA(&cmsg)); } #else - const struct cmsghdr&) { + const cmsghdr&) { #endif return absl::nullopt; } @@ -230,6 +225,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, ++num_slices_for_read; } } + if (num_slices_for_read == 0) { + return Api::ioCallUint64ResultNoError(); + } sockaddr_storage peer_addr; msghdr hdr; @@ -238,13 +236,11 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, hdr.msg_iov = iov.begin(); hdr.msg_iovlen = num_slices_for_read; hdr.msg_flags = 0; - - auto cmsg = reinterpret_cast(cbuf.begin()); + auto cmsg = reinterpret_cast(cbuf.begin()); cmsg->cmsg_len = cmsg_space; hdr.msg_control = cmsg; hdr.msg_controllen = cmsg_space; - auto& os_sys_calls = Api::OsSysCallsSingleton::get(); - const Api::SysCallSizeResult result = os_sys_calls.recvmsg(fd_, &hdr, 0); + const Api::SysCallSizeResult result = Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, 0); if (result.rc_ < 0) { return sysCallResultToIoCallResult(result); } @@ -269,26 +265,23 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, } // Get overflow, local and peer addresses from control message. - if (hdr.msg_controllen > 0) { - struct cmsghdr* cmsg; - for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(&hdr, cmsg)) { - if (output.local_address_ == nullptr) { - try { - Address::InstanceConstSharedPtr addr = maybeGetDstAddressFromHeader(*cmsg, self_port); - if (addr != nullptr) { - // This is a IP packet info message. - output.local_address_ = std::move(addr); - continue; - } - } catch (const EnvoyException& e) { - PANIC(fmt::format("Invalid destination address for fd: {}, error: {}", fd_, e.what())); + for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(&hdr, cmsg)) { + if (output.local_address_ == nullptr) { + try { + Address::InstanceConstSharedPtr addr = maybeGetDstAddressFromHeader(*cmsg, self_port); + if (addr != nullptr) { + // This is a IP packet info message. + output.local_address_ = std::move(addr); + continue; } + } catch (const EnvoyException& e) { + PANIC(fmt::format("Invalid destination address for fd: {}, error: {}", fd_, e.what())); } - if (output.dropped_packets_ != nullptr) { - absl::optional maybe_dropped = maybeGetPacketsDroppedFromHeader(*cmsg); - if (maybe_dropped) { - *output.dropped_packets_ = *maybe_dropped; - } + } + if (output.dropped_packets_ != nullptr) { + absl::optional maybe_dropped = maybeGetPacketsDroppedFromHeader(*cmsg); + if (maybe_dropped) { + *output.dropped_packets_ = *maybe_dropped; } } } diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 3dd5a5bdba62..7191c174af7f 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -1,5 +1,6 @@ #include "common/network/raw_buffer_socket.h" +#include "common/api/os_sys_calls_impl.h" #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/http/headers.h" @@ -55,7 +56,7 @@ IoResult RawBufferSocket::doWrite(Buffer::Instance& buffer, bool end_stream) { if (end_stream && !shutdown_) { // Ignore the result. This can only fail if the connection failed. In that case, the // error will be detected on the next read, and dealt with appropriately. - ::shutdown(callbacks_->ioHandle().fd(), SHUT_WR); + Api::OsSysCallsSingleton::get().shutdown(callbacks_->ioHandle().fd(), ENVOY_SHUT_WR); shutdown_ = true; } action = PostIoAction::KeepOpen; diff --git a/source/extensions/filters/network/mysql_proxy/mysql_utils.cc b/source/extensions/filters/network/mysql_proxy/mysql_utils.cc index 9e154fa69167..aacf16f09e7d 100644 --- a/source/extensions/filters/network/mysql_proxy/mysql_utils.cc +++ b/source/extensions/filters/network/mysql_proxy/mysql_utils.cc @@ -150,7 +150,7 @@ int BufferHelper::peekHdr(Buffer::Instance& buffer, uint32_t& len, uint8_t& seq) if (peekUint32(buffer, val) != MYSQL_SUCCESS) { return MYSQL_FAILURE; } - seq = htonl(val) & MYSQL_HDR_SEQ_MASK; + seq = htobe32(val) & MYSQL_HDR_SEQ_MASK; len = val & MYSQL_HDR_PKT_SIZE_MASK; ENVOY_LOG(trace, "mysql_proxy: MYSQL-hdrseq {}, len {}", seq, len); return MYSQL_SUCCESS; diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index f0efb2b2438c..be3cfedaf671 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -198,7 +198,7 @@ TEST_F(WatermarkBufferTest, MoveOneByte) { } TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { - int pipe_fds[2] = {0, 0}; + os_fd_t pipe_fds[2] = {0, 0}; ASSERT_EQ(0, pipe(pipe_fds)); buffer_.add(TEN_BYTES, 10); diff --git a/test/common/event/file_event_impl_test.cc b/test/common/event/file_event_impl_test.cc index b7e11bc54540..92bec2c65be2 100644 --- a/test/common/event/file_event_impl_test.cc +++ b/test/common/event/file_event_impl_test.cc @@ -2,6 +2,7 @@ #include "envoy/event/file_event.h" +#include "common/api/os_sys_calls_impl.h" #include "common/event/dispatcher_impl.h" #include "common/stats/isolated_store_impl.h" @@ -17,28 +18,41 @@ namespace { class FileEventImplTest : public testing::Test { public: - FileEventImplTest() : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher()) {} + FileEventImplTest() + : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher()), + os_sys_calls_(Api::OsSysCallsSingleton::get()) {} void SetUp() override { - int rc = socketpair(AF_UNIX, SOCK_DGRAM, 0, fds_); - ASSERT_EQ(0, rc); +#ifdef WIN32 + ASSERT_EQ(0, os_sys_calls_.socketpair(AF_INET, SOCK_STREAM, 0, fds_).rc_); +#else + ASSERT_EQ(0, os_sys_calls_.socketpair(AF_UNIX, SOCK_DGRAM, 0, fds_).rc_); +#endif int data = 1; - rc = write(fds_[1], &data, sizeof(data)); - ASSERT_EQ(sizeof(data), static_cast(rc)); + + const Api::SysCallSizeResult result = os_sys_calls_.write(fds_[1], &data, sizeof(data)); + ASSERT_EQ(sizeof(data), static_cast(result.rc_)); } void TearDown() override { - close(fds_[0]); - close(fds_[1]); + os_sys_calls_.close(fds_[0]); + os_sys_calls_.close(fds_[1]); } protected: os_fd_t fds_[2]; Api::ApiPtr api_; DispatcherPtr dispatcher_; + Api::OsSysCalls& os_sys_calls_; }; -class FileEventImplActivateTest : public testing::TestWithParam {}; +class FileEventImplActivateTest : public testing::TestWithParam { +public: + FileEventImplActivateTest() : os_sys_calls_(Api::OsSysCallsSingleton::get()) {} + +protected: + Api::OsSysCalls& os_sys_calls_; +}; INSTANTIATE_TEST_SUITE_P(IpVersions, FileEventImplActivateTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), @@ -46,12 +60,9 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, FileEventImplActivateTest, TEST_P(FileEventImplActivateTest, Activate) { os_fd_t fd; - if (GetParam() == Network::Address::IpVersion::v4) { - fd = socket(AF_INET, SOCK_STREAM, 0); - } else { - fd = socket(AF_INET6, SOCK_STREAM, 0); - } - ASSERT_NE(-1, fd); + int domain = GetParam() == Network::Address::IpVersion::v4 ? AF_INET : AF_INET6; + fd = os_sys_calls_.socket(domain, SOCK_STREAM, 0).rc_; + ASSERT_TRUE(SOCKET_VALID(fd)); Api::ApiPtr api = Api::createApiForTest(); DispatcherPtr dispatcher(api->allocateDispatcher()); @@ -62,6 +73,12 @@ TEST_P(FileEventImplActivateTest, Activate) { ReadyWatcher closed_event; EXPECT_CALL(closed_event, ready()).Times(1); +#ifdef WIN32 + const FileTriggerType trigger = FileTriggerType::Level; +#else + const FileTriggerType trigger = FileTriggerType::Edge; +#endif + Event::FileEventPtr file_event = dispatcher->createFileEvent( fd, [&](uint32_t events) -> void { @@ -77,14 +94,15 @@ TEST_P(FileEventImplActivateTest, Activate) { closed_event.ready(); } }, - FileTriggerType::Edge, FileReadyType::Read | FileReadyType::Write | FileReadyType::Closed); + trigger, FileReadyType::Read | FileReadyType::Write | FileReadyType::Closed); file_event->activate(FileReadyType::Read | FileReadyType::Write | FileReadyType::Closed); dispatcher->run(Event::Dispatcher::RunType::NonBlock); - close(fd); + os_sys_calls_.close(fd); } +#ifndef WIN32 // Libevent on Windows doesn't support edge trigger. TEST_F(FileEventImplTest, EdgeTrigger) { ReadyWatcher read_event; EXPECT_CALL(read_event, ready()).Times(1); @@ -106,6 +124,7 @@ TEST_F(FileEventImplTest, EdgeTrigger) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } +#endif TEST_F(FileEventImplTest, LevelTrigger) { ReadyWatcher read_event; @@ -140,6 +159,11 @@ TEST_F(FileEventImplTest, SetEnabled) { ReadyWatcher write_event; EXPECT_CALL(write_event, ready()).Times(2); +#ifdef WIN32 + const FileTriggerType trigger = FileTriggerType::Level; +#else + const FileTriggerType trigger = FileTriggerType::Edge; +#endif Event::FileEventPtr file_event = dispatcher_->createFileEvent( fds_[0], [&](uint32_t events) -> void { @@ -151,7 +175,7 @@ TEST_F(FileEventImplTest, SetEnabled) { write_event.ready(); } }, - FileTriggerType::Edge, FileReadyType::Read | FileReadyType::Write); + trigger, FileReadyType::Read | FileReadyType::Write); file_event->setEnabled(FileReadyType::Read); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index 27e0e60dbbb0..4d6524fe8b0d 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -15,8 +15,8 @@ class AddrFamilyAwareSocketOptionImplTest : public SocketOptionTest { protected: void SetUp() override { EXPECT_CALL(os_sys_calls_, socket) - .WillRepeatedly(Invoke([](int domain, int type, int protocol) { - return Api::SysCallIntResult{::socket(domain, type, protocol), 0}; + .WillRepeatedly(Invoke([&](int domain, int type, int protocol) { + return os_sys_calls_actual_.socket(domain, type, protocol); })); EXPECT_CALL(os_sys_calls_, close(_)).Times(testing::AnyNumber()); } @@ -145,7 +145,9 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Precedence) { // GetSocketOptionName returns the v4 information for a v4 address TEST_F(AddrFamilyAwareSocketOptionImplTest, V4GetSocketOptionName) { - socket_.local_address_ = Utility::parseInternetAddress("1.2.3.4", 5678); + Address::Ipv4Instance address("1.2.3.4", 5678); + IoHandlePtr io_handle = address.socket(Address::SocketType::Stream); + EXPECT_CALL(testing::Const(socket_), ioHandle()).WillRepeatedly(testing::ReturnRef(*io_handle)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_MAKE_SOCKET_OPTION_NAME(5, 10), @@ -158,7 +160,9 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4GetSocketOptionName) { // GetSocketOptionName returns the v4 information for a v6 address TEST_F(AddrFamilyAwareSocketOptionImplTest, V6GetSocketOptionName) { - socket_.local_address_ = Utility::parseInternetAddress("2::1", 5678); + Address::Ipv6Instance address("2::1", 5678); + IoHandlePtr io_handle = address.socket(Address::SocketType::Stream); + EXPECT_CALL(testing::Const(socket_), ioHandle()).WillRepeatedly(testing::ReturnRef(*io_handle)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_MAKE_SOCKET_OPTION_NAME(5, 10), diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 88258a1a1cce..33997fc4bb0f 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -36,12 +36,6 @@ bool addressesEqual(const InstanceConstSharedPtr& a, const Instance& b) { } } -void makeFdBlocking(int fd) { - const int flags = ::fcntl(fd, F_GETFL, 0); - ASSERT_GE(flags, 0); - ASSERT_EQ(::fcntl(fd, F_SETFL, flags & (~O_NONBLOCK)), 0); -} - void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6only) { auto addr_port = Network::Utility::parseInternetAddressAndPort( fmt::format("{}:0", Network::Test::getAnyAddressUrlString(ip_version)), v6only); @@ -56,14 +50,17 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl // Create a socket on which we'll listen for connections from clients. IoHandlePtr io_handle = addr_port->socket(SocketType::Stream); ASSERT_GE(io_handle->fd(), 0) << addr_port->asString(); + auto& os_sys_calls = Api::OsSysCallsSingleton::get(); // Check that IPv6 sockets accept IPv6 connections only. if (addr_port->ip()->version() == IpVersion::v6) { int socket_v6only = 0; socklen_t size_int = sizeof(socket_v6only); - ASSERT_GE(::getsockopt(io_handle->fd(), IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int), + ASSERT_GE(os_sys_calls + .getsockopt(io_handle->fd(), IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int) + .rc_, 0); - EXPECT_EQ(v6only, socket_v6only); + EXPECT_EQ(v6only, socket_v6only != 0); } // Bind the socket to the desired address and port. @@ -73,9 +70,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl // Do a bare listen syscall. Not bothering to accept connections as that would // require another thread. - ASSERT_EQ(::listen(io_handle->fd(), 128), 0); + ASSERT_EQ(os_sys_calls.listen(io_handle->fd(), 128).rc_, 0); - auto client_connect = [](Address::InstanceConstSharedPtr addr_port) { + auto client_connect = [&os_sys_calls](Address::InstanceConstSharedPtr addr_port) { // Create a client socket and connect to the server. IoHandlePtr client_handle = addr_port->socket(SocketType::Stream); ASSERT_GE(client_handle->fd(), 0) << addr_port->asString(); @@ -84,7 +81,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl // operation of ::connect(), so connect returns with errno==EWOULDBLOCK before the tcp // handshake can complete. For testing convenience, re-enable blocking on the socket // so that connect will wait for the handshake to complete. - makeFdBlocking(client_handle->fd()); + ASSERT_EQ(os_sys_calls.setsocketblocking(client_handle->fd(), true).rc_, 0); // Connect to the server. const Api::SysCallIntResult result = addr_port->connect(client_handle->fd()); @@ -92,7 +89,12 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl << "\nerrno: " << result.errno_; }; - client_connect(addr_port); + auto client_addr_port = Network::Utility::parseInternetAddressAndPort( + fmt::format("{}:{}", Network::Test::getLoopbackAddressUrlString(ip_version), + addr_port->ip()->port()), + v6only); + ASSERT_NE(client_addr_port, nullptr); + client_connect(client_addr_port); if (!v6only) { ASSERT_EQ(IpVersion::v6, addr_port->ip()->version()); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index a068c30cf2aa..19522d188df4 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -89,7 +89,7 @@ TEST_P(ConnectionImplDeathTest, BadFd) { ConnectionImpl(*dispatcher, std::make_unique(std::move(io_handle), nullptr, nullptr), Network::Test::createRawBufferSocket(), false), - ".*assert failure: ioHandle\\(\\).fd\\(\\) != -1.*"); + ".*assert failure: SOCKET_VALID\\(ioHandle\\(\\)\\.fd\\(\\)\\).*"); } class ConnectionImplTest : public testing::TestWithParam { @@ -103,10 +103,11 @@ class ConnectionImplTest : public testing::TestWithParam { socket_ = std::make_shared(Network::Test::getAnyAddress(GetParam()), nullptr, true); listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true); - client_connection_ = dispatcher_->createClientConnection( - socket_->localAddress(), source_address_, Network::Test::createRawBufferSocket(), - socket_options_); + Utility::resolveUrl(fmt::format("tcp://{}:{}", + Network::Test::getLoopbackAddressUrlString(GetParam()), + socket_->localAddress()->ip()->port())), + source_address_, Network::Test::createRawBufferSocket(), socket_options_); client_connection_->addConnectionCallbacks(client_callbacks_); EXPECT_EQ(nullptr, client_connection_->ssl()); const Network::ClientConnection& const_connection = *client_connection_; @@ -616,12 +617,12 @@ TEST_P(ConnectionImplTest, ReadDisableAfterCloseHandledGracefully) { #endif } +// On our current macOS build, the client connection does not get the early +// close notification and instead gets the close after reading the FIN. +// The Windows backend in libevent does not support the EV_CLOSED flag +// so it won't detect the early close +#if !defined(__APPLE__) && !defined(WIN32) TEST_P(ConnectionImplTest, EarlyCloseOnReadDisabledConnection) { -#ifdef __APPLE__ - // On our current macOS build, the client connection does not get the early - // close notification and instead gets the close after reading the FIN. - return; -#endif setUpBasicConnection(); connect(); @@ -633,6 +634,7 @@ TEST_P(ConnectionImplTest, EarlyCloseOnReadDisabledConnection) { server_connection_->close(ConnectionCloseType::FlushWrite); dispatcher_->run(Event::Dispatcher::RunType::Block); } +#endif TEST_P(ConnectionImplTest, CloseOnReadDisableWithoutCloseDetection) { setUpBasicConnection(); @@ -1089,15 +1091,16 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTest) { dispatcher_->run(Event::Dispatcher::RunType::Block); } -// Test that a FlushWriteAndDelay close causes Envoy to flush the write and wait for the client/peer -// to close (until a configured timeout which is not expected to trigger in this test). +// Test that a FlushWriteAndDelay close causes Envoy to flush the write and wait for the +// client/peer to close (until a configured timeout which is not expected to trigger in this +// test). +// +// libevent does not provide early close notifications on the currently supported non-Linux +// builds, so the server connection is never notified of the close. For now, we have chosen to +// disable tests that rely on this behavior on macOS and Windows (see +// https://github.com/envoyproxy/envoy/pull/4299). +#if !defined(__APPLE__) && !defined(WIN32) TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { -#ifdef __APPLE__ - // libevent does not provide early close notifications on the currently supported macOS builds, so - // the server connection is never notified of the close. For now, we have chosen to disable tests - // that rely on this behavior on macOS (see https://github.com/envoyproxy/envoy/pull/4299). - return; -#endif setUpBasicConnection(); connect(); @@ -1133,6 +1136,7 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { server_connection_->close(ConnectionCloseType::FlushWriteAndDelay); dispatcher_->run(Event::Dispatcher::RunType::Block); } +#endif // Test that a FlushWriteAndDelay close triggers a timeout which forces Envoy to close the // connection when a client has not issued a close within the configured interval. @@ -1156,8 +1160,8 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTimerTriggerTest) { time_system_.setMonotonicTime(std::chrono::milliseconds(0)); - // The client _will not_ close the connection. Instead, expect the delayed close timer to trigger - // on the server connection. + // The client _will not_ close the connection. Instead, expect the delayed close timer to + // trigger on the server connection. EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("Connection: Close"), false)) .Times(1) .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { @@ -1245,8 +1249,8 @@ TEST_P(ConnectionImplTest, FlushWriteAfterFlushWriteAndDelayWithoutPendingWrite) })); dispatcher_->run(Event::Dispatcher::RunType::Block); - // The write buffer has been flushed and a delayed close timer has been set. The socket close will - // happen as part of the close() since the timeout is no longer required. + // The write buffer has been flushed and a delayed close timer has been set. The socket close + // will happen as part of the close() since the timeout is no longer required. EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); server_connection_->close(ConnectionCloseType::FlushWrite); EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); @@ -1256,8 +1260,8 @@ TEST_P(ConnectionImplTest, FlushWriteAfterFlushWriteAndDelayWithoutPendingWrite) dispatcher_->run(Event::Dispatcher::RunType::Block); } -// Test that delayed close processing can be disabled by setting the delayed close timeout interval -// to 0. +// Test that delayed close processing can be disabled by setting the delayed close timeout +// interval to 0. TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) { InSequence s1; @@ -1275,8 +1279,8 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) { time_system_.setMonotonicTime(std::chrono::milliseconds(0)); - // Ensure the delayed close timer is not created when the delayedCloseTimeout config value is set - // to 0. + // Ensure the delayed close timer is not created when the delayedCloseTimeout config value is + // set to 0. server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(0)); EXPECT_CALL(dispatcher, createTimer_(_)).Times(0); @@ -1289,13 +1293,13 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) { // trigger the delayed close timer callback if set. time_system_.setMonotonicTime(std::chrono::milliseconds(10000)); - // Since the delayed close timer never triggers, the connection never closes. Close it here to end - // the test cleanly due to the (fd == -1) assert in ~ConnectionImpl(). + // Since the delayed close timer never triggers, the connection never closes. Close it here to + // end the test cleanly due to the (fd == -1) assert in ~ConnectionImpl(). server_connection->close(ConnectionCloseType::NoFlush); } -// Test that the delayed close timer is reset while write flushes are happening when a connection is -// in delayed close mode. +// Test that the delayed close timer is reset while write flushes are happening when a connection +// is in delayed close mode. TEST_P(ConnectionImplTest, DelayedCloseTimerResetWithPendingWriteBufferFlushes) { ConnectionMocks mocks = createConnectionMocks(); MockTransportSocket* transport_socket = mocks.transport_socket_.get(); @@ -1324,8 +1328,8 @@ TEST_P(ConnectionImplTest, DelayedCloseTimerResetWithPendingWriteBufferFlushes) EXPECT_CALL(*mocks.timer_, enableTimer(timeout, _)).Times(1); server_connection->close(ConnectionCloseType::FlushWriteAndDelay); - // The write ready event cb (ConnectionImpl::onWriteReady()) will reset the timer to its original - // timeout value to avoid triggering while the write buffer is being actively flushed. + // The write ready event cb (ConnectionImpl::onWriteReady()) will reset the timer to its + // original timeout value to avoid triggering while the write buffer is being actively flushed. EXPECT_CALL(*transport_socket, doWrite(BufferStringEqual("data"), _)) .WillOnce(Invoke([&](Buffer::Instance&, bool) -> IoResult { // Partial flush. @@ -1395,13 +1399,13 @@ TEST_P(ConnectionImplTest, DelayedCloseTimeoutNullStats) { InSequence s1; - // The actual timeout is insignificant, we just need to enable delayed close processing by setting - // it to > 0. + // The actual timeout is insignificant, we just need to enable delayed close processing by + // setting it to > 0. server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(100)); - // NOTE: Avoid providing stats storage to the connection via setConnectionStats(). This guarantees - // that connection_stats_ is a nullptr and that the callback resiliency validation below tests - // that edge case. + // NOTE: Avoid providing stats storage to the connection via setConnectionStats(). This + // guarantees that connection_stats_ is a nullptr and that the callback resiliency validation + // below tests that edge case. Buffer::OwnedImpl data("data"); EXPECT_CALL(*mocks.file_event_, activate(Event::FileReadyType::Write)) @@ -1664,7 +1668,8 @@ TEST_F(MockTransportConnectionImplTest, BothHalfCloseWriteFirst) { } // Test that if both sides half-close, but writes have not yet been written to the Transport, that -// the connection closes only when the writes complete flushing. The write half-close happens first. +// the connection closes only when the writes complete flushing. The write half-close happens +// first. TEST_F(MockTransportConnectionImplTest, BothHalfCloseWritesNotFlushedWriteFirst) { std::shared_ptr read_filter(new NiceMock()); connection_->enableHalfClose(true); @@ -1686,7 +1691,8 @@ TEST_F(MockTransportConnectionImplTest, BothHalfCloseWritesNotFlushedWriteFirst) } // Test that if both sides half-close, but writes have not yet been written to the Transport, that -// the connection closes only when the writes complete flushing. The read half-close happens first. +// the connection closes only when the writes complete flushing. The read half-close happens +// first. TEST_F(MockTransportConnectionImplTest, BothHalfCloseWritesNotFlushedReadFirst) { std::shared_ptr read_filter(new NiceMock()); connection_->enableHalfClose(true); @@ -1991,8 +1997,11 @@ class ReadBufferLimitTest : public ConnectionImplTest { listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true); client_connection_ = dispatcher_->createClientConnection( - socket_->localAddress(), Network::Address::InstanceConstSharedPtr(), - Network::Test::createRawBufferSocket(), nullptr); + Utility::resolveUrl(fmt::format("tcp://{}:{}", + Network::Test::getLoopbackAddressUrlString(GetParam()), + socket_->localAddress()->ip()->port())), + Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), + nullptr); client_connection_->addConnectionCallbacks(client_callbacks_); client_connection_->connect(); diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 7a3bf198d52c..866fcfc47bae 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -1,5 +1,7 @@ +#include "envoy/common/platform.h" #include "envoy/config/core/v3/base.pb.h" +#include "common/api/os_sys_calls_impl.h" #include "common/network/io_socket_handle_impl.h" #include "common/network/listen_socket_impl.h" #include "common/network/utility.h" @@ -79,7 +81,7 @@ class ListenSocketImplTest : public testing::TestWithParam { // TODO (conqerAtapple): This is unfortunate. We should be able to templatize this // instead of if block. if (NetworkSocketTrait::type == Address::SocketType::Stream) { - EXPECT_EQ(0, listen(socket1->ioHandle().fd(), 0)); + EXPECT_EQ(0, Api::OsSysCallsSingleton::get().listen(socket1->ioHandle().fd(), 0).rc_); } EXPECT_EQ(addr->ip()->port(), socket1->localAddress()->ip()->port()); @@ -94,11 +96,6 @@ class ListenSocketImplTest : public testing::TestWithParam { // The address and port are bound already, should throw exception. EXPECT_THROW(createListenSocketPtr(addr, options2, true), SocketBindException); - // Test the case of a socket with fd and given address and port. - IoHandlePtr dup_handle = std::make_unique(dup(socket1->ioHandle().fd())); - auto socket3 = createListenSocketPtr(std::move(dup_handle), addr, nullptr); - EXPECT_EQ(addr->asString(), socket3->localAddress()->asString()); - // Test successful. return; } diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 0ca9b283bf8a..04f1c51c8a3f 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -228,9 +228,12 @@ TEST_P(ListenerImplTest, DisableAndEnableListener) { // When listener is disabled, the timer should fire before any connection is accepted. listener.disable(); - ClientConnectionPtr client_connection = - dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), - Network::Test::createRawBufferSocket(), nullptr); + Network::Address::InstanceConstSharedPtr remote = Utility::resolveUrl( + fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(GetParam()), + socket->localAddress()->ip()->port())); + + ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + remote, Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); client_connection->addConnectionCallbacks(connection_callbacks); client_connection->connect(); diff --git a/test/common/network/socket_option_test.h b/test/common/network/socket_option_test.h index 7ad62da85b58..aab277f97858 100644 --- a/test/common/network/socket_option_test.h +++ b/test/common/network/socket_option_test.h @@ -2,6 +2,7 @@ #include "envoy/config/core/v3/base.pb.h" +#include "common/api/os_sys_calls_impl.h" #include "common/network/address_impl.h" #include "common/network/socket_option_impl.h" @@ -13,6 +14,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AnyNumber; using testing::Invoke; using testing::NiceMock; @@ -22,10 +24,43 @@ namespace { class SocketOptionTest : public testing::Test { public: - SocketOptionTest() { socket_.local_address_.reset(); } + SocketOptionTest() { + socket_.local_address_.reset(); + + EXPECT_CALL(os_sys_calls_, socket(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly( + Invoke([this](int domain, int type, int protocol) -> Api::SysCallSocketResult { + return os_sys_calls_actual_.socket(domain, type, protocol); + })); + EXPECT_CALL(os_sys_calls_, setsocketblocking(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, bool block) -> Api::SysCallIntResult { + return os_sys_calls_actual_.setsocketblocking(sockfd, block); + })); + EXPECT_CALL(os_sys_calls_, setsockopt_(_, IPPROTO_IPV6, IPV6_V6ONLY, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, int level, int optname, const void* optval, + socklen_t optlen) -> int { + return os_sys_calls_actual_.setsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls_, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls_, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + } NiceMock socket_; Api::MockOsSysCalls os_sys_calls_; + Api::OsSysCallsImpl os_sys_calls_actual_; TestThreadsafeSingletonInjector os_calls_{[this]() { // Before injecting OsSysCallsImpl, make sure validateIpv{4,6}Supported is called so the static @@ -42,7 +77,7 @@ class SocketOptionTest : public testing::Test { if (option_name.has_value()) { EXPECT_CALL(os_sys_calls_, setsockopt_(_, option_name.level(), option_name.option(), _, sizeof(int))) - .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { + .WillOnce(Invoke([option_val](os_fd_t, int, int, const void* optval, socklen_t) -> int { EXPECT_EQ(option_val, *static_cast(optval)); return 0; })); diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index 53b5f5c07f97..e086cae1f975 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -2,9 +2,11 @@ #include #include +#include "envoy/common/platform.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/stats/scope.h" +#include "common/api/os_sys_calls_impl.h" #include "common/buffer/buffer_impl.h" #include "common/event/dispatcher_impl.h" #include "common/network/connection_balancer_impl.h" @@ -182,6 +184,7 @@ class ProxyProtocolTest : public testing::TestWithParam read_filter_; std::string name_; + Api::OsSysCallsImpl os_sys_calls_actual_; const Network::FilterChainSharedPtr filter_chain_; }; @@ -291,9 +294,8 @@ TEST_P(ProxyProtocolTest, errorRecv_2) { .WillOnce(Return(Api::SysCallSizeResult{-1, 0})); EXPECT_CALL(os_sys_calls, ioctl(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, unsigned long int request, void* argp) { - const int rc = ::ioctl(fd, request, argp); - return Api::SysCallIntResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { + return os_sys_calls_actual_.ioctl(fd, request, argp); })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) @@ -307,6 +309,22 @@ TEST_P(ProxyProtocolTest, errorRecv_2) { const ssize_t rc = ::readv(fd, iov, iovcnt); return Api::SysCallSizeResult{rc, errno}; })); + EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + EXPECT_CALL(os_sys_calls, shutdown(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { const int rc = ::close(fd); return Api::SysCallIntResult{rc, errno}; @@ -339,6 +357,22 @@ TEST_P(ProxyProtocolTest, errorFIONREAD_1) { const ssize_t rc = ::readv(fd, iov, iovcnt); return Api::SysCallSizeResult{rc, errno}; })); + EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + EXPECT_CALL(os_sys_calls, shutdown(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { const int rc = ::close(fd); return Api::SysCallIntResult{rc, errno}; @@ -525,20 +559,19 @@ TEST_P(ProxyProtocolTest, v2ParseExtensionsIoctlError) { EXPECT_CALL(os_sys_calls, ioctl(_, FIONREAD, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, unsigned long int request, void* argp) { - int x = ::ioctl(fd, request, argp); - if (x == 0 && *static_cast(argp) == sizeof(tlv)) { - return Api::SysCallIntResult{-1, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { + const Api::SysCallIntResult x = os_sys_calls_actual_.ioctl(fd, request, argp); + if (x.rc_ == 0 && *static_cast(argp) == sizeof(tlv)) { + return Api::SysCallIntResult{-1, x.errno_}; } else { - return Api::SysCallIntResult{x, errno}; + return x; } })); EXPECT_CALL(os_sys_calls, recv(_, _, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, void* buf, size_t len, int flags) { - const ssize_t rc = ::recv(fd, buf, len, flags); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, void* buf, size_t len, int flags) { + return os_sys_calls_actual_.recv(fd, buf, len, flags); })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) @@ -553,6 +586,22 @@ TEST_P(ProxyProtocolTest, v2ParseExtensionsIoctlError) { const ssize_t rc = ::readv(fd, iov, iovcnt); return Api::SysCallSizeResult{rc, errno}; })); + EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + EXPECT_CALL(os_sys_calls, shutdown(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { const int rc = ::close(fd); return Api::SysCallIntResult{rc, errno}; @@ -659,9 +708,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { EXPECT_CALL(os_sys_calls, recv(_, _, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, void* buf, size_t len, int flags) { - const ssize_t rc = ::recv(fd, buf, len, flags); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, void* buf, size_t len, int flags) { + return os_sys_calls_actual_.recv(fd, buf, len, flags); })); EXPECT_CALL(os_sys_calls, recv(_, _, 1, _)) .Times(AnyNumber()) @@ -669,9 +717,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { EXPECT_CALL(os_sys_calls, ioctl(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, unsigned long int request, void* argp) { - const int rc = ::ioctl(fd, request, argp); - return Api::SysCallIntResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { + return os_sys_calls_actual_.ioctl(fd, request, argp); })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) @@ -685,6 +732,22 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { const ssize_t rc = ::readv(fd, iov, iovcnt); return Api::SysCallSizeResult{rc, errno}; })); + EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + EXPECT_CALL(os_sys_calls, shutdown(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { const int rc = ::close(fd); return Api::SysCallIntResult{rc, errno}; @@ -708,9 +771,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { EXPECT_CALL(os_sys_calls, recv(_, _, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, void* buf, size_t len, int flags) { - const ssize_t rc = ::recv(fd, buf, len, flags); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, void* buf, size_t len, int flags) { + return os_sys_calls_actual_.recv(fd, buf, len, flags); })); EXPECT_CALL(os_sys_calls, recv(_, _, 4, _)) .Times(AnyNumber()) @@ -718,9 +780,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { EXPECT_CALL(os_sys_calls, ioctl(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, unsigned long int request, void* argp) { - const int rc = ::ioctl(fd, request, argp); - return Api::SysCallIntResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { + return os_sys_calls_actual_.ioctl(fd, request, argp); })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) @@ -734,6 +795,22 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { const ssize_t rc = ::readv(fd, iov, iovcnt); return Api::SysCallSizeResult{rc, errno}; })); + EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int level, int optname, void* optval, socklen_t* optlen) -> int { + return os_sys_calls_actual_.getsockopt(sockfd, level, optname, optval, optlen).rc_; + })); + EXPECT_CALL(os_sys_calls, getsockname(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, sockaddr* name, socklen_t* namelen) -> Api::SysCallIntResult { + return os_sys_calls_actual_.getsockname(sockfd, name, namelen); + })); + EXPECT_CALL(os_sys_calls, shutdown(_, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke( + [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { const int rc = ::close(fd); return Api::SysCallIntResult{rc, errno}; diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc index 9555f547b450..6ee3b75484d8 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc @@ -1,5 +1,6 @@ #include +#include "common/api/os_sys_calls_impl.h" #include "common/network/io_socket_handle_impl.h" #include "common/network/listen_socket_impl.h" @@ -43,7 +44,7 @@ class FastMockFileEvent : public Event::FileEvent { class FastMockDispatcher : public Event::MockDispatcher { public: - Event::FileEventPtr createFileEvent(int, Event::FileReadyCb cb, Event::FileTriggerType, + Event::FileEventPtr createFileEvent(os_fd_t, Event::FileReadyCb cb, Event::FileTriggerType, uint32_t) override { file_event_callback_ = cb; return std::make_unique(); @@ -56,7 +57,7 @@ class FastMockOsSysCalls : public Api::MockOsSysCalls { public: FastMockOsSysCalls(const std::vector& client_hello) : client_hello_(client_hello) {} - Api::SysCallSizeResult recv(int, void* buffer, size_t length, int) override { + Api::SysCallSizeResult recv(os_fd_t, void* buffer, size_t length, int) override { RELEASE_ASSERT(length >= client_hello_.size(), ""); memcpy(buffer, client_hello_.data(), client_hello_.size()); return Api::SysCallSizeResult{ssize_t(client_hello_.size()), 0}; diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index be0fae3bf073..53dd0b1d337c 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -99,8 +99,8 @@ TEST_F(TlsInspectorTest, SniRegistered) { const std::string servername("example.com"); std::vector client_hello = Tls::Test::generateClientHello(servername, ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce( - Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + .WillOnce(Invoke( + [&client_hello](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { ASSERT(length >= client_hello.size()); memcpy(buffer, client_hello.data(), client_hello.size()); return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; @@ -122,8 +122,8 @@ TEST_F(TlsInspectorTest, AlpnRegistered) { absl::string_view("http/1.1")}; std::vector client_hello = Tls::Test::generateClientHello("", "\x02h2\x08http/1.1"); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce( - Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + .WillOnce(Invoke( + [&client_hello](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { ASSERT(length >= client_hello.size()); memcpy(buffer, client_hello.data(), client_hello.size()); return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; @@ -181,8 +181,8 @@ TEST_F(TlsInspectorTest, NoExtensions) { init(); std::vector client_hello = Tls::Test::generateClientHello("", ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce( - Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + .WillOnce(Invoke( + [&client_hello](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { ASSERT(length >= client_hello.size()); memcpy(buffer, client_hello.data(), client_hello.size()); return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; @@ -201,17 +201,17 @@ TEST_F(TlsInspectorTest, NoExtensions) { // maximum allowed size. TEST_F(TlsInspectorTest, ClientHelloTooBig) { const size_t max_size = 50; - cfg_ = std::make_shared(store_, max_size); + cfg_ = std::make_shared(store_, static_cast(max_size)); std::vector client_hello = Tls::Test::generateClientHello("example.com", ""); ASSERT(client_hello.size() > max_size); init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce( - Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { - ASSERT(length == max_size); - memcpy(buffer, client_hello.data(), length); - return Api::SysCallSizeResult{ssize_t(length), 0}; - })); + .WillOnce(Invoke([=, &client_hello](os_fd_t, void* buffer, size_t length, + int) -> Api::SysCallSizeResult { + ASSERT(length == max_size); + memcpy(buffer, client_hello.data(), length); + return Api::SysCallSizeResult{ssize_t(length), 0}; + })); EXPECT_CALL(cb_, continueFilterChain(false)); file_event_callback_(Event::FileReadyType::Read); EXPECT_EQ(1, cfg_->stats().client_hello_too_large_.value()); @@ -226,11 +226,12 @@ TEST_F(TlsInspectorTest, NotSsl) { data.resize(100); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce(Invoke([&data](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { - ASSERT(length >= data.size()); - memcpy(buffer, data.data(), data.size()); - return Api::SysCallSizeResult{ssize_t(data.size()), 0}; - })); + .WillOnce( + Invoke([&data](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + ASSERT(length >= data.size()); + memcpy(buffer, data.data(), data.size()); + return Api::SysCallSizeResult{ssize_t(data.size()), 0}; + })); EXPECT_CALL(cb_, continueFilterChain(true)); file_event_callback_(Event::FileReadyType::Read); EXPECT_EQ(1, cfg_->stats().tls_not_found_.value()); diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 1bf6535846f4..8765711f31c2 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -5,6 +5,7 @@ #include "envoy/common/platform.h" +#include "common/api/os_sys_calls_impl.h" #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/network/address_impl.h" @@ -31,29 +32,27 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared // However, because we're going to use the address while checking if it is available, we'll need // to set REUSEADDR on listener sockets created by tests using an address validated by this means. Api::SysCallIntResult result = addr_port->bind(io_handle->fd()); - int err; const char* failing_fn = nullptr; if (result.rc_ != 0) { - err = result.errno_; failing_fn = "bind"; } else if (type == Address::SocketType::Stream) { // Try listening on the port also, if the type is TCP. - if (::listen(io_handle->fd(), 1) != 0) { - err = errno; + result = Api::OsSysCallsSingleton::get().listen(io_handle->fd(), 1); + if (result.rc_ != 0) { failing_fn = "listen"; } } if (failing_fn != nullptr) { - if (err == EADDRINUSE) { + if (result.errno_ == EADDRINUSE) { // The port is already in use. Perfectly normal. return nullptr; - } else if (err == EACCES) { + } else if (result.errno_ == EACCES) { // A privileged port, and we don't have privileges. Might want to log this. return nullptr; } // Unexpected failure. ADD_FAILURE() << failing_fn << " failed for '" << addr_port->asString() - << "' with error: " << strerror(err) << " (" << err << ")"; + << "' with error: " << strerror(result.errno_) << " (" << result.errno_ << ")"; return nullptr; } // If the port we bind is zero, then the OS will pick a free port for us (assuming there are @@ -218,9 +217,9 @@ Api::IoCallUint64Result readFromSocket(IoHandle& handle, const Address::Instance UdpSyncPeer::UdpSyncPeer(Network::Address::IpVersion version) : socket_( std::make_unique(getCanonicalLoopbackAddress(version), nullptr, true)) { - // TODO(mattklein123): Right now all sockets are non-blocking. Move this non-blocking - // modification back to the abstraction layer so it will work for multiple platforms. - RELEASE_ASSERT(fcntl(socket_->ioHandle().fd(), F_SETFL, 0) != -1, ""); + RELEASE_ASSERT( + Api::OsSysCallsSingleton::get().setsocketblocking(socket_->ioHandle().fd(), true).rc_ != -1, + ""); } void UdpSyncPeer::write(const std::string& buffer, const Network::Address::Instance& peer) { diff --git a/test/test_common/network_utility_test.cc b/test/test_common/network_utility_test.cc index 2d467cc97f73..195fc0991b11 100644 --- a/test/test_common/network_utility_test.cc +++ b/test/test_common/network_utility_test.cc @@ -1,5 +1,7 @@ #include +#include "common/api/os_sys_calls_impl.h" + #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" @@ -34,7 +36,7 @@ TEST_P(NetworkUtilityTest, DISABLED_ValidateBindFreeLoopbackPort) { const size_t kLimit = 50; for (size_t n = 0; n < kLimit; ++n) { auto addr_fd = Network::Test::bindFreeLoopbackPort(version_, Address::SocketType::Stream); - close(addr_fd.second); + Api::OsSysCallsSingleton::get().close(addr_fd.second); auto addr = addr_fd.first->asString(); auto search = seen.find(addr); if (search != seen.end()) { From eaaea3358419c8cfe274920e015637591d381fe6 Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Wed, 19 Feb 2020 16:15:51 -0500 Subject: [PATCH 2/9] Fix formatting Signed-off-by: William A Rowe Jr Co-authored-by: Sunjay Bhatia --- .../listener/tls_inspector/tls_inspector_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index 53dd0b1d337c..d33a11d578da 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -206,12 +206,12 @@ TEST_F(TlsInspectorTest, ClientHelloTooBig) { ASSERT(client_hello.size() > max_size); init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) - .WillOnce(Invoke([=, &client_hello](os_fd_t, void* buffer, size_t length, - int) -> Api::SysCallSizeResult { - ASSERT(length == max_size); - memcpy(buffer, client_hello.data(), length); - return Api::SysCallSizeResult{ssize_t(length), 0}; - })); + .WillOnce(Invoke( + [=, &client_hello](os_fd_t, void* buffer, size_t length, int) -> Api::SysCallSizeResult { + ASSERT(length == max_size); + memcpy(buffer, client_hello.data(), length); + return Api::SysCallSizeResult{ssize_t(length), 0}; + })); EXPECT_CALL(cb_, continueFilterChain(false)); file_event_callback_(Event::FileReadyType::Read); EXPECT_EQ(1, cfg_->stats().client_hello_too_large_.value()); From 04f29850d76ee0643d1e842e1248284061c0cf28 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Wed, 19 Feb 2020 16:37:28 -0500 Subject: [PATCH 3/9] proxy protocol test should not use raw syscalls Signed-off-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr --- ...dr_family_aware_socket_option_impl_test.cc | 2 +- .../proxy_protocol/proxy_protocol_test.cc | 75 ++++++++----------- 2 files changed, 31 insertions(+), 46 deletions(-) diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index 4d6524fe8b0d..ce315917b80b 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -15,7 +15,7 @@ class AddrFamilyAwareSocketOptionImplTest : public SocketOptionTest { protected: void SetUp() override { EXPECT_CALL(os_sys_calls_, socket) - .WillRepeatedly(Invoke([&](int domain, int type, int protocol) { + .WillRepeatedly(Invoke([this](int domain, int type, int protocol) { return os_sys_calls_actual_.socket(domain, type, protocol); })); EXPECT_CALL(os_sys_calls_, close(_)).Times(testing::AnyNumber()); diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index e086cae1f975..5d1bff5ed0d4 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -299,15 +299,13 @@ TEST_P(ProxyProtocolTest, errorRecv_2) { })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::writev(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.writev(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, readv(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::readv(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.readv(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) .Times(AnyNumber()) @@ -325,9 +323,8 @@ TEST_P(ProxyProtocolTest, errorRecv_2) { .Times(AnyNumber()) .WillRepeatedly(Invoke( [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); - EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { - const int rc = ::close(fd); - return Api::SysCallIntResult{rc, errno}; + EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([this](os_fd_t fd) { + return os_sys_calls_actual_.close(fd); })); connect(false); write(buffer, sizeof(buffer)); @@ -347,15 +344,13 @@ TEST_P(ProxyProtocolTest, errorFIONREAD_1) { EXPECT_CALL(os_sys_calls, ioctl(_, FIONREAD, _)).WillOnce(Return(Api::SysCallIntResult{-1, 0})); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::writev(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.writev(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, readv(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::readv(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.readv(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) .Times(AnyNumber()) @@ -373,9 +368,8 @@ TEST_P(ProxyProtocolTest, errorFIONREAD_1) { .Times(AnyNumber()) .WillRepeatedly(Invoke( [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); - EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { - const int rc = ::close(fd); - return Api::SysCallIntResult{rc, errno}; + EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([this](os_fd_t fd) { + return os_sys_calls_actual_.close(fd); })); connect(false); write(buffer, sizeof(buffer)); @@ -576,15 +570,13 @@ TEST_P(ProxyProtocolTest, v2ParseExtensionsIoctlError) { EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::writev(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.writev(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, readv(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::readv(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.readv(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) .Times(AnyNumber()) @@ -602,9 +594,8 @@ TEST_P(ProxyProtocolTest, v2ParseExtensionsIoctlError) { .Times(AnyNumber()) .WillRepeatedly(Invoke( [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); - EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { - const int rc = ::close(fd); - return Api::SysCallIntResult{rc, errno}; + EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([this](os_fd_t fd) { + return os_sys_calls_actual_.close(fd); })); connect(false); write(buffer, sizeof(buffer)); @@ -722,15 +713,13 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::writev(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.writev(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, readv(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::readv(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.readv(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) .Times(AnyNumber()) @@ -748,9 +737,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { .Times(AnyNumber()) .WillRepeatedly(Invoke( [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); - EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { - const int rc = ::close(fd); - return Api::SysCallIntResult{rc, errno}; + EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([this](os_fd_t fd) { + return os_sys_calls_actual_.close(fd); })); connect(false); write(buffer, 17); @@ -785,15 +773,13 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::writev(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.writev(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, readv(_, _, _)) .Times(AnyNumber()) - .WillRepeatedly(Invoke([](os_fd_t fd, const iovec* iov, int iovcnt) { - const ssize_t rc = ::readv(fd, iov, iovcnt); - return Api::SysCallSizeResult{rc, errno}; + .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { + return os_sys_calls_actual_.readv(fd, iov, iovcnt); })); EXPECT_CALL(os_sys_calls, getsockopt_(_, _, _, _, _)) .Times(AnyNumber()) @@ -811,9 +797,8 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { .Times(AnyNumber()) .WillRepeatedly(Invoke( [this](os_fd_t sockfd, int how) { return os_sys_calls_actual_.shutdown(sockfd, how); })); - EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([](os_fd_t fd) { - const int rc = ::close(fd); - return Api::SysCallIntResult{rc, errno}; + EXPECT_CALL(os_sys_calls, close(_)).Times(AnyNumber()).WillRepeatedly(Invoke([this](os_fd_t fd) { + return os_sys_calls_actual_.close(fd); })); connect(false); write(buffer, 10); From 1b739150cbb90e5063c50e97b0e03c75afd8a3f9 Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Wed, 26 Feb 2020 10:17:47 -0500 Subject: [PATCH 4/9] Corrections per PR feedback - Relies on localAddress() to return an explicit local if:port as proposed by zuercher - Correct test regressions by introducing appropriate mocks - Drop posix ::dup call by closing and reusing the same if:port for socket in ListenSocketImplTest/testBindSpecificPort Signed-off-by: William A Rowe Jr Co-authored-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr --- source/common/network/address_impl.cc | 27 +++++++---------- test/common/network/connection_impl_test.cc | 13 +++------ .../common/network/listen_socket_impl_test.cc | 16 ++++++++-- test/common/network/listener_impl_test.cc | 9 ++---- .../proxy_protocol/proxy_protocol_test.cc | 29 ++++++++++++++++--- test/server/listener_manager_impl_test.cc | 6 ++-- test/server/listener_manager_impl_test.h | 5 ++++ 7 files changed, 63 insertions(+), 42 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 222f3108e1c2..cc16916b788b 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -425,18 +425,16 @@ PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode) : Instance bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } Api::SysCallIntResult PipeInstance::bind(os_fd_t fd) const { - Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); - if (abstract_namespace_) { - return os_sys_calls.bind(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_); + if (!abstract_namespace_) { + // Try to unlink an existing filesystem object at the requested path. Ignore + // errors -- it's fine if the path doesn't exist, and if it exists but can't + // be unlinked then `::bind()` will generate a reasonable errno. + unlink(address_.sun_path); } - // Try to unlink an existing filesystem object at the requested path. Ignore - // errors -- it's fine if the path doesn't exist, and if it exists but can't - // be unlinked then `::bind()` will generate a reasonable errno. - unlink(address_.sun_path); - auto bind_result = os_sys_calls.bind(fd, sockAddr(), sockAddrLen()); - if (mode != 0 && bind_result.rc_ == 0) { - auto set_permissions = os_sys_calls.chmod(address_.sun_path, mode); + auto& os_syscalls = Api::OsSysCallsSingleton::get(); + auto bind_result = os_syscalls.bind(fd, sockAddr(), sockAddrLen()); + if (mode != 0 && !abstract_namespace_ && bind_result.rc_ == 0) { + auto set_permissions = os_syscalls.chmod(address_.sun_path, mode); if (set_permissions.rc_ != 0) { throw EnvoyException(absl::StrCat("Failed to create socket with mode ", mode)); } @@ -445,12 +443,7 @@ Api::SysCallIntResult PipeInstance::bind(os_fd_t fd) const { } Api::SysCallIntResult PipeInstance::connect(os_fd_t fd) const { - Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); - if (abstract_namespace_) { - return os_sys_calls.connect(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_); - } - return os_sys_calls.connect(fd, reinterpret_cast(&address_), sizeof(address_)); + return Api::OsSysCallsSingleton::get().connect(fd, sockAddr(), sockAddrLen()); } IoHandlePtr PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 19522d188df4..22f4d014111e 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -104,10 +104,8 @@ class ConnectionImplTest : public testing::TestWithParam { nullptr, true); listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true); client_connection_ = dispatcher_->createClientConnection( - Utility::resolveUrl(fmt::format("tcp://{}:{}", - Network::Test::getLoopbackAddressUrlString(GetParam()), - socket_->localAddress()->ip()->port())), - source_address_, Network::Test::createRawBufferSocket(), socket_options_); + socket_->localAddress(), source_address_, Network::Test::createRawBufferSocket(), + socket_options_); client_connection_->addConnectionCallbacks(client_callbacks_); EXPECT_EQ(nullptr, client_connection_->ssl()); const Network::ClientConnection& const_connection = *client_connection_; @@ -1997,11 +1995,8 @@ class ReadBufferLimitTest : public ConnectionImplTest { listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true); client_connection_ = dispatcher_->createClientConnection( - Utility::resolveUrl(fmt::format("tcp://{}:{}", - Network::Test::getLoopbackAddressUrlString(GetParam()), - socket_->localAddress()->ip()->port())), - Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), - nullptr); + socket_->localAddress(), Network::Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); client_connection_->addConnectionCallbacks(client_callbacks_); client_connection_->connect(); diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 866fcfc47bae..f705f9163f91 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -47,7 +47,7 @@ class ListenSocketImplTest : public testing::TestWithParam { auto addr_fd = Network::Test::bindFreeLoopbackPort(version_, Address::SocketType::Stream); auto addr = addr_fd.first; Network::IoHandlePtr& io_handle = addr_fd.second; - EXPECT_LE(0, io_handle->fd()); + EXPECT_TRUE(SOCKET_VALID(io_handle->fd())); // Confirm that we got a reasonable address and port. ASSERT_EQ(Address::Type::Ip, addr->type()); @@ -80,8 +80,9 @@ class ListenSocketImplTest : public testing::TestWithParam { // TODO (conqerAtapple): This is unfortunate. We should be able to templatize this // instead of if block. + auto os_sys_calls = Api::OsSysCallsSingleton::get(); if (NetworkSocketTrait::type == Address::SocketType::Stream) { - EXPECT_EQ(0, Api::OsSysCallsSingleton::get().listen(socket1->ioHandle().fd(), 0).rc_); + EXPECT_EQ(0, os_sys_calls.listen(socket1->ioHandle().fd(), 0).rc_); } EXPECT_EQ(addr->ip()->port(), socket1->localAddress()->ip()->port()); @@ -96,6 +97,17 @@ class ListenSocketImplTest : public testing::TestWithParam { // The address and port are bound already, should throw exception. EXPECT_THROW(createListenSocketPtr(addr, options2, true), SocketBindException); + // Release socket and re-bind it. + socket1->close(); + + // Test createListenSocketPtr from IoHandlePtr's os_fd_t constructor + int domain = version_ == Address::IpVersion::v4 ? AF_INET : AF_INET6; + auto socket_result = os_sys_calls.socket(domain, SOCK_STREAM, 0); + EXPECT_TRUE(SOCKET_VALID(socket_result.rc_)); + io_handle = std::make_unique(socket_result.rc_); + auto socket3 = createListenSocketPtr(std::move(io_handle), addr, nullptr); + EXPECT_EQ(socket3->localAddress()->asString(), addr->asString()); + // Test successful. return; } diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 04f1c51c8a3f..0ca9b283bf8a 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -228,12 +228,9 @@ TEST_P(ListenerImplTest, DisableAndEnableListener) { // When listener is disabled, the timer should fire before any connection is accepted. listener.disable(); - Network::Address::InstanceConstSharedPtr remote = Utility::resolveUrl( - fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(GetParam()), - socket->localAddress()->ip()->port())); - - ClientConnectionPtr client_connection = dispatcher_->createClientConnection( - remote, Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); + ClientConnectionPtr client_connection = + dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); client_connection->addConnectionCallbacks(connection_callbacks); client_connection->connect(); diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index 5d1bff5ed0d4..bc27aa860ece 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -289,6 +289,11 @@ TEST_P(ProxyProtocolTest, errorRecv_2) { 'r', 'e', ' ', 'd', 'a', 't', 'a'}; Api::MockOsSysCalls os_sys_calls; TestThreadsafeSingletonInjector os_calls(&os_sys_calls); + EXPECT_CALL(os_sys_calls, connect(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, const sockaddr* addr, socklen_t addrlen) { + return os_sys_calls_actual_.connect(sockfd, addr, addrlen); + })); EXPECT_CALL(os_sys_calls, recv(_, _, _, _)) .Times(AnyNumber()) .WillOnce(Return(Api::SysCallSizeResult{-1, 0})); @@ -342,6 +347,11 @@ TEST_P(ProxyProtocolTest, errorFIONREAD_1) { Api::MockOsSysCalls os_sys_calls; TestThreadsafeSingletonInjector os_calls(&os_sys_calls); EXPECT_CALL(os_sys_calls, ioctl(_, FIONREAD, _)).WillOnce(Return(Api::SysCallIntResult{-1, 0})); + EXPECT_CALL(os_sys_calls, connect(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, const sockaddr* addr, socklen_t addrlen) { + return os_sys_calls_actual_.connect(sockfd, addr, addrlen); + })); EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { @@ -561,13 +571,16 @@ TEST_P(ProxyProtocolTest, v2ParseExtensionsIoctlError) { return x; } })); - + EXPECT_CALL(os_sys_calls, connect(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, const sockaddr* addr, socklen_t addrlen) { + return os_sys_calls_actual_.connect(sockfd, addr, addrlen); + })); EXPECT_CALL(os_sys_calls, recv(_, _, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](os_fd_t fd, void* buf, size_t len, int flags) { return os_sys_calls_actual_.recv(fd, buf, len, flags); })); - EXPECT_CALL(os_sys_calls, writev(_, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](os_fd_t fd, const iovec* iov, int iovcnt) { @@ -705,7 +718,11 @@ TEST_P(ProxyProtocolTest, v2Fragmented3Error) { EXPECT_CALL(os_sys_calls, recv(_, _, 1, _)) .Times(AnyNumber()) .WillOnce(Return(Api::SysCallSizeResult{-1, 0})); - + EXPECT_CALL(os_sys_calls, connect(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, const sockaddr* addr, socklen_t addrlen) { + return os_sys_calls_actual_.connect(sockfd, addr, addrlen); + })); EXPECT_CALL(os_sys_calls, ioctl(_, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { @@ -765,7 +782,11 @@ TEST_P(ProxyProtocolTest, v2Fragmented4Error) { EXPECT_CALL(os_sys_calls, recv(_, _, 4, _)) .Times(AnyNumber()) .WillOnce(Return(Api::SysCallSizeResult{-1, 0})); - + EXPECT_CALL(os_sys_calls, connect(_, _, _)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, const sockaddr* addr, socklen_t addrlen) { + return os_sys_calls_actual_.connect(sockfd, addr, addrlen); + })); EXPECT_CALL(os_sys_calls, ioctl(_, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke([this](os_fd_t fd, unsigned long int request, void* argp) { diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 5da246dade36..eab1029cd85b 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1107,8 +1107,7 @@ name: foo - filters: [] )EOF"; - Api::OsSysCallsImpl os_syscall; - auto syscall_result = os_syscall.socket(AF_INET, SOCK_STREAM, 0); + auto syscall_result = os_sys_calls_actual_.socket(AF_INET, SOCK_STREAM, 0); ASSERT_TRUE(SOCKET_VALID(syscall_result.rc_)); ListenerHandle* listener_foo = expectListenerCreate(true, true); @@ -1145,8 +1144,7 @@ reuse_port: true - filters: [] )EOF"; - Api::OsSysCallsImpl os_syscall; - auto syscall_result = os_syscall.socket(AF_INET, SOCK_STREAM, 0); + auto syscall_result = os_sys_calls_actual_.socket(AF_INET, SOCK_STREAM, 0); ASSERT_TRUE(SOCKET_VALID(syscall_result.rc_)); ListenerHandle* listener_foo = expectListenerCreate(true, true); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index 68bcc65525fc..772c90c606fa 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -82,6 +82,10 @@ class ListenerManagerImplTest : public testing::Test { local_address_.reset(new Network::Address::Ipv4Instance("127.0.0.1", 1234)); remote_address_.reset(new Network::Address::Ipv4Instance("127.0.0.1", 1234)); EXPECT_CALL(os_sys_calls_, close(_)).WillRepeatedly(Return(Api::SysCallIntResult{0, errno})); + EXPECT_CALL(os_sys_calls_, getsockname) + .WillRepeatedly(Invoke([this](os_fd_t sockfd, sockaddr* addr, socklen_t* addrlen) { + return os_sys_calls_actual_.getsockname(sockfd, addr, addrlen); + })); socket_ = std::make_unique>(); } @@ -219,6 +223,7 @@ class ListenerManagerImplTest : public testing::Test { NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls_{&os_sys_calls_}; + Api::OsSysCallsImpl os_sys_calls_actual_; NiceMock server_; NiceMock listener_factory_; MockWorker* worker_ = new MockWorker(); From 925480c0b766c6a786ec9f33f51e3e2d0ae794c3 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Wed, 26 Feb 2020 16:55:02 -0500 Subject: [PATCH 5/9] Correct asan leak detected Need to keep this apparently duplicate logic due to the scope of the cmsg buffer Signed-off-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr --- source/common/network/io_socket_handle_impl.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index da936757ffcd..6e745ded373b 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -94,9 +94,12 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic message.msg_iov = iov.begin(); message.msg_iovlen = num_slices_to_write; message.msg_flags = 0; + auto& os_syscalls = Api::OsSysCallsSingleton::get(); if (self_ip == nullptr) { message.msg_control = nullptr; message.msg_controllen = 0; + const Api::SysCallSizeResult result = os_syscalls.sendmsg(fd_, &message, flags); + return sysCallResultToIoCallResult(result); } else { const size_t space_v6 = CMSG_SPACE(sizeof(in6_pktinfo)); // FreeBSD only needs in_addr size, but allocates more to unify code in two platforms. @@ -136,8 +139,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic pktinfo->ipi6_ifindex = 0; *(reinterpret_cast(pktinfo->ipi6_addr.s6_addr)) = self_ip->ipv6()->address(); } + const Api::SysCallSizeResult result = os_syscalls.sendmsg(fd_, &message, flags); + return sysCallResultToIoCallResult(result); } - return sysCallResultToIoCallResult(Api::OsSysCallsSingleton::get().sendmsg(fd_, &message, flags)); } Api::IoCallUint64Result From b959d1ac8f6c2e61b50f94165dd2208e8e6afb2d Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Thu, 27 Feb 2020 09:27:51 -0500 Subject: [PATCH 6/9] Revert to sockAddr/sockAddrLen per zuerchr's request Signed-off-by: William A Rowe Jr --- source/common/network/address_impl.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index cc16916b788b..592e1efdaf45 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -255,8 +255,7 @@ Api::SysCallIntResult Ipv4Instance::bind(os_fd_t fd) const { } Api::SysCallIntResult Ipv4Instance::connect(os_fd_t fd) const { - return Api::OsSysCallsSingleton::get().connect( - fd, reinterpret_cast(&ip_.ipv4_.address_), sizeof(ip_.ipv4_.address_)); + return Api::OsSysCallsSingleton::get().connect(fd, sockAddr(), sockAddrLen()); } IoHandlePtr Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } @@ -344,8 +343,7 @@ Api::SysCallIntResult Ipv6Instance::bind(os_fd_t fd) const { } Api::SysCallIntResult Ipv6Instance::connect(os_fd_t fd) const { - return Api::OsSysCallsSingleton::get().connect( - fd, reinterpret_cast(&ip_.ipv6_.address_), sizeof(ip_.ipv6_.address_)); + return Api::OsSysCallsSingleton::get().connect(fd, sockAddr(), sockAddrLen()); } IoHandlePtr Ipv6Instance::socket(SocketType type) const { From 88bc6280f36543eba5a0e4a76f7e963de65e80b4 Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Thu, 27 Feb 2020 13:58:48 -0500 Subject: [PATCH 7/9] Kick CI Signed-off-by: William A Rowe Jr Co-authored-by: Sunjay Bhatia From 0ffe19205b4152861e2ba97a30367f672fd30426 Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia Date: Thu, 27 Feb 2020 13:59:21 -0500 Subject: [PATCH 8/9] Kick CI Signed-off-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr From c2316efa5e5616bdb83d12b302f731b2836412e9 Mon Sep 17 00:00:00 2001 From: William A Rowe Jr Date: Fri, 28 Feb 2020 14:38:19 -0500 Subject: [PATCH 9/9] Remove socket shutdown for now - so we can re-discuss as it changes linux functionality - we can re-evaluate for Windows as well Signed-off-by: William A Rowe Jr Co-authored-by: Sunjay Bhatia --- source/common/network/connection_impl.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 6215a8bf83bc..a79f7506ee84 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -213,9 +213,6 @@ void ConnectionImpl::closeSocket(ConnectionEvent close_type) { file_event_.reset(); - // Shutdown the socket so the other side can read any data in flight. - Api::OsSysCallsSingleton::get().shutdown(ioHandle().fd(), ENVOY_SHUT_WR); - socket_->close(); raiseEvent(close_type);