Skip to content

Commit

Permalink
Use Pool for mDNS udp endpoint, fix endpoint leak (#12417)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kghost @pull
kghost authored and pull[bot] committed Dec 17, 2021
1 parent d908d14 commit 1211928
Show file tree
Hide file tree
Showing 7 changed files with 206 additions and 158 deletions.
25 changes: 13 additions & 12 deletions src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,29 +724,30 @@ FullQName AdvertiserMinMdns::GetCommissioningTxtEntries(const CommissionAdvertis
bool AdvertiserMinMdns::ShouldAdvertiseOn(const chip::Inet::InterfaceId id, const chip::Inet::IPAddress & addr)
{
auto & server = GlobalMinimalMdnsServer::Server();
for (unsigned i = 0; i < server.GetEndpointCount(); i++)
{
const ServerBase::EndpointInfo & info = server.GetEndpoints()[i];

if (info.listen_udp == nullptr)
bool result = false;

server.ForEachEndPoints([&](auto * info) {
if (info->mListenUdp == nullptr)
{
continue;
return chip::Loop::Continue;
}

if (info.interfaceId != id)
if (info->mInterfaceId != id)
{
continue;
return chip::Loop::Continue;
}

if (info.addressType != addr.Type())
if (info->mAddressType != addr.Type())
{
continue;
return chip::Loop::Continue;
}

return true;
}
result = true;
return chip::Loop::Break;
});

return false;
return result;
}

void AdvertiserMinMdns::AdvertiseRecords()
Expand Down
204 changes: 101 additions & 103 deletions src/lib/dnssd/minimal_mdns/Server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class ShutdownOnError
class ListenSocketPickerDelegate : public ServerBase::BroadcastSendDelegate
{
public:
chip::Inet::UDPEndPoint * Accept(ServerBase::EndpointInfo * info) override { return info->listen_udp; }
chip::Inet::UDPEndPoint * Accept(ServerBase::EndpointInfo * info) override { return info->mListenUdp; }
};

#if CHIP_MINMDNS_USE_EPHEMERAL_UNICAST_PORT
Expand All @@ -65,7 +65,7 @@ class ListenSocketPickerDelegate : public ServerBase::BroadcastSendDelegate
class QuerySocketPickerDelegate : public ServerBase::BroadcastSendDelegate
{
public:
chip::Inet::UDPEndPoint * Accept(ServerBase::EndpointInfo * info) override { return info->unicast_query_udp; }
chip::Inet::UDPEndPoint * Accept(ServerBase::EndpointInfo * info) override { return info->mUnicastQueryUdp; }
};

#else
Expand Down Expand Up @@ -96,12 +96,12 @@ class InterfaceTypeFilterDelegate : public ServerBase::BroadcastSendDelegate

chip::Inet::UDPEndPoint * Accept(ServerBase::EndpointInfo * info) override
{
if ((info->interfaceId != mInterface) && (info->interfaceId != chip::Inet::InterfaceId::Null()))
if ((info->mInterfaceId != mInterface) && (info->mInterfaceId != chip::Inet::InterfaceId::Null()))
{
return nullptr;
}

if ((mAddressType != chip::Inet::IPAddressType::kAny) && (info->addressType != mAddressType))
if ((mAddressType != chip::Inet::IPAddressType::kAny) && (info->mAddressType != mAddressType))
{
return nullptr;
}
Expand Down Expand Up @@ -180,23 +180,6 @@ const char * AddressTypeStr(chip::Inet::IPAddressType addressType)
}
}

void ShutdownEndpoint(mdns::Minimal::ServerBase::EndpointInfo & aEndpoint)
{
if (aEndpoint.listen_udp != nullptr)
{
aEndpoint.listen_udp->Free();
aEndpoint.listen_udp = nullptr;
}

#if CHIP_MINMDNS_USE_EPHEMERAL_UNICAST_PORT
if (aEndpoint.unicast_query_udp != nullptr)
{
aEndpoint.unicast_query_udp->Free();
aEndpoint.unicast_query_udp = nullptr;
}
#endif
}

} // namespace

ServerBase::~ServerBase()
Expand All @@ -206,49 +189,51 @@ ServerBase::~ServerBase()

void ServerBase::Shutdown()
{
for (size_t i = 0; i < mEndpointCount; i++)
{
ShutdownEndpoint(mEndpoints[i]);
}
mEndpoints.ForEachActiveObject([&](auto * endpoint) {
ShutdownEndpoint(*endpoint);
return chip::Loop::Continue;
});
}

void ServerBase::ShutdownEndpoint(EndpointInfo & aEndpoint)
{
mEndpoints.ReleaseObject(&aEndpoint);
}

