[Inet] Explicit error return from FromSockAddr (#13049)

#### Problem A review raised questions about failure cases of `IPAddress::FromSockAddr` (see link in the issue for details). Fixes #10800 _IPAddress::FromSockAddr falls through to return Any — is this correct?_ #### Change overview - Replace `FromSockAddr(SockAddr)` with `GetIPAddressFromSockAddr()` with an explicit error return. - Change its main use, `InterfaceIterator::GetAddress()`, to also have an explicit error return. #### Testing CI (`GetAddress` is unit tested).
project-chip · Feb 15, 2022 · 1528780 · 1528780
1 parent e4c1e2f
commit 1528780
Show file tree

Hide file tree

Showing 13 changed files with 91 additions and 86 deletions.
diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp
@@ -119,16 +119,24 @@ static bool ParseAddressWithInterface(const char * addressString, Command::Addre
  return false;
  }
 
- address->address = ::chip::Inet::IPAddress::FromSockAddr(*result->ai_addr);
  if (result->ai_family == AF_INET6)
  {
  struct sockaddr_in6 * addr = reinterpret_cast<struct sockaddr_in6 *>(result->ai_addr);
+ address->address = ::chip::Inet::IPAddress::FromSockAddr(*addr);
  address->interfaceId = ::chip::Inet::InterfaceId(addr->sin6_scope_id);
  }
- else
+#if INET_CONFIG_ENABLE_IPV4
+ else if (result->ai_family == AF_INET)
  {
+ address->address = ::chip::Inet::IPAddress::FromSockAddr(*reinterpret_cast<struct sockaddr_in *>(result->ai_addr));
  address->interfaceId = chip::Inet::InterfaceId::Null();
  }
+#endif // INET_CONFIG_ENABLE_IPV4
+ else
+ {
+ ChipLogError(chipTool, "Unsupported address: %s", addressString);
+ return false;
+ }
 
  return true;
 }

diff --git a/src/inet/IPAddress.cpp b/src/inet/IPAddress.cpp
@@ -218,15 +218,21 @@ struct in6_addr IPAddress::ToIPv6() const
  return ipAddr;
 }
 
-IPAddress IPAddress::FromSockAddr(const SockAddr & sockaddr)
+CHIP_ERROR IPAddress::GetIPAddressFromSockAddr(const SockAddr & sockaddr, IPAddress & outIPAddress)
 {
 #if INET_CONFIG_ENABLE_IPV4
  if (sockaddr.any.sa_family == AF_INET)
- return FromSockAddr(sockaddr.in);
+ {
+ outIPAddress = FromSockAddr(sockaddr.in);
+ return CHIP_NO_ERROR;
+ }
 #endif // INET_CONFIG_ENABLE_IPV4
  if (sockaddr.any.sa_family == AF_INET6)
- return FromSockAddr(sockaddr.in6);
- return Any;
+ {
+ outIPAddress = FromSockAddr(sockaddr.in6);
+ return CHIP_NO_ERROR;
+ }
+ return INET_ERROR_WRONG_ADDRESS_TYPE;
 }
 
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK

diff --git a/src/inet/IPAddress.h b/src/inet/IPAddress.h
@@ -37,6 +37,7 @@
 #include <lib/support/DLLUtil.h>
 
 #include <inet/InetConfig.h>
+#include <inet/InetError.h>
 
 #include "inet/IANAConstants.h"
 
@@ -512,8 +513,11 @@ class DLL_EXPORT IPAddress
  /**
  * Get the IP address from a SockAddr.
  */
