From 1227467ba9bb4bfd0f1dff268cf28c512906bd34 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 7 Jul 2023 17:37:11 -0400 Subject: [PATCH] Make MinMDNS only report commissiabe nodes for active browse. (#27670) * Make MinMDNS only report commissiabe nodes for active browse. MinMDNS had no way to differentiate what type of active commissionable browse operations existed so would report any commissioner or commissionable node the same to devices. This would make commissioner devices like chip-tool be able to detect themselves as 'commissionable' resulting in failed processing. * Restyled by clang-format * Fix unit tests --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io --- src/lib/dnssd/ActiveResolveAttempts.cpp | 34 ++++++++++++++++- src/lib/dnssd/ActiveResolveAttempts.h | 16 ++++---- src/lib/dnssd/IncrementalResolve.cpp | 24 +++++------- src/lib/dnssd/IncrementalResolve.h | 14 ++++++- src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 37 ++++++++++++++++--- .../dnssd/tests/TestActiveResolveAttempts.cpp | 4 +- 6 files changed, 97 insertions(+), 32 deletions(-) diff --git a/src/lib/dnssd/ActiveResolveAttempts.cpp b/src/lib/dnssd/ActiveResolveAttempts.cpp index 2065dca3440d53..fe0cc16ebb26aa 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.cpp +++ b/src/lib/dnssd/ActiveResolveAttempts.cpp @@ -55,11 +55,11 @@ void ActiveResolveAttempts::Complete(const PeerId & peerId) #endif } -void ActiveResolveAttempts::Complete(const chip::Dnssd::DiscoveredNodeData & data) +void ActiveResolveAttempts::CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data) { for (auto & item : mRetryQueue) { - if (item.attempt.Matches(data)) + if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionerNode)) { item.attempt.Clear(); return; @@ -67,6 +67,36 @@ void ActiveResolveAttempts::Complete(const chip::Dnssd::DiscoveredNodeData & dat } } +void ActiveResolveAttempts::CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data) +{ + for (auto & item : mRetryQueue) + { + if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionableNode)) + { + item.attempt.Clear(); + return; + } + } +} + +bool ActiveResolveAttempts::HasBrowseFor(chip::Dnssd::DiscoveryType type) const +{ + for (auto & item : mRetryQueue) + { + if (!item.attempt.IsBrowse()) + { + continue; + } + + if (item.attempt.BrowseData().type == type) + { + return true; + } + } + + return false; +} + void ActiveResolveAttempts::CompleteIpResolution(SerializedQNameIterator targetHostName) { for (auto & item : mRetryQueue) diff --git a/src/lib/dnssd/ActiveResolveAttempts.h b/src/lib/dnssd/ActiveResolveAttempts.h index 4a4789364d3dff..15796abf85b20c 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.h +++ b/src/lib/dnssd/ActiveResolveAttempts.h @@ -140,7 +140,7 @@ class ActiveResolveAttempts { return resolveData.Is() && (resolveData.Get().peerId == peer); } - bool Matches(const chip::Dnssd::DiscoveredNodeData & data) const + bool Matches(const chip::Dnssd::DiscoveredNodeData & data, chip::Dnssd::DiscoveryType type) const { if (!resolveData.Is()) { @@ -149,13 +149,11 @@ class ActiveResolveAttempts auto & browse = resolveData.Get(); - // TODO: we should mark returned node data based on the query - if (browse.type != chip::Dnssd::DiscoveryType::kCommissionableNode) + if (browse.type != type) { - // We don't currently have markers in the returned DiscoveredNodeData to differentiate these, so assume all returned - // packets match - return true; + return false; } + switch (browse.filter.type) { case chip::Dnssd::DiscoveryFilterType::kNone: @@ -251,7 +249,8 @@ class ActiveResolveAttempts /// Mark a resolution as a success, removing it from the internal list void Complete(const chip::PeerId & peerId); - void Complete(const chip::Dnssd::DiscoveredNodeData & data); + void CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data); + void CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data); void CompleteIpResolution(SerializedQNameIterator targetHostName); /// Mark all browse-type scheduled attemptes as a success, removing them @@ -289,6 +288,9 @@ class ActiveResolveAttempts /// IP resolution. bool IsWaitingForIpResolutionFor(SerializedQNameIterator hostName) const; + /// Check if a browse operation is active for the given discovery type + bool HasBrowseFor(chip::Dnssd::DiscoveryType type) const; + private: struct RetryEntry { diff --git a/src/lib/dnssd/IncrementalResolve.cpp b/src/lib/dnssd/IncrementalResolve.cpp index 69d475238b40ff..c36efbde177a6d 100644 --- a/src/lib/dnssd/IncrementalResolve.cpp +++ b/src/lib/dnssd/IncrementalResolve.cpp @@ -54,20 +54,12 @@ class TxtParser : public mdns::Minimal::TxtRecordDelegate DataType & mData; }; -enum class ServiceNameType -{ - kInvalid, // not a matter service name - kOperational, - kCommissioner, - kCommissionable, -}; - // Common prefix to check for all operational/commissioner/commissionable name parts constexpr QNamePart kOperationalSuffix[] = { kOperationalServiceName, kOperationalProtocol, kLocalDomain }; constexpr QNamePart kCommissionableSuffix[] = { kCommissionableServiceName, kCommissionProtocol, kLocalDomain }; constexpr QNamePart kCommissionerSuffix[] = { kCommissionerServiceName, kCommissionProtocol, kLocalDomain }; -ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) +IncrementalResolver::ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) { // SRV record names look like: // -._matter._tcp.local (operational) @@ -78,25 +70,25 @@ ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) if (!name.Next() || !name.IsValid()) { // missing required components - empty service name - return ServiceNameType::kInvalid; + return IncrementalResolver::ServiceNameType::kInvalid; } if (name == kOperationalSuffix) { - return ServiceNameType::kOperational; + return IncrementalResolver::ServiceNameType::kOperational; } if (name == kCommissionableSuffix) { - return ServiceNameType::kCommissionable; + return IncrementalResolver::ServiceNameType::kCommissionable; } if (name == kCommissionerSuffix) { - return ServiceNameType::kCommissioner; + return IncrementalResolver::ServiceNameType::kCommissioner; } - return ServiceNameType::kInvalid; + return IncrementalResolver::ServiceNameType::kInvalid; } /// Automatically resets a IncrementalResolver to inactive in its destructor @@ -171,7 +163,9 @@ CHIP_ERROR IncrementalResolver::InitializeParsing(mdns::Minimal::SerializedQName Platform::CopyString(mCommonResolutionData.hostName, serverName.Value()); } - switch (ComputeServiceNameType(name)) + mServiceNameType = ComputeServiceNameType(name); + + switch (mServiceNameType) { case ServiceNameType::kOperational: mSpecificResolutionData.Set(); diff --git a/src/lib/dnssd/IncrementalResolve.h b/src/lib/dnssd/IncrementalResolve.h index eff4f04fd28b72..b28b3cb897b5f3 100644 --- a/src/lib/dnssd/IncrementalResolve.h +++ b/src/lib/dnssd/IncrementalResolve.h @@ -84,7 +84,16 @@ class IncrementalResolver }; using RequiredInformationFlags = BitFlags; - IncrementalResolver() {} + // Type of service name that is contained in this resolver + enum class ServiceNameType + { + kInvalid, // not a matter service name + kOperational, + kCommissioner, + kCommissionable, + }; + + IncrementalResolver() = default; /// Checks if object has been initialized using the `InitializeParsing` /// method. @@ -93,6 +102,8 @@ class IncrementalResolver bool IsActiveCommissionParse() const { return mSpecificResolutionData.Is(); } bool IsActiveOperationalParse() const { return mSpecificResolutionData.Is(); } + ServiceNameType GetCurrentType() const { return mServiceNameType; } + /// Start parsing a new record. SRV records are the records we are mainly /// interested on, after which TXT and A/AAAA are looked for. /// @@ -171,6 +182,7 @@ class IncrementalResolver StoredServerName mRecordName; // Record name for what is parsed (SRV/PTR/TXT) StoredServerName mTargetHostName; // `Target` for the SRV record + ServiceNameType mServiceNameType = ServiceNameType::kInvalid; CommonResolutionData mCommonResolutionData; ParsedRecordSpecificData mSpecificResolutionData; }; diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 5c09413f7a7246..36ead8c9e5cb75 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -380,18 +380,45 @@ void MinMdnsResolver::AdvancePendingResolverStates() if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Failed to take discovery result: %" CHIP_ERROR_FORMAT, err.Format()); + continue; } - mActiveResolves.Complete(nodeData); - if (mCommissioningDelegate != nullptr) + // TODO: Ideally commissioning delegates should be aware of the + // node types they receive, however they are currently not + // so try to help out by only calling the delegate when an + // active browse exists. + // + // This is NOT ok and probably we should have separate comissioner + // or commissionable delegates or pass in a node type argument. + bool discoveredNodeIsRelevant = false; + + switch (resolver->GetCurrentType()) { - mCommissioningDelegate->OnNodeDiscovered(nodeData); + case IncrementalResolver::ServiceNameType::kCommissioner: + discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionerNode); + mActiveResolves.CompleteCommissioner(nodeData); + break; + case IncrementalResolver::ServiceNameType::kCommissionable: + discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionableNode); + mActiveResolves.CompleteCommissionable(nodeData); + break; + default: + ChipLogError(Discovery, "Unexpected type for commission data parsing"); + continue; } - else + + if (discoveredNodeIsRelevant) { + if (mCommissioningDelegate != nullptr) + { + mCommissioningDelegate->OnNodeDiscovered(nodeData); + } + else + { #if CHIP_MINMDNS_HIGH_VERBOSITY - ChipLogError(Discovery, "No delegate to report commissioning node discovery"); + ChipLogError(Discovery, "No delegate to report commissioning node discovery"); #endif + } } } else if (resolver->IsActiveOperationalParse()) diff --git a/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp b/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp index 097aa11fec1d05..574268bb3c8e55 100644 --- a/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp +++ b/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp @@ -127,7 +127,7 @@ void TestSingleBrowseAddRemove(nlTestSuite * inSuite, void * inContext) // once complete, nothing to schedule Dnssd::DiscoveredNodeData data; data.commissionData.longDiscriminator = 1234; - attempts.Complete(data); + attempts.CompleteCommissionable(data); NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue()); NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue()); } @@ -376,7 +376,7 @@ void TestCombination(nlTestSuite * inSuite, void * inContext) attempts.Complete(MakePeerId(1)); Dnssd::DiscoveredNodeData data; data.commissionData.longDiscriminator = 1234; - attempts.Complete(data); + attempts.CompleteCommissionable(data); NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue()); NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue());