Align commissionable instance name behavior between minimal and platf…

…orm 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

@@ -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;

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();

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<chip::Inet::UDPEndPoint> * 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<chip::Inet::UDPEn
 
     AdvertiseRecords();
 
+    mIsInitialized = true;
+
     return CHIP_NO_ERROR;
 }
 
 void AdvertiserMinMdns::Shutdown()
 {
     GlobalMinimalMdnsServer::Server().Shutdown();
+    mIsInitialized = false;
 }
 
 CHIP_ERROR AdvertiserMinMdns::RemoveServices()
@@ -509,7 +521,7 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters &
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR AdvertiserMinMdns::GetCommissionableInstanceName(char * instanceName, size_t maxLength)
+CHIP_ERROR AdvertiserMinMdns::GetCommissionableInstanceName(char * instanceName, size_t maxLength) const
 {
     if (maxLength < (Commission::kInstanceNameMaxLength + 1))
     {
@@ -520,6 +532,14 @@ CHIP_ERROR AdvertiserMinMdns::GetCommissionableInstanceName(char * instanceName,
                                                      instanceName, maxLength);
 }
 
+CHIP_ERROR AdvertiserMinMdns::UpdateCommissionableInstanceName()
+{
+    uint64_t random_instance_name = chip::Crypto::GetRandU64();
+    static_assert(sizeof(mCommissionableInstanceName) == sizeof(random_instance_name), "Not copying the right amount of data");
+    memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName));
+    return CHIP_NO_ERROR;
+}
+
 CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters & params)
 {
     if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -344,8 +344,7 @@ CHIP_ERROR DiscoveryImplPlatform::InitImpl()
     ReturnErrorCodeIf(mDnssdInitialized, CHIP_NO_ERROR);
     ReturnErrorOnFailure(ChipDnssdInit(HandleDnssdInit, HandleDnssdError, this));
 
-    uint64_t random_instance_name = chip::Crypto::GetRandU64();
-    memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName));
+    UpdateCommissionableInstanceName();
 
     return CHIP_NO_ERROR;
 }
@@ -425,7 +424,7 @@ void DiscoveryImplPlatform::HandleDnssdError(void * context, CHIP_ERROR error)
     }
 }
 
-CHIP_ERROR DiscoveryImplPlatform::GetCommissionableInstanceName(char * instanceName, size_t maxLength)
+CHIP_ERROR DiscoveryImplPlatform::GetCommissionableInstanceName(char * instanceName, size_t maxLength) const
 {
     if (maxLength < (chip::Dnssd::Commission::kInstanceNameMaxLength + 1))
     {
@@ -436,6 +435,14 @@ CHIP_ERROR DiscoveryImplPlatform::GetCommissionableInstanceName(char * instanceN
                                                      instanceName, maxLength);
 }
 
+CHIP_ERROR DiscoveryImplPlatform::UpdateCommissionableInstanceName()
+{
+    uint64_t random_instance_name = chip::Crypto::GetRandU64();
+    static_assert(sizeof(mCommissionableInstanceName) == sizeof(random_instance_name), "Not copying the right amount of data");
+    memcpy(&mCommissionableInstanceName[0], &random_instance_name, sizeof(mCommissionableInstanceName));
+    return CHIP_NO_ERROR;
+}
+
 void DiscoveryImplPlatform::HandleDnssdPublish(void * context, const char * type, CHIP_ERROR error)
 {
     if (CHIP_NO_ERROR == error)

src/lib/dnssd/Discovery_ImplPlatform.h

@@ -45,7 +45,8 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
     CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) override;
     CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) override;
     CHIP_ERROR FinalizeServiceUpdate() override;
-    CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) override;
+    CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) const override;
+    CHIP_ERROR UpdateCommissionableInstanceName() override;
 
     // Members that implement Resolver interface.
     void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mResolverProxy.SetOperationalDelegate(delegate); }