From 8dba38fa79b54448d8fe74b8338646a78e1bf6e4 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 5 May 2021 16:44:31 +0000 Subject: [PATCH 01/33] refactor Signed-off-by: chaoqin-li1123 --- include/envoy/network/address.h | 1 + source/common/network/BUILD | 2 + source/common/network/address_impl.cc | 253 ++++++++++++++++-- source/common/network/address_impl.h | 108 +++++++- .../common/network/io_socket_handle_impl.cc | 39 +-- source/common/network/tcp_listener_impl.cc | 15 +- source/common/network/utility.cc | 53 +++- source/common/network/utility.h | 7 +- source/common/quic/envoy_quic_utils.cc | 20 +- source/common/upstream/logical_dns_cluster.cc | 9 +- source/common/upstream/logical_host.h | 2 +- source/common/upstream/strict_dns_cluster.cc | 11 +- source/common/upstream/upstream_impl.cc | 2 +- .../clusters/redis/redis_cluster.cc | 2 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 2 +- .../original_src/socket_option_factory.cc | 2 +- test/common/network/address_impl_test.cc | 36 +-- test/common/network/dns_impl_test.cc | 31 +-- test/common/network/listener_impl_test.cc | 6 +- .../quic/envoy_quic_client_session_test.cc | 4 +- .../quic/envoy_quic_client_stream_test.cc | 4 +- test/common/quic/envoy_quic_writer_test.cc | 4 +- .../proxy_protocol/proxy_protocol_test.cc | 2 +- test/integration/base_integration_test.cc | 2 +- test/server/connection_handler_test.cc | 4 +- 25 files changed, 480 insertions(+), 141 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index bd28205bd630..4a305be74694 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -209,6 +209,7 @@ class Instance { }; using InstanceConstSharedPtr = std::shared_ptr; +using InstanceConstPtr = std::unique_ptr; } // namespace Address } // namespace Network diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 4991af1ad899..6ea2da5de64b 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -17,6 +17,8 @@ envoy_cc_library( "//include/envoy/network:address_interface", "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", + "//source/common/common:statusor_lib", + "//source/common/common:thread_lib", "//source/common/common:safe_memcpy_lib", "//source/common/common:utility_lib", ], diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 478299a503ee..38afc5f17c96 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -4,6 +4,7 @@ #include #include +#include "common/common/thread.h" #include "envoy/common/exception.h" #include "envoy/common/platform.h" @@ -19,25 +20,35 @@ namespace Address { namespace { -// Validate that IPv4 is supported on this platform, raise an exception for the -// given address if not. -void validateIpv4Supported(const std::string& address) { - static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET); - if (!supported) { - throw EnvoyException( - fmt::format("IPv4 addresses are not supported on this machine: {}", address)); +class InstanceFactory { + template + StatusOr createInstance(Args&&... args) { + absl::Status status; + status = InstanceType::validateProtocolSupported(); + if (!status.ok()) { + return status; + } + InstanceType instance(status, std::forward(args)...); + if (!status.ok()) { + return status; + } + return instance; } -} -// Validate that IPv6 is supported on this platform, raise an exception for the -// given address if not. -void validateIpv6Supported(const std::string& address) { - static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET6); - if (!supported) { - throw EnvoyException( - fmt::format("IPv6 addresses are not supported on this machine: {}", address)); + template + StatusOr createInstancePtr(Args&&... args) { + absl::Status status; + status = InstanceType::validateProtocolSupported(); + if (!status.ok()) { + return status; + } + std::shared_ptr instance{new InstanceType(status, std::forward(args)...)}; + if (!status.ok()) { + return status; + } + return instance; } -} +}; // Constructs a readable string with the embedded nulls in the abstract path replaced with '@'. std::string friendlyNameFromAbstractPath(absl::string_view path) { @@ -52,7 +63,7 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf } // namespace -Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, socklen_t ss_len, +StatusOr addressFromSockAddr(const sockaddr_storage& ss, socklen_t ss_len, bool v6only) { RELEASE_ASSERT(ss_len == 0 || static_cast(ss_len) >= sizeof(sa_family_t), ""); switch (ss.ss_family) { @@ -60,7 +71,7 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, RELEASE_ASSERT(ss_len == 0 || static_cast(ss_len) == sizeof(sockaddr_in), ""); const struct sockaddr_in* sin = reinterpret_cast(&ss); ASSERT(AF_INET == sin->sin_family); - return std::make_shared(sin); + return Address::InstanceFactory::createInstancePtr(sin); } case AF_INET6: { RELEASE_ASSERT(ss_len == 0 || static_cast(ss_len) == sizeof(sockaddr_in6), ""); @@ -77,9 +88,9 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, #else struct sockaddr_in sin = {AF_INET, sin6->sin6_port, {sin6->sin6_addr.s6_addr32[3]}, {}}; #endif - return std::make_shared(&sin); + return Address::InstanceFactory::createInstancePtr(&sin); } else { - return std::make_shared(*sin6, v6only); + return Address::InstanceFactory::createInstancePtr(*sin6, v6only); } } case AF_UNIX: { @@ -88,16 +99,18 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, RELEASE_ASSERT(ss_len == 0 || static_cast(ss_len) >= offsetof(struct sockaddr_un, sun_path) + 1, ""); - return std::make_shared(sun, ss_len); + return Address::InstanceFactory::createInstancePtr(sun, ss_len); } default: - throw EnvoyException(fmt::format("Unexpected sockaddr family: {}", ss.ss_family)); + return absl::InvalidArgumentError(fmt::format("Unexpected sockaddr family: {}", ss.ss_family)); } NOT_REACHED_GCOVR_EXCL_LINE; } + Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + //ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_ = *address; ip_.friendly_address_ = sockaddrToString(*address); @@ -108,7 +121,9 @@ Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* so friendly_name_.append(ip_.friendly_address_); friendly_name_.push_back(':'); friendly_name_.append(port.data(), port.size()); - validateIpv4Supported(friendly_name_); + if (!validateProtocolSupported().ok()) { + throw EnvoyException("Ipv4 not supported"); + } } Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* sock_interface) @@ -117,6 +132,7 @@ Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* so Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + //ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -126,18 +142,67 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, } friendly_name_ = absl::StrCat(address, ":", port); - validateIpv4Supported(friendly_name_); + if (!validateProtocolSupported().ok()) { + throw EnvoyException("Ipv4 not supported"); + } ip_.friendly_address_ = address; } Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + //ASSERT(Thread::MainThread::isMainThread()); + memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); + ip_.ipv4_.address_.sin_family = AF_INET; + ip_.ipv4_.address_.sin_port = htons(port); + ip_.ipv4_.address_.sin_addr.s_addr = INADDR_ANY; + friendly_name_ = absl::StrCat("0.0.0.0:", port); + if (!validateProtocolSupported().ok()) { + throw EnvoyException("Ipv4 not supported"); + } + ip_.friendly_address_ = "0.0.0.0"; +} + +Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); + ip_.ipv4_.address_ = *address; + ip_.friendly_address_ = sockaddrToString(*address); + + // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. + fmt::format_int port(ntohs(address->sin_port)); + friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); + friendly_name_.append(ip_.friendly_address_); + friendly_name_.push_back(':'); + friendly_name_.append(port.data(), port.size()); +} + + +Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, const SocketInterface* sock_interface) + : Ipv4Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} + +Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, uint32_t port, + const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); + ip_.ipv4_.address_.sin_family = AF_INET; + ip_.ipv4_.address_.sin_port = htons(port); + int rc = inet_pton(AF_INET, address.c_str(), &ip_.ipv4_.address_.sin_addr); + if (1 != rc) { + // Invalid ipv4 address, return. + error = absl::InvalidArgumentError(fmt::format("invalid ipv4 address '{}'", address)); + return; + } + friendly_name_ = absl::StrCat(address, ":", port); + ip_.friendly_address_ = address; +} + +Ipv4Instance::Ipv4Instance(absl::Status&, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); ip_.ipv4_.address_.sin_addr.s_addr = INADDR_ANY; friendly_name_ = absl::StrCat("0.0.0.0:", port); - validateIpv4Supported(friendly_name_); ip_.friendly_address_ = "0.0.0.0"; } @@ -173,6 +238,14 @@ std::string Ipv4Instance::sockaddrToString(const sockaddr_in& addr) { return std::string(start, str + BufferSize - start); } +absl::Status Ipv4Instance::validateProtocolSupported() { + static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET); + if (supported) { + return absl::OkStatus(); + } + return absl::FailedPreconditionError("IPv4 addresses are not supported on this machine"); +} + absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; static_assert(sizeof(absl::uint128) == 16, "The size of asbl::uint128 is not 16."); @@ -194,11 +267,14 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const { Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + //ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_ = address; ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); ip_.ipv6_.v6only_ = v6only; friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - validateIpv6Supported(friendly_name_); + if (!validateProtocolSupported().ok()) { + throw EnvoyException("Ipv6 not supported"); + } } Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* sock_interface) @@ -207,6 +283,7 @@ Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* so Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + //ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_.sin6_family = AF_INET6; ip_.ipv6_.address_.sin6_port = htons(port); if (!address.empty()) { @@ -219,7 +296,9 @@ Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, // Just in case address is in a non-canonical format, format from network address. ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - validateIpv6Supported(friendly_name_); + if (!validateProtocolSupported().ok()) { + throw EnvoyException("Ipv6 not supported"); + } } Ipv6Instance::Ipv6Instance(uint32_t port, const SocketInterface* sock_interface) @@ -231,6 +310,14 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } +absl::Status Ipv6Instance::validateProtocolSupported() { + static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET6); + if (supported) { + return absl::OkStatus(); + } + return absl::FailedPreconditionError("IPv6 addresses are not supported on this machine"); +} + PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { @@ -257,6 +344,40 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t pipe_.mode_ = mode; } +Ipv6Instance::Ipv6Instance(absl::Status&, const sockaddr_in6& address, bool v6only, + const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + ip_.ipv6_.address_ = address; + ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); + ip_.ipv6_.v6only_ = v6only; + friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); +} + +Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, const SocketInterface* sock_interface) + : Ipv6Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} + +Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, uint32_t port, + const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + ip_.ipv6_.address_.sin6_family = AF_INET6; + ip_.ipv6_.address_.sin6_port = htons(port); + if (!address.empty()) { + if (1 != inet_pton(AF_INET6, address.c_str(), &ip_.ipv6_.address_.sin6_addr)) { + // Invalid ipv6 address return. + error = absl::InvalidArgumentError(fmt::format("invalid ipv6 address '{}'", address)); + return; + } + } else { + ip_.ipv6_.address_.sin6_addr = in6addr_any; + } + // Just in case address is in a non-canonical format, format from network address. + ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); + friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); +} + +Ipv6Instance::Ipv6Instance(absl::Status& error, uint32_t port, const SocketInterface* sock_interface) + : Ipv6Instance(error, "", port, sockInterfaceOrDefault(sock_interface)) {} + PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { @@ -300,6 +421,84 @@ PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, pipe_.mode_ = mode; } +PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode, + const SocketInterface* sock_interface) + : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { + if (address->sun_path[0] == '\0') { +#if !defined(__linux__) + error = + absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); + return; +#endif + RELEASE_ASSERT(static_cast(ss_len) >= offsetof(struct sockaddr_un, sun_path) + 1, + ""); + pipe_.abstract_namespace_ = true; + pipe_.address_length_ = ss_len - offsetof(struct sockaddr_un, sun_path); + } + pipe_.address_ = *address; + if (pipe_.abstract_namespace_) { + if (mode != 0) { + error = absl::FailedPreconditionError("Cannot set mode for Abstract AF_UNIX sockets"); + return; + } + // Replace all null characters with '@' in friendly_name_. + friendly_name_ = friendlyNameFromAbstractPath( + absl::string_view(pipe_.address_.sun_path, pipe_.address_length_)); + } else { + friendly_name_ = address->sun_path; + } + pipe_.mode_ = mode; +} + +PipeInstance::PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode, + const SocketInterface* sock_interface) + : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { + if (pipe_path.size() >= sizeof(pipe_.address_.sun_path)) { + error = absl::InvalidArgumentError( + fmt::format("Path \"{}\" exceeds maximum UNIX domain socket path size of {}.", pipe_path, + sizeof(pipe_.address_.sun_path))); + return; + } + memset(&pipe_.address_, 0, sizeof(pipe_.address_)); + pipe_.address_.sun_family = AF_UNIX; + if (pipe_path[0] == '@') { + // This indicates an abstract namespace. + // In this case, null bytes in the name have no special significance, and so we copy all + // characters of pipe_path to sun_path, including null bytes in the name. The pathname must also + // be null terminated. The friendly name is the address path with embedded nulls replaced with + // '@' for consistency with the first character. +#if !defined(__linux__) + error = + absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); + return; +#endif + if (mode != 0) { + error = absl::InvalidArgumentError("Cannot set mode for Abstract AF_UNIX sockets"); + return; + } + pipe_.abstract_namespace_ = true; + pipe_.address_length_ = pipe_path.size(); + // The following statement is safe since pipe_path size was checked at the beginning of this + // function + memcpy(&pipe_.address_.sun_path[0], pipe_path.data(), pipe_path.size()); // NOLINT(safe-memcpy) + pipe_.address_.sun_path[0] = '\0'; + pipe_.address_.sun_path[pipe_path.size()] = '\0'; + friendly_name_ = friendlyNameFromAbstractPath( + absl::string_view(pipe_.address_.sun_path, pipe_.address_length_)); + } else { + // Reports an error if the pipe path has an embedded null character. + if (pipe_path.size() != strlen(pipe_path.c_str())) { + error = absl::InvalidArgumentError( + "UNIX domain socket pathname contains embedded null characters"); + return; + } + StringUtil::strlcpy(&pipe_.address_.sun_path[0], pipe_path.c_str(), + sizeof(pipe_.address_.sun_path)); + friendly_name_ = pipe_.address_.sun_path; + } + pipe_.mode_ = mode; +} + bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } EnvoyInternalInstance::EnvoyInternalInstance(const std::string& address_id, diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 584b5384d140..390d61534458 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -11,6 +11,8 @@ #include "envoy/network/socket.h" #include "common/common/assert.h" +#include "common/common/statusor.h" +#include "common/common/thread.h" namespace Envoy { namespace Network { @@ -25,8 +27,8 @@ namespace Address { * @param v6only disable IPv4-IPv6 mapping for IPv6 addresses? * @return InstanceConstSharedPtr the address. */ -InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, socklen_t len, - bool v6only = true); +StatusOr addressFromSockAddr(const sockaddr_storage& ss, socklen_t len, + bool v6only = true); /** * Base class for all address types. @@ -53,6 +55,37 @@ class InstanceBase : public Instance { const Type type_; }; +class InstanceFactory { +public: + template + static StatusOr createInstancePtr(Args&&... args) { + absl::Status status; + status = InstanceType::validateProtocolSupported(); + if (!status.ok()) { + return status; + } + std::unique_ptr instance(new InstanceType(status, std::forward(args)...)); + if (!status.ok()) { + return status; + } + return instance; + } + + template + static StatusOr createInstance(Args&&... args) { + absl::Status status; + status = InstanceType::validateProtocolSupported(); + if (!status.ok()) { + return status; + } + InstanceType instance(status, std::forward(args)...); + if (!status.ok()) { + return status; + } + return instance; + } +}; + /** * Implementation of an IPv4 address. */ @@ -82,6 +115,31 @@ class Ipv4Instance : public InstanceBase { */ explicit Ipv4Instance(uint32_t port, const SocketInterface* sock_interface = nullptr); + /** + * Construct from an existing unix IPv4 socket address (IP v4 address and port). + */ + explicit Ipv4Instance(absl::Status& error, const sockaddr_in* address, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string IPv4 address such as "1.2.3.4". Port will be unset/0. + */ + explicit Ipv4Instance(absl::Status& error, const std::string& address, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string IPv4 address such as "1.2.3.4" as well as a port. + */ + Ipv4Instance(absl::Status& error, const std::string& address, uint32_t port, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a port. The IPv4 address will be set to "any" and is suitable for binding + * a port to any available address. + */ + explicit Ipv4Instance(absl::Status& error, uint32_t port, + const SocketInterface* sock_interface = nullptr); + // Network::Address::Instance bool operator==(const Instance& rhs) const override; const Ip* ip() const override { return &ip_; } @@ -100,6 +158,10 @@ class Ipv4Instance : public InstanceBase { */ static std::string sockaddrToString(const sockaddr_in& addr); + // Validate that IPv4 is supported on this platform, raise an exception for the + // given address if not. + static absl::Status validateProtocolSupported(); + private: struct Ipv4Helper : public Ipv4 { uint32_t address() const override { return address_.sin_addr.s_addr; } @@ -156,6 +218,31 @@ class Ipv6Instance : public InstanceBase { */ explicit Ipv6Instance(uint32_t port, const SocketInterface* sock_interface = nullptr); + /** + * Construct from an existing unix IPv6 socket address (IP v6 address and port). + */ + Ipv6Instance(absl::Status& error, const sockaddr_in6& address, bool v6only = true, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string IPv6 address such as "12:34::5". Port will be unset/0. + */ + explicit Ipv6Instance(absl::Status& error, const std::string& address, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string IPv6 address such as "12:34::5" as well as a port. + */ + Ipv6Instance(absl::Status& error, const std::string& address, uint32_t port, + const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a port. The IPv6 address will be set to "any" and is suitable for binding + * a port to any available address. + */ + explicit Ipv6Instance(absl::Status& error, uint32_t port, + const SocketInterface* sock_interface = nullptr); + // Network::Address::Instance bool operator==(const Instance& rhs) const override; const Ip* ip() const override { return &ip_; } @@ -166,6 +253,9 @@ class Ipv6Instance : public InstanceBase { } socklen_t sockAddrLen() const override { return sizeof(sockaddr_in6); } + // Validate that IPv6 is supported on this platform + static absl::Status validateProtocolSupported(); + private: struct Ipv6Helper : public Ipv6 { Ipv6Helper() { memset(&address_, 0, sizeof(address_)); } @@ -219,6 +309,20 @@ class PipeInstance : public InstanceBase { explicit PipeInstance(const std::string& pipe_path, mode_t mode = 0, const SocketInterface* sock_interface = nullptr); + /** + * Construct from an existing unix address. + */ + explicit PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, + mode_t mode = 0, const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string pipe path. + */ + explicit PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode = 0, + const SocketInterface* sock_interface = nullptr); + + static absl::Status validateProtocolSupported() { return absl::OkStatus(); } + // Network::Address::Instance bool operator==(const Instance& rhs) const override; const Ip* ip() const override { return nullptr; } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index b8bd7f2ac1c9..627b7dad3137 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -9,6 +9,7 @@ #include "absl/container/fixed_array.h" #include "absl/types/optional.h" +#include "source/common/common/_virtual_includes/statusor_lib/common/common/statusor.h" using Envoy::Api::SysCallIntResult; using Envoy::Api::SysCallSizeResult; @@ -277,22 +278,20 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd) { - // TODO(chaoqin-li1123): remove exception catching and make Address::addressFromSockAddr return - // null on error. - TRY_NEEDS_AUDIT { - // Set v6only to false so that mapped-v6 address can be normalize to v4 - // address. Though dual stack may be disabled, it's still okay to assume the - // address is from a dual stack socket. This is because mapped-v6 address - // must come from a dual stack socket. An actual v6 address can come from - // both dual stack socket and v6 only socket. If |peer_addr| is an actual v6 - // address and the socket is actually v6 only, the returned address will be - // regarded as a v6 address from dual stack socket. However, this address is not going to be - // used to create socket. Wrong knowledge of dual stack support won't hurt. - return Address::addressFromSockAddr(ss, ss_len, /*v6only=*/false); - } - catch (const EnvoyException& e) { - PANIC(fmt::format("Invalid address for fd: {}, error: {}", fd, e.what())); - } + // Set v6only to false so that mapped-v6 address can be normalize to v4 + // address. Though dual stack may be disabled, it's still okay to assume the + // address is from a dual stack socket. This is because mapped-v6 address + // must come from a dual stack socket. An actual v6 address can come from + // both dual stack socket and v6 only socket. If |peer_addr| is an actual v6 + // address and the socket is actually v6 only, the returned address will be + // regarded as a v6 address from dual stack socket. However, this address is not going to be + // used to create socket. Wrong knowledge of dual stack support won't hurt. + StatusOr error_or_addr = + Address::addressFromSockAddr(ss, ss_len, /*v6only=*/false); + if (!error_or_addr.ok()) { + PANIC(fmt::format("Invalid address for fd: {}", fd)); + } + return *error_or_addr; } Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg, @@ -605,7 +604,9 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_, errorDetails(result.errno_))); } - return Address::addressFromSockAddr(ss, ss_len, socket_v6only_); + StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len, socket_v6only_); + ASSERT(error_or_address.ok()); + return *error_or_address; } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { @@ -630,7 +631,9 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - return Address::addressFromSockAddr(ss, ss_len); + StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len); + ASSERT(error_or_address.ok()); + return *error_or_address; } void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index 38c77db31e44..d2dc464e125c 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -82,12 +82,15 @@ void TcpListenerImpl::onSocketEvent(short flags) { // Pass the 'v6only' parameter as true if the local_address is an IPv6 address. This has no // effect if the socket is a v4 socket, but for v6 sockets this will create an IPv4 remote // address if an IPv4 local_address was created from an IPv6 mapped IPv4 address. - const Address::InstanceConstSharedPtr& remote_address = - (remote_addr.ss_family == AF_UNIX) - ? io_handle->peerAddress() - : Address::addressFromSockAddr(remote_addr, remote_addr_len, - local_address->ip()->version() == - Address::IpVersion::v6); + Address::InstanceConstSharedPtr remote_address; + if (remote_addr.ss_family == AF_UNIX) { + remote_address = io_handle->peerAddress(); + } else { + StatusOr error_or_address = Address::addressFromSockAddr( + remote_addr, remote_addr_len, local_address->ip()->version() == Address::IpVersion::v6); + ASSERT(error_or_address.ok()); + remote_address = *error_or_address; + } cb_.onAccept( std::make_unique(std::move(io_handle), local_address, remote_address)); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index d30b5770b63e..b9df458b45b8 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -11,6 +11,7 @@ #include "envoy/common/exception.h" #include "envoy/common/platform.h" #include "envoy/config/core/v3/address.pb.h" +#include "envoy/network/address.h" #include "envoy/network/connection.h" #include "common/api/os_sys_calls_impl.h" @@ -27,6 +28,7 @@ #include "absl/container/fixed_array.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "source/common/common/_virtual_includes/statusor_lib/common/common/statusor.h" namespace Envoy { namespace Network { @@ -128,18 +130,25 @@ uint32_t Utility::portFromUdpUrl(const std::string& url) { Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std::string& ip_address, uint16_t port, bool v6only) { + Address::InstanceConstSharedPtr address; sockaddr_in sa4; if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) { sa4.sin_family = AF_INET; sa4.sin_port = htons(port); - return std::make_shared(&sa4); + StatusOr error_or_address = + Address::InstanceFactory::createInstancePtr(&sa4); + ASSERT(error_or_address.ok()); + return *error_or_address; } sockaddr_in6 sa6; memset(&sa6, 0, sizeof(sa6)); if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) { sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port); - return std::make_shared(sa6, v6only); + StatusOr error_or_address = + Address::InstanceFactory::createInstancePtr(sa6, v6only); + ASSERT(error_or_address.ok()); + return *error_or_address; } return nullptr; } @@ -178,7 +187,10 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port64); - return std::make_shared(sa6, v6only); + StatusOr error_or_address = + Address::InstanceFactory::createInstancePtr(sa6, v6only); + ASSERT(error_or_address.ok()); + return *error_or_address; } // Treat it as an IPv4 address followed by a port. const auto pos = ip_address.rfind(':'); @@ -198,7 +210,10 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa4.sin_family = AF_INET; sa4.sin_port = htons(port64); - return std::make_shared(&sa4); + StatusOr error_or_address = + Address::InstanceFactory::createInstancePtr(&sa4); + ASSERT(error_or_address.ok()); + return *error_or_address; } Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address, @@ -246,8 +261,10 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio (ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) { const struct sockaddr_storage* addr = reinterpret_cast(ifa->ifa_addr); - ret = Address::addressFromSockAddr( + StatusOr error_or_instance = Address::addressFromSockAddr( *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); + ASSERT(error_or_instance.ok()); + ret = *error_or_instance; if (!isLoopbackAddress(*ret)) { break; } @@ -366,17 +383,28 @@ const std::string& Utility::getIpv6CidrCatchAllAddress() { CONSTRUCT_ON_FIRST_USE(std::string, "::/0"); } -Address::InstanceConstSharedPtr Utility::getAddressWithPort(const Address::Instance& address, - uint32_t port) { +StatusOr +Utility::getAddressWithPort(const Address::Instance& address, uint32_t port) { switch (address.ip()->version()) { case Address::IpVersion::v4: - return std::make_shared(address.ip()->addressAsString(), port); + return Address::InstanceFactory::createInstancePtr( + address.ip()->addressAsString(), port); case Address::IpVersion::v6: - return std::make_shared(address.ip()->addressAsString(), port); + return Address::InstanceFactory::createInstancePtr( + address.ip()->addressAsString(), port); } NOT_REACHED_GCOVR_EXCL_LINE; } +Address::InstanceConstSharedPtr Utility::getAddressWithPortOrThrow(const Address::Instance& address, + uint32_t port) { + StatusOr error_or_address = getAddressWithPort(address, port); + if (!error_or_address.ok()) { + throw EnvoyException(error_or_address.status().ToString()); + } + return *error_or_address; +} + Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) { #ifdef SOL_IP @@ -404,7 +432,12 @@ Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) { return nullptr; } - return Address::addressFromSockAddr(orig_addr, 0, true /* default for v6 constructor */); + StatusOr error_or_address = + Address::addressFromSockAddr(orig_addr, 0, true /* default for v6 constructor */); + if (!error_or_address.ok()) { + throw EnvoyException(error_or_address.status().ToString()); + } + return *error_or_address; #else // TODO(zuercher): determine if connection redirection is possible under macOS (c.f. pfctl and // divert), and whether it's possible to find the learn destination address. diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 2e58c1d9606b..2ef610bb2542 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -8,6 +8,7 @@ #include "envoy/config/core/v3/address.pb.h" #include "envoy/network/connection.h" #include "envoy/network/listener.h" +#include "common/common/statusor.h" #include "absl/strings/string_view.h" @@ -264,14 +265,18 @@ class Utility { */ static const std::string& getIpv6CidrCatchAllAddress(); + static StatusOr getAddressWithPort(const Address::Instance& address, + uint32_t port); + /** * @param address IP address instance. * @param port to update. * @return Address::InstanceConstSharedPtr a new address instance with updated port. */ - static Address::InstanceConstSharedPtr getAddressWithPort(const Address::Instance& address, + static Address::InstanceConstSharedPtr getAddressWithPortOrThrow(const Address::Instance& address, uint32_t port); + /** * Retrieve the original destination address from an accepted socket. * The address (IP and port) may be not local and the port may differ from diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 8f9460db3e6c..f93703101b18 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -17,14 +17,18 @@ namespace Quic { // the heap allocation is too expensive. Network::Address::InstanceConstSharedPtr quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { - return quic_address.IsInitialized() - ? Network::Address::addressFromSockAddr(quic_address.generic_address(), - quic_address.host().address_family() == - quic::IpAddressFamily::IP_V4 - ? sizeof(sockaddr_in) - : sizeof(sockaddr_in6), - false) - : nullptr; + if (!quic_address.IsInitialized()) { + return nullptr; + } + StatusOr error_or_instance = + Network::Address::addressFromSockAddr(quic_address.generic_address(), + quic_address.host().address_family() == + quic::IpAddressFamily::IP_V4 + ? sizeof(sockaddr_in) + : sizeof(sockaddr_in6), + false); + ASSERT(error_or_instance.ok()); + return *error_or_instance; } quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip) { diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index c0a3433f49f9..e9e951492d6a 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -10,6 +10,7 @@ #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/config/core/v3/address.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" +#include "envoy/network/address.h" #include "envoy/stats/scope.h" #include "common/common/fmt.h" @@ -121,10 +122,14 @@ void LogicalDnsCluster::startResolve() { info_->stats().update_success_.inc(); // TODO(mattklein123): Move port handling into the DNS interface. ASSERT(response.front().address_ != nullptr); - Network::Address::InstanceConstSharedPtr new_address = + StatusOr error_or_address = Network::Utility::getAddressWithPort(*(response.front().address_), Network::Utility::portFromTcpUrl(dns_url_)); - + if (!error_or_address.ok()) { + ENVOY_LOG(debug, "Fail to create new address."); + return; + } + Network::Address::InstanceConstSharedPtr new_address = *error_or_address; if (!logical_host_) { logical_host_ = std::make_shared(info_, hostname_, new_address, localityLbEndpoint(), diff --git a/source/common/upstream/logical_host.h b/source/common/upstream/logical_host.h index ae2efb3ebb45..f6ccc9abe481 100644 --- a/source/common/upstream/logical_host.h +++ b/source/common/upstream/logical_host.h @@ -37,7 +37,7 @@ class LogicalHost : public HostImpl { const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint) { const auto& port_value = lb_endpoint.endpoint().health_check_config().port_value(); auto health_check_address = - port_value == 0 ? address : Network::Utility::getAddressWithPort(*address, port_value); + port_value == 0 ? address : Network::Utility::getAddressWithPortOrThrow(*address, port_value); absl::WriterMutexLock lock(&address_lock_); setAddress(address); diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc index 726070b6389b..54706b293f67 100644 --- a/source/common/upstream/strict_dns_cluster.cc +++ b/source/common/upstream/strict_dns_cluster.cc @@ -129,10 +129,19 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // make a new address that has port in it. We need to both support IPv6 as well as // potentially move port handling into the DNS interface itself, which would work better // for SRV. + ASSERT(resp.address_ != nullptr); + StatusOr error_or_address = + Network::Utility::getAddressWithPort(*(resp.address_), + port_); + if (!error_or_address.ok()) { + ENVOY_LOG(debug, "Fail to create new address."); + return; + } + Network::Address::InstanceConstSharedPtr new_address = *error_or_address; new_hosts.emplace_back(new HostImpl( parent_.info_, hostname_, - Network::Utility::getAddressWithPort(*(resp.address_), port_), + new_address, // TODO(zyfjeff): Created through metadata shared pool std::make_shared(lb_endpoint_.metadata()), lb_endpoint_.load_balancing_weight().value(), locality_lb_endpoints_.locality(), diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 07db4f18f2ff..55b9fa7976c3 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -267,7 +267,7 @@ HostDescriptionImpl::HostDescriptionImpl( health_check_address_ = health_check_config.port_value() == 0 ? dest_address - : Network::Utility::getAddressWithPort(*dest_address, health_check_config.port_value()); + : Network::Utility::getAddressWithPortOrThrow(*dest_address, health_check_config.port_value()); } Network::TransportSocketFactory& HostDescriptionImpl::resolveTransportSocketFactory( diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 297248a6b6a1..4871ce7085a6 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -261,7 +261,7 @@ void RedisCluster::RedisDiscoverySession::registerDiscoveryAddress( // port in it. for (const Network::DnsResponse& res : response) { ASSERT(res.address_ != nullptr); - discovery_address_list_.push_back(Network::Utility::getAddressWithPort(*(res.address_), port)); + discovery_address_list_.push_back(Network::Utility::getAddressWithPortOrThrow(*(res.address_), port)); } } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index c2f6cd82a858..443c2cee548c 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -243,7 +243,7 @@ void DnsCacheImpl::finishResolve(const std::string& host, // update. This ensures that a potentially previously resolved address does not stabilize back to // 0 hosts. const auto new_address = !response.empty() - ? Network::Utility::getAddressWithPort(*(response.front().address_), + ? Network::Utility::getAddressWithPortOrThrow(*(response.front().address_), primary_host_info->port_) : nullptr; diff --git a/source/extensions/filters/common/original_src/socket_option_factory.cc b/source/extensions/filters/common/original_src/socket_option_factory.cc index cf84b9d26596..907a13d7835a 100644 --- a/source/extensions/filters/common/original_src/socket_option_factory.cc +++ b/source/extensions/filters/common/original_src/socket_option_factory.cc @@ -13,7 +13,7 @@ namespace OriginalSrc { Network::Socket::OptionsSharedPtr buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_t mark) { - const auto address_without_port = Network::Utility::getAddressWithPort(*source, 0); + const auto address_without_port = Network::Utility::getAddressWithPortOrThrow(*source, 0); // Note: we don't expect this to change the behaviour of the socket. We expect it to be copied // into the upstream connection later. diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 5284464bb165..8e87f8d7284d 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -466,15 +466,15 @@ TEST(AddressFromSockAddrDeathTest, IPv4) { EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &sin.sin_addr)); sin.sin_port = htons(6502); - EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len"); - EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in) - 1), "ss_len"); - EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in) + 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, sizeof(sockaddr_in) - 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, sizeof(sockaddr_in) + 1), "ss_len"); - EXPECT_EQ("1.2.3.4:6502", addressFromSockAddr(ss, sizeof(sockaddr_in))->asString()); + EXPECT_EQ("1.2.3.4:6502", (*addressFromSockAddr(ss, sizeof(sockaddr_in)))->asString()); // Invalid family. sin.sin_family = AF_UNSPEC; - EXPECT_THROW(addressFromSockAddr(ss, sizeof(sockaddr_in)), EnvoyException); + EXPECT_FALSE(addressFromSockAddr(ss, sizeof(sockaddr_in)).ok()); } TEST(AddressFromSockAddrDeathTest, IPv6) { @@ -485,20 +485,20 @@ TEST(AddressFromSockAddrDeathTest, IPv6) { EXPECT_EQ(1, inet_pton(AF_INET6, "01:023::00Ef", &sin6.sin6_addr)); sin6.sin6_port = htons(32000); - EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len"); - EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in6) - 1), "ss_len"); - EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in6) + 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, sizeof(sockaddr_in6) - 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, sizeof(sockaddr_in6) + 1), "ss_len"); - EXPECT_EQ("[1:23::ef]:32000", addressFromSockAddr(ss, sizeof(sockaddr_in6))->asString()); + EXPECT_EQ("[1:23::ef]:32000", (*addressFromSockAddr(ss, sizeof(sockaddr_in6)))->asString()); // Test that IPv4-mapped IPv6 address is returned as an Ipv4Instance when 'v6only' parameter is // 'false', but not otherwise. EXPECT_EQ(1, inet_pton(AF_INET6, "::ffff:192.0.2.128", &sin6.sin6_addr)); - EXPECT_EQ(IpVersion::v4, addressFromSockAddr(ss, sizeof(sockaddr_in6), false)->ip()->version()); - EXPECT_EQ("192.0.2.128:32000", addressFromSockAddr(ss, sizeof(sockaddr_in6), false)->asString()); - EXPECT_EQ(IpVersion::v6, addressFromSockAddr(ss, sizeof(sockaddr_in6), true)->ip()->version()); + EXPECT_EQ(IpVersion::v4, (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->ip()->version()); + EXPECT_EQ("192.0.2.128:32000", (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->asString()); + EXPECT_EQ(IpVersion::v6, (*addressFromSockAddr(ss, sizeof(sockaddr_in6), true))->ip()->version()); EXPECT_EQ("[::ffff:192.0.2.128]:32000", - addressFromSockAddr(ss, sizeof(sockaddr_in6), true)->asString()); + (*addressFromSockAddr(ss, sizeof(sockaddr_in6), true))->asString()); } TEST(AddressFromSockAddrDeathTest, Pipe) { @@ -508,20 +508,20 @@ TEST(AddressFromSockAddrDeathTest, Pipe) { StringUtil::strlcpy(sun.sun_path, "/some/path", sizeof sun.sun_path); - EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len"); - EXPECT_DEATH(addressFromSockAddr(ss, offsetof(struct sockaddr_un, sun_path)), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, 1), "ss_len"); + EXPECT_DEATH(*addressFromSockAddr(ss, offsetof(struct sockaddr_un, sun_path)), "ss_len"); socklen_t ss_len = offsetof(struct sockaddr_un, sun_path) + 1 + strlen(sun.sun_path); - EXPECT_EQ("/some/path", addressFromSockAddr(ss, ss_len)->asString()); + EXPECT_EQ("/some/path", (*addressFromSockAddr(ss, ss_len))->asString()); // Abstract socket namespace. StringUtil::strlcpy(&sun.sun_path[1], "/some/abstract/path", sizeof sun.sun_path); sun.sun_path[0] = '\0'; ss_len = offsetof(struct sockaddr_un, sun_path) + 1 + strlen("/some/abstract/path"); #if defined(__linux__) - EXPECT_EQ("@/some/abstract/path", addressFromSockAddr(ss, ss_len)->asString()); + EXPECT_EQ("@/some/abstract/path", (*addressFromSockAddr(ss, ss_len))->asString()); #else - EXPECT_THROW(addressFromSockAddr(ss, ss_len), EnvoyException); + EXPECT_FALSE(addressFromSockAddr(ss, ss_len).ok()); #endif } diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 47476fa76a32..44314bff395a 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -531,18 +531,6 @@ class DnsImplTest : public testing::TestWithParam { }); } - template - ActiveDnsQuery* resolveWithException(const std::string& address, - const DnsLookupFamily lookup_family, T exception_object) { - return resolver_->resolve(address, lookup_family, - [exception_object](DnsResolver::ResolutionStatus status, - std::list&& results) -> void { - UNREFERENCED_PARAMETER(status); - UNREFERENCED_PARAMETER(results); - throw exception_object; - }); - } - protected: // Should the DnsResolverImpl use a zero timeout for c-ares queries? virtual bool zeroTimeout() const { return false; } @@ -660,24 +648,6 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { dispatcher_->run(Event::Dispatcher::RunType::Block); } -// Validate exception behavior during c-ares callbacks. -TEST_P(DnsImplTest, CallbackException) { - // Force immediate resolution, which will trigger a c-ares exception unsafe - // state providing regression coverage for #4307. - EXPECT_EQ(nullptr, resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, - EnvoyException("Envoy exception"))); - EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, - "Envoy exception"); - EXPECT_EQ(nullptr, resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, - std::runtime_error("runtime error"))); - EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, - "runtime error"); - EXPECT_EQ(nullptr, - resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, std::string())); - EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, - "unknown"); -} - // Validate that the c-ares channel is destroyed and re-initialized when c-ares returns // ARES_ECONNREFUSED as its callback status. TEST_P(DnsImplTest, DestroyChannelOnRefused) { @@ -697,6 +667,7 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) { // If that flag needs to be set, or c-ares changes its handling this test will need to be updated // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == // ARES_ECONNREFUSED. + EXPECT_TRUE(peer_->isChannelDirty()); server_->setRefused(false); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index cea2b1ed13f0..b8b9feb66388 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -227,7 +227,7 @@ TEST_P(TcpListenerImplTest, WildcardListenerUseActualDst) { listener_callbacks, true); auto local_dst_address = - Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + Network::Utility::getAddressWithPortOrThrow(*Network::Test::getCanonicalLoopbackAddress(version_), socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), @@ -271,10 +271,10 @@ TEST_P(TcpListenerImplTest, WildcardListenerIpv4Compat) { listener_callbacks, true); auto listener_address = - Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + Network::Utility::getAddressWithPortOrThrow(*Network::Test::getCanonicalLoopbackAddress(version_), socket->addressProvider().localAddress()->ip()->port()); auto local_dst_address = - Network::Utility::getAddressWithPort(*Network::Utility::getCanonicalIpv4LoopbackAddress(), + Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getCanonicalIpv4LoopbackAddress(), socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index 8617d8953e83..f1efdf572184 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -102,9 +102,9 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::ParsedVersionOfIndex(quic::CurrentSupportedVersions(), 0); }()), - peer_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + peer_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), 12345)), - self_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + self_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), 54321)), quic_connection_(new TestEnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_, diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 3cda1218570a..98d67d183354 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -28,9 +28,9 @@ class EnvoyQuicClientStreamTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::CurrentSupportedVersions()[0]; }()), - peer_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + peer_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), 12345)), - self_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + self_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), 54321)), quic_connection_(new EnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, &writer_, diff --git a/test/common/quic/envoy_quic_writer_test.cc b/test/common/quic/envoy_quic_writer_test.cc index 87c345962c33..ae784ed06fee 100644 --- a/test/common/quic/envoy_quic_writer_test.cc +++ b/test/common/quic/envoy_quic_writer_test.cc @@ -33,10 +33,10 @@ class EnvoyQuicWriterTest : public ::testing::Test { } void verifySendData(const std::string& content, const msghdr* message) { - EXPECT_EQ(peer_address_.ToString(), Network::Address::addressFromSockAddr( + EXPECT_EQ(peer_address_.ToString(), (*Network::Address::addressFromSockAddr( *reinterpret_cast(message->msg_name), message->msg_namelen, /*v6only=*/false) - ->asString()); + )->asString()); cmsghdr* const cmsg = CMSG_FIRSTHDR(message); auto pktinfo = reinterpret_cast(CMSG_DATA(cmsg)); EXPECT_EQ(0, memcmp(self_address_.GetIPv6().s6_addr, pktinfo->ipi6_addr.s6_addr, 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 2ab7186e8b16..1c92e9c07094 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -1284,7 +1284,7 @@ class WildcardProxyProtocolTest : public testing::TestWithParamallocateDispatcher("test_thread")), socket_(std::make_shared(Network::Test::getAnyAddress(GetParam()), nullptr, true)), - local_dst_address_(Network::Utility::getAddressWithPort( + local_dst_address_(Network::Utility::getAddressWithPortOrThrow( *Network::Test::getCanonicalLoopbackAddress(GetParam()), socket_->addressProvider().localAddress()->ip()->port())), connection_handler_(new Server::ConnectionHandlerImpl(*dispatcher_, absl::nullopt)), diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index cf0b1d193aaf..b85bcde5a1dd 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -556,7 +556,7 @@ AssertionResult BaseIntegrationTest::waitForPortAvailable(uint32_t port, Event::TestTimeSystem::RealTimeBound bound(timeout); while (bound.withinBound()) { try { - Network::TcpListenSocket(Network::Utility::getAddressWithPort( + Network::TcpListenSocket(Network::Utility::getAddressWithPortOrThrow( *Network::Test::getCanonicalLoopbackAddress(version_), port), nullptr, true); return AssertionSuccess(); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 1e9a174fee06..d207c9f53757 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -261,7 +261,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggableip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener); @@ -772,7 +772,7 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); - Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPort( + Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPortOrThrow( *Network::Utility::getIpv4AnyAddress(), normal_address->ip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener1); From 6f8c113f4804a08521b3cffd248e782c597021ce Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 5 May 2021 20:15:33 +0000 Subject: [PATCH 02/33] refactor Signed-off-by: chaoqin-li1123 --- source/common/network/BUILD | 2 +- source/common/network/address_impl.cc | 40 +++---- source/common/network/address_impl.h | 100 +++++++++--------- .../common/network/io_socket_handle_impl.cc | 7 +- source/common/network/utility.cc | 3 +- source/common/network/utility.h | 8 +- source/common/upstream/logical_host.h | 3 +- source/common/upstream/strict_dns_cluster.cc | 18 ++-- source/common/upstream/upstream_impl.cc | 8 +- .../clusters/redis/redis_cluster.cc | 3 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 6 +- test/common/network/address_impl_test.cc | 6 +- test/common/network/dns_impl_test.cc | 2 +- test/common/network/listener_impl_test.cc | 18 ++-- .../quic/envoy_quic_client_session_test.cc | 8 +- .../quic/envoy_quic_client_stream_test.cc | 8 +- test/common/quic/envoy_quic_writer_test.cc | 9 +- test/server/connection_handler_test.cc | 10 +- 18 files changed, 133 insertions(+), 126 deletions(-) diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 6ea2da5de64b..f74ab0f256aa 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -17,9 +17,9 @@ envoy_cc_library( "//include/envoy/network:address_interface", "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", + "//source/common/common:safe_memcpy_lib", "//source/common/common:statusor_lib", "//source/common/common:thread_lib", - "//source/common/common:safe_memcpy_lib", "//source/common/common:utility_lib", ], ) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 38afc5f17c96..94d9fdc6e322 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -4,13 +4,13 @@ #include #include -#include "common/common/thread.h" #include "envoy/common/exception.h" #include "envoy/common/platform.h" #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/safe_memcpy.h" +#include "common/common/thread.h" #include "common/common/utility.h" #include "common/network/socket_interface.h" @@ -63,8 +63,8 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf } // namespace -StatusOr addressFromSockAddr(const sockaddr_storage& ss, socklen_t ss_len, - bool v6only) { +StatusOr addressFromSockAddr(const sockaddr_storage& ss, + socklen_t ss_len, bool v6only) { RELEASE_ASSERT(ss_len == 0 || static_cast(ss_len) >= sizeof(sa_family_t), ""); switch (ss.ss_family) { case AF_INET: { @@ -107,10 +107,9 @@ StatusOr addressFromSockAddr(const sockaddr_sto NOT_REACHED_GCOVR_EXCL_LINE; } - Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - //ASSERT(Thread::MainThread::isMainThread()); + // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_ = *address; ip_.friendly_address_ = sockaddrToString(*address); @@ -132,7 +131,7 @@ Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* so Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - //ASSERT(Thread::MainThread::isMainThread()); + // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -150,7 +149,7 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - //ASSERT(Thread::MainThread::isMainThread()); + // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -162,7 +161,8 @@ Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) ip_.friendly_address_ = "0.0.0.0"; } -Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, const SocketInterface* sock_interface) +Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, + const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_ = *address; @@ -176,8 +176,8 @@ Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, const Sock friendly_name_.append(port.data(), port.size()); } - -Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, const SocketInterface* sock_interface) +Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, + const SocketInterface* sock_interface) : Ipv4Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, uint32_t port, @@ -267,7 +267,7 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const { Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - //ASSERT(Thread::MainThread::isMainThread()); + // ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_ = address; ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); ip_.ipv6_.v6only_ = v6only; @@ -283,7 +283,7 @@ Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* so Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - //ASSERT(Thread::MainThread::isMainThread()); + // ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_.sin6_family = AF_INET6; ip_.ipv6_.address_.sin6_port = htons(port); if (!address.empty()) { @@ -353,7 +353,8 @@ Ipv6Instance::Ipv6Instance(absl::Status&, const sockaddr_in6& address, bool v6on friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); } -Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, const SocketInterface* sock_interface) +Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, + const SocketInterface* sock_interface) : Ipv6Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, uint32_t port, @@ -375,7 +376,8 @@ Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, uint friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); } -Ipv6Instance::Ipv6Instance(absl::Status& error, uint32_t port, const SocketInterface* sock_interface) +Ipv6Instance::Ipv6Instance(absl::Status& error, uint32_t port, + const SocketInterface* sock_interface) : Ipv6Instance(error, "", port, sockInterfaceOrDefault(sock_interface)) {} PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, @@ -421,13 +423,12 @@ PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, pipe_.mode_ = mode; } -PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode, - const SocketInterface* sock_interface) +PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, + mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { if (address->sun_path[0] == '\0') { #if !defined(__linux__) - error = - absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); + error = absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); return; #endif RELEASE_ASSERT(static_cast(ss_len) >= offsetof(struct sockaddr_un, sun_path) + 1, @@ -468,8 +469,7 @@ PipeInstance::PipeInstance(absl::Status& error, const std::string& pipe_path, mo // be null terminated. The friendly name is the address path with embedded nulls replaced with // '@' for consistency with the first character. #if !defined(__linux__) - error = - absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); + error = absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); return; #endif if (mode != 0) { diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 390d61534458..1ac38586786f 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -115,6 +115,29 @@ class Ipv4Instance : public InstanceBase { */ explicit Ipv4Instance(uint32_t port, const SocketInterface* sock_interface = nullptr); + // Network::Address::Instance + bool operator==(const Instance& rhs) const override; + const Ip* ip() const override { return &ip_; } + const Pipe* pipe() const override { return nullptr; } + const EnvoyInternalAddress* envoyInternalAddress() const override { return nullptr; } + const sockaddr* sockAddr() const override { + return reinterpret_cast(&ip_.ipv4_.address_); + } + socklen_t sockAddrLen() const override { return sizeof(sockaddr_in); } + + /** + * Convenience function to convert an IPv4 address to canonical string format. + * @note This works similarly to inet_ntop() but is faster. + * @param addr address to format. + * @return the address in dotted-decimal string format. + */ + static std::string sockaddrToString(const sockaddr_in& addr); + + // Validate that IPv4 is supported on this platform, raise an exception for the + // given address if not. + static absl::Status validateProtocolSupported(); + +private: /** * Construct from an existing unix IPv4 socket address (IP v4 address and port). */ @@ -140,29 +163,6 @@ class Ipv4Instance : public InstanceBase { explicit Ipv4Instance(absl::Status& error, uint32_t port, const SocketInterface* sock_interface = nullptr); - // Network::Address::Instance - bool operator==(const Instance& rhs) const override; - const Ip* ip() const override { return &ip_; } - const Pipe* pipe() const override { return nullptr; } - const EnvoyInternalAddress* envoyInternalAddress() const override { return nullptr; } - const sockaddr* sockAddr() const override { - return reinterpret_cast(&ip_.ipv4_.address_); - } - socklen_t sockAddrLen() const override { return sizeof(sockaddr_in); } - - /** - * Convenience function to convert an IPv4 address to canonical string format. - * @note This works similarly to inet_ntop() but is faster. - * @param addr address to format. - * @return the address in dotted-decimal string format. - */ - static std::string sockaddrToString(const sockaddr_in& addr); - - // Validate that IPv4 is supported on this platform, raise an exception for the - // given address if not. - static absl::Status validateProtocolSupported(); - -private: struct Ipv4Helper : public Ipv4 { uint32_t address() const override { return address_.sin_addr.s_addr; } @@ -187,6 +187,7 @@ class Ipv4Instance : public InstanceBase { }; IpHelper ip_; + friend class InstanceFactory; }; /** @@ -218,6 +219,20 @@ class Ipv6Instance : public InstanceBase { */ explicit Ipv6Instance(uint32_t port, const SocketInterface* sock_interface = nullptr); + // Network::Address::Instance + bool operator==(const Instance& rhs) const override; + const Ip* ip() const override { return &ip_; } + const Pipe* pipe() const override { return nullptr; } + const EnvoyInternalAddress* envoyInternalAddress() const override { return nullptr; } + const sockaddr* sockAddr() const override { + return reinterpret_cast(&ip_.ipv6_.address_); + } + socklen_t sockAddrLen() const override { return sizeof(sockaddr_in6); } + + // Validate that IPv6 is supported on this platform + static absl::Status validateProtocolSupported(); + +private: /** * Construct from an existing unix IPv6 socket address (IP v6 address and port). */ @@ -243,20 +258,6 @@ class Ipv6Instance : public InstanceBase { explicit Ipv6Instance(absl::Status& error, uint32_t port, const SocketInterface* sock_interface = nullptr); - // Network::Address::Instance - bool operator==(const Instance& rhs) const override; - const Ip* ip() const override { return &ip_; } - const Pipe* pipe() const override { return nullptr; } - const EnvoyInternalAddress* envoyInternalAddress() const override { return nullptr; } - const sockaddr* sockAddr() const override { - return reinterpret_cast(&ip_.ipv6_.address_); - } - socklen_t sockAddrLen() const override { return sizeof(sockaddr_in6); } - - // Validate that IPv6 is supported on this platform - static absl::Status validateProtocolSupported(); - -private: struct Ipv6Helper : public Ipv6 { Ipv6Helper() { memset(&address_, 0, sizeof(address_)); } absl::uint128 address() const override; @@ -290,6 +291,7 @@ class Ipv6Instance : public InstanceBase { }; IpHelper ip_; + friend class InstanceFactory; }; /** @@ -309,18 +311,6 @@ class PipeInstance : public InstanceBase { explicit PipeInstance(const std::string& pipe_path, mode_t mode = 0, const SocketInterface* sock_interface = nullptr); - /** - * Construct from an existing unix address. - */ - explicit PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, - mode_t mode = 0, const SocketInterface* sock_interface = nullptr); - - /** - * Construct from a string pipe path. - */ - explicit PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode = 0, - const SocketInterface* sock_interface = nullptr); - static absl::Status validateProtocolSupported() { return absl::OkStatus(); } // Network::Address::Instance @@ -340,6 +330,17 @@ class PipeInstance : public InstanceBase { } private: + /** + * Construct from an existing unix address. + */ + explicit PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, + mode_t mode = 0, const SocketInterface* sock_interface = nullptr); + + /** + * Construct from a string pipe path. + */ + explicit PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode = 0, + const SocketInterface* sock_interface = nullptr); struct PipeHelper : public Pipe { bool abstractNamespace() const override { return abstract_namespace_; } @@ -353,6 +354,7 @@ class PipeInstance : public InstanceBase { }; PipeHelper pipe_; + friend class InstanceFactory; }; class EnvoyInternalInstance : public InstanceBase { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 627b7dad3137..22987bee2119 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -9,7 +9,6 @@ #include "absl/container/fixed_array.h" #include "absl/types/optional.h" -#include "source/common/common/_virtual_includes/statusor_lib/common/common/statusor.h" using Envoy::Api::SysCallIntResult; using Envoy::Api::SysCallSizeResult; @@ -604,7 +603,8 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_, errorDetails(result.errno_))); } - StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len, socket_v6only_); + StatusOr error_or_address = + Address::addressFromSockAddr(ss, ss_len, socket_v6only_); ASSERT(error_or_address.ok()); return *error_or_address; } @@ -631,7 +631,8 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len); + StatusOr error_or_address = + Address::addressFromSockAddr(ss, ss_len); ASSERT(error_or_address.ok()); return *error_or_address; } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index b9df458b45b8..c0a40105da7b 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -28,7 +28,6 @@ #include "absl/container/fixed_array.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" -#include "source/common/common/_virtual_includes/statusor_lib/common/common/statusor.h" namespace Envoy { namespace Network { @@ -391,7 +390,7 @@ Utility::getAddressWithPort(const Address::Instance& address, uint32_t port) { address.ip()->addressAsString(), port); case Address::IpVersion::v6: return Address::InstanceFactory::createInstancePtr( - address.ip()->addressAsString(), port); + address.ip()->addressAsString(), port); } NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 2ef610bb2542..a97e7f8173b6 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -8,6 +8,7 @@ #include "envoy/config/core/v3/address.pb.h" #include "envoy/network/connection.h" #include "envoy/network/listener.h" + #include "common/common/statusor.h" #include "absl/strings/string_view.h" @@ -265,8 +266,8 @@ class Utility { */ static const std::string& getIpv6CidrCatchAllAddress(); - static StatusOr getAddressWithPort(const Address::Instance& address, - uint32_t port); + static StatusOr + getAddressWithPort(const Address::Instance& address, uint32_t port); /** * @param address IP address instance. @@ -274,8 +275,7 @@ class Utility { * @return Address::InstanceConstSharedPtr a new address instance with updated port. */ static Address::InstanceConstSharedPtr getAddressWithPortOrThrow(const Address::Instance& address, - uint32_t port); - + uint32_t port); /** * Retrieve the original destination address from an accepted socket. diff --git a/source/common/upstream/logical_host.h b/source/common/upstream/logical_host.h index f6ccc9abe481..288db374b78a 100644 --- a/source/common/upstream/logical_host.h +++ b/source/common/upstream/logical_host.h @@ -37,7 +37,8 @@ class LogicalHost : public HostImpl { const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint) { const auto& port_value = lb_endpoint.endpoint().health_check_config().port_value(); auto health_check_address = - port_value == 0 ? address : Network::Utility::getAddressWithPortOrThrow(*address, port_value); + port_value == 0 ? address + : Network::Utility::getAddressWithPortOrThrow(*address, port_value); absl::WriterMutexLock lock(&address_lock_); setAddress(address); diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc index 54706b293f67..bbe52617a454 100644 --- a/source/common/upstream/strict_dns_cluster.cc +++ b/source/common/upstream/strict_dns_cluster.cc @@ -131,17 +131,15 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // for SRV. ASSERT(resp.address_ != nullptr); - StatusOr error_or_address = - Network::Utility::getAddressWithPort(*(resp.address_), - port_); - if (!error_or_address.ok()) { - ENVOY_LOG(debug, "Fail to create new address."); - return; - } - Network::Address::InstanceConstSharedPtr new_address = *error_or_address; + StatusOr error_or_address = + Network::Utility::getAddressWithPort(*(resp.address_), port_); + if (!error_or_address.ok()) { + ENVOY_LOG(debug, "Fail to create new address."); + return; + } + Network::Address::InstanceConstSharedPtr new_address = *error_or_address; new_hosts.emplace_back(new HostImpl( - parent_.info_, hostname_, - new_address, + parent_.info_, hostname_, new_address, // TODO(zyfjeff): Created through metadata shared pool std::make_shared(lb_endpoint_.metadata()), lb_endpoint_.load_balancing_weight().value(), locality_lb_endpoints_.locality(), diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 55b9fa7976c3..d62994d40ed2 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -264,10 +264,10 @@ HostDescriptionImpl::HostDescriptionImpl( throw EnvoyException( fmt::format("Invalid host configuration: non-zero port for non-IP address")); } - health_check_address_ = - health_check_config.port_value() == 0 - ? dest_address - : Network::Utility::getAddressWithPortOrThrow(*dest_address, health_check_config.port_value()); + health_check_address_ = health_check_config.port_value() == 0 + ? dest_address + : Network::Utility::getAddressWithPortOrThrow( + *dest_address, health_check_config.port_value()); } Network::TransportSocketFactory& HostDescriptionImpl::resolveTransportSocketFactory( diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 4871ce7085a6..f0dee5658902 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -261,7 +261,8 @@ void RedisCluster::RedisDiscoverySession::registerDiscoveryAddress( // port in it. for (const Network::DnsResponse& res : response) { ASSERT(res.address_ != nullptr); - discovery_address_list_.push_back(Network::Utility::getAddressWithPortOrThrow(*(res.address_), port)); + discovery_address_list_.push_back( + Network::Utility::getAddressWithPortOrThrow(*(res.address_), port)); } } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 443c2cee548c..adfc13c9842a 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -242,10 +242,10 @@ void DnsCacheImpl::finishResolve(const std::string& host, // If the DNS resolver successfully resolved with an empty response list, the dns cache does not // update. This ensures that a potentially previously resolved address does not stabilize back to // 0 hosts. - const auto new_address = !response.empty() - ? Network::Utility::getAddressWithPortOrThrow(*(response.front().address_), + const auto new_address = + !response.empty() ? Network::Utility::getAddressWithPortOrThrow(*(response.front().address_), primary_host_info->port_) - : nullptr; + : nullptr; if (status == Network::DnsResolver::ResolutionStatus::Failure) { stats_.dns_query_failure_.inc(); diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 8e87f8d7284d..b72b8cb11f8d 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -494,8 +494,10 @@ TEST(AddressFromSockAddrDeathTest, IPv6) { // Test that IPv4-mapped IPv6 address is returned as an Ipv4Instance when 'v6only' parameter is // 'false', but not otherwise. EXPECT_EQ(1, inet_pton(AF_INET6, "::ffff:192.0.2.128", &sin6.sin6_addr)); - EXPECT_EQ(IpVersion::v4, (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->ip()->version()); - EXPECT_EQ("192.0.2.128:32000", (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->asString()); + EXPECT_EQ(IpVersion::v4, + (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->ip()->version()); + EXPECT_EQ("192.0.2.128:32000", + (*addressFromSockAddr(ss, sizeof(sockaddr_in6), false))->asString()); EXPECT_EQ(IpVersion::v6, (*addressFromSockAddr(ss, sizeof(sockaddr_in6), true))->ip()->version()); EXPECT_EQ("[::ffff:192.0.2.128]:32000", (*addressFromSockAddr(ss, sizeof(sockaddr_in6), true))->asString()); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 44314bff395a..c637a6389f41 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -667,7 +667,7 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) { // If that flag needs to be set, or c-ares changes its handling this test will need to be updated // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == // ARES_ECONNREFUSED. - + EXPECT_TRUE(peer_->isChannelDirty()); server_->setRefused(false); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index b8b9feb66388..541f7b8b0787 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -226,9 +226,9 @@ TEST_P(TcpListenerImplTest, WildcardListenerUseActualDst) { Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, true); - auto local_dst_address = - Network::Utility::getAddressWithPortOrThrow(*Network::Test::getCanonicalLoopbackAddress(version_), - socket->addressProvider().localAddress()->ip()->port()); + auto local_dst_address = Network::Utility::getAddressWithPortOrThrow( + *Network::Test::getCanonicalLoopbackAddress(version_), + socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); @@ -270,12 +270,12 @@ TEST_P(TcpListenerImplTest, WildcardListenerIpv4Compat) { Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, true); - auto listener_address = - Network::Utility::getAddressWithPortOrThrow(*Network::Test::getCanonicalLoopbackAddress(version_), - socket->addressProvider().localAddress()->ip()->port()); - auto local_dst_address = - Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getCanonicalIpv4LoopbackAddress(), - socket->addressProvider().localAddress()->ip()->port()); + auto listener_address = Network::Utility::getAddressWithPortOrThrow( + *Network::Test::getCanonicalLoopbackAddress(version_), + socket->addressProvider().localAddress()->ip()->port()); + auto local_dst_address = Network::Utility::getAddressWithPortOrThrow( + *Network::Utility::getCanonicalIpv4LoopbackAddress(), + socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index f1efdf572184..a392b967c669 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -102,10 +102,10 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::ParsedVersionOfIndex(quic::CurrentSupportedVersions(), 0); }()), - peer_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), - 12345)), - self_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), - 54321)), + peer_addr_(Network::Utility::getAddressWithPortOrThrow( + *Network::Utility::getIpv6LoopbackAddress(), 12345)), + self_addr_(Network::Utility::getAddressWithPortOrThrow( + *Network::Utility::getIpv6LoopbackAddress(), 54321)), quic_connection_(new TestEnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_, quic_version_, *dispatcher_, createConnectionSocket(peer_addr_, self_addr_, nullptr))), diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 98d67d183354..6d0884fa26af 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -28,10 +28,10 @@ class EnvoyQuicClientStreamTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::CurrentSupportedVersions()[0]; }()), - peer_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), - 12345)), - self_addr_(Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv6LoopbackAddress(), - 54321)), + peer_addr_(Network::Utility::getAddressWithPortOrThrow( + *Network::Utility::getIpv6LoopbackAddress(), 12345)), + self_addr_(Network::Utility::getAddressWithPortOrThrow( + *Network::Utility::getIpv6LoopbackAddress(), 54321)), quic_connection_(new EnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, &writer_, /*owns_writer=*/false, {quic_version_}, *dispatcher_, diff --git a/test/common/quic/envoy_quic_writer_test.cc b/test/common/quic/envoy_quic_writer_test.cc index ae784ed06fee..6aee9476b2a6 100644 --- a/test/common/quic/envoy_quic_writer_test.cc +++ b/test/common/quic/envoy_quic_writer_test.cc @@ -33,10 +33,11 @@ class EnvoyQuicWriterTest : public ::testing::Test { } void verifySendData(const std::string& content, const msghdr* message) { - EXPECT_EQ(peer_address_.ToString(), (*Network::Address::addressFromSockAddr( - *reinterpret_cast(message->msg_name), - message->msg_namelen, /*v6only=*/false) - )->asString()); + EXPECT_EQ(peer_address_.ToString(), + (*Network::Address::addressFromSockAddr( + *reinterpret_cast(message->msg_name), message->msg_namelen, + /*v6only=*/false)) + ->asString()); cmsghdr* const cmsg = CMSG_FIRSTHDR(message); auto pktinfo = reinterpret_cast(CMSG_DATA(cmsg)); EXPECT_EQ(0, memcmp(self_address_.GetIPv6().s6_addr, pktinfo->ipi6_addr.s6_addr, diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index d207c9f53757..06fca2b76e3a 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -261,8 +261,9 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggableip()->port()); + Network::Address::InstanceConstSharedPtr any_address = + Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv4AnyAddress(), + normal_address->ip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener); @@ -772,8 +773,9 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); - Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getIpv4AnyAddress(), normal_address->ip()->port()); + Network::Address::InstanceConstSharedPtr any_address = + Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv4AnyAddress(), + normal_address->ip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener1); From c56499896c2baf7129bc1b5b2591b58ce4b641ba Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 5 May 2021 22:11:46 +0000 Subject: [PATCH 03/33] remove exception from dns_impl Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index c2b0411a9696..fe33cc401df7 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -169,24 +169,7 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - // TODO(chaoqin-li1123): remove this exception catching by refactoring. - // We can't add a main thread assertion here because both this code is reused by dns filter - // and executed in both main thread and worker thread. Maybe split the code for filter and - // main thread. - TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } - catch (const EnvoyException& e) { - ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); - dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } - catch (const std::exception& e) { - ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); - dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } - catch (...) { - ENVOY_LOG(critical, "Unknown exception in c-ares callback"); - dispatcher_.post([] { throw EnvoyException("unknown"); }); - } - } + callback_(resolution_status, std::move(address_list)); if (owned_) { delete this; return; From 99b85bab2999d4ff6d1790346f805496e2821042 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 7 May 2021 15:53:33 +0000 Subject: [PATCH 04/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 108 ------------------ source/common/network/address_impl.h | 43 ------- source/common/network/dns_impl.cc | 14 ++- .../common/network/io_socket_handle_impl.cc | 9 +- source/common/network/tcp_listener_impl.cc | 2 +- source/common/network/utility.cc | 21 +--- source/common/network/utility.h | 7 +- source/common/quic/envoy_quic_utils.cc | 2 +- source/common/upstream/logical_dns_cluster.cc | 9 +- source/common/upstream/logical_host.h | 3 +- source/common/upstream/strict_dns_cluster.cc | 7 +- source/common/upstream/upstream_impl.cc | 8 +- .../clusters/redis/redis_cluster.cc | 3 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 6 +- .../original_src/socket_option_factory.cc | 2 +- test/common/network/dns_impl_test.cc | 30 +++++ test/common/network/listener_impl_test.cc | 18 +-- .../quic/envoy_quic_client_session_test.cc | 8 +- .../quic/envoy_quic_client_stream_test.cc | 8 +- .../proxy_protocol/proxy_protocol_test.cc | 2 +- test/integration/base_integration_test.cc | 2 +- test/server/connection_handler_test.cc | 10 +- tools/code_format/check_format.py | 1 + 23 files changed, 95 insertions(+), 228 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 94d9fdc6e322..058073933213 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -149,7 +149,6 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -176,36 +175,6 @@ Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, friendly_name_.append(port.data(), port.size()); } -Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, - const SocketInterface* sock_interface) - : Ipv4Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} - -Ipv4Instance::Ipv4Instance(absl::Status& error, const std::string& address, uint32_t port, - const SocketInterface* sock_interface) - : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); - ip_.ipv4_.address_.sin_family = AF_INET; - ip_.ipv4_.address_.sin_port = htons(port); - int rc = inet_pton(AF_INET, address.c_str(), &ip_.ipv4_.address_.sin_addr); - if (1 != rc) { - // Invalid ipv4 address, return. - error = absl::InvalidArgumentError(fmt::format("invalid ipv4 address '{}'", address)); - return; - } - friendly_name_ = absl::StrCat(address, ":", port); - ip_.friendly_address_ = address; -} - -Ipv4Instance::Ipv4Instance(absl::Status&, uint32_t port, const SocketInterface* sock_interface) - : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); - ip_.ipv4_.address_.sin_family = AF_INET; - ip_.ipv4_.address_.sin_port = htons(port); - ip_.ipv4_.address_.sin_addr.s_addr = INADDR_ANY; - friendly_name_ = absl::StrCat("0.0.0.0:", port); - ip_.friendly_address_ = "0.0.0.0"; -} - bool Ipv4Instance::operator==(const Instance& rhs) const { const Ipv4Instance* rhs_casted = dynamic_cast(&rhs); return (rhs_casted && (ip_.ipv4_.address() == rhs_casted->ip_.ipv4_.address()) && @@ -267,7 +236,6 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const { Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - // ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_ = address; ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); ip_.ipv6_.v6only_ = v6only; @@ -283,7 +251,6 @@ Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* so Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - // ASSERT(Thread::MainThread::isMainThread()); ip_.ipv6_.address_.sin6_family = AF_INET6; ip_.ipv6_.address_.sin6_port = htons(port); if (!address.empty()) { @@ -353,33 +320,6 @@ Ipv6Instance::Ipv6Instance(absl::Status&, const sockaddr_in6& address, bool v6on friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); } -Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, - const SocketInterface* sock_interface) - : Ipv6Instance(error, address, 0, sockInterfaceOrDefault(sock_interface)) {} - -Ipv6Instance::Ipv6Instance(absl::Status& error, const std::string& address, uint32_t port, - const SocketInterface* sock_interface) - : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - ip_.ipv6_.address_.sin6_family = AF_INET6; - ip_.ipv6_.address_.sin6_port = htons(port); - if (!address.empty()) { - if (1 != inet_pton(AF_INET6, address.c_str(), &ip_.ipv6_.address_.sin6_addr)) { - // Invalid ipv6 address return. - error = absl::InvalidArgumentError(fmt::format("invalid ipv6 address '{}'", address)); - return; - } - } else { - ip_.ipv6_.address_.sin6_addr = in6addr_any; - } - // Just in case address is in a non-canonical format, format from network address. - ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); - friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); -} - -Ipv6Instance::Ipv6Instance(absl::Status& error, uint32_t port, - const SocketInterface* sock_interface) - : Ipv6Instance(error, "", port, sockInterfaceOrDefault(sock_interface)) {} - PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { @@ -451,54 +391,6 @@ PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, sock pipe_.mode_ = mode; } -PipeInstance::PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode, - const SocketInterface* sock_interface) - : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { - if (pipe_path.size() >= sizeof(pipe_.address_.sun_path)) { - error = absl::InvalidArgumentError( - fmt::format("Path \"{}\" exceeds maximum UNIX domain socket path size of {}.", pipe_path, - sizeof(pipe_.address_.sun_path))); - return; - } - memset(&pipe_.address_, 0, sizeof(pipe_.address_)); - pipe_.address_.sun_family = AF_UNIX; - if (pipe_path[0] == '@') { - // This indicates an abstract namespace. - // In this case, null bytes in the name have no special significance, and so we copy all - // characters of pipe_path to sun_path, including null bytes in the name. The pathname must also - // be null terminated. The friendly name is the address path with embedded nulls replaced with - // '@' for consistency with the first character. -#if !defined(__linux__) - error = absl::FailedPreconditionError("Abstract AF_UNIX sockets are only supported on linux."); - return; -#endif - if (mode != 0) { - error = absl::InvalidArgumentError("Cannot set mode for Abstract AF_UNIX sockets"); - return; - } - pipe_.abstract_namespace_ = true; - pipe_.address_length_ = pipe_path.size(); - // The following statement is safe since pipe_path size was checked at the beginning of this - // function - memcpy(&pipe_.address_.sun_path[0], pipe_path.data(), pipe_path.size()); // NOLINT(safe-memcpy) - pipe_.address_.sun_path[0] = '\0'; - pipe_.address_.sun_path[pipe_path.size()] = '\0'; - friendly_name_ = friendlyNameFromAbstractPath( - absl::string_view(pipe_.address_.sun_path, pipe_.address_length_)); - } else { - // Reports an error if the pipe path has an embedded null character. - if (pipe_path.size() != strlen(pipe_path.c_str())) { - error = absl::InvalidArgumentError( - "UNIX domain socket pathname contains embedded null characters"); - return; - } - StringUtil::strlcpy(&pipe_.address_.sun_path[0], pipe_path.c_str(), - sizeof(pipe_.address_.sun_path)); - friendly_name_ = pipe_.address_.sun_path; - } - pipe_.mode_ = mode; -} - bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } EnvoyInternalInstance::EnvoyInternalInstance(const std::string& address_id, diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 1ac38586786f..8c3c2a3989ea 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -144,25 +144,6 @@ class Ipv4Instance : public InstanceBase { explicit Ipv4Instance(absl::Status& error, const sockaddr_in* address, const SocketInterface* sock_interface = nullptr); - /** - * Construct from a string IPv4 address such as "1.2.3.4". Port will be unset/0. - */ - explicit Ipv4Instance(absl::Status& error, const std::string& address, - const SocketInterface* sock_interface = nullptr); - - /** - * Construct from a string IPv4 address such as "1.2.3.4" as well as a port. - */ - Ipv4Instance(absl::Status& error, const std::string& address, uint32_t port, - const SocketInterface* sock_interface = nullptr); - - /** - * Construct from a port. The IPv4 address will be set to "any" and is suitable for binding - * a port to any available address. - */ - explicit Ipv4Instance(absl::Status& error, uint32_t port, - const SocketInterface* sock_interface = nullptr); - struct Ipv4Helper : public Ipv4 { uint32_t address() const override { return address_.sin_addr.s_addr; } @@ -239,25 +220,6 @@ class Ipv6Instance : public InstanceBase { Ipv6Instance(absl::Status& error, const sockaddr_in6& address, bool v6only = true, const SocketInterface* sock_interface = nullptr); - /** - * Construct from a string IPv6 address such as "12:34::5". Port will be unset/0. - */ - explicit Ipv6Instance(absl::Status& error, const std::string& address, - const SocketInterface* sock_interface = nullptr); - - /** - * Construct from a string IPv6 address such as "12:34::5" as well as a port. - */ - Ipv6Instance(absl::Status& error, const std::string& address, uint32_t port, - const SocketInterface* sock_interface = nullptr); - - /** - * Construct from a port. The IPv6 address will be set to "any" and is suitable for binding - * a port to any available address. - */ - explicit Ipv6Instance(absl::Status& error, uint32_t port, - const SocketInterface* sock_interface = nullptr); - struct Ipv6Helper : public Ipv6 { Ipv6Helper() { memset(&address_, 0, sizeof(address_)); } absl::uint128 address() const override; @@ -336,11 +298,6 @@ class PipeInstance : public InstanceBase { explicit PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0, const SocketInterface* sock_interface = nullptr); - /** - * Construct from a string pipe path. - */ - explicit PipeInstance(absl::Status& error, const std::string& pipe_path, mode_t mode = 0, - const SocketInterface* sock_interface = nullptr); struct PipeHelper : public Pipe { bool abstractNamespace() const override { return abstract_namespace_; } diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index fe33cc401df7..7b73897bbdf0 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -169,7 +169,19 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - callback_(resolution_status, std::move(address_list)); + try { + callback_(resolution_status, std::move(address_list)); + } catch (const EnvoyException& e) { + ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); + dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); + } catch (const std::exception& e) { + ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); + dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); + } catch (...) { + ENVOY_LOG(critical, "Unknown exception in c-ares callback"); + dispatcher_.post([] { throw EnvoyException("unknown"); }); + } + } if (owned_) { delete this; return; diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 22987bee2119..885fd5d417b0 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -1,6 +1,7 @@ #include "common/network/io_socket_handle_impl.h" #include "envoy/buffer/buffer.h" +#include "envoy/common/exception.h" #include "common/api/os_sys_calls_impl.h" #include "common/common/utility.h" @@ -605,7 +606,9 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { } StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len, socket_v6only_); - ASSERT(error_or_address.ok()); + if (!error_or_address.ok()) { + throw EnvoyException(error_or_address.status().ToString()); + } return *error_or_address; } @@ -633,7 +636,9 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { } StatusOr error_or_address = Address::addressFromSockAddr(ss, ss_len); - ASSERT(error_or_address.ok()); + if (!error_or_address.ok()) { + throw EnvoyException(error_or_address.status().ToString()); + } return *error_or_address; } diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index d2dc464e125c..d022304c0e95 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -88,7 +88,7 @@ void TcpListenerImpl::onSocketEvent(short flags) { } else { StatusOr error_or_address = Address::addressFromSockAddr( remote_addr, remote_addr_len, local_address->ip()->version() == Address::IpVersion::v6); - ASSERT(error_or_address.ok()); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); remote_address = *error_or_address; } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index c0a40105da7b..82200bae3cb0 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -262,7 +262,7 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio reinterpret_cast(ifa->ifa_addr); StatusOr error_or_instance = Address::addressFromSockAddr( *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); - ASSERT(error_or_instance.ok()); + RELEASE_ASSERT(error_or_instance.ok(), error_or_instance.status().ToString()); ret = *error_or_instance; if (!isLoopbackAddress(*ret)) { break; @@ -382,28 +382,17 @@ const std::string& Utility::getIpv6CidrCatchAllAddress() { CONSTRUCT_ON_FIRST_USE(std::string, "::/0"); } -StatusOr -Utility::getAddressWithPort(const Address::Instance& address, uint32_t port) { +Address::InstanceConstSharedPtr Utility::getAddressWithPort(const Address::Instance& address, + uint32_t port) { switch (address.ip()->version()) { case Address::IpVersion::v4: - return Address::InstanceFactory::createInstancePtr( - address.ip()->addressAsString(), port); + return std::make_shared(address.ip()->addressAsString(), port); case Address::IpVersion::v6: - return Address::InstanceFactory::createInstancePtr( - address.ip()->addressAsString(), port); + return std::make_shared(address.ip()->addressAsString(), port); } NOT_REACHED_GCOVR_EXCL_LINE; } -Address::InstanceConstSharedPtr Utility::getAddressWithPortOrThrow(const Address::Instance& address, - uint32_t port) { - StatusOr error_or_address = getAddressWithPort(address, port); - if (!error_or_address.ok()) { - throw EnvoyException(error_or_address.status().ToString()); - } - return *error_or_address; -} - Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) { #ifdef SOL_IP diff --git a/source/common/network/utility.h b/source/common/network/utility.h index a97e7f8173b6..59af37f90815 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -266,16 +266,13 @@ class Utility { */ static const std::string& getIpv6CidrCatchAllAddress(); - static StatusOr - getAddressWithPort(const Address::Instance& address, uint32_t port); - /** * @param address IP address instance. * @param port to update. * @return Address::InstanceConstSharedPtr a new address instance with updated port. */ - static Address::InstanceConstSharedPtr getAddressWithPortOrThrow(const Address::Instance& address, - uint32_t port); + static Address::InstanceConstSharedPtr getAddressWithPort(const Address::Instance& address, + uint32_t port); /** * Retrieve the original destination address from an accepted socket. diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index f93703101b18..c12bc5699f2e 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -27,7 +27,7 @@ quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { ? sizeof(sockaddr_in) : sizeof(sockaddr_in6), false); - ASSERT(error_or_instance.ok()); + RELEASE_ASSERT(error_or_instance.ok(), error_or_instance.status().ToString()); return *error_or_instance; } diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index e9e951492d6a..68b3a2842c50 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -120,16 +120,9 @@ void LogicalDnsCluster::startResolve() { // not stabilize back to 0 hosts. if (status == Network::DnsResolver::ResolutionStatus::Success && !response.empty()) { info_->stats().update_success_.inc(); - // TODO(mattklein123): Move port handling into the DNS interface. - ASSERT(response.front().address_ != nullptr); - StatusOr error_or_address = + Network::Address::InstanceConstSharedPtr new_address = Network::Utility::getAddressWithPort(*(response.front().address_), Network::Utility::portFromTcpUrl(dns_url_)); - if (!error_or_address.ok()) { - ENVOY_LOG(debug, "Fail to create new address."); - return; - } - Network::Address::InstanceConstSharedPtr new_address = *error_or_address; if (!logical_host_) { logical_host_ = std::make_shared(info_, hostname_, new_address, localityLbEndpoint(), diff --git a/source/common/upstream/logical_host.h b/source/common/upstream/logical_host.h index 288db374b78a..ae2efb3ebb45 100644 --- a/source/common/upstream/logical_host.h +++ b/source/common/upstream/logical_host.h @@ -37,8 +37,7 @@ class LogicalHost : public HostImpl { const envoy::config::endpoint::v3::LbEndpoint& lb_endpoint) { const auto& port_value = lb_endpoint.endpoint().health_check_config().port_value(); auto health_check_address = - port_value == 0 ? address - : Network::Utility::getAddressWithPortOrThrow(*address, port_value); + port_value == 0 ? address : Network::Utility::getAddressWithPort(*address, port_value); absl::WriterMutexLock lock(&address_lock_); setAddress(address); diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc index bbe52617a454..3b0146bc3d98 100644 --- a/source/common/upstream/strict_dns_cluster.cc +++ b/source/common/upstream/strict_dns_cluster.cc @@ -131,13 +131,8 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // for SRV. ASSERT(resp.address_ != nullptr); - StatusOr error_or_address = + Network::Address::InstanceConstSharedPtr new_address = Network::Utility::getAddressWithPort(*(resp.address_), port_); - if (!error_or_address.ok()) { - ENVOY_LOG(debug, "Fail to create new address."); - return; - } - Network::Address::InstanceConstSharedPtr new_address = *error_or_address; new_hosts.emplace_back(new HostImpl( parent_.info_, hostname_, new_address, // TODO(zyfjeff): Created through metadata shared pool diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index d62994d40ed2..07db4f18f2ff 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -264,10 +264,10 @@ HostDescriptionImpl::HostDescriptionImpl( throw EnvoyException( fmt::format("Invalid host configuration: non-zero port for non-IP address")); } - health_check_address_ = health_check_config.port_value() == 0 - ? dest_address - : Network::Utility::getAddressWithPortOrThrow( - *dest_address, health_check_config.port_value()); + health_check_address_ = + health_check_config.port_value() == 0 + ? dest_address + : Network::Utility::getAddressWithPort(*dest_address, health_check_config.port_value()); } Network::TransportSocketFactory& HostDescriptionImpl::resolveTransportSocketFactory( diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index f0dee5658902..297248a6b6a1 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -261,8 +261,7 @@ void RedisCluster::RedisDiscoverySession::registerDiscoveryAddress( // port in it. for (const Network::DnsResponse& res : response) { ASSERT(res.address_ != nullptr); - discovery_address_list_.push_back( - Network::Utility::getAddressWithPortOrThrow(*(res.address_), port)); + discovery_address_list_.push_back(Network::Utility::getAddressWithPort(*(res.address_), port)); } } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index adfc13c9842a..c2f6cd82a858 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -242,10 +242,10 @@ void DnsCacheImpl::finishResolve(const std::string& host, // If the DNS resolver successfully resolved with an empty response list, the dns cache does not // update. This ensures that a potentially previously resolved address does not stabilize back to // 0 hosts. - const auto new_address = - !response.empty() ? Network::Utility::getAddressWithPortOrThrow(*(response.front().address_), + const auto new_address = !response.empty() + ? Network::Utility::getAddressWithPort(*(response.front().address_), primary_host_info->port_) - : nullptr; + : nullptr; if (status == Network::DnsResolver::ResolutionStatus::Failure) { stats_.dns_query_failure_.inc(); diff --git a/source/extensions/filters/common/original_src/socket_option_factory.cc b/source/extensions/filters/common/original_src/socket_option_factory.cc index 907a13d7835a..cf84b9d26596 100644 --- a/source/extensions/filters/common/original_src/socket_option_factory.cc +++ b/source/extensions/filters/common/original_src/socket_option_factory.cc @@ -13,7 +13,7 @@ namespace OriginalSrc { Network::Socket::OptionsSharedPtr buildOriginalSrcOptions(Network::Address::InstanceConstSharedPtr source, uint32_t mark) { - const auto address_without_port = Network::Utility::getAddressWithPortOrThrow(*source, 0); + const auto address_without_port = Network::Utility::getAddressWithPort(*source, 0); // Note: we don't expect this to change the behaviour of the socket. We expect it to be copied // into the upstream connection later. diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index c637a6389f41..58f74663832d 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -531,6 +531,18 @@ class DnsImplTest : public testing::TestWithParam { }); } + template + ActiveDnsQuery* resolveWithException(const std::string& address, + const DnsLookupFamily lookup_family, T exception_object) { + return resolver_->resolve(address, lookup_family, + [exception_object](DnsResolver::ResolutionStatus status, + std::list&& results) -> void { + UNREFERENCED_PARAMETER(status); + UNREFERENCED_PARAMETER(results); + throw exception_object; + }); + } + protected: // Should the DnsResolverImpl use a zero timeout for c-ares queries? virtual bool zeroTimeout() const { return false; } @@ -648,6 +660,24 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +// Validate exception behavior during c-ares callbacks. +TEST_P(DnsImplTest, CallbackException) { + // Force immediate resolution, which will trigger a c-ares exception unsafe + // state providing regression coverage for #4307. + EXPECT_EQ(nullptr, resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, + EnvoyException("Envoy exception"))); + EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, + "Envoy exception"); + EXPECT_EQ(nullptr, resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, + std::runtime_error("runtime error"))); + EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, + "runtime error"); + EXPECT_EQ(nullptr, + resolveWithException("1.2.3.4", DnsLookupFamily::V4Only, std::string())); + EXPECT_THROW_WITH_MESSAGE(dispatcher_->run(Event::Dispatcher::RunType::Block), EnvoyException, + "unknown"); +} + // Validate that the c-ares channel is destroyed and re-initialized when c-ares returns // ARES_ECONNREFUSED as its callback status. TEST_P(DnsImplTest, DestroyChannelOnRefused) { diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 541f7b8b0787..cea2b1ed13f0 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -226,9 +226,9 @@ TEST_P(TcpListenerImplTest, WildcardListenerUseActualDst) { Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, true); - auto local_dst_address = Network::Utility::getAddressWithPortOrThrow( - *Network::Test::getCanonicalLoopbackAddress(version_), - socket->addressProvider().localAddress()->ip()->port()); + auto local_dst_address = + Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); @@ -270,12 +270,12 @@ TEST_P(TcpListenerImplTest, WildcardListenerIpv4Compat) { Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, true); - auto listener_address = Network::Utility::getAddressWithPortOrThrow( - *Network::Test::getCanonicalLoopbackAddress(version_), - socket->addressProvider().localAddress()->ip()->port()); - auto local_dst_address = Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getCanonicalIpv4LoopbackAddress(), - socket->addressProvider().localAddress()->ip()->port()); + auto listener_address = + Network::Utility::getAddressWithPort(*Network::Test::getCanonicalLoopbackAddress(version_), + socket->addressProvider().localAddress()->ip()->port()); + auto local_dst_address = + Network::Utility::getAddressWithPort(*Network::Utility::getCanonicalIpv4LoopbackAddress(), + socket->addressProvider().localAddress()->ip()->port()); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( local_dst_address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr); diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index a392b967c669..8617d8953e83 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -102,10 +102,10 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::ParsedVersionOfIndex(quic::CurrentSupportedVersions(), 0); }()), - peer_addr_(Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getIpv6LoopbackAddress(), 12345)), - self_addr_(Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getIpv6LoopbackAddress(), 54321)), + peer_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + 12345)), + self_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + 54321)), quic_connection_(new TestEnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_, quic_version_, *dispatcher_, createConnectionSocket(peer_addr_, self_addr_, nullptr))), diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 6d0884fa26af..3cda1218570a 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -28,10 +28,10 @@ class EnvoyQuicClientStreamTest : public testing::TestWithParam { SetQuicReloadableFlag(quic_disable_version_draft_29, !GetParam()); return quic::CurrentSupportedVersions()[0]; }()), - peer_addr_(Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getIpv6LoopbackAddress(), 12345)), - self_addr_(Network::Utility::getAddressWithPortOrThrow( - *Network::Utility::getIpv6LoopbackAddress(), 54321)), + peer_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + 12345)), + self_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), + 54321)), quic_connection_(new EnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, &writer_, /*owns_writer=*/false, {quic_version_}, *dispatcher_, 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 1c92e9c07094..2ab7186e8b16 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -1284,7 +1284,7 @@ class WildcardProxyProtocolTest : public testing::TestWithParamallocateDispatcher("test_thread")), socket_(std::make_shared(Network::Test::getAnyAddress(GetParam()), nullptr, true)), - local_dst_address_(Network::Utility::getAddressWithPortOrThrow( + local_dst_address_(Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(GetParam()), socket_->addressProvider().localAddress()->ip()->port())), connection_handler_(new Server::ConnectionHandlerImpl(*dispatcher_, absl::nullopt)), diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index b85bcde5a1dd..cf0b1d193aaf 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -556,7 +556,7 @@ AssertionResult BaseIntegrationTest::waitForPortAvailable(uint32_t port, Event::TestTimeSystem::RealTimeBound bound(timeout); while (bound.withinBound()) { try { - Network::TcpListenSocket(Network::Utility::getAddressWithPortOrThrow( + Network::TcpListenSocket(Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), port), nullptr, true); return AssertionSuccess(); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 06fca2b76e3a..1e9a174fee06 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -261,9 +261,8 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggableip()->port()); + Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPort( + *Network::Utility::getIpv4AnyAddress(), normal_address->ip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener); @@ -773,9 +772,8 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); - Network::Address::InstanceConstSharedPtr any_address = - Network::Utility::getAddressWithPortOrThrow(*Network::Utility::getIpv4AnyAddress(), - normal_address->ip()->port()); + Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPort( + *Network::Utility::getIpv4AnyAddress(), normal_address->ip()->port()); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(absl::nullopt, *test_listener1); diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index c03697fec7ac..eac244a8d281 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -121,6 +121,7 @@ "./source/common/common/regex.cc", "./source/common/common/thread.h", "./source/common/network/utility.cc", + "./source/common/network/dns_impl.cc" ) # Header files that can throw exceptions. These should be limited; the only From 85cff4248ff505a4ce068f132ba45860fc606f5b Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 7 May 2021 15:56:04 +0000 Subject: [PATCH 05/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 058073933213..690bd117a57c 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -109,7 +109,6 @@ StatusOr addressFromSockAddr(const sockaddr_sto Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_ = *address; ip_.friendly_address_ = sockaddrToString(*address); @@ -131,7 +130,6 @@ Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* so Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - // ASSERT(Thread::MainThread::isMainThread()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); From 5b44d5fc510e2292e23f26e3b84d9f57745b6ab7 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 7 May 2021 18:51:28 +0000 Subject: [PATCH 06/33] use release assert Signed-off-by: chaoqin-li1123 --- source/common/network/utility.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 82200bae3cb0..096fd08c8bc3 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -136,7 +136,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: sa4.sin_port = htons(port); StatusOr error_or_address = Address::InstanceFactory::createInstancePtr(&sa4); - ASSERT(error_or_address.ok()); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); return *error_or_address; } sockaddr_in6 sa6; @@ -146,7 +146,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: sa6.sin6_port = htons(port); StatusOr error_or_address = Address::InstanceFactory::createInstancePtr(sa6, v6only); - ASSERT(error_or_address.ok()); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); return *error_or_address; } return nullptr; @@ -188,7 +188,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool sa6.sin6_port = htons(port64); StatusOr error_or_address = Address::InstanceFactory::createInstancePtr(sa6, v6only); - ASSERT(error_or_address.ok()); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); return *error_or_address; } // Treat it as an IPv4 address followed by a port. @@ -211,7 +211,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool sa4.sin_port = htons(port64); StatusOr error_or_address = Address::InstanceFactory::createInstancePtr(&sa4); - ASSERT(error_or_address.ok()); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); return *error_or_address; } From b12f57cd70a1982f4ad6b25efc9929d483ea3fb5 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 12 May 2021 14:40:06 +0000 Subject: [PATCH 07/33] use getAddreOrDie Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 49 ++++++++++++++----- source/common/network/address_impl.h | 9 ++-- source/common/network/dns_impl.cc | 2 + .../common/network/io_socket_handle_impl.cc | 41 +++------------- source/common/network/tcp_listener_impl.cc | 7 ++- source/common/network/utility.cc | 16 +++--- source/common/quic/envoy_quic_utils.cc | 14 ++---- tools/code_format/check_format.py | 7 +-- 8 files changed, 66 insertions(+), 79 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 690bd117a57c..b52284877da6 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -107,6 +107,24 @@ StatusOr addressFromSockAddr(const sockaddr_sto NOT_REACHED_GCOVR_EXCL_LINE; } +Address::InstanceConstSharedPtr +getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, bool v6only) { + // Set v6only to false so that mapped-v6 address can be normalize to v4 + // address. Though dual stack may be disabled, it's still okay to assume the + // address is from a dual stack socket. This is because mapped-v6 address + // must come from a dual stack socket. An actual v6 address can come from + // both dual stack socket and v6 only socket. If |peer_addr| is an actual v6 + // address and the socket is actually v6 only, the returned address will be + // regarded as a v6 address from dual stack socket. However, this address is not going to be + // used to create socket. Wrong knowledge of dual stack support won't hurt. + StatusOr error_or_addr = + Address::addressFromSockAddr(ss, ss_len, v6only); + if (!error_or_addr.ok()) { + PANIC(fmt::format("Invalid address for fd: {}", fd)); + } + return *error_or_addr; +} + Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); @@ -119,8 +137,9 @@ Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* so friendly_name_.append(ip_.friendly_address_); friendly_name_.push_back(':'); friendly_name_.append(port.data(), port.size()); - if (!validateProtocolSupported().ok()) { - throw EnvoyException("Ipv4 not supported"); + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } } @@ -139,8 +158,9 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, } friendly_name_ = absl::StrCat(address, ":", port); - if (!validateProtocolSupported().ok()) { - throw EnvoyException("Ipv4 not supported"); + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } ip_.friendly_address_ = address; } @@ -152,13 +172,14 @@ Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) ip_.ipv4_.address_.sin_port = htons(port); ip_.ipv4_.address_.sin_addr.s_addr = INADDR_ANY; friendly_name_ = absl::StrCat("0.0.0.0:", port); - if (!validateProtocolSupported().ok()) { - throw EnvoyException("Ipv4 not supported"); + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } ip_.friendly_address_ = "0.0.0.0"; } -Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, +Ipv4Instance::Ipv4Instance(absl::Status& error, const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); @@ -171,6 +192,7 @@ Ipv4Instance::Ipv4Instance(absl::Status&, const sockaddr_in* address, friendly_name_.append(ip_.friendly_address_); friendly_name_.push_back(':'); friendly_name_.append(port.data(), port.size()); + error = validateProtocolSupported(); } bool Ipv4Instance::operator==(const Instance& rhs) const { @@ -238,8 +260,9 @@ Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); ip_.ipv6_.v6only_ = v6only; friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - if (!validateProtocolSupported().ok()) { - throw EnvoyException("Ipv6 not supported"); + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } } @@ -261,8 +284,9 @@ Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, // Just in case address is in a non-canonical format, format from network address. ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - if (!validateProtocolSupported().ok()) { - throw EnvoyException("Ipv6 not supported"); + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } } @@ -309,13 +333,14 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t pipe_.mode_ = mode; } -Ipv6Instance::Ipv6Instance(absl::Status&, const sockaddr_in6& address, bool v6only, +Ipv6Instance::Ipv6Instance(absl::Status& status, const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { ip_.ipv6_.address_ = address; ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); ip_.ipv6_.v6only_ = v6only; friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); + status = validateProtocolSupported(); } PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 8c3c2a3989ea..ec87c4b46bd5 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -30,6 +30,10 @@ namespace Address { StatusOr addressFromSockAddr(const sockaddr_storage& ss, socklen_t len, bool v6only = true); +Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, + socklen_t ss_len, os_fd_t fd, + bool v6only = true); + /** * Base class for all address types. */ @@ -60,10 +64,6 @@ class InstanceFactory { template static StatusOr createInstancePtr(Args&&... args) { absl::Status status; - status = InstanceType::validateProtocolSupported(); - if (!status.ok()) { - return status; - } std::unique_ptr instance(new InstanceType(status, std::forward(args)...)); if (!status.ok()) { return status; @@ -74,7 +74,6 @@ class InstanceFactory { template static StatusOr createInstance(Args&&... args) { absl::Status status; - status = InstanceType::validateProtocolSupported(); if (!status.ok()) { return status; } diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 7b73897bbdf0..dcbda6b1030f 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -169,6 +169,8 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { + // Use a raw try here because it is used in both main thread and filter. + // Can not convert to use status code as there may be unexpected exceptions. try { callback_(resolution_status, std::move(address_list)); } catch (const EnvoyException& e) { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 885fd5d417b0..d903b536d19a 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -276,24 +276,6 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic } } -Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, - socklen_t ss_len, os_fd_t fd) { - // Set v6only to false so that mapped-v6 address can be normalize to v4 - // address. Though dual stack may be disabled, it's still okay to assume the - // address is from a dual stack socket. This is because mapped-v6 address - // must come from a dual stack socket. An actual v6 address can come from - // both dual stack socket and v6 only socket. If |peer_addr| is an actual v6 - // address and the socket is actually v6 only, the returned address will be - // regarded as a v6 address from dual stack socket. However, this address is not going to be - // used to create socket. Wrong knowledge of dual stack support won't hurt. - StatusOr error_or_addr = - Address::addressFromSockAddr(ss, ss_len, /*v6only=*/false); - if (!error_or_addr.ok()) { - PANIC(fmt::format("Invalid address for fd: {}", fd)); - } - return *error_or_addr; -} - Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg, uint32_t self_port, os_fd_t fd) { if (cmsg.cmsg_type == IPV6_PKTINFO) { @@ -304,7 +286,7 @@ Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg ipv6_addr->sin6_family = AF_INET6; ipv6_addr->sin6_addr = info->ipi6_addr; ipv6_addr->sin6_port = htons(self_port); - return getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in6), fd); + return Address::getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in6), fd); } if (cmsg.cmsg_type == messageTypeContainsIP()) { @@ -314,7 +296,7 @@ Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg ipv4_addr->sin_family = AF_INET; ipv4_addr->sin_addr = addressFromMessage(cmsg); ipv4_addr->sin_port = htons(self_port); - return getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in), fd); + return Address::getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in), fd); } return nullptr; @@ -384,7 +366,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, fmt::format("Incorrectly set control message length: {}", hdr.msg_controllen)); RELEASE_ASSERT(hdr.msg_namelen > 0, fmt::format("Unable to get remote address from recvmsg() for fd: {}", fd_)); - output.msg_[0].peer_address_ = getAddressFromSockAddrOrDie(peer_addr, hdr.msg_namelen, fd_); + output.msg_[0].peer_address_ = + Address::getAddressFromSockAddrOrDie(peer_addr, hdr.msg_namelen, fd_); output.msg_[0].gso_size_ = 0; if (hdr.msg_controllen > 0) { @@ -491,7 +474,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin output.msg_[i].msg_len_ = mmsg_hdr[i].msg_len; // Get local and peer addresses for each packet. output.msg_[i].peer_address_ = - getAddressFromSockAddrOrDie(raw_addresses[i], hdr.msg_namelen, fd_); + Address::getAddressFromSockAddrOrDie(raw_addresses[i], hdr.msg_namelen, fd_); if (hdr.msg_controllen > 0) { struct cmsghdr* cmsg; for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(&hdr, cmsg)) { @@ -604,12 +587,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_, errorDetails(result.errno_))); } - StatusOr error_or_address = - Address::addressFromSockAddr(ss, ss_len, socket_v6only_); - if (!error_or_address.ok()) { - throw EnvoyException(error_or_address.status().ToString()); - } - return *error_or_address; + return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, socket_v6only_); } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { @@ -634,12 +612,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - StatusOr error_or_address = - Address::addressFromSockAddr(ss, ss_len); - if (!error_or_address.ok()) { - throw EnvoyException(error_or_address.status().ToString()); - } - return *error_or_address; + return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, true); } void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index d022304c0e95..5617fa607e2a 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -86,10 +86,9 @@ void TcpListenerImpl::onSocketEvent(short flags) { if (remote_addr.ss_family == AF_UNIX) { remote_address = io_handle->peerAddress(); } else { - StatusOr error_or_address = Address::addressFromSockAddr( - remote_addr, remote_addr_len, local_address->ip()->version() == Address::IpVersion::v6); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - remote_address = *error_or_address; + remote_address = Address::getAddressFromSockAddrOrDie(remote_addr, remote_addr_len, -1, + local_address->ip()->version() == + Address::IpVersion::v6); } cb_.onAccept( diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 096fd08c8bc3..54fff4751c7f 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -260,10 +260,9 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio (ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) { const struct sockaddr_storage* addr = reinterpret_cast(ifa->ifa_addr); - StatusOr error_or_instance = Address::addressFromSockAddr( - *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); - RELEASE_ASSERT(error_or_instance.ok(), error_or_instance.status().ToString()); - ret = *error_or_instance; + ret = Address::getAddressFromSockAddrOrDie( + *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6), + -1, true); if (!isLoopbackAddress(*ret)) { break; } @@ -420,12 +419,9 @@ Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) { return nullptr; } - StatusOr error_or_address = - Address::addressFromSockAddr(orig_addr, 0, true /* default for v6 constructor */); - if (!error_or_address.ok()) { - throw EnvoyException(error_or_address.status().ToString()); - } - return *error_or_address; + return Address::getAddressFromSockAddrOrDie(orig_addr, 0, -1, + true /* default for v6 constructor */); + #else // TODO(zuercher): determine if connection redirection is possible under macOS (c.f. pfctl and // divert), and whether it's possible to find the learn destination address. diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index c12bc5699f2e..a2a9be6a3e32 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -20,15 +20,11 @@ quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { if (!quic_address.IsInitialized()) { return nullptr; } - StatusOr error_or_instance = - Network::Address::addressFromSockAddr(quic_address.generic_address(), - quic_address.host().address_family() == - quic::IpAddressFamily::IP_V4 - ? sizeof(sockaddr_in) - : sizeof(sockaddr_in6), - false); - RELEASE_ASSERT(error_or_instance.ok(), error_or_instance.status().ToString()); - return *error_or_instance; + return Network::Address::getAddressFromSockAddrOrDie( + quic_address.generic_address(), + quic_address.host().address_family() == quic::IpAddressFamily::IP_V4 ? sizeof(sockaddr_in) + : sizeof(sockaddr_in6), + -1, false); } quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip) { diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index eac244a8d281..c67d225ac9d1 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -118,11 +118,8 @@ "./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") RAW_TRY_ALLOWLIST = ( - "./source/common/common/regex.cc", - "./source/common/common/thread.h", - "./source/common/network/utility.cc", - "./source/common/network/dns_impl.cc" -) + "./source/common/common/regex.cc", "./source/common/common/thread.h", + "./source/common/network/utility.cc", "./source/common/network/dns_impl.cc") # Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config From 9bfc32e0cf7fe509f98aa8f0e40deedfd07b71db Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 12 May 2021 17:46:28 +0000 Subject: [PATCH 08/33] clean up template in .cc Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 30 --------------------------- 1 file changed, 30 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index b52284877da6..9db09fbbc163 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -20,36 +20,6 @@ namespace Address { namespace { -class InstanceFactory { - template - StatusOr createInstance(Args&&... args) { - absl::Status status; - status = InstanceType::validateProtocolSupported(); - if (!status.ok()) { - return status; - } - InstanceType instance(status, std::forward(args)...); - if (!status.ok()) { - return status; - } - return instance; - } - - template - StatusOr createInstancePtr(Args&&... args) { - absl::Status status; - status = InstanceType::validateProtocolSupported(); - if (!status.ok()) { - return status; - } - std::shared_ptr instance{new InstanceType(status, std::forward(args)...)}; - if (!status.ok()) { - return status; - } - return instance; - } -}; - // Constructs a readable string with the embedded nulls in the abstract path replaced with '@'. std::string friendlyNameFromAbstractPath(absl::string_view path) { std::string friendly_name(path.data(), path.size()); From 7c73ff89e5f4a81f6077aece121b560135959686 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 14 May 2021 02:54:33 +0000 Subject: [PATCH 09/33] move error handling to the begining of ctor Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 62 +++++++++++++++------------ 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 9db09fbbc163..4f823bf162bd 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -119,6 +119,10 @@ Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* so Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); + } memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -128,30 +132,30 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, } friendly_name_ = absl::StrCat(address, ":", port); - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } ip_.friendly_address_ = address; } Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); + } memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); ip_.ipv4_.address_.sin_addr.s_addr = INADDR_ANY; friendly_name_ = absl::StrCat("0.0.0.0:", port); - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } ip_.friendly_address_ = "0.0.0.0"; } -Ipv4Instance::Ipv4Instance(absl::Status& error, const sockaddr_in* address, +Ipv4Instance::Ipv4Instance(absl::Status& status, const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + status = validateProtocolSupported(); + if (!status.ok()) { + return; + } memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_ = *address; ip_.friendly_address_ = sockaddrToString(*address); @@ -162,7 +166,6 @@ Ipv4Instance::Ipv4Instance(absl::Status& error, const sockaddr_in* address, friendly_name_.append(ip_.friendly_address_); friendly_name_.push_back(':'); friendly_name_.append(port.data(), port.size()); - error = validateProtocolSupported(); } bool Ipv4Instance::operator==(const Instance& rhs) const { @@ -226,14 +229,14 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const { Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - ip_.ipv6_.address_ = address; - ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); - ip_.ipv6_.v6only_ = v6only; - friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); absl::Status status = validateProtocolSupported(); if (!status.ok()) { throw EnvoyException(status.ToString()); } + ip_.ipv6_.address_ = address; + ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); + ip_.ipv6_.v6only_ = v6only; + friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); } Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* sock_interface) @@ -242,6 +245,10 @@ Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* so Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + absl::Status status = validateProtocolSupported(); + if (!status.ok()) { + throw EnvoyException(status.ToString()); + } ip_.ipv6_.address_.sin6_family = AF_INET6; ip_.ipv6_.address_.sin6_port = htons(port); if (!address.empty()) { @@ -254,10 +261,6 @@ Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, // Just in case address is in a non-canonical format, format from network address. ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } } Ipv6Instance::Ipv6Instance(uint32_t port, const SocketInterface* sock_interface) @@ -269,6 +272,19 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } +Ipv6Instance::Ipv6Instance(absl::Status& status, const sockaddr_in6& address, bool v6only, + const SocketInterface* sock_interface) + : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { + status = validateProtocolSupported(); + if (!status.ok()) { + return; + } + ip_.ipv6_.address_ = address; + ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); + ip_.ipv6_.v6only_ = v6only; + friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); +} + absl::Status Ipv6Instance::validateProtocolSupported() { static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET6); if (supported) { @@ -303,16 +319,6 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t pipe_.mode_ = mode; } -Ipv6Instance::Ipv6Instance(absl::Status& status, const sockaddr_in6& address, bool v6only, - const SocketInterface* sock_interface) - : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - ip_.ipv6_.address_ = address; - ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); - ip_.ipv6_.v6only_ = v6only; - friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); - status = validateProtocolSupported(); -} - PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { From 979079becb26f72fc0137ee64779025bfc1bb014 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Sun, 16 May 2021 23:18:02 +0000 Subject: [PATCH 10/33] refactor Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 11 +---------- source/common/network/address_impl.h | 13 +++++++++++++ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 4f823bf162bd..524189f1aeae 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -156,16 +156,7 @@ Ipv4Instance::Ipv4Instance(absl::Status& status, const sockaddr_in* address, if (!status.ok()) { return; } - memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); - ip_.ipv4_.address_ = *address; - ip_.friendly_address_ = sockaddrToString(*address); - - // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. - fmt::format_int port(ntohs(address->sin_port)); - friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); - friendly_name_.append(ip_.friendly_address_); - friendly_name_.push_back(':'); - friendly_name_.append(port.data(), port.size()); + initHelper(address); } bool Ipv4Instance::operator==(const Instance& rhs) const { diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index ec87c4b46bd5..1cf2c0055158 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -166,6 +166,19 @@ class Ipv4Instance : public InstanceBase { std::string friendly_address_; }; + void initHelper(const sockaddr_in* address) { + memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); + ip_.ipv4_.address_ = *address; + ip_.friendly_address_ = sockaddrToString(*address); + + // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. + fmt::format_int port(ntohs(address->sin_port)); + friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); + friendly_name_.append(ip_.friendly_address_); + friendly_name_.push_back(':'); + friendly_name_.append(port.data(), port.size()); + } + IpHelper ip_; friend class InstanceFactory; }; From 77bc948d3e4aad280d81d096846891c75ae29f0a Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 04:31:23 +0000 Subject: [PATCH 11/33] avoid repeating code Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 41 +++++++++++-------- source/common/network/address_impl.h | 35 +++++----------- source/common/network/dns_impl.cc | 3 +- .../common/network/io_socket_handle_impl.cc | 1 - source/common/network/utility.cc | 20 ++------- 5 files changed, 39 insertions(+), 61 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 524189f1aeae..de501bf748b9 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -97,20 +97,11 @@ getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_ Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); - ip_.ipv4_.address_ = *address; - ip_.friendly_address_ = sockaddrToString(*address); - - // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. - fmt::format_int port(ntohs(address->sin_port)); - friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); - friendly_name_.append(ip_.friendly_address_); - friendly_name_.push_back(':'); - friendly_name_.append(port.data(), port.size()); absl::Status status = validateProtocolSupported(); if (!status.ok()) { throw EnvoyException(status.ToString()); } + initHelper(address); } Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* sock_interface) @@ -199,6 +190,19 @@ absl::Status Ipv4Instance::validateProtocolSupported() { return absl::FailedPreconditionError("IPv4 addresses are not supported on this machine"); } +void Ipv4Instance::initHelper(const sockaddr_in* address) { + memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); + ip_.ipv4_.address_ = *address; + ip_.friendly_address_ = sockaddrToString(*address); + + // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. + fmt::format_int port(ntohs(address->sin_port)); + friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); + friendly_name_.append(ip_.friendly_address_); + friendly_name_.push_back(':'); + friendly_name_.append(port.data(), port.size()); +} + absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; static_assert(sizeof(absl::uint128) == 16, "The size of asbl::uint128 is not 16."); @@ -224,10 +228,7 @@ Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, if (!status.ok()) { throw EnvoyException(status.ToString()); } - ip_.ipv6_.address_ = address; - ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); - ip_.ipv6_.v6only_ = v6only; - friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); + initHelper(address, v6only); } Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* sock_interface) @@ -270,10 +271,7 @@ Ipv6Instance::Ipv6Instance(absl::Status& status, const sockaddr_in6& address, bo if (!status.ok()) { return; } - ip_.ipv6_.address_ = address; - ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); - ip_.ipv6_.v6only_ = v6only; - friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); + initHelper(address, v6only); } absl::Status Ipv6Instance::validateProtocolSupported() { @@ -284,6 +282,13 @@ absl::Status Ipv6Instance::validateProtocolSupported() { return absl::FailedPreconditionError("IPv6 addresses are not supported on this machine"); } +void Ipv6Instance::initHelper(const sockaddr_in6& address, bool v6only) { + ip_.ipv6_.address_ = address; + ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress(); + ip_.ipv6_.v6only_ = v6only; + friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port()); +} + PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode, const SocketInterface* sock_interface) : InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) { diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 1cf2c0055158..1f269a1fef0a 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -70,18 +70,12 @@ class InstanceFactory { } return instance; } - template - static StatusOr createInstance(Args&&... args) { - absl::Status status; - if (!status.ok()) { - return status; - } - InstanceType instance(status, std::forward(args)...); - if (!status.ok()) { - return status; - } - return instance; + static InstanceConstPtr createInstancePtrOrDie(Args&&... args) { + StatusOr error_or_address = + createInstancePtr(std::forward(args)...); + RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); + return move(*error_or_address); } }; @@ -166,18 +160,7 @@ class Ipv4Instance : public InstanceBase { std::string friendly_address_; }; - void initHelper(const sockaddr_in* address) { - memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); - ip_.ipv4_.address_ = *address; - ip_.friendly_address_ = sockaddrToString(*address); - - // Based on benchmark testing, this reserve+append implementation runs faster than absl::StrCat. - fmt::format_int port(ntohs(address->sin_port)); - friendly_name_.reserve(ip_.friendly_address_.size() + 1 + port.size()); - friendly_name_.append(ip_.friendly_address_); - friendly_name_.push_back(':'); - friendly_name_.append(port.data(), port.size()); - } + void initHelper(const sockaddr_in* address); IpHelper ip_; friend class InstanceFactory; @@ -264,6 +247,8 @@ class Ipv6Instance : public InstanceBase { std::string friendly_address_; }; + void initHelper(const sockaddr_in6& address, bool v6only); + IpHelper ip_; friend class InstanceFactory; }; @@ -307,8 +292,8 @@ class PipeInstance : public InstanceBase { /** * Construct from an existing unix address. */ - explicit PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, - mode_t mode = 0, const SocketInterface* sock_interface = nullptr); + PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0, + const SocketInterface* sock_interface = nullptr); struct PipeHelper : public Pipe { diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index dcbda6b1030f..455a5ecf9e69 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -170,7 +170,8 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { // Use a raw try here because it is used in both main thread and filter. - // Can not convert to use status code as there may be unexpected exceptions. + // Can not convert to use status code as there may be unexpected exceptions in server fuzz + // tests, which must be handled. try { callback_(resolution_status, std::move(address_list)); } catch (const EnvoyException& e) { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index d903b536d19a..a85ed39b9582 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -1,7 +1,6 @@ #include "common/network/io_socket_handle_impl.h" #include "envoy/buffer/buffer.h" -#include "envoy/common/exception.h" #include "common/api/os_sys_calls_impl.h" #include "common/common/utility.h" diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 54fff4751c7f..940500c6ac21 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -134,20 +134,14 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) { sa4.sin_family = AF_INET; sa4.sin_port = htons(port); - StatusOr error_or_address = - Address::InstanceFactory::createInstancePtr(&sa4); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - return *error_or_address; + return Address::InstanceFactory::createInstancePtrOrDie(&sa4); } sockaddr_in6 sa6; memset(&sa6, 0, sizeof(sa6)); if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) { sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port); - StatusOr error_or_address = - Address::InstanceFactory::createInstancePtr(sa6, v6only); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - return *error_or_address; + return Address::InstanceFactory::createInstancePtrOrDie(sa6, v6only); } return nullptr; } @@ -186,10 +180,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port64); - StatusOr error_or_address = - Address::InstanceFactory::createInstancePtr(sa6, v6only); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - return *error_or_address; + return Address::InstanceFactory::createInstancePtrOrDie(sa6, v6only); } // Treat it as an IPv4 address followed by a port. const auto pos = ip_address.rfind(':'); @@ -209,10 +200,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa4.sin_family = AF_INET; sa4.sin_port = htons(port64); - StatusOr error_or_address = - Address::InstanceFactory::createInstancePtr(&sa4); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - return *error_or_address; + return Address::InstanceFactory::createInstancePtrOrDie(&sa4); } Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address, From d4636ae97b16589b85702cdc20b0ee556542edbf Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 04:42:50 +0000 Subject: [PATCH 12/33] add comments Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 1f269a1fef0a..8fa1a16aaa29 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -64,6 +64,8 @@ class InstanceFactory { template static StatusOr createInstancePtr(Args&&... args) { absl::Status status; + // Use new instead of make_unique here because the ctors are private and must be called directly + // here. std::unique_ptr instance(new InstanceType(status, std::forward(args)...)); if (!status.ok()) { return status; From 42b98177ccade44ddf41b14d9730785080e8a3f9 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 13:28:43 +0000 Subject: [PATCH 13/33] fix typo Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 8fa1a16aaa29..179505817dea 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -64,8 +64,8 @@ class InstanceFactory { template static StatusOr createInstancePtr(Args&&... args) { absl::Status status; - // Use new instead of make_unique here because the ctors are private and must be called directly - // here. + // Use new instead of make_unique here because the instance constructors are private and must be + // called directly here. std::unique_ptr instance(new InstanceType(status, std::forward(args)...)); if (!status.ok()) { return status; From e932b99702b1168cadf2816f8c3fa1d7c94c1e5a Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 20:30:21 +0000 Subject: [PATCH 14/33] use ctors that throw in main thread and clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 9 +++++ source/common/network/address_impl.h | 4 +++ .../common/network/io_socket_handle_impl.cc | 4 +-- source/common/network/tcp_listener_impl.cc | 15 ++++---- source/common/network/utility.cc | 34 ++++++++++++++----- source/common/quic/envoy_quic_utils.cc | 16 ++++----- source/common/upstream/strict_dns_cluster.cc | 6 ++-- test/common/network/dns_impl_test.cc | 1 - 8 files changed, 58 insertions(+), 31 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index de501bf748b9..9697b6fcda85 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -77,6 +77,15 @@ StatusOr addressFromSockAddr(const sockaddr_sto NOT_REACHED_GCOVR_EXCL_LINE; } +Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, + socklen_t ss_len, bool v6only) { + StatusOr address = addressFromSockAddr(ss, ss_len, v6only); + if (!address.ok()) { + throw EnvoyException(address.status().ToString()); + } + return *address; +} + Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, bool v6only) { // Set v6only to false so that mapped-v6 address can be normalize to v4 diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 179505817dea..c44e59b51769 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -29,6 +29,8 @@ namespace Address { */ StatusOr addressFromSockAddr(const sockaddr_storage& ss, socklen_t len, bool v6only = true); +InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t len, + bool v6only = true); Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, @@ -135,6 +137,7 @@ class Ipv4Instance : public InstanceBase { private: /** * Construct from an existing unix IPv4 socket address (IP v4 address and port). + * Store the status code in passed in parameter instead of throwing. */ explicit Ipv4Instance(absl::Status& error, const sockaddr_in* address, const SocketInterface* sock_interface = nullptr); @@ -213,6 +216,7 @@ class Ipv6Instance : public InstanceBase { private: /** * Construct from an existing unix IPv6 socket address (IP v6 address and port). + * Store the status code in passed in parameter instead of throwing. */ Ipv6Instance(absl::Status& error, const sockaddr_in6& address, bool v6only = true, const SocketInterface* sock_interface = nullptr); diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index a85ed39b9582..ac12d4776668 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -586,7 +586,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_, errorDetails(result.errno_))); } - return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, socket_v6only_); + return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { @@ -611,7 +611,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, true); + return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index 5617fa607e2a..a13597775f48 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -82,14 +82,13 @@ void TcpListenerImpl::onSocketEvent(short flags) { // Pass the 'v6only' parameter as true if the local_address is an IPv6 address. This has no // effect if the socket is a v4 socket, but for v6 sockets this will create an IPv4 remote // address if an IPv4 local_address was created from an IPv6 mapped IPv4 address. - Address::InstanceConstSharedPtr remote_address; - if (remote_addr.ss_family == AF_UNIX) { - remote_address = io_handle->peerAddress(); - } else { - remote_address = Address::getAddressFromSockAddrOrDie(remote_addr, remote_addr_len, -1, - local_address->ip()->version() == - Address::IpVersion::v6); - } + + const Address::InstanceConstSharedPtr remote_address = + (remote_addr.ss_family == AF_UNIX) + ? io_handle->peerAddress() + : Address::addressFromSockAddrOrThrow(remote_addr, remote_addr_len, + local_address->ip()->version() == + Address::IpVersion::v6); cb_.onAccept( std::make_unique(std::move(io_handle), local_address, remote_address)); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 940500c6ac21..c40226849928 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -129,19 +129,28 @@ uint32_t Utility::portFromUdpUrl(const std::string& url) { Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std::string& ip_address, uint16_t port, bool v6only) { - Address::InstanceConstSharedPtr address; sockaddr_in sa4; if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) { sa4.sin_family = AF_INET; sa4.sin_port = htons(port); - return Address::InstanceFactory::createInstancePtrOrDie(&sa4); + StatusOr address = + Address::InstanceFactory::createInstancePtr(&sa4); + if (address.ok()) { + return *address; + } + return nullptr; } sockaddr_in6 sa6; memset(&sa6, 0, sizeof(sa6)); if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) { sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port); - return Address::InstanceFactory::createInstancePtrOrDie(sa6, v6only); + StatusOr address = + Address::InstanceFactory::createInstancePtr(sa6, v6only); + if (address.ok()) { + return *address; + } + return nullptr; } return nullptr; } @@ -180,7 +189,12 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port64); - return Address::InstanceFactory::createInstancePtrOrDie(sa6, v6only); + StatusOr address = + Address::InstanceFactory::createInstancePtr(sa6, v6only); + if (address.ok()) { + return *address; + } + return nullptr; } // Treat it as an IPv4 address followed by a port. const auto pos = ip_address.rfind(':'); @@ -200,7 +214,12 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa4.sin_family = AF_INET; sa4.sin_port = htons(port64); - return Address::InstanceFactory::createInstancePtrOrDie(&sa4); + StatusOr address = + Address::InstanceFactory::createInstancePtrOrDie(&sa4); + if (address.ok()) { + return *address; + } + return nullptr; } Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address, @@ -248,9 +267,8 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio (ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) { const struct sockaddr_storage* addr = reinterpret_cast(ifa->ifa_addr); - ret = Address::getAddressFromSockAddrOrDie( - *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6), - -1, true); + ret = Address::addressFromSockAddrOrThrow( + *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); if (!isLoopbackAddress(*ret)) { break; } diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index a2a9be6a3e32..fbfcd2ffb208 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -17,14 +17,14 @@ namespace Quic { // the heap allocation is too expensive. Network::Address::InstanceConstSharedPtr quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { - if (!quic_address.IsInitialized()) { - return nullptr; - } - return Network::Address::getAddressFromSockAddrOrDie( - quic_address.generic_address(), - quic_address.host().address_family() == quic::IpAddressFamily::IP_V4 ? sizeof(sockaddr_in) - : sizeof(sockaddr_in6), - -1, false); + return quic_address.IsInitialized() + ? Network::Address::addressFromSockAddrOrThrow(quic_address.generic_address(), + quic_address.host().address_family() == + quic::IpAddressFamily::IP_V4 + ? sizeof(sockaddr_in) + : sizeof(sockaddr_in6), + false) + : nullptr; } quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip) { diff --git a/source/common/upstream/strict_dns_cluster.cc b/source/common/upstream/strict_dns_cluster.cc index 3b0146bc3d98..726070b6389b 100644 --- a/source/common/upstream/strict_dns_cluster.cc +++ b/source/common/upstream/strict_dns_cluster.cc @@ -129,12 +129,10 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { // make a new address that has port in it. We need to both support IPv6 as well as // potentially move port handling into the DNS interface itself, which would work better // for SRV. - ASSERT(resp.address_ != nullptr); - Network::Address::InstanceConstSharedPtr new_address = - Network::Utility::getAddressWithPort(*(resp.address_), port_); new_hosts.emplace_back(new HostImpl( - parent_.info_, hostname_, new_address, + parent_.info_, hostname_, + Network::Utility::getAddressWithPort(*(resp.address_), port_), // TODO(zyfjeff): Created through metadata shared pool std::make_shared(lb_endpoint_.metadata()), lb_endpoint_.load_balancing_weight().value(), locality_lb_endpoints_.locality(), diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 58f74663832d..47476fa76a32 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -697,7 +697,6 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) { // If that flag needs to be set, or c-ares changes its handling this test will need to be updated // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status == // ARES_ECONNREFUSED. - EXPECT_TRUE(peer_->isChannelDirty()); server_->setRefused(false); From 42c5432796ca16fe198ef841e6f7fd1a4c4c1542 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 20:59:25 +0000 Subject: [PATCH 15/33] remove dns resolve try Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 7 ------- source/common/network/utility.cc | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index c44e59b51769..773e0809e9e9 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -74,13 +74,6 @@ class InstanceFactory { } return instance; } - template - static InstanceConstPtr createInstancePtrOrDie(Args&&... args) { - StatusOr error_or_address = - createInstancePtr(std::forward(args)...); - RELEASE_ASSERT(error_or_address.ok(), error_or_address.status().ToString()); - return move(*error_or_address); - } }; /** diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index c40226849928..6b94bee6f8f2 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -215,7 +215,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool sa4.sin_family = AF_INET; sa4.sin_port = htons(port64); StatusOr address = - Address::InstanceFactory::createInstancePtrOrDie(&sa4); + Address::InstanceFactory::createInstancePtr(&sa4); if (address.ok()) { return *address; } From b1a2675744dc8a29aa66b098a454baffd4e9d240 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 17 May 2021 21:38:14 +0000 Subject: [PATCH 16/33] refactor pipe instance ctor Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 26 +++++++++++--------------- source/common/network/address_impl.h | 2 ++ 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 9697b6fcda85..08f774967c5c 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -310,18 +310,10 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t pipe_.abstract_namespace_ = true; pipe_.address_length_ = ss_len - offsetof(struct sockaddr_un, sun_path); } - pipe_.address_ = *address; - if (pipe_.abstract_namespace_) { - if (mode != 0) { - throw EnvoyException("Cannot set mode for Abstract AF_UNIX sockets"); - } - // Replace all null characters with '@' in friendly_name_. - friendly_name_ = friendlyNameFromAbstractPath( - absl::string_view(pipe_.address_.sun_path, pipe_.address_length_)); - } else { - friendly_name_ = address->sun_path; + absl::Status status = initHelper(address, mode); + if (!status.ok()) { + throw EnvoyException(status.ToString()); } - pipe_.mode_ = mode; } PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, @@ -380,11 +372,16 @@ PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, sock pipe_.abstract_namespace_ = true; pipe_.address_length_ = ss_len - offsetof(struct sockaddr_un, sun_path); } + error = initHelper(address, mode); +} + +bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } + +absl::Status PipeInstance::initHelper(const sockaddr_un* address, mode_t mode) { pipe_.address_ = *address; if (pipe_.abstract_namespace_) { if (mode != 0) { - error = absl::FailedPreconditionError("Cannot set mode for Abstract AF_UNIX sockets"); - return; + return absl::FailedPreconditionError("Cannot set mode for Abstract AF_UNIX sockets"); } // Replace all null characters with '@' in friendly_name_. friendly_name_ = friendlyNameFromAbstractPath( @@ -393,10 +390,9 @@ PipeInstance::PipeInstance(absl::Status& error, const sockaddr_un* address, sock friendly_name_ = address->sun_path; } pipe_.mode_ = mode; + return absl::OkStatus(); } -bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } - EnvoyInternalInstance::EnvoyInternalInstance(const std::string& address_id, const SocketInterface* sock_interface) : InstanceBase(Type::EnvoyInternal, sockInterfaceOrDefault(sock_interface)), diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 773e0809e9e9..d05314973b4a 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -306,6 +306,8 @@ class PipeInstance : public InstanceBase { mode_t mode_{0}; }; + absl::Status initHelper(const sockaddr_un* address, mode_t mode); + PipeHelper pipe_; friend class InstanceFactory; }; From 59c1329ff5923ea8760691288504ad800932a31d Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 18 May 2021 15:32:56 +0000 Subject: [PATCH 17/33] main thread assertion int helper Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 48 +++++++++---------- source/common/network/address_impl.h | 1 + source/common/network/dns_impl.cc | 5 +- .../common/network/io_socket_handle_impl.cc | 4 +- source/common/network/tcp_listener_impl.cc | 6 +-- source/common/network/utility.cc | 32 ++++++------- source/common/quic/envoy_quic_utils.cc | 12 ++--- 7 files changed, 52 insertions(+), 56 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 08f774967c5c..a737b6a1af7b 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -31,6 +31,19 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf return sock_interface == nullptr ? &SocketInterfaceSingleton::get() : sock_interface; } +void throwOnError(StatusOr address) { + ASSERT(Thread::MainThread::isMainThread()); + if (!address.ok()) { + throw EnvoyException(address.status().ToString()); + } +} + +void throwOnError(absl::Status address) { + if (!address.ok()) { + throw EnvoyException(address.ToString()); + } +} + } // namespace StatusOr addressFromSockAddr(const sockaddr_storage& ss, @@ -80,9 +93,7 @@ StatusOr addressFromSockAddr(const sockaddr_sto Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t ss_len, bool v6only) { StatusOr address = addressFromSockAddr(ss, ss_len, v6only); - if (!address.ok()) { - throw EnvoyException(address.status().ToString()); - } + throwOnError(address); return *address; } @@ -96,20 +107,17 @@ getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_ // address and the socket is actually v6 only, the returned address will be // regarded as a v6 address from dual stack socket. However, this address is not going to be // used to create socket. Wrong knowledge of dual stack support won't hurt. - StatusOr error_or_addr = + StatusOr address = Address::addressFromSockAddr(ss, ss_len, v6only); - if (!error_or_addr.ok()) { + if (!address.ok()) { PANIC(fmt::format("Invalid address for fd: {}", fd)); } - return *error_or_addr; + return *address; } Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(validateProtocolSupported()); initHelper(address); } @@ -119,10 +127,7 @@ Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* so Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(validateProtocolSupported()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -137,10 +142,7 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port, Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(validateProtocolSupported()); memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_)); ip_.ipv4_.address_.sin_family = AF_INET; ip_.ipv4_.address_.sin_port = htons(port); @@ -233,10 +235,7 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const { Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(validateProtocolSupported()); initHelper(address, v6only); } @@ -246,10 +245,7 @@ Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* so Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port, const SocketInterface* sock_interface) : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) { - absl::Status status = validateProtocolSupported(); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(validateProtocolSupported()); ip_.ipv6_.address_.sin6_family = AF_INET6; ip_.ipv6_.address_.sin6_port = htons(port); if (!address.empty()) { diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index d05314973b4a..b9b18a705223 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -290,6 +290,7 @@ class PipeInstance : public InstanceBase { private: /** * Construct from an existing unix address. + * Store the status code in passed in parameter instead of throwing. */ PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0, const SocketInterface* sock_interface = nullptr); diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 455a5ecf9e69..35c006330fbd 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -171,7 +171,10 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (!cancelled_) { // Use a raw try here because it is used in both main thread and filter. // Can not convert to use status code as there may be unexpected exceptions in server fuzz - // tests, which must be handled. + // tests, which must be handled. Potential exception may come from getAddressWithPort() or + // portFromTcpUrl(). + // TODO(chaoqin-li1123): remove try catch pattern here once we figure how to handle unexpected + // exception in fuzz tests. try { callback_(resolution_status, std::move(address_list)); } catch (const EnvoyException& e) { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index ac12d4776668..2fe812d1bb8a 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -586,7 +586,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_, errorDetails(result.errno_))); } - return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); + return Address::getAddressFromSockAddrOrDie(ss, ss_len, fd_, socket_v6only_); } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { @@ -611,7 +611,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); + return Address::getAddressFromSockAddrOrDie(ss, ss_len, fd_, socket_v6only_); } void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index a13597775f48..e814b7ae1986 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -86,9 +86,9 @@ void TcpListenerImpl::onSocketEvent(short flags) { const Address::InstanceConstSharedPtr remote_address = (remote_addr.ss_family == AF_UNIX) ? io_handle->peerAddress() - : Address::addressFromSockAddrOrThrow(remote_addr, remote_addr_len, - local_address->ip()->version() == - Address::IpVersion::v6); + : Address::getAddressFromSockAddrOrDie(remote_addr, remote_addr_len, -1, + local_address->ip()->version() == + Address::IpVersion::v6); cb_.onAccept( std::make_unique(std::move(io_handle), local_address, remote_address)); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 6b94bee6f8f2..2782c689d64e 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -32,6 +32,13 @@ namespace Envoy { namespace Network { +Address::InstanceConstSharedPtr instanceOrNull(StatusOr address) { + if (address.ok()) { + return *address; + } + return nullptr; +} + Address::InstanceConstSharedPtr Utility::resolveUrl(const std::string& url) { if (urlIsTcpScheme(url)) { return parseInternetAddressAndPort(url.substr(TCP_SCHEME.size())); @@ -135,10 +142,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: sa4.sin_port = htons(port); StatusOr address = Address::InstanceFactory::createInstancePtr(&sa4); - if (address.ok()) { - return *address; - } - return nullptr; + return instanceOrNull(address); } sockaddr_in6 sa6; memset(&sa6, 0, sizeof(sa6)); @@ -147,10 +151,7 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: sa6.sin6_port = htons(port); StatusOr address = Address::InstanceFactory::createInstancePtr(sa6, v6only); - if (address.ok()) { - return *address; - } - return nullptr; + return instanceOrNull(address); } return nullptr; } @@ -191,10 +192,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool sa6.sin6_port = htons(port64); StatusOr address = Address::InstanceFactory::createInstancePtr(sa6, v6only); - if (address.ok()) { - return *address; - } - return nullptr; + return instanceOrNull(address); } // Treat it as an IPv4 address followed by a port. const auto pos = ip_address.rfind(':'); @@ -216,10 +214,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool sa4.sin_port = htons(port64); StatusOr address = Address::InstanceFactory::createInstancePtr(&sa4); - if (address.ok()) { - return *address; - } - return nullptr; + return instanceOrNull(address); } Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address, @@ -267,8 +262,9 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio (ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) { const struct sockaddr_storage* addr = reinterpret_cast(ifa->ifa_addr); - ret = Address::addressFromSockAddrOrThrow( - *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); + ret = Address::getAddressFromSockAddrOrDie( + *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6), + -1); if (!isLoopbackAddress(*ret)) { break; } diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index fbfcd2ffb208..e7d935c88bce 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -18,12 +18,12 @@ namespace Quic { Network::Address::InstanceConstSharedPtr quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { return quic_address.IsInitialized() - ? Network::Address::addressFromSockAddrOrThrow(quic_address.generic_address(), - quic_address.host().address_family() == - quic::IpAddressFamily::IP_V4 - ? sizeof(sockaddr_in) - : sizeof(sockaddr_in6), - false) + ? Network::Address::getAddressFromSockAddrOrDie(quic_address.generic_address(), + quic_address.host().address_family() == + quic::IpAddressFamily::IP_V4 + ? sizeof(sockaddr_in) + : sizeof(sockaddr_in6), + -1, false) : nullptr; } From f4b4aacda965bb35ad2f28a95b77e2c3dcc6d0fc Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 18 May 2021 16:49:51 +0000 Subject: [PATCH 18/33] add comment Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index b9b18a705223..4c997c8170c7 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -32,9 +32,18 @@ StatusOr addressFromSockAddr(const sockaddr_storage& ss, InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t len, bool v6only = true); -Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, - socklen_t ss_len, os_fd_t fd, - bool v6only = true); +/** + * Convert an address in the form of the socket address struct defined by Posix, Linux, etc. into + * a Network::Address::Instance and return a pointer to it. Die on failure. + * @param ss a valid address with family AF_INET, AF_INET6 or AF_UNIX. + * @param len length of the address (e.g. from accept, getsockname or getpeername). If len > 0, + * it is used to validate the structure contents; else if len == 0, it is ignored. + * @param fd the file descriptor for the created address instance. + * @param v6only disable IPv4-IPv6 mapping for IPv6 addresses? + * @return InstanceConstSharedPtr the address. + */ +InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, + os_fd_t fd, bool v6only = true); /** * Base class for all address types. From 010fb7c75627c38ed149a0982f06dccecfa63628 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 20 May 2021 16:54:37 +0000 Subject: [PATCH 19/33] refactor throw on error Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index a737b6a1af7b..398b7810c9e0 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -31,17 +31,18 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf return sock_interface == nullptr ? &SocketInterfaceSingleton::get() : sock_interface; } -void throwOnError(StatusOr address) { - ASSERT(Thread::MainThread::isMainThread()); +void throwOnError(absl::Status address) { if (!address.ok()) { - throw EnvoyException(address.status().ToString()); + throw EnvoyException(address.ToString()); } } -void throwOnError(absl::Status address) { +InstanceConstSharedPtr throwOnError(StatusOr address) { + ASSERT(Thread::MainThread::isMainThread()); if (!address.ok()) { - throw EnvoyException(address.ToString()); + throwOnError(address.status()); } + return *address; } } // namespace @@ -93,8 +94,7 @@ StatusOr addressFromSockAddr(const sockaddr_sto Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t ss_len, bool v6only) { StatusOr address = addressFromSockAddr(ss, ss_len, v6only); - throwOnError(address); - return *address; + return throwOnError(address); } Address::InstanceConstSharedPtr From 33579ac139f1dfe135d9a471b1175389dbf485d3 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 20 May 2021 21:03:11 +0000 Subject: [PATCH 20/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 +- source/common/upstream/logical_dns_cluster.cc | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 398b7810c9e0..676ea6952a47 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -110,7 +110,7 @@ getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_ StatusOr address = Address::addressFromSockAddr(ss, ss_len, v6only); if (!address.ok()) { - PANIC(fmt::format("Invalid address for fd: {}", fd)); + PANIC(fmt::format("Invalid address for fd: {}, error: {}", fd, address.status().ToString())); } return *address; } diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 68b3a2842c50..a0a2cfb0f37b 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -120,9 +120,12 @@ void LogicalDnsCluster::startResolve() { // not stabilize back to 0 hosts. if (status == Network::DnsResolver::ResolutionStatus::Success && !response.empty()) { info_->stats().update_success_.inc(); + // TODO(mattklein123): Move port handling into the DNS interface. + ASSERT(response.front().address_ != nullptr); Network::Address::InstanceConstSharedPtr new_address = Network::Utility::getAddressWithPort(*(response.front().address_), Network::Utility::portFromTcpUrl(dns_url_)); + if (!logical_host_) { logical_host_ = std::make_shared(info_, hostname_, new_address, localityLbEndpoint(), From 1f574db694860c61fe0b43bb308e57ac30285d19 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 21 May 2021 20:58:40 +0000 Subject: [PATCH 21/33] ... Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 8 +++++++ source/common/network/address_impl.cc | 2 +- .../common/network/io_socket_handle_impl.cc | 24 +++++++++++++++---- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index e84fb7b38bf7..05a38579e1ac 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -194,6 +194,14 @@ struct MainThread { */ static void clear(); static bool isMainThread(); + static bool isNotMainThread() { + auto main_thread_singleton = MainThreadSingleton::getExisting(); + if (main_thread_singleton == nullptr) { + return true; + } + // When threading is on, compare thread id with main thread id. + return !main_thread_singleton->inMainThread(); + } private: std::thread::id main_thread_id_; diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 676ea6952a47..895be3e1053e 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -32,13 +32,13 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf } void throwOnError(absl::Status address) { + // ASSERT(Thread::MainThread::isMainThread()); if (!address.ok()) { throw EnvoyException(address.ToString()); } } InstanceConstSharedPtr throwOnError(StatusOr address) { - ASSERT(Thread::MainThread::isMainThread()); if (!address.ok()) { throwOnError(address.status()); } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 2fe812d1bb8a..69d1949f379b 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -9,6 +9,7 @@ #include "absl/container/fixed_array.h" #include "absl/types/optional.h" +#include "envoy/common/exception.h" using Envoy::Api::SysCallIntResult; using Envoy::Api::SysCallSizeResult; @@ -577,27 +578,38 @@ IoHandlePtr IoSocketHandleImpl::duplicate() { absl::optional IoSocketHandleImpl::domain() { return domain_; } Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { + ASSERT(Thread::MainThread::isNotMainThread()); sockaddr_storage ss; socklen_t ss_len = sizeof(ss); 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_, errorDetails(result.errno_))); + */ + ASSERT(false); + return nullptr; } - return Address::getAddressFromSockAddrOrDie(ss, ss_len, fd_, socket_v6only_); + // throw EnvoyException("miao miao miao\n"); + return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { + ASSERT(Thread::MainThread::isNotMainThread()); sockaddr_storage ss; socklen_t ss_len = sizeof ss; 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_, errorDetails(result.errno_))); + fmt::format("getpeername failed for '{}': {}", errorDetails(result.errno_))); + */ + ASSERT(false); + return nullptr; } if (ss_len == udsAddressLength() && ss.ss_family == AF_UNIX) { @@ -607,11 +619,15 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { ss_len = sizeof ss; result = os_sys_calls.getsockname(fd_, reinterpret_cast(&ss), &ss_len); if (result.rc_ != 0) { + /* throw EnvoyException( - fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); + fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_)));*/ + ASSERT(false); + return nullptr; } } - return Address::getAddressFromSockAddrOrDie(ss, ss_len, fd_, socket_v6only_); + // throw EnvoyException("miao miao miao\n"); + return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, From 59224a013474e31a187daa16ac060055ff8ff78a Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 24 May 2021 16:30:36 +0000 Subject: [PATCH 22/33] add assertion to OrDie Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 +- source/common/network/io_socket_handle_impl.cc | 18 ++---------------- source/common/network/tcp_listener_impl.cc | 6 +++--- source/common/network/utility.cc | 5 ++--- 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 895be3e1053e..548d57577289 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -32,7 +32,6 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf } void throwOnError(absl::Status address) { - // ASSERT(Thread::MainThread::isMainThread()); if (!address.ok()) { throw EnvoyException(address.ToString()); } @@ -107,6 +106,7 @@ getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_ // address and the socket is actually v6 only, the returned address will be // regarded as a v6 address from dual stack socket. However, this address is not going to be // used to create socket. Wrong knowledge of dual stack support won't hurt. + ASSERT(Thread::MainThread::isNotMainThread()); StatusOr address = Address::addressFromSockAddr(ss, ss_len, v6only); if (!address.ok()) { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 69d1949f379b..8c513b92718f 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -1,6 +1,7 @@ #include "common/network/io_socket_handle_impl.h" #include "envoy/buffer/buffer.h" +#include "envoy/common/exception.h" #include "common/api/os_sys_calls_impl.h" #include "common/common/utility.h" @@ -9,7 +10,6 @@ #include "absl/container/fixed_array.h" #include "absl/types/optional.h" -#include "envoy/common/exception.h" using Envoy::Api::SysCallIntResult; using Envoy::Api::SysCallSizeResult; @@ -578,37 +578,27 @@ IoHandlePtr IoSocketHandleImpl::duplicate() { absl::optional IoSocketHandleImpl::domain() { return domain_; } Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() { - ASSERT(Thread::MainThread::isNotMainThread()); sockaddr_storage ss; socklen_t ss_len = sizeof(ss); 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_, errorDetails(result.errno_))); - */ - ASSERT(false); - return nullptr; } - // throw EnvoyException("miao miao miao\n"); return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { - ASSERT(Thread::MainThread::isNotMainThread()); sockaddr_storage ss; socklen_t ss_len = sizeof ss; 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 '{}': {}", errorDetails(result.errno_))); - */ - ASSERT(false); return nullptr; } @@ -619,14 +609,10 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { ss_len = sizeof ss; result = os_sys_calls.getsockname(fd_, reinterpret_cast(&ss), &ss_len); if (result.rc_ != 0) { - /* throw EnvoyException( - fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_)));*/ - ASSERT(false); - return nullptr; + fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_))); } } - // throw EnvoyException("miao miao miao\n"); return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_); } diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index e814b7ae1986..a13597775f48 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -86,9 +86,9 @@ void TcpListenerImpl::onSocketEvent(short flags) { const Address::InstanceConstSharedPtr remote_address = (remote_addr.ss_family == AF_UNIX) ? io_handle->peerAddress() - : Address::getAddressFromSockAddrOrDie(remote_addr, remote_addr_len, -1, - local_address->ip()->version() == - Address::IpVersion::v6); + : Address::addressFromSockAddrOrThrow(remote_addr, remote_addr_len, + local_address->ip()->version() == + Address::IpVersion::v6); cb_.onAccept( std::make_unique(std::move(io_handle), local_address, remote_address)); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 2782c689d64e..896d6daad839 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -262,9 +262,8 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio (ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) { const struct sockaddr_storage* addr = reinterpret_cast(ifa->ifa_addr); - ret = Address::getAddressFromSockAddrOrDie( - *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6), - -1); + ret = Address::addressFromSockAddrOrThrow( + *addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6)); if (!isLoopbackAddress(*ret)) { break; } From abc66de8143a96bd0bb783597c1b45bd2d0a3749 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 3 Jun 2021 02:36:52 +0000 Subject: [PATCH 23/33] assert worker thread for OrDie Signed-off-by: chaoqin-li1123 --- source/common/common/thread.cc | 9 +++++++++ source/common/common/thread.h | 9 +-------- source/common/network/address_impl.cc | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/source/common/common/thread.cc b/source/common/common/thread.cc index d32907be0676..7a89c523fc45 100644 --- a/source/common/common/thread.cc +++ b/source/common/common/thread.cc @@ -13,6 +13,15 @@ bool MainThread::isMainThread() { return main_thread_singleton->inMainThread() || main_thread_singleton->inTestThread(); } +bool MainThread::isWorkerThread() { + auto main_thread_singleton = MainThreadSingleton::getExisting(); + if (main_thread_singleton == nullptr) { + return true; + } + // When threading is on, compare thread id with main thread id. + return !main_thread_singleton->inMainThread(); +} + void MainThread::clear() { delete MainThreadSingleton::getExisting(); MainThreadSingleton::clear(); diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 05a38579e1ac..42b8ef5f9073 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -194,14 +194,7 @@ struct MainThread { */ static void clear(); static bool isMainThread(); - static bool isNotMainThread() { - auto main_thread_singleton = MainThreadSingleton::getExisting(); - if (main_thread_singleton == nullptr) { - return true; - } - // When threading is on, compare thread id with main thread id. - return !main_thread_singleton->inMainThread(); - } + static bool isWorkerThread(); private: std::thread::id main_thread_id_; diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 548d57577289..6cef003bf58b 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -106,7 +106,7 @@ getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_ // address and the socket is actually v6 only, the returned address will be // regarded as a v6 address from dual stack socket. However, this address is not going to be // used to create socket. Wrong knowledge of dual stack support won't hurt. - ASSERT(Thread::MainThread::isNotMainThread()); + ASSERT(Thread::MainThread::isWorkerThread()); StatusOr address = Address::addressFromSockAddr(ss, ss_len, v6only); if (!address.ok()) { From 50d1cfe5d0f012606091348ff5e2c5e411c983cc Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 4 Jun 2021 16:58:04 +0000 Subject: [PATCH 24/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 4 +--- source/common/network/io_socket_handle_impl.cc | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 6cef003bf58b..7f66af0bb1a4 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -307,9 +307,7 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t pipe_.address_length_ = ss_len - offsetof(struct sockaddr_un, sun_path); } absl::Status status = initHelper(address, mode); - if (!status.ok()) { - throw EnvoyException(status.ToString()); - } + throwOnError(status); } PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode, diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 8c513b92718f..31f8301da648 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -599,7 +599,6 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { if (result.rc_ != 0) { throw EnvoyException( fmt::format("getpeername failed for '{}': {}", errorDetails(result.errno_))); - return nullptr; } if (ss_len == udsAddressLength() && ss.ss_family == AF_UNIX) { From 6aa09858cf84e4fc9c01d6da9eed9fd40a9a3610 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 11 Jun 2021 20:48:04 +0000 Subject: [PATCH 25/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/utility.h | 2 -- source/common/upstream/logical_dns_cluster.cc | 1 - 2 files changed, 3 deletions(-) diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 6e02cf13bbd3..c892a8efb4bb 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -9,8 +9,6 @@ #include "envoy/network/connection.h" #include "envoy/network/listener.h" -#include "source/common/common/statusor.h" - #include "absl/strings/string_view.h" namespace Envoy { diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 63db5918a788..7424ba44fbb5 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -10,7 +10,6 @@ #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/config/core/v3/address.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" -#include "envoy/network/address.h" #include "envoy/stats/scope.h" #include "source/common/common/fmt.h" From cc13e11d6a2d5ae65390b2562373e26af82fe1a5 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 11 Jun 2021 21:15:07 +0000 Subject: [PATCH 26/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 1 - source/common/network/io_socket_handle_impl.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index dcda67e21096..39d5e97d3514 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -12,7 +12,6 @@ #include "source/common/common/assert.h" #include "source/common/common/statusor.h" -#include "source/common/common/thread.h" namespace Envoy { namespace Network { diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 162206366974..b95ee5165dfa 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -1,7 +1,6 @@ #include "source/common/network/io_socket_handle_impl.h" #include "envoy/buffer/buffer.h" -#include "envoy/common/exception.h" #include "source/common/api/os_sys_calls_impl.h" #include "source/common/common/utility.h" From a6b1cabbc455638348a7f79dc05355c3f4011e97 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 15 Jun 2021 16:57:42 +0000 Subject: [PATCH 27/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/common/thread.cc | 1 + source/common/network/address_impl.cc | 12 ++++++++---- source/common/network/utility.cc | 18 ++++++------------ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/source/common/common/thread.cc b/source/common/common/thread.cc index 576645a07e1a..282858399000 100644 --- a/source/common/common/thread.cc +++ b/source/common/common/thread.cc @@ -15,6 +15,7 @@ bool MainThread::isMainThread() { bool MainThread::isWorkerThread() { auto main_thread_singleton = MainThreadSingleton::getExisting(); + // Allow worker thread code to be executed in test thread. if (main_thread_singleton == nullptr) { return true; } diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index fec6cbca659e..41a4c7f5d347 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -31,9 +31,9 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf return sock_interface == nullptr ? &SocketInterfaceSingleton::get() : sock_interface; } -void throwOnError(absl::Status address) { - if (!address.ok()) { - throw EnvoyException(address.ToString()); +void throwOnError(absl::Status status) { + if (!status.ok()) { + throw EnvoyException(status.ToString()); } } @@ -41,7 +41,7 @@ InstanceConstSharedPtr throwOnError(StatusOr address) { if (!address.ok()) { throwOnError(address.status()); } - return *address; + return *address;git checkout } } // namespace @@ -92,6 +92,10 @@ StatusOr addressFromSockAddr(const sockaddr_sto Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t ss_len, bool v6only) { + // Though we don't have any test coverage where address validation in addressFromSockAddr() fails, + // this code is called in worker thread and can throw in theory. In that case, the program will + // crash due to uncaught exception. In pratice, we don't expect any address validation in + // addressFromSockAddr() to fail in worker thread. StatusOr address = addressFromSockAddr(ss, ss_len, v6only); return throwOnError(address); } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index aa71d28ca1f3..23eeadf099b5 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -141,18 +141,15 @@ Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std:: if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) { sa4.sin_family = AF_INET; sa4.sin_port = htons(port); - StatusOr address = - Address::InstanceFactory::createInstancePtr(&sa4); - return instanceOrNull(address); + return instanceOrNull(Address::InstanceFactory::createInstancePtr(&sa4)); } sockaddr_in6 sa6; memset(&sa6, 0, sizeof(sa6)); if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) { sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port); - StatusOr address = - Address::InstanceFactory::createInstancePtr(sa6, v6only); - return instanceOrNull(address); + return instanceOrNull( + Address::InstanceFactory::createInstancePtr(sa6, v6only)); } return nullptr; } @@ -191,9 +188,8 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa6.sin6_family = AF_INET6; sa6.sin6_port = htons(port64); - StatusOr address = - Address::InstanceFactory::createInstancePtr(sa6, v6only); - return instanceOrNull(address); + return instanceOrNull( + Address::InstanceFactory::createInstancePtr(sa6, v6only)); } // Treat it as an IPv4 address followed by a port. const auto pos = ip_address.rfind(':'); @@ -213,9 +209,7 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool } sa4.sin_family = AF_INET; sa4.sin_port = htons(port64); - StatusOr address = - Address::InstanceFactory::createInstancePtr(&sa4); - return instanceOrNull(address); + return instanceOrNull(Address::InstanceFactory::createInstancePtr(&sa4)); } Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address, From d2dde83173494174e3ae22259715913a07cae73f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 15 Jun 2021 16:58:23 +0000 Subject: [PATCH 28/33] clean up Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 41a4c7f5d347..f4b6fce62b9a 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -41,7 +41,7 @@ InstanceConstSharedPtr throwOnError(StatusOr address) { if (!address.ok()) { throwOnError(address.status()); } - return *address;git checkout + return *address; } } // namespace From f686b733a6ddcc7884c55aeb07ff33559db9ea7e Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 15 Jun 2021 20:50:01 +0000 Subject: [PATCH 29/33] add comments Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 +- tools/code_format/check_format.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index f4b6fce62b9a..128501f16df3 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -94,7 +94,7 @@ Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storag socklen_t ss_len, bool v6only) { // Though we don't have any test coverage where address validation in addressFromSockAddr() fails, // this code is called in worker thread and can throw in theory. In that case, the program will - // crash due to uncaught exception. In pratice, we don't expect any address validation in + // crash due to uncaught exception. In practice, we don't expect any address validation in // addressFromSockAddr() to fail in worker thread. StatusOr address = addressFromSockAddr(ss, ss_len, v6only); return throwOnError(address); diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index fd0559917622..c7818dbd36f4 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -117,6 +117,8 @@ EXCEPTION_DENYLIST = ( "./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") +# Files that are allowed to use try without main thread assertion. +# TODO(chaoqin-li1123): remove try catch pattern in dns_impl.cc. RAW_TRY_ALLOWLIST = ( "./source/common/common/regex.cc", "./source/common/common/thread.h", "./source/common/network/utility.cc", "./source/common/network/dns_impl.cc") From cee98345cef319ec9a59c1707791d7b72f262c08 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 17 Jun 2021 15:30:18 +0000 Subject: [PATCH 30/33] reverse raw try Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 11 ++++++----- tools/code_format/check_format.py | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 95a5d929abd5..8d5edd28d6b1 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -180,15 +180,16 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i // portFromTcpUrl(). // TODO(chaoqin-li1123): remove try catch pattern here once we figure how to handle unexpected // exception in fuzz tests. - try { - callback_(resolution_status, std::move(address_list)); - } catch (const EnvoyException& e) { + TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index c7818dbd36f4..5ececebd31f6 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -118,10 +118,9 @@ "./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") # Files that are allowed to use try without main thread assertion. -# TODO(chaoqin-li1123): remove try catch pattern in dns_impl.cc. RAW_TRY_ALLOWLIST = ( "./source/common/common/regex.cc", "./source/common/common/thread.h", - "./source/common/network/utility.cc", "./source/common/network/dns_impl.cc") + "./source/common/network/utility.cc") # These are entire files that are allowed to use std::string_view vs. individual exclusions. Right # now this is just WASM which makes use of std::string_view heavily so we need to convert to From e564aff7b775611279bc91b38fd8c848c6022dbf Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 18 Jun 2021 16:23:14 +0000 Subject: [PATCH 31/33] add comment Signed-off-by: chaoqin-li1123 --- envoy/network/address.h | 1 - source/common/network/address_impl.h | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/envoy/network/address.h b/envoy/network/address.h index 4a305be74694..bd28205bd630 100644 --- a/envoy/network/address.h +++ b/envoy/network/address.h @@ -209,7 +209,6 @@ class Instance { }; using InstanceConstSharedPtr = std::shared_ptr; -using InstanceConstPtr = std::unique_ptr; } // namespace Address } // namespace Network diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 39d5e97d3514..f1b011a8917e 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -69,14 +69,15 @@ class InstanceBase : public Instance { const Type type_; }; +// Create an address instance. Upon failure, return an error status without throwing. class InstanceFactory { public: template - static StatusOr createInstancePtr(Args&&... args) { + static StatusOr createInstancePtr(Args&&... args) { absl::Status status; // Use new instead of make_unique here because the instance constructors are private and must be // called directly here. - std::unique_ptr instance(new InstanceType(status, std::forward(args)...)); + std::shared_ptr instance(new InstanceType(status, std::forward(args)...)); if (!status.ok()) { return status; } From 83d2b399234ae76aa4ebaf26a718b3a343878f66 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 18 Jun 2021 16:24:19 +0000 Subject: [PATCH 32/33] add comment Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index f1b011a8917e..cd6248236cec 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -75,7 +75,7 @@ class InstanceFactory { template static StatusOr createInstancePtr(Args&&... args) { absl::Status status; - // Use new instead of make_unique here because the instance constructors are private and must be + // Use new instead of make_shared here because the instance constructors are private and must be // called directly here. std::shared_ptr instance(new InstanceType(status, std::forward(args)...)); if (!status.ok()) { From c498b05457268571c8456c9e9d8f8e8ebfc92c70 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 21 Jun 2021 05:39:07 +0000 Subject: [PATCH 33/33] add comment Signed-off-by: chaoqin-li1123 --- source/common/network/address_impl.cc | 2 +- source/common/network/address_impl.h | 12 +++++++++--- source/common/network/io_socket_handle_impl.cc | 9 ++++----- source/common/network/utility.cc | 3 +-- source/common/quic/envoy_quic_utils.cc | 12 ++++++------ 5 files changed, 21 insertions(+), 17 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 128501f16df3..e612505d8336 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -101,7 +101,7 @@ Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storag } Address::InstanceConstSharedPtr -getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, bool v6only) { +addressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, bool v6only) { // Set v6only to false so that mapped-v6 address can be normalize to v4 // address. Though dual stack may be disabled, it's still okay to assume the // address is from a dual stack socket. This is because mapped-v6 address diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index cd6248236cec..2ade85fc8998 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -41,8 +41,8 @@ InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, so * @param v6only disable IPv4-IPv6 mapping for IPv6 addresses? * @return InstanceConstSharedPtr the address. */ -InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, - os_fd_t fd, bool v6only = true); +InstanceConstSharedPtr addressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, + os_fd_t fd, bool v6only = true); /** * Base class for all address types. @@ -140,6 +140,8 @@ class Ipv4Instance : public InstanceBase { /** * Construct from an existing unix IPv4 socket address (IP v4 address and port). * Store the status code in passed in parameter instead of throwing. + * It is called by the factory method and the partially constructed instance will be discarded + * upon error. */ explicit Ipv4Instance(absl::Status& error, const sockaddr_in* address, const SocketInterface* sock_interface = nullptr); @@ -219,6 +221,8 @@ class Ipv6Instance : public InstanceBase { /** * Construct from an existing unix IPv6 socket address (IP v6 address and port). * Store the status code in passed in parameter instead of throwing. + * It is called by the factory method and the partially constructed instance will be discarded + * upon error. */ Ipv6Instance(absl::Status& error, const sockaddr_in6& address, bool v6only = true, const SocketInterface* sock_interface = nullptr); @@ -299,7 +303,9 @@ class PipeInstance : public InstanceBase { private: /** * Construct from an existing unix address. - * Store the status code in passed in parameter instead of throwing. + * Store the error status code in passed in parameter instead of throwing. + * It is called by the factory method and the partially constructed instance will be discarded + * upon error. */ PipeInstance(absl::Status& error, const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0, const SocketInterface* sock_interface = nullptr); diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index b95ee5165dfa..8f2d2e0b5687 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -285,7 +285,7 @@ Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg ipv6_addr->sin6_family = AF_INET6; ipv6_addr->sin6_addr = info->ipi6_addr; ipv6_addr->sin6_port = htons(self_port); - return Address::getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in6), fd); + return Address::addressFromSockAddrOrDie(ss, sizeof(sockaddr_in6), fd); } if (cmsg.cmsg_type == messageTypeContainsIP()) { @@ -295,7 +295,7 @@ Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg ipv4_addr->sin_family = AF_INET; ipv4_addr->sin_addr = addressFromMessage(cmsg); ipv4_addr->sin_port = htons(self_port); - return Address::getAddressFromSockAddrOrDie(ss, sizeof(sockaddr_in), fd); + return Address::addressFromSockAddrOrDie(ss, sizeof(sockaddr_in), fd); } return nullptr; @@ -365,8 +365,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, fmt::format("Incorrectly set control message length: {}", hdr.msg_controllen)); RELEASE_ASSERT(hdr.msg_namelen > 0, fmt::format("Unable to get remote address from recvmsg() for fd: {}", fd_)); - output.msg_[0].peer_address_ = - Address::getAddressFromSockAddrOrDie(peer_addr, hdr.msg_namelen, fd_); + output.msg_[0].peer_address_ = Address::addressFromSockAddrOrDie(peer_addr, hdr.msg_namelen, fd_); output.msg_[0].gso_size_ = 0; if (hdr.msg_controllen > 0) { @@ -473,7 +472,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin output.msg_[i].msg_len_ = mmsg_hdr[i].msg_len; // Get local and peer addresses for each packet. output.msg_[i].peer_address_ = - Address::getAddressFromSockAddrOrDie(raw_addresses[i], hdr.msg_namelen, fd_); + Address::addressFromSockAddrOrDie(raw_addresses[i], hdr.msg_namelen, fd_); if (hdr.msg_controllen > 0) { struct cmsghdr* cmsg; for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(&hdr, cmsg)) { diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 23eeadf099b5..bfca08c8897f 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -415,8 +415,7 @@ Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) { return nullptr; } - return Address::getAddressFromSockAddrOrDie(orig_addr, 0, -1, - true /* default for v6 constructor */); + return Address::addressFromSockAddrOrDie(orig_addr, 0, -1, true /* default for v6 constructor */); #else // TODO(zuercher): determine if connection redirection is possible under macOS (c.f. pfctl and diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 7fa3126e3d8a..769f8cf08649 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -17,12 +17,12 @@ namespace Quic { Network::Address::InstanceConstSharedPtr quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) { return quic_address.IsInitialized() - ? Network::Address::getAddressFromSockAddrOrDie(quic_address.generic_address(), - quic_address.host().address_family() == - quic::IpAddressFamily::IP_V4 - ? sizeof(sockaddr_in) - : sizeof(sockaddr_in6), - -1, false) + ? Network::Address::addressFromSockAddrOrDie(quic_address.generic_address(), + quic_address.host().address_family() == + quic::IpAddressFamily::IP_V4 + ? sizeof(sockaddr_in) + : sizeof(sockaddr_in6), + -1, false) : nullptr; }