Use platform alloc & intrusive lists for managing MinMdns responders (#…

…16181)

* Initial version: use an intrusive list for query responders

* Updated some tests

* Mark no upper limit on advertisement

* Remove TODO comment - I did it

* Simplified iterators - shorter code

* Add log and return error if query responder addition fails

* Fix back the test: out of memory is from the advertiser not allocators

* Updated comment

* Remove LogErrorOnFailure

* Review comments applied - typo/spacing fixed

* Enable DNSSD on ipv4 if broadcast is available

* Ensure minmdns cleans up its lists at shutdown

* Revert "Enable DNSSD on ipv4 if broadcast is available"

This reverts commit c4d3f6c.

src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp

@@ -35,6 +35,7 @@
 #include <lib/dnssd/minimal_mdns/responders/Txt.h>
 #include <lib/support/BytesToHex.h>
 #include <lib/support/CHIPMem.h>
+#include <lib/support/IntrusiveList.h>
 #include <lib/support/StringBuilder.h>
 
 // Enable detailed mDNS logging for received queries
@@ -98,6 +99,54 @@ void LogQuery(const QueryData & data)
 void LogQuery(const QueryData & data) {}
 #endif
 
+// Max number of records for operational = PTR, SRV, TXT, A, AAAA, I subtype.
+constexpr size_t kMaxOperationalRecords = 6;
+
+/// Represents an allocated operational responder.
+///
+/// Wraps a QueryResponderAllocator.
+class OperationalQueryAllocator : public chip::IntrusiveListNodeBase
+{
+public:
+    using Allocator = QueryResponderAllocator<kMaxOperationalRecords>;
+
+    /// Prefer to use `::New` for allocations instead of this direct call
+    explicit OperationalQueryAllocator(Allocator * allocator) : mAllocator(allocator) {}
+    ~OperationalQueryAllocator()
+    {
+        chip::Platform::Delete(mAllocator);
+        mAllocator = nullptr;
+    }
+
+    Allocator * GetAllocator() { return mAllocator; }
+    const Allocator * GetAllocator() const { return mAllocator; }
+
+    /// Allocate a new entry for this type.
+    ///
+    /// May return null on allocation failures.
+    static OperationalQueryAllocator * New()
+    {
+        Allocator * allocator = chip::Platform::New<Allocator>();
+
+        if (allocator == nullptr)
+        {
+            return nullptr;
+        }
+
+        OperationalQueryAllocator * result = chip::Platform::New<OperationalQueryAllocator>(allocator);
+        if (result == nullptr)
+        {
+            chip::Platform::Delete(allocator);
+            return nullptr;
+        }
+
+        return result;
+    }
+
+private:
+    Allocator * mAllocator = nullptr;
+};
+
 class AdvertiserMinMdns : public ServiceAdvertiser,
                           public MdnsPacketDelegate, // receive query packets
                           public ParserDelegate      // parses queries
@@ -106,14 +155,21 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
     AdvertiserMinMdns() : mResponseSender(&GlobalMinimalMdnsServer::Server())
     {
         GlobalMinimalMdnsServer::Instance().SetQueryDelegate(this);
-        for (auto & allocator : mQueryResponderAllocatorOperational)
+
+        CHIP_ERROR err = mResponseSender.AddQueryResponder(mQueryResponderAllocatorCommissionable.GetQueryResponder());
+
+        if (err != CHIP_NO_ERROR)
         {
-            mResponseSender.AddQueryResponder(allocator.GetQueryResponder());
+            ChipLogError(Discovery, "Failed to set up commissionable responder: %" CHIP_ERROR_FORMAT, err.Format());
+        }
+
+        err = mResponseSender.AddQueryResponder(mQueryResponderAllocatorCommissioner.GetQueryResponder());
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Discovery, "Failed to set up commissioner responder: %" CHIP_ERROR_FORMAT, err.Format());
         }
