Fix SocketAddress AF_UNIX support

Summary: check external_ vs. AF_UNIX.  Also fix reset() to reset storage_.addr

Test Plan: fbconfig folly/test:network_address_test; fbmake runtests

Reviewed By: soren@fb.com

Subscribers: tudorb, trunkagent, doug, njormrod

FB internal diff: D1575098

diff --git a/folly/SocketAddress.cpp b/folly/SocketAddress.cpp
index 4834a134a86307f3411eb4ca0fae3a37f5457dc7..f624b3a605bc6534d053861b650b545c351fb237 100644 (file)
--- a/folly/SocketAddress.cpp
+++ b/folly/SocketAddress.cpp
@@ -112,7 +112,7 @@ bool SocketAddress::isPrivateAddress() const {
   if (family == AF_INET || family == AF_INET6) {
     return storage_.addr.isPrivate() ||
       (storage_.addr.isV6() && storage_.addr.asV6().isLinkLocal());
-  } else if (family == AF_UNIX) {
+  } else if (external_) {
     // Unix addresses are always local to a host.  Return true,
     // since this conforms to the semantics of returning true for IP loopback
     // addresses.
@@ -125,7 +125,7 @@ bool SocketAddress::isLoopbackAddress() const {
   auto family = getFamily();
   if (family == AF_INET || family == AF_INET6) {
     return storage_.addr.isLoopback();
-  } else if (family == AF_UNIX) {
+  } else if (external_) {
     // Return true for UNIX addresses, since they are always local to a host.
     return true;
   }
@@ -172,7 +172,7 @@ void SocketAddress::setFromHostPort(const char* hostAndPort) {
 }
 
 void SocketAddress::setFromPath(const char* path, size_t len) {
-  if (getFamily() != AF_UNIX) {
+  if (!external_) {
     storage_.un.init();
     external_ = true;
   }
@@ -201,12 +201,12 @@ void SocketAddress::setFromLocalAddress(int socket) {
 }
 
 void SocketAddress::setFromSockaddr(const struct sockaddr* address) {
+  uint16_t port;
+
   if (address->sa_family == AF_INET) {
-    storage_.addr = folly::IPAddress(address);
-    port_ = ntohs(((sockaddr_in*)address)->sin_port);
+    port = ntohs(((sockaddr_in*)address)->sin_port);
   } else if (address->sa_family == AF_INET6) {
-    storage_.addr = folly::IPAddress(address);
-    port_ = ntohs(((sockaddr_in6*)address)->sin6_port);
+    port = ntohs(((sockaddr_in6*)address)->sin6_port);
   } else if (address->sa_family == AF_UNIX) {
     // We need an explicitly specified length for AF_UNIX addresses,
     // to be able to distinguish anonymous addresses from addresses
@@ -220,7 +220,12 @@ void SocketAddress::setFromSockaddr(const struct sockaddr* address) {
       "SocketAddress::setFromSockaddr() called "
       "with unsupported address type");
   }
-  external_ = false;
+  if (external_) {
+    storage_.un.free();
+    external_ = false;
+  }
+  storage_.addr = folly::IPAddress(address);
+  port_ = port;
 }
 
 void SocketAddress::setFromSockaddr(const struct sockaddr* address,
@@ -296,14 +301,15 @@ const folly::IPAddress& SocketAddress::getIPAddress() const {
 }
 
 socklen_t SocketAddress::getActualSize() const {
+  if (external_) {
+    return storage_.un.len;
+  }
   switch (getFamily()) {
     case AF_UNSPEC:
     case AF_INET:
       return sizeof(struct sockaddr_in);
     case AF_INET6:
       return sizeof(struct sockaddr_in6);
-    case AF_UNIX:
-      return storage_.un.len;
     default:
       throw std::invalid_argument(
         "SocketAddress::getActualSize() called "
@@ -392,7 +398,7 @@ std::string SocketAddress::getHostStr() const {
 }
 
 std::string SocketAddress::getPath() const {
-  if (getFamily() != AF_UNIX) {
+  if (!external_) {
     throw std::invalid_argument(
       "SocketAddress: attempting to get path "
       "for a non-Unix address");
@@ -413,6 +419,20 @@ std::string SocketAddress::getPath() const {
 }
 
 std::string SocketAddress::describe() const {
+  if (external_) {
+    if (storage_.un.pathLength() == 0) {
+      return "<anonymous unix address>";
+    }
+
+    if (storage_.un.addr->sun_path[0] == '\0') {
+      // Linux supports an abstract namespace for unix socket addresses
+      return "<abstract unix address>";
+    }
+
+    return std::string(storage_.un.addr->sun_path,
+                       strnlen(storage_.un.addr->sun_path,
+                               storage_.un.pathLength()));
+  }
   switch (getFamily()) {
     case AF_UNSPEC:
       return "<uninitialized address>";
@@ -433,21 +453,6 @@ std::string SocketAddress::describe() const {
       snprintf(buf + iplen, sizeof(buf) - iplen, "]:%" PRIu16, getPort());
       return buf;
     }
-    case AF_UNIX:
-    {
-      if (storage_.un.pathLength() == 0) {
-        return "<anonymous unix address>";
-      }
-
-      if (storage_.un.addr->sun_path[0] == '\0') {
-        // Linux supports an abstract namespace for unix socket addresses
-        return "<abstract unix address>";
-      }
-
-      return std::string(storage_.un.addr->sun_path,
-                         strnlen(storage_.un.addr->sun_path,
-                                 storage_.un.pathLength()));
-    }
     default:
     {
       char buf[64];
@@ -459,31 +464,30 @@ std::string SocketAddress::describe() const {
 }
 
 bool SocketAddress::operator==(const SocketAddress& other) const {
-  if (other.getFamily() != getFamily()) {
+  if (external_ != other.external_ || other.getFamily() != getFamily()) {
     return false;
   }
+  if (external_) {
+    // anonymous addresses are never equal to any other addresses
+    if (storage_.un.pathLength() == 0 ||
+        other.storage_.un.pathLength() == 0) {
+      return false;
+    }
+
+    if (storage_.un.len != other.storage_.un.len) {
+      return false;
+    }
+    int cmp = memcmp(storage_.un.addr->sun_path,
+                     other.storage_.un.addr->sun_path,
+                     storage_.un.pathLength());
+    return cmp == 0;
+  }
 
   switch (getFamily()) {
     case AF_INET:
     case AF_INET6:
       return (other.storage_.addr == storage_.addr) &&
         (other.port_ == port_);
-    case AF_UNIX:
-    {
-      // anonymous addresses are never equal to any other addresses
-      if (storage_.un.pathLength() == 0 ||
-          other.storage_.un.pathLength() == 0) {
-        return false;
-      }
-
-      if (storage_.un.len != other.storage_.un.len) {
-        return false;
-      }
-      int cmp = memcmp(storage_.un.addr->sun_path,
-                       other.storage_.un.addr->sun_path,
-                       storage_.un.pathLength());
-      return cmp == 0;
-    }
     default:
       throw std::invalid_argument(
         "SocketAddress: unsupported address family "
@@ -517,6 +521,16 @@ bool SocketAddress::prefixMatch(const SocketAddress& other,
 size_t SocketAddress::hash() const {
   size_t seed = folly::hash::twang_mix64(getFamily());
 
+  if (external_) {
+    enum { kUnixPathMax = sizeof(storage_.un.addr->sun_path) };
+    const char *path = storage_.un.addr->sun_path;
+    size_t pathLength = storage_.un.pathLength();
+    // TODO: this probably could be made more efficient
+    for (unsigned int n = 0; n < pathLength; ++n) {
+      boost::hash_combine(seed, folly::hash::twang_mix64(path[n]));
+    }
+  }
+
   switch (getFamily()) {
     case AF_INET:
     case AF_INET6: {
@@ -525,16 +539,8 @@ size_t SocketAddress::hash() const {
       break;
     }
     case AF_UNIX:
-    {
-      enum { kUnixPathMax = sizeof(storage_.un.addr->sun_path) };
-      const char *path = storage_.un.addr->sun_path;
-      size_t pathLength = storage_.un.pathLength();
-      // TODO: this probably could be made more efficient
-      for (unsigned int n = 0; n < pathLength; ++n) {
-        boost::hash_combine(seed, folly::hash::twang_mix64(path[n]));
-      }
+      DCHECK(external_);
       break;
-    }
     case AF_UNSPEC:
     default:
       throw std::invalid_argument(
@@ -596,15 +602,6 @@ void SocketAddress::setFromLocalAddr(const struct addrinfo* info) {
 
 void SocketAddress::setFromSocket(int socket,
                                   int (*fn)(int, sockaddr*, socklen_t*)) {
-  // If this was previously an AF_UNIX socket, free the external buffer.
-  // TODO: It would be smarter to just remember the external buffer, and then
-  // re-use it or free it depending on if the new address is also a unix
-  // socket.
-  if (getFamily() == AF_UNIX) {
-    storage_.un.free();
-    external_ = false;
-  }
-
   // Try to put the address into a local storage buffer.
   sockaddr_storage tmp_sock;
   socklen_t addrLen = sizeof(tmp_sock);
@@ -670,6 +667,30 @@ bool SocketAddress::operator<(const SocketAddress& other) const {
     return getFamily() < other.getFamily();
   }
 
+  if (external_) {
+    // Anonymous addresses can't be compared to anything else.
+    // Return that they are never less than anything.
+    //
+    // Note that this still meets the requirements for a strict weak
+    // ordering, so we can use this operator<() with standard C++ containers.
+    size_t thisPathLength = storage_.un.pathLength();
+    if (thisPathLength == 0) {
+      return false;
+    }
+    size_t otherPathLength = other.storage_.un.pathLength();
+    if (otherPathLength == 0) {
+      return true;
+    }
+
+    // Compare based on path length first, for efficiency
+    if (thisPathLength != otherPathLength) {
+      return thisPathLength < otherPathLength;
+    }
+    int cmp = memcmp(storage_.un.addr->sun_path,
+                     other.storage_.un.addr->sun_path,
+                     thisPathLength);
+    return cmp < 0;
+  }
   switch (getFamily()) {
     case AF_INET:
     case AF_INET6: {
@@ -680,30 +701,6 @@ bool SocketAddress::operator<(const SocketAddress& other) const {
       return
         storage_.addr < other.storage_.addr;
     }
-    case AF_UNIX: {
-      // Anonymous addresses can't be compared to anything else.
-      // Return that they are never less than anything.
-      //
-      // Note that this still meets the requirements for a strict weak
-      // ordering, so we can use this operator<() with standard C++ containers.
-      size_t thisPathLength = storage_.un.pathLength();
-      if (thisPathLength == 0) {
-        return false;
-      }
-      size_t otherPathLength = other.storage_.un.pathLength();
-      if (otherPathLength == 0) {
-        return true;
-      }
-
-      // Compare based on path length first, for efficiency
-      if (thisPathLength != otherPathLength) {
-        return thisPathLength < otherPathLength;
-      }
-      int cmp = memcmp(storage_.un.addr->sun_path,
-                       other.storage_.un.addr->sun_path,
-                       thisPathLength);
-      return cmp < 0;
-    }
     case AF_UNSPEC:
     default:
       throw std::invalid_argument(

diff --git a/folly/SocketAddress.h b/folly/SocketAddress.h
index ac1771c484064760d445605a3a8a7179e964b776..f2b1de12f4a19ab141aebae2ffa1459b90d56b88 100644 (file)
--- a/folly/SocketAddress.h
+++ b/folly/SocketAddress.h
@@ -32,9 +32,7 @@ namespace folly {
 
 class SocketAddress {
  public:
-  SocketAddress() {
-    storage_.addr = folly::IPAddress();
-  }
+  SocketAddress() {}
 
   /**
    * Construct a SocketAddress from a hostname and port.
@@ -76,16 +74,17 @@ class SocketAddress {
   }
 
   SocketAddress(const SocketAddress& addr) {
-    storage_ = addr.storage_;
     port_ = addr.port_;
     if (addr.getFamily() == AF_UNIX) {
       storage_.un.init(addr.storage_.un);
+    } else {
+      storage_ = addr.storage_;
     }
     external_ = addr.external_;
   }
 
   SocketAddress& operator=(const SocketAddress& addr) {
-    if (getFamily() != AF_UNIX) {
+    if (!external_) {
       if (addr.getFamily() != AF_UNIX) {
         storage_ = addr.storage_;
       } else {
@@ -105,7 +104,7 @@ class SocketAddress {
     return *this;
   }
 
-  SocketAddress(SocketAddress&& addr) {
+  SocketAddress(SocketAddress&& addr) noexcept {
     storage_ = addr.storage_;
     port_ = addr.port_;
     external_ = addr.external_;
@@ -120,7 +119,7 @@ class SocketAddress {
   }
 
   ~SocketAddress() {
-    if (getFamily() == AF_UNIX) {
+    if (external_) {
       storage_.un.free();
     }
   }
@@ -349,7 +348,7 @@ class SocketAddress {
    * Returns the actual size of the storage used.
    */
   socklen_t getAddress(sockaddr_storage* addr) const {
-    if (getFamily() != AF_UNIX) {
+    if (!external_) {
       return storage_.addr.toSockaddrStorage(addr, htons(port_));
     } else {
       memcpy(addr, storage_.un.addr, sizeof(*storage_.un.addr));
@@ -363,6 +362,7 @@ class SocketAddress {
   socklen_t getActualSize() const;
 
   sa_family_t getFamily() const {
+    DCHECK(external_ || AF_UNIX != storage_.addr.family());
     return external_ ? AF_UNIX : storage_.addr.family();
   }
 
@@ -547,12 +547,13 @@ class SocketAddress {
 
   void prepFamilyChange(sa_family_t newFamily) {
     if (newFamily != AF_UNIX) {
-      if (getFamily() == AF_UNIX) {
+      if (external_) {
         storage_.un.free();
+        storage_.addr = folly::IPAddress();
       }
       external_ = false;
     } else {
-      if (getFamily() != AF_UNIX) {
+      if (!external_) {
         storage_.un.init();
       }
       external_ = true;
@@ -569,7 +570,7 @@ class SocketAddress {
   union {
     folly::IPAddress addr{};
     ExternalUnixAddr un;
-  } storage_;
+  } storage_{};
   // IPAddress class does nto save zone or port, and must be saved here
   uint16_t port_;
 

diff --git a/folly/test/SocketAddressTest.cpp b/folly/test/SocketAddressTest.cpp
index 8165f99cbd9b2a48801287a109a3f7636dc67a0d..39eb9820e16be285719e5a7b004aa60c8acb8a12 100644 (file)
--- a/folly/test/SocketAddressTest.cpp
+++ b/folly/test/SocketAddressTest.cpp
@@ -840,3 +840,11 @@ TEST(SocketAddress, SetFromSocketUnixAnonymous) {
   EXPECT_EQ(clientAddr.getPath(), "");
   EXPECT_EQ(acceptAddr.getPath(), "");
 }
+
+TEST(SocketAddress, ResetUnixAddress) {
+  SocketAddress addy;
+  addy.setFromPath("/foo");
+
+  addy.reset();
+  EXPECT_EQ(addy.getFamily(), AF_UNSPEC);
+}