address: removing some exceptions (#36754)

Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Nov 2, 2024 · fcdc9d6 · fcdc9d6
1 parent 72b7507
commit fcdc9d6
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 45 deletions.
diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc
@@ -32,19 +32,6 @@ const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interf
   return sock_interface == nullptr ? &SocketInterfaceSingleton::get() : sock_interface;
 }
 
-void throwOnError(absl::Status status) {
-  if (!status.ok()) {
-    throwEnvoyExceptionOrPanic(status.ToString());
-  }
-}
-
-InstanceConstSharedPtr throwOnError(StatusOr<InstanceConstSharedPtr> address) {
-  if (!address.ok()) {
-    throwOnError(address.status());
-  }
-  return *address;
-}
-
 } // namespace
 
 bool forceV6() {
@@ -108,16 +95,6 @@ StatusOr<Address::InstanceConstSharedPtr> 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 practice, we don't expect any address validation in
-  // addressFromSockAddr() to fail in worker thread.
-  StatusOr<InstanceConstSharedPtr> address = addressFromSockAddr(ss, ss_len, v6only);
-  return throwOnError(address);
-}
-
 Address::InstanceConstSharedPtr
 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
@@ -138,7 +115,7 @@ addressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t f
 
 Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface)
     : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
-  throwOnError(validateProtocolSupported());
+  THROW_IF_NOT_OK(validateProtocolSupported());
   initHelper(address);
 }
 
@@ -148,7 +125,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)) {
-  throwOnError(validateProtocolSupported());
+  THROW_IF_NOT_OK(validateProtocolSupported());
   memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_));
   ip_.ipv4_.address_.sin_family = AF_INET;
   ip_.ipv4_.address_.sin_port = htons(port);
@@ -163,7 +140,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)) {
-  throwOnError(validateProtocolSupported());
+  THROW_IF_NOT_OK(validateProtocolSupported());
   memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_));
   ip_.ipv4_.address_.sin_family = AF_INET;
   ip_.ipv4_.address_.sin_port = htons(port);
@@ -296,7 +273,7 @@ InstanceConstSharedPtr Ipv6Instance::Ipv6Helper::addressWithoutScopeId() const {
 Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only,
                            const SocketInterface* sock_interface)
     : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
-  throwOnError(validateProtocolSupported());
+  THROW_IF_NOT_OK(validateProtocolSupported());
   initHelper(address, v6only);
 }
 
@@ -306,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, bool v6only)
     : InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
-  throwOnError(validateProtocolSupported());
+  THROW_IF_NOT_OK(validateProtocolSupported());
   sockaddr_in6 addr_in;
   memset(&addr_in, 0, sizeof(addr_in));
   addr_in.sin6_family = AF_INET6;

diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h
@@ -36,8 +36,6 @@ bool forceV6();
  */
 StatusOr<InstanceConstSharedPtr> addressFromSockAddr(const sockaddr_storage& ss, socklen_t len,
                                                      bool v6only = true);
-InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t len,
-                                                  bool v6only = true);
 
 /**
  * Convert an address in the form of the socket address struct defined by Posix, Linux, etc. into

diff --git a/source/common/network/io_socket_handle_base_impl.cc b/source/common/network/io_socket_handle_base_impl.cc
@@ -74,7 +74,8 @@ Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::localAddress() {
     throwEnvoyExceptionOrPanic(fmt::format("getsockname failed for '{}': ({}) {}", fd_,
                                            result.errno_, errorDetails(result.errno_)));
   }
-  return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_);
+  return THROW_OR_RETURN_VALUE(Address::addressFromSockAddr(ss, ss_len, socket_v6only_),
+                               Address::InstanceConstSharedPtr);
 }
 
 Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::peerAddress() {
@@ -102,7 +103,8 @@ Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::peerAddress() {
           fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_)));
     }
   }
-  return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_);
+  return THROW_OR_RETURN_VALUE(Address::addressFromSockAddr(ss, ss_len, socket_v6only_),
+                               Address::InstanceConstSharedPtr);
 }
 
 absl::optional<std::chrono::milliseconds> IoSocketHandleBaseImpl::lastRoundTripTime() {

diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc
@@ -53,7 +53,7 @@ bool TcpListenerImpl::rejectCxOverGlobalLimit() const {
   }
 }
 
-void TcpListenerImpl::onSocketEvent(short flags) {
+absl::Status TcpListenerImpl::onSocketEvent(short flags) {
   ASSERT(bind_to_port_);
   ASSERT(flags & (Event::FileReadyType::Read));
 
@@ -98,12 +98,15 @@ void TcpListenerImpl::onSocketEvent(short flags) {
     // 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::addressFromSockAddrOrThrow(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 {
+      auto address_or_error = Address::addressFromSockAddr(
+          remote_addr, remote_addr_len, local_address->ip()->version() == Address::IpVersion::v6);
+      RETURN_IF_NOT_OK_REF(address_or_error.status());
+      remote_address = *address_or_error;
+    }
 
     cb_.onAccept(std::make_unique<AcceptedSocketImpl>(std::move(io_handle), local_address,
                                                       remote_address, overload_state_,
@@ -113,6 +116,7 @@ void TcpListenerImpl::onSocketEvent(short flags) {
   ENVOY_LOG_MISC(trace, "TcpListener accepted {} new connections.",
                  connections_accepted_from_kernel_count);
   cb_.recordConnectionsAcceptedOnSocketEvent(connections_accepted_from_kernel_count);
+  return absl::OkStatus();
 }
 
 TcpListenerImpl::TcpListenerImpl(Event::Dispatcher& dispatcher, Random::RandomGenerator& random,
@@ -137,11 +141,7 @@ TcpListenerImpl::TcpListenerImpl(Event::Dispatcher& dispatcher, Random::RandomGe
     // transient accept errors or early termination due to accepting
     // max_connections_to_accept_per_socket_event connections.
     socket_->ioHandle().initializeFileEvent(
-        dispatcher,
-        [this](uint32_t events) {
-          onSocketEvent(events);
-          return absl::OkStatus();
-        },
+        dispatcher, [this](uint32_t events) { return onSocketEvent(events); },
         Event::FileTriggerType::Level, Event::FileReadyType::Read);
   }
 }

diff --git a/source/common/network/tcp_listener_impl.h b/source/common/network/tcp_listener_impl.h
@@ -36,7 +36,7 @@ class TcpListenerImpl : public BaseListenerImpl {
   TcpListenerCallbacks& cb_;
 
 private:
-  void onSocketEvent(short flags);
+  absl::Status onSocketEvent(short flags);
 
   // Returns true if global connection limit has been reached and the accepted socket should be
   // rejected/closed. If the accepted socket is to be admitted, false is returned.