socket: removing some exceptions (#36991)

unlike most exception PRs this includes a functional change to how Envoy handles completely invalid sockets or failed system calls. Errors should be connection-local rather than throwing to whatever might catch, so it should be a whole sale improvement and is unlikely to affect anyone whose syscalls work. Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Nov 8, 2024 · c3d9d92 · c3d9d92
1 parent 46b954c
commit c3d9d92
Show file tree

Hide file tree

Showing 18 changed files with 67 additions and 48 deletions.
diff --git a/contrib/vcl/source/vcl_io_handle.cc b/contrib/vcl/source/vcl_io_handle.cc
@@ -625,7 +625,7 @@ absl::optional<int> VclIoHandle::domain() {
   return {AF_INET};
 };
 
-Envoy::Network::Address::InstanceConstSharedPtr VclIoHandle::localAddress() {
+absl::StatusOr<Envoy::Network::Address::InstanceConstSharedPtr> VclIoHandle::localAddress() {
   vppcom_endpt_t ep;
   uint32_t eplen = sizeof(ep);
   uint8_t addr_buf[sizeof(struct sockaddr_in6)];
@@ -636,7 +636,7 @@ Envoy::Network::Address::InstanceConstSharedPtr VclIoHandle::localAddress() {
   return vclEndptToAddress(ep, sh_);
 }
 
-Envoy::Network::Address::InstanceConstSharedPtr VclIoHandle::peerAddress() {
+absl::StatusOr<Envoy::Network::Address::InstanceConstSharedPtr> VclIoHandle::peerAddress() {
   VCL_LOG("grabbing peer address sh {:x}", sh_);
   vppcom_endpt_t ep;
   uint32_t eplen = sizeof(ep);

diff --git a/contrib/vcl/source/vcl_io_handle.h b/contrib/vcl/source/vcl_io_handle.h
@@ -66,8 +66,8 @@ class VclIoHandle : public Envoy::Network::IoHandle,
                               unsigned long out_buffer_len, unsigned long* bytes_returned) override;
   Api::SysCallIntResult setBlocking(bool blocking) override;
   absl::optional<int> domain() override;
-  Envoy::Network::Address::InstanceConstSharedPtr localAddress() override;
-  Envoy::Network::Address::InstanceConstSharedPtr peerAddress() override;
+  absl::StatusOr<Envoy::Network::Address::InstanceConstSharedPtr> localAddress() override;
+  absl::StatusOr<Envoy::Network::Address::InstanceConstSharedPtr> peerAddress() override;
   Api::SysCallIntResult shutdown(int) override { return {0, 0}; }
 
   void initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,

diff --git a/envoy/network/io_handle.h b/envoy/network/io_handle.h
@@ -293,15 +293,15 @@ class IoHandle {
 
   /**
    * Get local address (ip:port pair)
-   * @return local address as @ref Address::InstanceConstSharedPtr
+   * @return local address as @ref Address::InstanceConstSharedPtr or error status.
    */
-  virtual Address::InstanceConstSharedPtr localAddress() PURE;
+  virtual absl::StatusOr<Address::InstanceConstSharedPtr> localAddress() PURE;
 
   /**
    * Get peer's address (ip:port pair)
-   * @return peer's address as @ref Address::InstanceConstSharedPtr
+   * @return peer's address as @ref Address::InstanceConstSharedPtr or error status.
    */
-  virtual Address::InstanceConstSharedPtr peerAddress() PURE;
+  virtual absl::StatusOr<Address::InstanceConstSharedPtr> peerAddress() PURE;
 
   /**
    * Duplicates the handle. This is intended to be used only on listener sockets. (see man dup)

diff --git a/source/common/network/io_socket_handle_base_impl.cc b/source/common/network/io_socket_handle_base_impl.cc
@@ -63,30 +63,29 @@ Api::SysCallIntResult IoSocketHandleBaseImpl::setBlocking(bool blocking) {
 
 absl::optional<int> IoSocketHandleBaseImpl::domain() { return domain_; }
 
-Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::localAddress() {
+absl::StatusOr<Address::InstanceConstSharedPtr> IoSocketHandleBaseImpl::localAddress() {
   sockaddr_storage ss;
   socklen_t ss_len = sizeof(ss);
   memset(&ss, 0, ss_len);
   auto& os_sys_calls = Api::OsSysCallsSingleton::get();
   Api::SysCallIntResult result =
       os_sys_calls.getsockname(fd_, reinterpret_cast<sockaddr*>(&ss), &ss_len);
   if (result.return_value_ != 0) {
-    throwEnvoyExceptionOrPanic(fmt::format("getsockname failed for '{}': ({}) {}", fd_,
-                                           result.errno_, errorDetails(result.errno_)));
+    return absl::InvalidArgumentError(fmt::format("getsockname failed for '{}': ({}) {}", fd_,
+                                                  result.errno_, errorDetails(result.errno_)));
   }
-  return THROW_OR_RETURN_VALUE(Address::addressFromSockAddr(ss, ss_len, socket_v6only_),
-                               Address::InstanceConstSharedPtr);
+  return Address::addressFromSockAddr(ss, ss_len, socket_v6only_);
 }
 
-Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::peerAddress() {
+absl::StatusOr<Address::InstanceConstSharedPtr> IoSocketHandleBaseImpl::peerAddress() {
   sockaddr_storage ss;
   socklen_t ss_len = sizeof(ss);
   memset(&ss, 0, ss_len);
   auto& os_sys_calls = Api::OsSysCallsSingleton::get();
   Api::SysCallIntResult result =
       os_sys_calls.getpeername(fd_, reinterpret_cast<sockaddr*>(&ss), &ss_len);
   if (result.return_value_ != 0) {
-    throwEnvoyExceptionOrPanic(
+    return absl::InvalidArgumentError(
         fmt::format("getpeername failed for '{}': {}", fd_, errorDetails(result.errno_)));
   }
 
@@ -103,8 +102,7 @@ Address::InstanceConstSharedPtr IoSocketHandleBaseImpl::peerAddress() {
           fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_)));
     }
   }
-  return THROW_OR_RETURN_VALUE(Address::addressFromSockAddr(ss, ss_len, socket_v6only_),
-                               Address::InstanceConstSharedPtr);
+  return Address::addressFromSockAddr(ss, ss_len, socket_v6only_);
 }
 
 absl::optional<std::chrono::milliseconds> IoSocketHandleBaseImpl::lastRoundTripTime() {
@@ -131,7 +129,11 @@ absl::optional<std::string> IoSocketHandleBaseImpl::interfaceName() {
     return absl::nullopt;
   }
 
-  Address::InstanceConstSharedPtr socket_address = localAddress();
+  auto address_or_error = localAddress();
+  if (!address_or_error.status().ok()) {
+    return absl::nullopt;
+  }
+  Address::InstanceConstSharedPtr& socket_address = *address_or_error;
   if (!socket_address || socket_address->type() != Address::Type::Ip) {
     return absl::nullopt;
   }

diff --git a/source/common/network/io_socket_handle_base_impl.h b/source/common/network/io_socket_handle_base_impl.h
@@ -29,8 +29,8 @@ class IoSocketHandleBaseImpl : public IoHandle, protected Logger::Loggable<Logge
                               unsigned long*) override;
   Api::SysCallIntResult setBlocking(bool blocking) override;
   absl::optional<int> domain() override;
-  Address::InstanceConstSharedPtr localAddress() override;
-  Address::InstanceConstSharedPtr peerAddress() override;
+  absl::StatusOr<Address::InstanceConstSharedPtr> localAddress() override;
+  absl::StatusOr<Address::InstanceConstSharedPtr> peerAddress() override;
   absl::optional<std::chrono::milliseconds> lastRoundTripTime() override;
   absl::optional<uint64_t> congestionWindowInBytes() const override;
   absl::optional<std::string> interfaceName() override;

diff --git a/source/common/network/socket_impl.cc b/source/common/network/socket_impl.cc
@@ -82,7 +82,11 @@ Api::SysCallIntResult SocketImpl::bind(Network::Address::InstanceConstSharedPtr
 
   bind_result = io_handle_->bind(address);
   if (bind_result.return_value_ == 0 && address->ip()->port() == 0) {
-    connection_info_provider_->setLocalAddress(io_handle_->localAddress());
+    auto address_or_error = io_handle_->localAddress();
+    if (!address_or_error.status().ok()) {
+      return Api::SysCallIntResult{-1, HANDLE_ERROR_INVALID};
+    }
+    connection_info_provider_->setLocalAddress(*address_or_error);
   }
   return bind_result;
 }
@@ -92,7 +96,11 @@ Api::SysCallIntResult SocketImpl::listen(int backlog) { return io_handle_->liste
 Api::SysCallIntResult SocketImpl::connect(const Network::Address::InstanceConstSharedPtr address) {
   auto result = io_handle_->connect(address);
   if (address->type() == Address::Type::Ip) {
-    connection_info_provider_->setLocalAddress(io_handle_->localAddress());
+    auto address_or_error = io_handle_->localAddress();
+    if (!address_or_error.status().ok()) {
+      return Api::SysCallIntResult{-1, HANDLE_ERROR_INVALID};
+    }
+    connection_info_provider_->setLocalAddress(*address_or_error);
   }
   return result;
 }

diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc
@@ -87,8 +87,12 @@ absl::Status TcpListenerImpl::onSocketEvent(short flags) {
 
     // Get the local address from the new socket if the listener is listening on IP ANY
     // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case).
-    const Address::InstanceConstSharedPtr& local_address =
-        local_address_ ? local_address_ : io_handle->localAddress();
+    Address::InstanceConstSharedPtr local_address = local_address_;
+    if (!local_address) {
+      auto address_or_error = io_handle->localAddress();
+      RETURN_IF_NOT_OK_REF(address_or_error.status());
+      local_address = *address_or_error;
+    }
 
     // The accept() call that filled in remote_addr doesn't fill in more than the sa_family field
     // for Unix domain sockets; apparently there isn't a mechanism in the kernel to get the
@@ -100,7 +104,9 @@ absl::Status TcpListenerImpl::onSocketEvent(short flags) {
 
     Address::InstanceConstSharedPtr remote_address;
     if (remote_addr.ss_family == AF_UNIX) {
-      remote_address = io_handle->peerAddress();
+      auto address_or_error = io_handle->peerAddress();
+      RETURN_IF_NOT_OK_REF(address_or_error.status());
+      remote_address = *address_or_error;
     } else {
       auto address_or_error = Address::addressFromSockAddr(
           remote_addr, remote_addr_len, local_address->ip()->version() == Address::IpVersion::v6);

diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc
@@ -394,7 +394,10 @@ Address::InstanceConstSharedPtr Utility::getOriginalDst(Socket& sock) {
       status =
           sock.getSocketOption(opt_tp.level(), opt_tp.option(), &is_tp, &flag_len).return_value_;
       if (status == 0 && is_tp) {
-        return sock.ioHandle().localAddress();
+        auto address_or_error = sock.ioHandle().localAddress();
+        if (address_or_error.status().ok()) {
+          return *address_or_error;
+        }
       }
     }
     return nullptr;

diff --git a/source/common/quic/quic_io_handle_wrapper.h b/source/common/quic/quic_io_handle_wrapper.h
@@ -117,10 +117,10 @@ class QuicIoHandleWrapper : public Network::IoHandle {
     return io_handle_.setBlocking(blocking);
   }
   absl::optional<int> domain() override { return io_handle_.domain(); }
-  Network::Address::InstanceConstSharedPtr localAddress() override {
+  absl::StatusOr<Network::Address::InstanceConstSharedPtr> localAddress() override {
     return io_handle_.localAddress();
   }
-  Network::Address::InstanceConstSharedPtr peerAddress() override {
+  absl::StatusOr<Network::Address::InstanceConstSharedPtr> peerAddress() override {
     return io_handle_.peerAddress();
   }
 

diff --git a/source/extensions/io_socket/user_space/io_handle_impl.cc b/source/extensions/io_socket/user_space/io_handle_impl.cc
@@ -315,11 +315,11 @@ Api::SysCallIntResult IoHandleImpl::setBlocking(bool) { return makeInvalidSyscal
 
 absl::optional<int> IoHandleImpl::domain() { return absl::nullopt; }
 
-Network::Address::InstanceConstSharedPtr IoHandleImpl::localAddress() {
+absl::StatusOr<Network::Address::InstanceConstSharedPtr> IoHandleImpl::localAddress() {
   return IoHandleImpl::getCommonInternalAddress();
 }
 
-Network::Address::InstanceConstSharedPtr IoHandleImpl::peerAddress() {
+absl::StatusOr<Network::Address::InstanceConstSharedPtr> IoHandleImpl::peerAddress() {
   return IoHandleImpl::getCommonInternalAddress();
 }
 

diff --git a/source/extensions/io_socket/user_space/io_handle_impl.h b/source/extensions/io_socket/user_space/io_handle_impl.h
@@ -80,8 +80,8 @@ class IoHandleImpl final : public Network::IoHandle,
                               unsigned long*) override;
   Api::SysCallIntResult setBlocking(bool blocking) override;
   absl::optional<int> domain() override;
-  Network::Address::InstanceConstSharedPtr localAddress() override;
-  Network::Address::InstanceConstSharedPtr peerAddress() override;
+  absl::StatusOr<Network::Address::InstanceConstSharedPtr> localAddress() override;
+  absl::StatusOr<Network::Address::InstanceConstSharedPtr> peerAddress() override;
 
   void initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
                            Event::FileTriggerType trigger, uint32_t events) override;

diff --git a/test/common/network/io_uring_socket_handle_impl_integration_test.cc b/test/common/network/io_uring_socket_handle_impl_integration_test.cc
@@ -125,7 +125,7 @@ class IoUringSocketHandleImplIntegrationTest : public testing::Test {
         Event::PlatformDefaultTriggerType, Event::FileReadyType::Read);
 
     // Connect from io_uring handle.
-    io_uring_socket_handle_->connect(listener->localAddress());
+    io_uring_socket_handle_->connect(*listener->localAddress());
     while (error == -1) {
       dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
     }
@@ -192,7 +192,7 @@ TEST_F(IoUringSocketHandleImplIntegrationTest, Accept) {
       Event::PlatformDefaultTriggerType, Event::FileReadyType::Read);
 
   // Connect from the socket handle.
-  io_socket_handle_->connect(io_uring_socket_handle_->localAddress());
+  io_socket_handle_->connect(*io_uring_socket_handle_->localAddress());
   while (!accepted) {
     dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
   }

diff --git a/test/common/quic/quic_io_handle_wrapper_test.cc b/test/common/quic/quic_io_handle_wrapper_test.cc
@@ -72,15 +72,15 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) {
         *addrlen = sizeof(sockaddr_in6);
         return Api::SysCallIntResult{0, 0};
       }));
-  addr = wrapper_->localAddress();
+  addr = *wrapper_->localAddress();
 
   EXPECT_CALL(os_sys_calls_, getpeername(_, _, _))
       .WillOnce(Invoke([](os_fd_t, sockaddr* addr, socklen_t* addrlen) -> Api::SysCallIntResult {
         addr->sa_family = AF_INET6;
         *addrlen = sizeof(sockaddr_in6);
         return Api::SysCallIntResult{0, 0};
       }));
-  addr = wrapper_->peerAddress();
+  addr = *wrapper_->peerAddress();
 
   Network::IoHandle::RecvMsgOutput output(1, nullptr);
   EXPECT_CALL(os_sys_calls_, recvmsg(fd, _, MSG_TRUNC))

diff --git a/test/extensions/io_socket/user_space/connection_compatbility_test.cc b/test/extensions/io_socket/user_space/connection_compatbility_test.cc
@@ -31,8 +31,8 @@ class InternalClientConnectionImplTest : public testing::Test {
 
   void SetUp() override {
     std::tie(io_handle_, io_handle_peer_) = IoHandleFactory::createIoHandlePair();
-    local_addr_ = io_handle_->localAddress();
-    remote_addr_ = io_handle_->peerAddress();
+    local_addr_ = *io_handle_->localAddress();
+    remote_addr_ = *io_handle_->peerAddress();
   }
   Api::ApiPtr api_;
   Event::DispatcherPtr dispatcher_;

diff --git a/test/extensions/io_socket/user_space/io_handle_impl_test.cc b/test/extensions/io_socket/user_space/io_handle_impl_test.cc
@@ -1059,12 +1059,12 @@ TEST_F(IoHandleImplTest, NotifyWritableAfterShutdownWrite) {
 }
 
 TEST_F(IoHandleImplTest, ReturnValidInternalAddress) {
-  const auto& local_address = io_handle_->localAddress();
+  const auto local_address = *io_handle_->localAddress();
   ASSERT_NE(nullptr, local_address);
   ASSERT_EQ(nullptr, local_address->ip());
   ASSERT_EQ(nullptr, local_address->pipe());
   ASSERT_NE(nullptr, local_address->envoyInternalAddress());
-  const auto& remote_address = io_handle_->peerAddress();
+  const auto remote_address = *io_handle_->peerAddress();
   ASSERT_NE(nullptr, remote_address);
   ASSERT_EQ(nullptr, remote_address->ip());
   ASSERT_EQ(nullptr, remote_address->pipe());

diff --git a/test/integration/filters/test_socket_interface.h b/test/integration/filters/test_socket_interface.h
@@ -62,7 +62,7 @@ class TestIoSocketHandle : public Test::IoSocketHandlePlatformImpl {
   // HTTP/3 sockets won't have a bound peer address, but instead get peer
   // address from the argument in sendmsg. TestIoSocketHandle::sendmsg will
   // stash that in peer_address_override_.
-  Address::InstanceConstSharedPtr peerAddress() override {
+  absl::StatusOr<Address::InstanceConstSharedPtr> peerAddress() override {
     if (peer_address_override_.has_value()) {
       return Network::Utility::getAddressWithPort(
           peer_address_override_.value().get(), peer_address_override_.value().get().ip()->port());

diff --git a/test/integration/socket_interface_swap.h b/test/integration/socket_interface_swap.h
@@ -21,8 +21,8 @@ class SocketInterfaceSwap {
                    Network::Address::InstanceConstSharedPtr& peer_address_override_out) {
       absl::MutexLock lock(&mutex_);
       if (socket_type_ == io_handle->getSocketType() && error_ &&
-          (io_handle->localAddress()->ip()->port() == src_port_ ||
-           (dst_port_ && io_handle->peerAddress()->ip()->port() == dst_port_))) {
+          ((*io_handle->localAddress())->ip()->port() == src_port_ ||
+           (dst_port_ && (*io_handle->peerAddress())->ip()->port() == dst_port_))) {
         ASSERT(matched_iohandle_ == nullptr || matched_iohandle_ == io_handle,
                "Matched multiple io_handles, expected at most one to match.");
         matched_iohandle_ = io_handle;
@@ -31,7 +31,7 @@ class SocketInterfaceSwap {
                    : Envoy::Network::IoSocketError::create(error_->getSystemErrorCode());
       }
 
-      if (orig_dnat_address_ != nullptr && *orig_dnat_address_ == *io_handle->peerAddress()) {
+      if (orig_dnat_address_ != nullptr && *orig_dnat_address_ == **io_handle->peerAddress()) {
         ASSERT(translated_dnat_address_ != nullptr);
         peer_address_override_out = translated_dnat_address_;
       }
@@ -44,8 +44,8 @@ class SocketInterfaceSwap {
                           Network::Address::InstanceConstSharedPtr& peer_address_override_out) {
       absl::MutexLock lock(&mutex_);
       if (block_connect_ && socket_type_ == io_handle->getSocketType() &&
-          (io_handle->localAddress()->ip()->port() == src_port_ ||
-           (dst_port_ && io_handle->peerAddress()->ip()->port() == dst_port_))) {
+          ((*io_handle->localAddress())->ip()->port() == src_port_ ||
+           (dst_port_ && (*io_handle->peerAddress())->ip()->port() == dst_port_))) {
         return Network::IoSocketError::getIoSocketEagainError();
       }
 

diff --git a/test/mocks/network/io_handle.h b/test/mocks/network/io_handle.h
@@ -52,8 +52,8 @@ class MockIoHandle : public IoHandle {
               (int level, int optname, void* optval, socklen_t* optlen));
   MOCK_METHOD(Api::SysCallIntResult, setBlocking, (bool blocking));
   MOCK_METHOD(absl::optional<int>, domain, ());
-  MOCK_METHOD(Address::InstanceConstSharedPtr, localAddress, ());
-  MOCK_METHOD(Address::InstanceConstSharedPtr, peerAddress, ());
+  MOCK_METHOD(absl::StatusOr<Address::InstanceConstSharedPtr>, localAddress, ());
+  MOCK_METHOD(absl::StatusOr<Address::InstanceConstSharedPtr>, peerAddress, ());
   MOCK_METHOD(IoHandlePtr, duplicate, ());
   MOCK_METHOD(void, createFileEvent_,
               (Event::Dispatcher & dispatcher, Event::FileReadyCb cb,