syscall: refactor address APIs for deeper errno latching

include/envoy/api/os_sys_calls.h

@@ -14,6 +14,22 @@
 namespace Envoy {
 namespace Api {
 
+/**
+ * SysCallResult holds the rc and errno values resulting from a system call.
+ */
+struct SysCallResult {
+
+  /**
+   * The return code from the system call.
+   */
+  int rc_;
+
+  /**
+   * The errno value as captured after the system call.
+   */
+  int errno_;
+};
+
 class OsSysCalls {
 public:
   virtual ~OsSysCalls() {}

include/envoy/network/BUILD

@@ -11,6 +11,7 @@ envoy_package()
 envoy_cc_library(
     name = "address_interface",
     hdrs = ["address.h"],
+    deps = ["//include/envoy/api:os_sys_calls_interface"],
 )
 
 envoy_cc_library(

include/envoy/network/address.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "envoy/api/os_sys_calls.h"
 #include "envoy/common/pure.h"
 
 #include "absl/numeric/int128.h"
@@ -128,19 +129,19 @@ class Instance {
    * Bind a socket to this address. The socket should have been created with a call to socket() on
    * an Instance of the same address family.
    * @param fd supplies the platform socket handle.
-   * @return 0 for success and -1 for failure. The error code associated with a failure will
-   * be accessible in a plaform dependent fashion (e.g. errno for Unix platforms).
+   * @return a Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call
+   *   is successful, errno_ shouldn't be used.
    */
-  virtual int bind(int fd) const PURE;
+  virtual Api::SysCallResult bind(int fd) const PURE;
 
   /**
    * Connect a socket to this address. The socket should have been created with a call to socket()
    * on this object.
    * @param fd supplies the platform socket handle.
-   * @return 0 for success and -1 for failure. The error code associated with a failure will
-   * be accessible in a plaform dependent fashion (e.g. errno for Unix platforms).
+   * @return a Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call
+   *   is successful, errno_ shouldn't be used.
    */
-  virtual int connect(int fd) const PURE;
+  virtual Api::SysCallResult connect(int fd) const PURE;
 
   /**
    * @return the IP address information IFF type() == Type::Ip, otherwise nullptr.
@@ -150,11 +151,11 @@ class Instance {
   /**
    * Create a socket for this address.
    * @param type supplies the socket type to create.
-   * @return the file descriptor naming the socket for success and -1 for failure. The error
-   * code associated with a failure will be accessible in a plaform dependent fashion (e.g.
-   * errno for Unix platforms).
+   * @return a Api::SysCallResult with rc_ = fd for success and rc_ = -1 for failure, where fd
+   *   represents the file descriptor naming the socket. If a valid file descriptor is returned,
+   *   errno_ shouldn't be used.
    */
-  virtual int socket(SocketType type) const PURE;
+  virtual Api::SysCallResult socket(SocketType type) const PURE;
 
   /**
    * @return the type of address.

source/common/network/address_impl.cc

@@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) {
   return addressFromSockAddr(ss, ss_len);
 }
 
-int InstanceBase::socketFromSocketType(SocketType socketType) const {
+Api::SysCallResult InstanceBase::socketFromSocketType(SocketType socketType) const {
 #if defined(__APPLE__)
   int flags = 0;
 #else
@@ -168,7 +168,7 @@ int InstanceBase::socketFromSocketType(SocketType socketType) const {
   RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1, "");
 #endif
 
-  return fd;
+  return {fd, errno};
 }
 
 Ipv4Instance::Ipv4Instance(const sockaddr_in* address) : InstanceBase(Type::Ip) {
@@ -212,17 +212,21 @@ bool Ipv4Instance::operator==(const Instance& rhs) const {
           (ip_.port() == rhs_casted->ip_.port()));
 }
 
-int Ipv4Instance::bind(int fd) const {
-  return ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
-                sizeof(ip_.ipv4_.address_));
+Api::SysCallResult Ipv4Instance::bind(int fd) const {
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
+                        sizeof(ip_.ipv4_.address_));
+  return {rc, errno};
 }
 
-int Ipv4Instance::connect(int fd) const {
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
-                   sizeof(ip_.ipv4_.address_));
+Api::SysCallResult Ipv4Instance::connect(int fd) const {
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
+                           sizeof(ip_.ipv4_.address_));
+  return {rc, errno};
 }
 
-int Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); }
+Api::SysCallResult Ipv4Instance::socket(SocketType type) const {
+  return socketFromSocketType(type);
+}
 
 absl::uint128 Ipv6Instance::Ipv6Helper::address() const {
   absl::uint128 result{0};
@@ -275,23 +279,25 @@ bool Ipv6Instance::operator==(const Instance& rhs) const {
           (ip_.port() == rhs_casted->ip_.port()));
 }
 
-int Ipv6Instance::bind(int fd) const {
-  return ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
-                sizeof(ip_.ipv6_.address_));
+Api::SysCallResult Ipv6Instance::bind(int fd) const {
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
+                        sizeof(ip_.ipv6_.address_));
+  return {rc, errno};
 }
 
-int Ipv6Instance::connect(int fd) const {
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
-                   sizeof(ip_.ipv6_.address_));
+Api::SysCallResult Ipv6Instance::connect(int fd) const {
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
+                           sizeof(ip_.ipv6_.address_));
+  return {rc, errno};
 }
 
-int Ipv6Instance::socket(SocketType type) const {
-  const int fd = socketFromSocketType(type);
-
+Api::SysCallResult Ipv6Instance::socket(SocketType type) const {
+  const Api::SysCallResult result = socketFromSocketType(type);
   // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only.
   const int v6only = ip_.v6only_;
-  RELEASE_ASSERT(::setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, "");
-  return fd;
+  RELEASE_ASSERT(::setsockopt(result.rc_, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1,
+                 "");
+  return result;
 }
 
 PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len)
@@ -333,27 +339,33 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi
 
 bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); }
 
-int PipeInstance::bind(int fd) const {
+Api::SysCallResult PipeInstance::bind(int fd) const {
   if (abstract_namespace_) {
-    return ::bind(fd, reinterpret_cast<const sockaddr*>(&address_),
-                  offsetof(struct sockaddr_un, sun_path) + address_length_);
+    const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&address_),
+                          offsetof(struct sockaddr_un, sun_path) + address_length_);
+    return {rc, errno};
   }
   // Try to unlink an existing filesystem object at the requested path. Ignore
   // errors -- it's fine if the path doesn't exist, and if it exists but can't
   // be unlinked then `::bind()` will generate a reasonable errno.
   unlink(address_.sun_path);
-  return ::bind(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  return {rc, errno};
 }
 
-int PipeInstance::connect(int fd) const {
+Api::SysCallResult PipeInstance::connect(int fd) const {
   if (abstract_namespace_) {
-    return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
-                     offsetof(struct sockaddr_un, sun_path) + address_length_);
+    const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
+                             offsetof(struct sockaddr_un, sun_path) + address_length_);
+    return {rc, errno};
   }
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  return {rc, errno};
 }
 
-int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); }
+Api::SysCallResult PipeInstance::socket(SocketType type) const {
+  return socketFromSocketType(type);
+}
 
 } // namespace Address
 } // namespace Network

source/common/network/address_impl.h

@@ -55,7 +55,7 @@ class InstanceBase : public Instance {
 
 protected:
   InstanceBase(Type type) : type_(type) {}
-  int socketFromSocketType(SocketType type) const;
+  Api::SysCallResult socketFromSocketType(SocketType type) const;
 
   std::string friendly_name_;
 
@@ -91,10 +91,10 @@ class Ipv4Instance : public InstanceBase {
 
   // Network::Address::Instance
   bool operator==(const Instance& rhs) const override;
-  int bind(int fd) const override;
-  int connect(int fd) const override;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult connect(int fd) const override;
   const Ip* ip() const override { return &ip_; }
-  int socket(SocketType type) const override;
+  Api::SysCallResult socket(SocketType type) const override;
 
 private:
   struct Ipv4Helper : public Ipv4 {
@@ -151,10 +151,10 @@ class Ipv6Instance : public InstanceBase {
 
   // Network::Address::Instance
   bool operator==(const Instance& rhs) const override;
-  int bind(int fd) const override;
-  int connect(int fd) const override;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult connect(int fd) const override;
   const Ip* ip() const override { return &ip_; }
-  int socket(SocketType type) const override;
+  Api::SysCallResult socket(SocketType type) const override;
 
 private:
   struct Ipv6Helper : public Ipv6 {
@@ -208,10 +208,10 @@ class PipeInstance : public InstanceBase {
 
   // Network::Address::Instance
   bool operator==(const Instance& rhs) const override;
-  int bind(int fd) const override;
-  int connect(int fd) const override;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult connect(int fd) const override;
   const Ip* ip() const override { return nullptr; }
-  int socket(SocketType type) const override;
+  Api::SysCallResult socket(SocketType type) const override;
 
 private:
   sockaddr_un address_;

source/common/network/connection_impl.cc

@@ -551,10 +551,10 @@ ClientConnectionImpl::ClientConnectionImpl(
   }
 
   if (source_address != nullptr) {
-    const int rc = source_address->bind(fd());
-    if (rc < 0) {
+    Api::SysCallResult result = source_address->bind(fd());
+    if (result.rc_ < 0) {
       ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
-                     strerror(errno));
+                     strerror(result.errno_));
       bind_error_ = true;
       // Set a special error state to ensure asynchronous close to give the owner of the
       // ConnectionImpl a chance to add callbacks and detect the "disconnect".
@@ -568,19 +568,19 @@ ClientConnectionImpl::ClientConnectionImpl(
 
 void ClientConnectionImpl::connect() {
   ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString());
-  const int rc = socket_->remoteAddress()->connect(fd());
-  if (rc == 0) {
+  Api::SysCallResult result = socket_->remoteAddress()->connect(fd());
+  if (result.rc_ == 0) {
     // write will become ready.
     ASSERT(connecting_);
   } else {
-    ASSERT(rc == -1);
-    if (errno == EINPROGRESS) {
+    ASSERT(result.rc_ == -1);
+    if (result.errno_ == EINPROGRESS) {
       ASSERT(connecting_);
       ENVOY_CONN_LOG(debug, "connection in progress", *this);
     } else {
       immediate_error_event_ = ConnectionEvent::RemoteClose;
       connecting_ = false;
-      ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, errno);
+      ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, result.errno_);
 
       // Trigger a write event. This is needed on OSX and seems harmless on Linux.
       file_event_->activate(Event::FileReadyType::Write);

source/common/network/listen_socket_impl.cc

@@ -16,11 +16,11 @@ namespace Envoy {
 namespace Network {
 
 void ListenSocketImpl::doBind() {
-  int rc = local_address_->bind(fd_);
-  if (rc == -1) {
+  const Api::SysCallResult result = local_address_->bind(fd_);
+  if (result.rc_ == -1) {
     close();
     throw EnvoyException(
-        fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(errno)));
+        fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(result.errno_)));
   }
   if (local_address_->type() == Address::Type::Ip && local_address_->ip()->port() == 0) {
     // If the port we bind is zero, then the OS will pick a free port for us (assuming there are
@@ -39,7 +39,7 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar
 TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address,
                                  const Network::Socket::OptionsSharedPtr& options,
                                  bool bind_to_port)
-    : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) {
+    : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) {
   RELEASE_ASSERT(fd_ != -1, "");
 
   // TODO(htuch): This might benefit from moving to SocketOptionImpl.
@@ -61,7 +61,7 @@ TcpListenSocket::TcpListenSocket(int fd, const Address::InstanceConstSharedPtr&
 }
 
 UdsListenSocket::UdsListenSocket(const Address::InstanceConstSharedPtr& address)
-    : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) {
+    : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) {
   RELEASE_ASSERT(fd_ != -1, "");
   doBind();
 }

source/common/network/listen_socket_impl.h

@@ -137,7 +137,7 @@ class AcceptedSocketImpl : public ConnectionSocketImpl {
 class ClientSocketImpl : public ConnectionSocketImpl {
 public:
   ClientSocketImpl(const Address::InstanceConstSharedPtr& remote_address)
-      : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream), nullptr,
+      : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc_, nullptr,
                              remote_address) {}
 };
 

source/extensions/stat_sinks/common/statsd/statsd.cc

@@ -20,10 +20,10 @@ namespace Common {
 namespace Statsd {
 
 Writer::Writer(Network::Address::InstanceConstSharedPtr address) {
-  fd_ = address->socket(Network::Address::SocketType::Datagram);
+  fd_ = address->socket(Network::Address::SocketType::Datagram).rc_;
   ASSERT(fd_ != -1);
 
-  int rc = address->connect(fd_);
+  const int rc = address->connect(fd_).rc_;
   ASSERT(rc != -1);
 }