-        mResponseSender.AddQueryResponder(mQueryResponderAllocatorCommissionable.GetQueryResponder());
-        mResponseSender.AddQueryResponder(mQueryResponderAllocatorCommissioner.GetQueryResponder());
     }
-    ~AdvertiserMinMdns() override {}
+    ~AdvertiserMinMdns() override { RemoveServices(); }
 
     // Service advertiser
     CHIP_ERROR Init(chip::Inet::EndPointManager<chip::Inet::UDPEndPoint> * udpEndPointManager) override;
@@ -143,7 +199,8 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
     bool ShouldAdvertiseOn(const chip::Inet::InterfaceId id, const chip::Inet::IPAddress & addr);
 
     FullQName GetCommissioningTxtEntries(const CommissionAdvertisingParameters & params);
-    FullQName GetOperationalTxtEntries(const OperationalAdvertisingParameters & params);
+    FullQName GetOperationalTxtEntries(OperationalQueryAllocator::Allocator * allocator,
+                                       const OperationalAdvertisingParameters & params);
 
     struct CommonTxtEntryStorage
     {
@@ -205,17 +262,15 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
         return CHIP_NO_ERROR;
     }
 
-    // Max number of records for operational = PTR, SRV, TXT, A, AAAA, I subtype.
-    static constexpr size_t kMaxOperationalRecords  = 6;
-    static constexpr size_t kMaxOperationalNetworks = 5;
-    QueryResponderAllocator<kMaxOperationalRecords> mQueryResponderAllocatorOperational[kMaxOperationalNetworks];
+    IntrusiveList<OperationalQueryAllocator> mOperationalResponders;
+
     // Max number of records for commissionable = 7 x PTR (base + 6 sub types - _S, _L, _D, _T, _C, _A), SRV, TXT, A, AAAA
     static constexpr size_t kMaxCommissionRecords = 11;
     QueryResponderAllocator<kMaxCommissionRecords> mQueryResponderAllocatorCommissionable;
     QueryResponderAllocator<kMaxCommissionRecords> mQueryResponderAllocatorCommissioner;
 
-    QueryResponderAllocator<kMaxOperationalRecords> * FindOperationalAllocator(const FullQName & qname);
-    QueryResponderAllocator<kMaxOperationalRecords> * FindEmptyOperationalAllocator();
+    OperationalQueryAllocator::Allocator * FindOperationalAllocator(const FullQName & qname);
+    OperationalQueryAllocator::Allocator * FindEmptyOperationalAllocator();
 
     ResponseSender mResponseSender;
     uint8_t mCommissionableInstanceName[sizeof(uint64_t)];
@@ -289,38 +344,66 @@ void AdvertiserMinMdns::Shutdown()
 
 CHIP_ERROR AdvertiserMinMdns::RemoveServices()
 {
-    for (auto & allocator : mQueryResponderAllocatorOperational)
+    while (mOperationalResponders.begin() != mOperationalResponders.end())
     {
-        allocator.Clear();
+        auto it = mOperationalResponders.begin();
+
+        // Need to free the memory once it is out of the list
+        OperationalQueryAllocator * ptr = &*it;
+
+        // Mark as unused
+        ptr->GetAllocator()->Clear();
+
+        CHIP_ERROR err = mResponseSender.RemoveQueryResponder(ptr->GetAllocator()->GetQueryResponder());
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Discovery, "Failed to remove query responder: %" CHIP_ERROR_FORMAT, err.Format());
+        }
+
+        mOperationalResponders.Remove(ptr);
+
+        // Finally release the memory
+        chip::Platform::Delete(ptr);
     }
+
     mQueryResponderAllocatorCommissionable.Clear();
     mQueryResponderAllocatorCommissioner.Clear();
     return CHIP_NO_ERROR;
 }
 