- static IPAddress FromSockAddr(const SockAddr & sockaddr);
- static IPAddress FromSockAddr(const sockaddr & sockaddr) { return FromSockAddr(reinterpret_cast<const SockAddr &>(sockaddr)); }
+ static CHIP_ERROR GetIPAddressFromSockAddr(const SockAddr & sockaddr, IPAddress & outIPAddress);
+ static CHIP_ERROR GetIPAddressFromSockAddr(const sockaddr & sockaddr, IPAddress & outIPAddress)
+ {
+ return GetIPAddressFromSockAddr(reinterpret_cast<const SockAddr &>(sockaddr), outIPAddress);
+ }
  static IPAddress FromSockAddr(const sockaddr_in6 & sockaddr) { return IPAddress(sockaddr.sin6_addr); }
 #if INET_CONFIG_ENABLE_IPV4
  static IPAddress FromSockAddr(const sockaddr_in & sockaddr) { return IPAddress(sockaddr.sin_addr); }

diff --git a/src/inet/IPPrefix.h b/src/inet/IPPrefix.h
@@ -46,6 +46,7 @@ class IPPrefix
 {
 public:
  IPPrefix() = default;
+ IPPrefix(const IPAddress & ipAddress, uint8_t length) : IPAddr(ipAddress), Length(length) {}
 
  /**
  * Copy constructor for the IPPrefix class.

diff --git a/src/inet/InetInterface.cpp b/src/inet/InetInterface.cpp
@@ -214,25 +214,28 @@ bool InterfaceAddressIterator::Next()
  return false;
 }
 
-IPAddress InterfaceAddressIterator::GetAddress()
+CHIP_ERROR InterfaceAddressIterator::GetAddress(IPAddress & outIPAddress)
 {
- if (HasCurrent())
+ if (!HasCurrent())
  {
- struct netif * curIntf = mIntfIter.GetInterfaceId().GetPlatformInterface();
+ return CHIP_ERROR_SENTINEL;
+ }
 
- if (mCurAddrIndex < LWIP_IPV6_NUM_ADDRESSES)
- {
- return IPAddress(*netif_ip6_addr(curIntf, mCurAddrIndex));
- }
+ struct netif * curIntf = mIntfIter.GetInterfaceId().GetPlatformInterface();
+
+ if (mCurAddrIndex < LWIP_IPV6_NUM_ADDRESSES)
+ {
+ outIPAddress = IPAddress(*netif_ip6_addr(curIntf, mCurAddrIndex));
+ return CHIP_NO_ERROR;
+ }
 #if INET_CONFIG_ENABLE_IPV4 && LWIP_IPV4
- else
- {
- return IPAddress(*netif_ip4_addr(curIntf));
- }
-#endif // INET_CONFIG_ENABLE_IPV4 && LWIP_IPV4
+ else
+ {
+ outIPAddress = IPAddress(*netif_ip4_addr(curIntf));
+ return CHIP_NO_ERROR;
  }
-
- return IPAddress::Any;
+#endif // INET_CONFIG_ENABLE_IPV4 && LWIP_IPV4
+ return CHIP_ERROR_INTERNAL;
 }
 
 uint8_t InterfaceAddressIterator::GetPrefixLength()
@@ -694,9 +697,9 @@ bool InterfaceAddressIterator::Next()
  }
 }
 
-IPAddress InterfaceAddressIterator::GetAddress()
+CHIP_ERROR InterfaceAddressIterator::GetAddress(IPAddress & outIPAddress)
 {
- return HasCurrent() ? IPAddress::FromSockAddr(*mCurAddr->ifa_addr) : IPAddress::Any;
+ return HasCurrent() ? IPAddress::GetIPAddressFromSockAddr(*mCurAddr->ifa_addr, outIPAddress) : CHIP_ERROR_SENTINEL;
 }
 
 uint8_t InterfaceAddressIterator::GetPrefixLength()
@@ -945,9 +948,14 @@ bool InterfaceAddressIterator::Next()
  return false;
 }
 
-IPAddress InterfaceAddressIterator::GetAddress()
+CHIP_ERROR InterfaceAddressIterator::GetAddress(IPAddress & outIPAddress)
 {
- return HasCurrent() ? IPAddress(mIpv6->unicast[mCurAddrIndex].address.in6_addr) : IPAddress::Any;
+ if (HasCurrent())
+ {
+ outIPAddress = IPAddress(mIpv6->unicast[mCurAddrIndex].address.in6_addr);
+ return CHIP_NO_ERROR;
+ }
+ return CHIP_ERROR_SENTINEL;
 }
 
 uint8_t InterfaceAddressIterator::GetPrefixLength()
@@ -1011,8 +1019,8 @@ InterfaceId InterfaceId::FromIPAddress(const IPAddress & addr)
 
  for (; addrIter.HasCurrent(); addrIter.Next())
  {
- IPAddress curAddr = addrIter.GetAddress();
- if (addr == curAddr)
+ IPAddress curAddr;
+ if ((addrIter.GetAddress(curAddr) == CHIP_NO_ERROR) && (addr == curAddr))
  {
  return addrIter.GetInterfaceId();
  }
@@ -1031,7 +1039,8 @@ bool InterfaceId::MatchLocalIPv6Subnet(const IPAddress & addr)
  for (; ifAddrIter.HasCurrent(); ifAddrIter.Next())
  {
  IPPrefix addrPrefix;
- addrPrefix.IPAddr = ifAddrIter.GetAddress();
+ if (ifAddrIter.GetAddress(addrPrefix.IPAddr) != CHIP_NO_ERROR)
+ continue;
 #if INET_CONFIG_ENABLE_IPV4
  if (addrPrefix.IPAddr.IsIPv4())
  continue;
@@ -1046,19 +1055,6 @@ bool InterfaceId::MatchLocalIPv6Subnet(const IPAddress & addr)
  return false;
 }
 
-void InterfaceAddressIterator::GetAddressWithPrefix(IPPrefix & addrWithPrefix)
-{
- if (HasCurrent())
- {
- addrWithPrefix.IPAddr = GetAddress();
- addrWithPrefix.Length = GetPrefixLength();
- }
- else
- {
- addrWithPrefix = IPPrefix::Zero;
- }
-}
-
 uint8_t NetmaskToPrefixLength(const uint8_t * netmask, uint16_t netmaskLen)
 {
  uint8_t prefixLen = 0;

diff --git a/src/inet/InetInterface.h b/src/inet/InetInterface.h
@@ -418,10 +418,13 @@ class DLL_EXPORT InterfaceAddressIterator
  *
  * @brief Get the current interface address.
  *
- * @return the current interface address or \c IPAddress::Any if the iterator
- * is positioned beyond the end of the address list.
+ * @param[out] outIPAddress The current interface address
+ *
+ * @return CHIP_NO_ERROR if the result IPAddress is valid.
+ * @return CHIP_ERROR_SENTINEL if the iterator is positioned beyond the end of the address list.
+ * @return other error from lower-level code
  */
- IPAddress GetAddress();
+ CHIP_ERROR GetAddress(IPAddress & outIPAddress);
 
  /**
  * @fn uint8_t InterfaceAddressIterator::GetPrefixLength(void)
@@ -442,14 +445,6 @@ class DLL_EXPORT InterfaceAddressIterator
  */
  uint8_t GetPrefixLength();
 
- /**
- * @fn void InterfaceAddressIterator::GetAddressWithPrefix(IPPrefix & addrWithPrefix)
- *
- * @brief Returns an IPPrefix containing the address and prefix length
- * for the current address.
- */
- void GetAddressWithPrefix(IPPrefix & addrWithPrefix);
-
  /**
  * @fn InterfaceId InterfaceAddressIterator::GetInterfaceId(void)
  *

diff --git a/src/inet/TCPEndPointImplSockets.cpp b/src/inet/TCPEndPointImplSockets.cpp
@@ -748,10 +748,8 @@ CHIP_ERROR TCPEndPointImplSockets::BindSrcAddrFromIntf(IPAddressType addrType, I
  bool ipAddrFound = false;
  for (InterfaceAddressIterator addrIter; addrIter.HasCurrent(); addrIter.Next())
  {
- const IPAddress curAddr = addrIter.GetAddress();
- const InterfaceId curIntfId = addrIter.GetInterfaceId();
-
- if (curIntfId == intfId)
+ IPAddress curAddr;
+ if ((addrIter.GetInterfaceId() == intfId) && (addrIter.GetAddress(curAddr) == CHIP_NO_ERROR))
  {
  // Search for an IPv4 address on the TargetInterface
 

diff --git a/src/inet/UDPEndPointImplSockets.cpp b/src/inet/UDPEndPointImplSockets.cpp
@@ -758,9 +758,8 @@ CHIP_ERROR UDPEndPointImplSockets::IPv4JoinLeaveMulticastGroupImpl(InterfaceId a
 
  for (InterfaceAddressIterator lAddressIterator; lAddressIterator.HasCurrent(); lAddressIterator.Next())
  {
- const IPAddress lCurrentAddress = lAddressIterator.GetAddress();
-
- if (lAddressIterator.GetInterfaceId() == aInterfaceId)
+ IPAddress lCurrentAddress;
+ if ((lAddressIterator.GetInterfaceId() == aInterfaceId) && (lAddressIterator.GetAddress(lCurrentAddress) == CHIP_NO_ERROR))
  {
  if (lCurrentAddress.IsIPv4())
  {

diff --git a/src/inet/tests/TestInetEndPoint.cpp b/src/inet/tests/TestInetEndPoint.cpp
@@ -130,7 +130,6 @@ static void TestInetInterface(nlTestSuite * inSuite, void * inContext)
  char intName[chip::Inet::InterfaceId::kMaxIfNameLength];
  InterfaceId intId;
  IPAddress addr;
- IPPrefix addrWithPrefix;
  InterfaceType intType;
  // 64 bit IEEE MAC address
  const uint8_t kMaxHardwareAddressSize = 8;
@@ -209,8 +208,9 @@ static void TestInetInterface(nlTestSuite * inSuite, void * inContext)
  printf(" Addresses:\n");
  for (; addrIterator.HasCurrent(); addrIterator.Next())
  {
- addr = addrIterator.GetAddress();
- addrIterator.GetAddressWithPrefix(addrWithPrefix);
+ err = addrIterator.GetAddress(addr);
+ NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+ IPPrefix addrWithPrefix(addr, addrIterator.GetPrefixLength());
  char addrStr[80];
  addrWithPrefix.IPAddr.ToString(addrStr);
  intId = addrIterator.GetInterfaceId();
@@ -231,8 +231,7 @@ static void TestInetInterface(nlTestSuite * inSuite, void * inContext)
  addrIterator.HasBroadcastAddress() ? "has" : "no");
  }
  NL_TEST_ASSERT(inSuite, !addrIterator.Next());
- addrIterator.GetAddressWithPrefix(addrWithPrefix);
- NL_TEST_ASSERT(inSuite, addrWithPrefix.IsZero());
+ NL_TEST_ASSERT(inSuite, addrIterator.GetAddress(addr) == CHIP_ERROR_SENTINEL);
  NL_TEST_ASSERT(inSuite, addrIterator.GetInterfaceId() == InterfaceId::Null());
  NL_TEST_ASSERT(inSuite, addrIterator.GetInterfaceName(intName, sizeof(intName)) == CHIP_ERROR_INCORRECT_STATE);
  NL_TEST_ASSERT(inSuite, !addrIterator.SupportsMulticast());

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -766,16 +766,21 @@ void AdvertiserMinMdns::AdvertiseRecords()
  continue;
  }
 
- if (!ShouldAdvertiseOn(interfaceAddress.GetInterfaceId(), interfaceAddress.GetAddress()))
+ Inet::IPAddress ipAddress;
+ if (interfaceAddress.GetAddress(ipAddress) != CHIP_NO_ERROR)
+ {
+ continue;
+ }
+ if (!ShouldAdvertiseOn(interfaceAddress.GetInterfaceId(), ipAddress))
  {
  continue;
  }
 
  chip::Inet::IPPacketInfo packetInfo;
 
  packetInfo.Clear();
- packetInfo.SrcAddress = interfaceAddress.GetAddress();
- if (interfaceAddress.GetAddress().IsIPv4())
+ packetInfo.SrcAddress = ipAddress;
+ if (ipAddress.IsIPv4())
  {
  BroadcastIpAddresses::GetIpv4Into(packetInfo.DestAddress);
  }

diff --git a/src/lib/dnssd/minimal_mdns/responders/IP.cpp b/src/lib/dnssd/minimal_mdns/responders/IP.cpp
@@ -23,19 +23,13 @@ namespace Minimal {
 
 void IPv4Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate)
 {
+ chip::Inet::IPAddress addr;
  for (chip::Inet::InterfaceAddressIterator it; it.HasCurrent(); it.Next())
  {
- if (it.GetInterfaceId() != source->Interface)
- {
- continue;
- }
-
- chip::Inet::IPAddress addr = it.GetAddress();
- if (!addr.IsIPv4())
+ if ((it.GetInterfaceId() == source->Interface) && (it.GetAddress(addr) == CHIP_NO_ERROR) && addr.IsIPv4())
  {
- continue;
+ delegate->AddResponse(IPResourceRecord(GetQName(), addr));
  }
- delegate->AddResponse(IPResourceRecord(GetQName(), addr));
  }
 }
 
@@ -48,13 +42,11 @@ void IPv6Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, Res
  continue;
  }
 
- chip::Inet::IPAddress addr = it.GetAddress();
- if (!addr.IsIPv6())
+ chip::Inet::IPAddress addr;
+ if ((it.GetInterfaceId() == source->Interface) && (it.GetAddress(addr) == CHIP_NO_ERROR) && addr.IsIPv6())
  {
- continue;
+ delegate->AddResponse(IPResourceRecord(GetQName(), addr));
  }
-
- delegate->AddResponse(IPResourceRecord(GetQName(), addr));
  }
 }
 

diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp
@@ -402,12 +402,14 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i
  service.mPort = sdCtx->port;
  service.mTextEntries = sdCtx->textEntries.empty() ? nullptr : sdCtx->textEntries.data();
  service.mTextEntrySize = sdCtx->textEntries.empty() ? 0 : sdCtx->textEntries.size();
- service.mAddress.SetValue(chip::Inet::IPAddress::FromSockAddr(*address));
+ chip::Inet::IPAddress ip;
+ CHIP_ERROR status = chip::Inet::IPAddress::GetIPAddressFromSockAddr(*address, ip);
+ service.mAddress.SetValue(ip);
  Platform::CopyString(service.mName, sdCtx->name);
  Platform::CopyString(service.mHostName, hostname);
  service.mInterface = Inet::InterfaceId(sdCtx->interfaceId);
 
- sdCtx->callback(sdCtx->context, &service, CHIP_NO_ERROR);
+ sdCtx->callback(sdCtx->context, &service, status);
  MdnsContexts::GetInstance().Remove(sdCtx);
 }
 

diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp
@@ -946,8 +946,8 @@ CHIP_ERROR ConnectivityManagerImpl::ProvisionWiFiNetwork(const char * ssid, cons
  if (it.IsUp() && CHIP_NO_ERROR == it.GetInterfaceName(ifName, sizeof(ifName)) &&
  strncmp(ifName, sWiFiIfName, sizeof(ifName)) == 0)
  {
- chip::Inet::IPAddress addr = it.GetAddress();
- if (addr.IsIPv4())
+ chip::Inet::IPAddress addr;
+ if ((it.GetAddress(addr) == CHIP_NO_ERROR) && addr.IsIPv4())
  {
  ChipDeviceEvent event;
  event.Type = DeviceEventType::kInternetConnectivityChange;