From 16178422e006b0e61bc3d051599bd88632b46562 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Thu, 16 Dec 2021 07:17:46 -0500 Subject: [PATCH] [Inet] Explicit error return from FromSockAddr (#13049) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### 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). --- .../chip-tool/commands/common/Command.cpp | 12 +++- src/inet/IPAddress.cpp | 14 ++-- src/inet/IPAddress.h | 8 ++- src/inet/IPPrefix.h | 1 + src/inet/InetInterface.cpp | 64 +++++++++---------- src/inet/InetInterface.h | 17 ++--- src/inet/TCPEndPointImplSockets.cpp | 6 +- src/inet/UDPEndPointImplSockets.cpp | 5 +- src/inet/tests/TestInetEndPoint.cpp | 9 ++- src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 11 +++- src/lib/dnssd/minimal_mdns/responders/IP.cpp | 20 ++---- src/platform/Darwin/DnssdImpl.cpp | 6 +- .../Linux/ConnectivityManagerImpl.cpp | 4 +- 13 files changed, 91 insertions(+), 86 deletions(-) diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp index 01559c5376599f..0310e412755f8b 100644 --- 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(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(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 index 74ef40144ec102..ce65f7875e811f 100644 --- 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 index ab326463ce4399..3b502a8293a1fc 100644 --- a/src/inet/IPAddress.h +++ b/src/inet/IPAddress.h @@ -37,6 +37,7 @@ #include #include +#include #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(sockaddr)); } + static CHIP_ERROR GetIPAddressFromSockAddr(const SockAddr & sockaddr, IPAddress & outIPAddress); + static CHIP_ERROR GetIPAddressFromSockAddr(const sockaddr & sockaddr, IPAddress & outIPAddress) + { + return GetIPAddressFromSockAddr(reinterpret_cast(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 index 124cf0445ac6f4..ce58c1b3a700a3 100644 --- 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 index 1ce272d4b60a45..be344cbe43b983 100644 --- 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 index 34638c3ea50e43..1189b0a9024e45 100644 --- 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 index 1f6bd368b1e948..faa69729c8a7ec 100644 --- 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 index 23428fa49c4a63..93b0828f5a1e21 100644 --- 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 index 34fc95267f2666..1e4ef41b8997d3 100644 --- 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 index ee6612c82093f0..7b4baa7f38a619 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -766,7 +766,12 @@ 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; } @@ -774,8 +779,8 @@ void AdvertiserMinMdns::AdvertiseRecords() 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 index 8d354b2a164ff0..c235c8f57d3ed9 100644 --- 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 index dede9652732e51..931631307b3cd0 100644 --- 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 index d1d50779e38009..8cc3fc1b9f933f 100644 --- 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;