-QueryResponderAllocator<AdvertiserMinMdns::kMaxOperationalRecords> *
-AdvertiserMinMdns::FindOperationalAllocator(const FullQName & qname)
+OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindOperationalAllocator(const FullQName & qname)
 {
-    for (auto & allocator : mQueryResponderAllocatorOperational)
+    for (auto & it : mOperationalResponders)
     {
-        if (allocator.GetResponder(QType::SRV, qname) != nullptr)
+        if (it.GetAllocator()->GetResponder(QType::SRV, qname) != nullptr)
         {
-            return &allocator;
+            return it.GetAllocator();
         }
     }
+
     return nullptr;
 }
 
-QueryResponderAllocator<AdvertiserMinMdns::kMaxOperationalRecords> * AdvertiserMinMdns::FindEmptyOperationalAllocator()
+OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindEmptyOperationalAllocator()
 {
-    for (auto & allocator : mQueryResponderAllocatorOperational)
+    OperationalQueryAllocator * result = OperationalQueryAllocator::New();
+
+    if (result == nullptr)
     {
-        if (allocator.IsEmpty())
-        {
-            return &allocator;
-        }
+        return nullptr;
     }
-    return nullptr;
+
+    CHIP_ERROR err = mResponseSender.AddQueryResponder(result->GetAllocator()->GetQueryResponder());
+
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(Discovery, "Failed to register query responder: %" CHIP_ERROR_FORMAT, err.Format());
+        Platform::Delete(result);
+        return nullptr;
+    }
+
+    mOperationalResponders.PushBack(result);
+    return result->GetAllocator();
 }
 
 CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters & params)
@@ -377,7 +460,8 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters &
         return CHIP_ERROR_NO_MEMORY;
     }
 
