Skip to content

Commit

Permalink
[Inet] Explicit error return from FromSockAddr (#13049)
Browse files Browse the repository at this point in the history 
#### 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).
  • Loading branch information
@kpschoedel
kpschoedel authored Dec 16, 2021
1 parent 9ed7c0c commit f86f21f
Show file tree
Hide file tree
Showing 13 changed files with 91 additions and 86 deletions.
12 changes: 10 additions & 2 deletions examples/chip-tool/commands/common/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
14 changes: 10 additions & 4 deletions src/inet/IPAddress.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions src/inet/IPAddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <lib/support/DLLUtil.h>

#include <inet/InetConfig.h>
#include <inet/InetError.h>

#include "inet/IANAConstants.h"

Expand Down Expand Up @@ -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); }
Expand Down
1 change: 1 addition & 0 deletions src/inet/IPPrefix.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
64 changes: 30 additions & 34 deletions src/inet/InetInterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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();
}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
17 changes: 6 additions & 11 deletions src/inet/InetInterface.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
*
Expand Down
6 changes: 2 additions & 4 deletions src/inet/TCPEndPointImplSockets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions src/inet/UDPEndPointImplSockets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand Down
9 changes: 4 additions & 5 deletions src/inet/tests/TestInetEndPoint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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());
Expand Down
11 changes: 8 additions & 3 deletions src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
20 changes: 6 additions & 14 deletions src/lib/dnssd/minimal_mdns/responders/IP.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

Expand All @@ -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));
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/platform/Darwin/DnssdImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/platform/Linux/ConnectivityManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit f86f21f

Please sign in to comment.