Skip to content

Commit

Permalink
[minimal mDNS] Fix sending mDNS goodbye packets (project-chip#23661)
Browse files Browse the repository at this point in the history
* [minimal mDNS] Fix sending mDNS goodbye packets

mDNS clients such as avahi would keep stale commissionable
node services in their caches because minimal mDNS did not
send "Goodbye" packets properly on service removal. That is,
it attempted to send records with TTL=0 in Advertise()
methods, called after RemoveServices() which clears all
allocated responders.

It could be reproduce by opening and closing the
commissioning window on the device because each open
operation regenerates commissionable node service instance
name.

Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>

* Code review

Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
  • Loading branch information
Damian-Nordic authored and adbridge committed Nov 18, 2022
1 parent 19fc4cb commit a0688e7
Showing 1 changed file with 15 additions and 13 deletions.
28 changes: 15 additions & 13 deletions src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
ChipLogError(Discovery, "Failed to set up commissioner responder: %" CHIP_ERROR_FORMAT, err.Format());
}
}
~AdvertiserMinMdns() override { RemoveServices(); }
~AdvertiserMinMdns() override { ClearServices(); }

// Service advertiser
CHIP_ERROR Init(chip::Inet::EndPointManager<chip::Inet::UDPEndPoint> * udpEndPointManager) override;
Expand Down Expand Up @@ -281,6 +281,8 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
OperationalQueryAllocator::Allocator * FindOperationalAllocator(const FullQName & qname);
OperationalQueryAllocator::Allocator * FindEmptyOperationalAllocator();

void ClearServices();

ResponseSender mResponseSender;
uint8_t mCommissionableInstanceName[sizeof(uint64_t)];

Expand Down Expand Up @@ -366,6 +368,18 @@ void AdvertiserMinMdns::Shutdown()
}

CHIP_ERROR AdvertiserMinMdns::RemoveServices()
{
// Send a "goodbye" packet for each RR being removed, as defined in RFC 6762.
// This allows mDNS clients to remove stale cached records which may not be re-added with
// subsequent Advertise() calls. In the case the same records are re-added, this extra
// is not harmful though suboptimal, so this is a subject to improvement in the future.
AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);
ClearServices();

return CHIP_NO_ERROR;
}

void AdvertiserMinMdns::ClearServices()
{
while (mOperationalResponders.begin() != mOperationalResponders.end())
{
Expand All @@ -391,7 +405,6 @@ CHIP_ERROR AdvertiserMinMdns::RemoveServices()

mQueryResponderAllocatorCommissionable.Clear();
mQueryResponderAllocatorCommissioner.Clear();
return CHIP_NO_ERROR;
}

OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindOperationalAllocator(const FullQName & qname)
Expand Down Expand Up @@ -431,17 +444,11 @@ OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindEmptyOperationalAl

CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters & params)
{

char nameBuffer[Operational::kInstanceNameMaxLength + 1] = "";

// need to set server name
ReturnErrorOnFailure(MakeInstanceName(nameBuffer, sizeof(nameBuffer), params.GetPeerId()));

// Advertising data changed. Send a TTL=0 for everything as a refresh,
// which will clear caches (including things we are about to remove). Once this is done
// we will re-advertise available records with a longer TTL again.
AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);

QNamePart nameCheckParts[] = { nameBuffer, kOperationalServiceName, kOperationalProtocol, kLocalDomain };
FullQName nameCheck = FullQName(nameCheckParts);
auto * operationalAllocator = FindOperationalAllocator(nameCheck);
Expand Down Expand Up @@ -558,11 +565,6 @@ CHIP_ERROR AdvertiserMinMdns::UpdateCommissionableInstanceName()

CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters & params)
{
// Advertising data changed. Send a TTL=0 for everything as a refresh,
// which will clear caches (including things we are about to remove). Once this is done
// we will re-advertise available records with a longer TTL again.
AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);

if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)
{
mQueryResponderAllocatorCommissionable.Clear();
Expand Down

0 comments on commit a0688e7

Please sign in to comment.