bool ServerBase::IsListening() const
{
for (size_t i = 0; i < mEndpointCount; i++)
{
if (mEndpoints[i].listen_udp != nullptr)
bool listening = false;
mEndpoints.ForEachActiveObject([&](auto * endpoint) {
if (endpoint->mListenUdp != nullptr)
{
return true;
listening = true;
return chip::Loop::Break;
}
}
return false;
return chip::Loop::Continue;
});
return listening;
}

CHIP_ERROR ServerBase::Listen(chip::Inet::InetLayer * inetLayer, ListenIterator * it, uint16_t port)
{
Shutdown(); // ensure everything starts fresh

size_t endpointIndex = 0;
chip::Inet::InterfaceId interfaceId = chip::Inet::InterfaceId::Null();
chip::Inet::IPAddressType addressType;

ShutdownOnError autoShutdown(this);

while (it->Next(&interfaceId, &addressType))
{
ReturnErrorCodeIf(endpointIndex >= mEndpointCount, CHIP_ERROR_NO_MEMORY);

EndpointInfo * info = &mEndpoints[endpointIndex];
info->addressType = addressType;
info->interfaceId = interfaceId;

ReturnErrorOnFailure(inetLayer->GetUDPEndPointManager()->NewEndPoint(&info->listen_udp));
chip::Inet::UDPEndPoint * listenUdp;
ReturnErrorOnFailure(inetLayer->GetUDPEndPointManager()->NewEndPoint(&listenUdp));
std::unique_ptr<chip::Inet::UDPEndPoint, EndpointInfo::EndPointDeletor> endPointHolder(listenUdp, {});

ReturnErrorOnFailure(info->listen_udp->Bind(addressType, chip::Inet::IPAddress::Any, port, interfaceId));
ReturnErrorOnFailure(listenUdp->Bind(addressType, chip::Inet::IPAddress::Any, port, interfaceId));

ReturnErrorOnFailure(info->listen_udp->Listen(OnUdpPacketReceived, nullptr /*OnReceiveError*/, this));
ReturnErrorOnFailure(listenUdp->Listen(OnUdpPacketReceived, nullptr /*OnReceiveError*/, this));

CHIP_ERROR err = JoinMulticastGroup(interfaceId, info->listen_udp, addressType);
CHIP_ERROR err = JoinMulticastGroup(interfaceId, listenUdp, addressType);
if (err != CHIP_NO_ERROR)
{
char interfaceName[chip::Inet::InterfaceId::kMaxIfNameLength];
Expand All @@ -257,21 +242,32 @@ CHIP_ERROR ServerBase::Listen(chip::Inet::InetLayer * inetLayer, ListenIterator
// Log only as non-fatal error. Failure to join will mean we reply to unicast queries only.
ChipLogError(DeviceLayer, "MDNS failed to join multicast group on %s for address type %s: %s", interfaceName,
AddressTypeStr(addressType), chip::ErrorStr(err));
ShutdownEndpoint(mEndpoints[endpointIndex]);
}
else
{
endpointIndex++;
}

#if CHIP_MINMDNS_USE_EPHEMERAL_UNICAST_PORT
// Separate UDP endpoint for unicast queries, bound to 0 (i.e. pick random ephemeral port)
// - helps in not having conflicts on port 5353, will receive unicast replies directly
// - has a *DRAWBACK* of unicast queries being considered LEGACY by mdns since they do
// not originate from 5353 and the answers will include a query section.
ReturnErrorOnFailure(inetLayer->GetUDPEndPointManager()->NewEndPoint(&info->unicast_query_udp));
ReturnErrorOnFailure(info->unicast_query_udp->Bind(addressType, chip::Inet::IPAddress::Any, 0, interfaceId));
ReturnErrorOnFailure(info->unicast_query_udp->Listen(OnUdpPacketReceived, nullptr /*OnReceiveError*/, this));
chip::Inet::UDPEndPoint * unicastQueryUdp;
ReturnErrorOnFailure(inetLayer->GetUDPEndPointManager()->NewEndPoint(&unicastQueryUdp));
std::unique_ptr<chip::Inet::UDPEndPoint, EndpointInfo::EndPointDeletor> endPointHolderUnicast(unicastQueryUdp, {});
ReturnErrorOnFailure(unicastQueryUdp->Bind(addressType, chip::Inet::IPAddress::Any, 0, interfaceId));
ReturnErrorOnFailure(unicastQueryUdp->Listen(OnUdpPacketReceived, nullptr /*OnReceiveError*/, this));
#endif

#if CHIP_MINMDNS_USE_EPHEMERAL_UNICAST_PORT
if (endPointHolder || endPointHolderUnicast)
{
// If allocation fails, the rref will not be consumed, so that the endpoint will also be freed correctly
mEndpoints.CreateObject(interfaceId, addressType, std::move(endPointHolder), std::move(endPointHolderUnicast));
}
#else
if (endPointHolder)
{
// If allocation fails, the rref will not be consumed, so that the endpoint will also be freed correctly
mEndpoints.CreateObject(interfaceId, addressType, std::move(endPointHolder));
}
#endif
}

Expand All @@ -281,30 +277,30 @@ CHIP_ERROR ServerBase::Listen(chip::Inet::InetLayer * inetLayer, ListenIterator
CHIP_ERROR ServerBase::DirectSend(chip::System::PacketBufferHandle && data, const chip::Inet::IPAddress & addr, uint16_t port,
chip::Inet::InterfaceId interface)
{
for (size_t i = 0; i < mEndpointCount; i++)
{
EndpointInfo * info = &mEndpoints[i];
if (info->listen_udp == nullptr)
CHIP_ERROR err = CHIP_ERROR_NOT_CONNECTED;
mEndpoints.ForEachActiveObject([&](auto * info) {
if (info->mListenUdp == nullptr)
{
continue;
return chip::Loop::Continue;
}

if (info->addressType != addr.Type())
if (info->mAddressType != addr.Type())
{
continue;
return chip::Loop::Continue;
}

chip::Inet::InterfaceId boundIf = info->listen_udp->GetBoundInterface();
chip::Inet::InterfaceId boundIf = info->mListenUdp->GetBoundInterface();

if ((boundIf.IsPresent()) && (boundIf != interface))
{
continue;
return chip::Loop::Continue;
}

return info->listen_udp->SendTo(addr, port, std::move(data));
}
err = info->mListenUdp->SendTo(addr, port, std::move(data));
return chip::Loop::Break;
});

return CHIP_ERROR_NOT_CONNECTED;
return err;
}

CHIP_ERROR ServerBase::BroadcastUnicastQuery(chip::System::PacketBufferHandle && data, uint16_t port)
Expand Down Expand Up @@ -350,49 +346,51 @@ CHIP_ERROR ServerBase::BroadcastImpl(chip::System::PacketBufferHandle && data, u
bool hadSuccesfulSend = false;
CHIP_ERROR lastError = CHIP_ERROR_NO_ENDPOINT;

for (size_t i = 0; i < mEndpointCount; i++)
{
EndpointInfo * info = &mEndpoints[i];
chip::Inet::UDPEndPoint * udp = delegate->Accept(info);

if (udp == nullptr)
{
continue;
}

CHIP_ERROR err;

/// The same packet needs to be sent over potentially multiple interfaces.
/// LWIP does not like having a pbuf sent over serparate interfaces, hence we create a copy
/// for sending via `CloneData`
///
/// TODO: this wastes one copy of the data and that could be optimized away
if (info->addressType == chip::Inet::IPAddressType::kIPv6)
{
err = udp->SendTo(mIpv6BroadcastAddress, port, data.CloneData(), udp->GetBoundInterface());
}
if (chip::Loop::Break == mEndpoints.ForEachActiveObject([&](auto * info) {
chip::Inet::UDPEndPoint * udp = delegate->Accept(info);

if (udp == nullptr)
{
return chip::Loop::Continue;
}

CHIP_ERROR err;

/// The same packet needs to be sent over potentially multiple interfaces.
/// LWIP does not like having a pbuf sent over serparate interfaces, hence we create a copy
/// for sending via `CloneData`
///
/// TODO: this wastes one copy of the data and that could be optimized away
if (info->mAddressType == chip::Inet::IPAddressType::kIPv6)
{
err = udp->SendTo(mIpv6BroadcastAddress, port, data.CloneData(), udp->GetBoundInterface());
}
#if INET_CONFIG_ENABLE_IPV4
else if (info->addressType == chip::Inet::IPAddressType::kIPv4)
{
err = udp->SendTo(mIpv4BroadcastAddress, port, data.CloneData(), udp->GetBoundInterface());
}
else if (info->mAddressType == chip::Inet::IPAddressType::kIPv4)
{
err = udp->SendTo(mIpv4BroadcastAddress, port, data.CloneData(), udp->GetBoundInterface());
}
#endif
else
{
// This is a general error of internal consistency: every address has a known type
// Fail completely otherwise.
return CHIP_ERROR_INCORRECT_STATE;
}

if (err == CHIP_NO_ERROR)
{
hadSuccesfulSend = true;
}
else
{
ChipLogError(Discovery, "Attempt to mDNS broadcast failed: %s", chip::ErrorStr(err));
lastError = err;
}
else
{
// This is a general error of internal consistency: every address has a known type. Fail completely otherwise.
lastError = CHIP_ERROR_INCORRECT_STATE;
return chip::Loop::Break;
}

if (err == CHIP_NO_ERROR)
{
hadSuccesfulSend = true;
}
else
{
ChipLogError(Discovery, "Attempt to mDNS broadcast failed: %s", chip::ErrorStr(err));
lastError = err;
}
return chip::Loop::Continue;
}))
{
return lastError;
}

if (!hadSuccesfulSend)
Expand Down
Loading

0 comments on commit 1211928

Please sign in to comment.