-    if (!operationalAllocator->AddResponder<TxtResponder>(TxtResourceRecord(instanceName, GetOperationalTxtEntries(params)))
+    if (!operationalAllocator
+             ->AddResponder<TxtResponder>(TxtResourceRecord(instanceName, GetOperationalTxtEntries(operationalAllocator, params)))
              .SetReportAdditional(hostName)
              .IsValid())
     {
@@ -616,11 +700,12 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters &
     return CHIP_NO_ERROR;
 }
 
-FullQName AdvertiserMinMdns::GetOperationalTxtEntries(const OperationalAdvertisingParameters & params)
+FullQName AdvertiserMinMdns::GetOperationalTxtEntries(OperationalQueryAllocator::Allocator * allocator,
+                                                      const OperationalAdvertisingParameters & params)
 {
     char * txtFields[OperationalAdvertisingParameters::kTxtMaxNumber];
     size_t numTxtFields = 0;
-    auto * allocator    = mQueryResponderAllocatorOperational;
+
     struct CommonTxtEntryStorage commonStorage;
     AddCommonTxtEntries<OperationalAdvertisingParameters>(params, commonStorage, txtFields, numTxtFields);
     if (numTxtFields == 0)
@@ -783,9 +868,9 @@ void AdvertiserMinMdns::AdvertiseRecords()
         QueryData queryData(QType::PTR, QClass::IN, false /* unicast */);
         queryData.SetIsBootAdvertising(true);
 
-        for (auto & allocator : mQueryResponderAllocatorOperational)
+        for (auto & it : mOperationalResponders)
         {
-            allocator.GetQueryResponder()->ClearBroadcastThrottle();
+            it.GetAllocator()->GetQueryResponder()->ClearBroadcastThrottle();
         }
         mQueryResponderAllocatorCommissionable.GetQueryResponder()->ClearBroadcastThrottle();
         mQueryResponderAllocatorCommissioner.GetQueryResponder()->ClearBroadcastThrottle();
@@ -798,9 +883,9 @@ void AdvertiserMinMdns::AdvertiseRecords()
     }
 
     // Once all automatic broadcasts are done, allow immediate replies once.
-    for (auto & allocator : mQueryResponderAllocatorOperational)
+    for (auto & it : mOperationalResponders)
     {
-        allocator.GetQueryResponder()->ClearBroadcastThrottle();
+        it.GetAllocator()->GetQueryResponder()->ClearBroadcastThrottle();
     }
     mQueryResponderAllocatorCommissionable.GetQueryResponder()->ClearBroadcastThrottle();
     mQueryResponderAllocatorCommissioner.GetQueryResponder()->ClearBroadcastThrottle();

src/lib/dnssd/minimal_mdns/ResponseSender.cpp

@@ -72,6 +72,19 @@ CHIP_ERROR ResponseSender::AddQueryResponder(QueryResponderBase * queryResponder
     return CHIP_ERROR_NO_MEMORY;
 }
 
+CHIP_ERROR ResponseSender::RemoveQueryResponder(QueryResponderBase * queryResponder)
+{
+    for (size_t i = 0; i < kMaxQueryResponders; ++i)
+    {
+        if (mResponder[i] == queryResponder)
+        {
+            mResponder[i] = nullptr;
+            return CHIP_NO_ERROR;
+        }
+    }
+    return CHIP_ERROR_NOT_FOUND;
+}
+
 CHIP_ERROR ResponseSender::Respond(uint32_t messageId, const QueryData & query, const chip::Inet::IPPacketInfo * querySource)
 {
     mSendState.Reset(messageId, query, querySource);

src/lib/dnssd/minimal_mdns/ResponseSender.h

@@ -93,6 +93,7 @@ class ResponseSender : public ResponderDelegate
     ResponseSender(ServerBase * server) : mServer(server) {}
 
     CHIP_ERROR AddQueryResponder(QueryResponderBase * queryResponder);
+    CHIP_ERROR RemoveQueryResponder(QueryResponderBase * queryResponder);
 
     /// Send back the response to a particular query
     CHIP_ERROR Respond(uint32_t messageId, const QueryData & query, const chip::Inet::IPPacketInfo * querySource);

src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp

@@ -328,7 +328,12 @@ void OperationalAdverts(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, server.GetSendCalled());
     NL_TEST_ASSERT(inSuite, server.GetHeaderFound());
 
-    // We should be able to add up to 5 operational networks total
+    // We should be able to add several operational networks
+    // As these are platform::new allocated, there is no upper limit
+    // TODO: Responder still has an upper limit of responders, which gets checked
+    //       here but should be removed as part of #8000 as we want to not have an
+    //       upper bound on number of operational networks (or at least not memory
+    //       enforced but rather policy enforced)
     NL_TEST_ASSERT(inSuite, mdnsAdvertiser.Advertise(operationalParams3) == CHIP_NO_ERROR);
     NL_TEST_ASSERT(inSuite, mdnsAdvertiser.Advertise(operationalParams4) == CHIP_NO_ERROR);
     NL_TEST_ASSERT(inSuite, mdnsAdvertiser.Advertise(operationalParams5) == CHIP_NO_ERROR);

src/lib/dnssd/minimal_mdns/tests/TestResponseSender.cpp

@@ -271,6 +271,16 @@ void AddManyQueryResponders(nlTestSuite * inSuite, void * inContext)
 
     // Last one should return a no memory error (no space)
     NL_TEST_ASSERT(inSuite, responseSender.AddQueryResponder(&q8) == CHIP_ERROR_NO_MEMORY);
+
+    // can make space
+    NL_TEST_ASSERT(inSuite, responseSender.RemoveQueryResponder(&q3) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, responseSender.AddQueryResponder(&q8) == CHIP_NO_ERROR);
+
+    // adding back does not fail
+    NL_TEST_ASSERT(inSuite, responseSender.AddQueryResponder(&q8) == CHIP_NO_ERROR);
+
+    // still full and cannot add more
+    NL_TEST_ASSERT(inSuite, responseSender.AddQueryResponder(&q3) == CHIP_ERROR_NO_MEMORY);
 }
 
 void PtrSrvTxtMultipleRespondersToInstance(nlTestSuite * inSuite, void * inContext)