From f4a1e92350f7647e6664922761a59df9220499de Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 25 Apr 2022 17:23:54 -0400 Subject: [PATCH] Align commissionable instance name behavior between minimal and platform advertising. (#17703) * Align commissionable instance name behavior between minimal and platform advertising. Before this change, platform dns-sd generated a commissionable instance name once, at boot. If you then commissioned the device and opened a new commissioning window it would keep advertising the instance name it advertised originally. On the other hand, minimal dns-sd generated a new instance name any time we updated anything about advertising. In particular, a failed commissioning attempt would lead to advertising with a different instance name, as part of the same commissioning window. After these changes we create a new instance name at init time, and whenever the commissioning window is opened (whether automatically or via the administrator commissioning commands). * Address review comment. --- src/app/server/CommissioningWindowManager.cpp | 2 ++ src/lib/dnssd/Advertiser.h | 9 +++++- src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 28 ++++++++++++++++--- src/lib/dnssd/Discovery_ImplPlatform.cpp | 13 +++++++-- src/lib/dnssd/Discovery_ImplPlatform.h | 3 +- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 8e9d6627f847b6..455337c5de8d9a 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -188,6 +188,8 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow(Seconds16 commiss DeviceLayer::FailSafeContext & failSafeContext = DeviceLayer::DeviceControlServer::DeviceControlSvr().GetFailSafeContext(); VerifyOrReturnError(!failSafeContext.IsFailSafeArmed(), CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure(Dnssd::ServiceAdvertiser::Instance().UpdateCommissionableInstanceName()); + ReturnErrorOnFailure(DeviceLayer::SystemLayer().StartTimer(commissioningTimeout, HandleCommissioningWindowTimeout, this)); mCommissioningTimeoutTimerArmed = true; diff --git a/src/lib/dnssd/Advertiser.h b/src/lib/dnssd/Advertiser.h index bf6900acdda5b2..d99d4864ec1302 100644 --- a/src/lib/dnssd/Advertiser.h +++ b/src/lib/dnssd/Advertiser.h @@ -333,7 +333,14 @@ class ServiceAdvertiser /** * Returns the commissionable node service instance name formatted as hex string. */ - virtual CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) = 0; + virtual CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) const = 0; + + /** + * Generates an updated commissionable instance name. This happens + * automatically when Init() is called, but may be needed at other times as + * well. + */ + virtual CHIP_ERROR UpdateCommissionableInstanceName() = 0; /// Provides the system-wide implementation of the service advertiser static ServiceAdvertiser & Instance(); diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index 3f9d5a433a08e6..7a27f8738ba8dc 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -178,7 +178,8 @@ class AdvertiserMinMdns : public ServiceAdvertiser, CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) override; CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) override; CHIP_ERROR FinalizeServiceUpdate() override { return CHIP_NO_ERROR; } - CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) override; + CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) const override; + CHIP_ERROR UpdateCommissionableInstanceName() override; // MdnsPacketDelegate void OnMdnsPacketData(const BytesRange & data, const chip::Inet::IPPacketInfo * info) override; @@ -275,6 +276,8 @@ class AdvertiserMinMdns : public ServiceAdvertiser, ResponseSender mResponseSender; uint8_t mCommissionableInstanceName[sizeof(uint64_t)]; + bool mIsInitialized = false; + // current request handling const chip::Inet::IPPacketInfo * mCurrentSource = nullptr; uint32_t mMessageId = 0; @@ -319,10 +322,16 @@ void AdvertiserMinMdns::OnQuery(const QueryData & data) CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::EndPointManager * udpEndPointManager) { + // TODO: Per API documentation, Init() should be a no-op if mIsInitialized + // is true. But we don't handle updates to our set of interfaces right now, + // so rely on the logic in this function to shut down and restart the + // GlobalMinimalMdnsServer to handle that. GlobalMinimalMdnsServer::Server().Shutdown(); - uint64_t random_instance_name = chip::Crypto::GetRandU64(); - memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName)); + if (!mIsInitialized) + { + UpdateCommissionableInstanceName(); + } // Re-set the server in the response sender in case this has been swapped in the // GlobalMinimalMdnsServer (used for testing). @@ -334,12 +343,15 @@ CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::EndPointManager