From a0688e78a6c7f3df4de8bd5d891bc53aa6823343 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Damian=20Kr=C3=B3lik?=
 <66667989+Damian-Nordic@users.noreply.github.com>
Date: Fri, 18 Nov 2022 10:58:14 +0100
Subject: [PATCH] [minimal mDNS] Fix sending mDNS goodbye packets (#23661)

* [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>
---
 src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 28 +++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
index 19975187a2937c..52a8be78bc6ffa 100644
--- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
+++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -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;
@@ -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)];
 
@@ -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())
     {
@@ -391,7 +405,6 @@ CHIP_ERROR AdvertiserMinMdns::RemoveServices()
 
     mQueryResponderAllocatorCommissionable.Clear();
     mQueryResponderAllocatorCommissioner.Clear();
-    return CHIP_NO_ERROR;
 }
 
 OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindOperationalAllocator(const FullQName & qname)
@@ -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);
@@ -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();