From 1730329d67254a7e286ff35226a02e00b4e0df3c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 18 Mar 2022 15:00:11 -0400 Subject: [PATCH] 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 c4d3f6c6bc0e001ceb1cca950632196f07e5497d. --- src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 151 ++++++++++++++---- src/lib/dnssd/minimal_mdns/ResponseSender.cpp | 13 ++ src/lib/dnssd/minimal_mdns/ResponseSender.h | 1 + .../minimal_mdns/tests/TestAdvertiser.cpp | 7 +- .../minimal_mdns/tests/TestResponseSender.cpp | 10 ++ 5 files changed, 148 insertions(+), 34 deletions(-) diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index 3ff275395cf5dd..3f9d5a433a08e6 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include // 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; + + /// 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(); + + if (allocator == nullptr) + { + return nullptr; + } + + OperationalQueryAllocator * result = chip::Platform::New(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 * 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 mQueryResponderAllocatorOperational[kMaxOperationalNetworks]; + IntrusiveList 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 mQueryResponderAllocatorCommissionable; QueryResponderAllocator mQueryResponderAllocatorCommissioner; - QueryResponderAllocator * FindOperationalAllocator(const FullQName & qname); - QueryResponderAllocator * 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::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::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(TxtResourceRecord(instanceName, GetOperationalTxtEntries(params))) + if (!operationalAllocator + ->AddResponder(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(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(); diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp b/src/lib/dnssd/minimal_mdns/ResponseSender.cpp index 0209ff8ca926c1..5d84e851ce3cd3 100644 --- a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp +++ b/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); diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.h b/src/lib/dnssd/minimal_mdns/ResponseSender.h index 1a40bfaa5f8138..8e5a2abb063e33 100644 --- a/src/lib/dnssd/minimal_mdns/ResponseSender.h +++ b/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); diff --git a/src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp b/src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp index 4483920e680821..8de65b7aba222c 100644 --- a/src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp +++ b/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); diff --git a/src/lib/dnssd/minimal_mdns/tests/TestResponseSender.cpp b/src/lib/dnssd/minimal_mdns/tests/TestResponseSender.cpp index b97b2e4b269853..4a42cd2dcd96ea 100644 --- a/src/lib/dnssd/minimal_mdns/tests/TestResponseSender.cpp +++ b/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)