From 5ff18182a55752ab6cca87ce3b4b92476ef087b7 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Thu, 4 May 2023 10:17:41 -0400
Subject: [PATCH] Make sure DnssdServer does not dereference a dangling fabric
 table. (#26354)

We could end up not calling StopServer if the last release of a system state was
a direct release on the state itself, not a call to ReleaseSystemState.

To fix this, move the DnssdServer::StopServer bits into the actual system state
shutdown, where we know whether we are destroying the fabric table that
DnssdServer uses and can clean up appropriately.
---
 src/app/server/Dnssd.cpp                         |  3 +++
 src/app/server/Dnssd.h                           |  8 +++++---
 src/controller/CHIPDeviceControllerFactory.cpp   | 15 +++++++++------
 src/controller/CHIPDeviceControllerSystemState.h |  8 +++++++-
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp
index 7c58541766aff0..b2bb1eca1075d9 100644
--- a/src/app/server/Dnssd.cpp
+++ b/src/app/server/Dnssd.cpp
@@ -345,6 +345,9 @@ void DnssdServer::StartServer()
 
 void DnssdServer::StopServer()
 {
+    // Make sure we don't hold on to a dangling fabric table pointer.
+    mFabricTable = nullptr;
+
     DeviceLayer::PlatformMgr().RemoveEventHandler(OnPlatformEventWrapper, 0);
 
 #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h
index 174b5dbd91d9b5..60f057ca9fd3a0 100644
--- a/src/app/server/Dnssd.h
+++ b/src/app/server/Dnssd.h
@@ -60,8 +60,9 @@ class DLL_EXPORT DnssdServer
     Inet::InterfaceId GetInterfaceId() { return mInterfaceId; }
 
     //
-    // Override the referenced fabric table from the default that is present
-    // in Server::GetInstance().GetFabricTable() to something else.
+    // Set the fabric table the DnssdServer should use for operational
+    // advertising.  This must be set before StartServer() is called for the
+    // first time.
     //
     void SetFabricTable(FabricTable * table)
     {
@@ -94,7 +95,8 @@ class DLL_EXPORT DnssdServer
     /// (Re-)starts the Dnssd server, using the provided commissioning mode.
     void StartServer(Dnssd::CommissioningMode mode);
 
-    //// Stop the Dnssd server.
+    //// Stop the Dnssd server.  After this call, SetFabricTable must be called
+    //// again before calling StartServer().
     void StopServer();
 
     CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);
diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp
index 19af89c60828d9..100801c7db9566 100644
--- a/src/controller/CHIPDeviceControllerFactory.cpp
+++ b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -272,7 +272,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
 
     // store the system state
     mSystemState = chip::Platform::New<DeviceControllerSystemState>(std::move(stateParams));
-    mSystemState->SetTempFabricTable(tempFabricTable);
+    mSystemState->SetTempFabricTable(tempFabricTable, params.enableServerInteractions);
     ChipLogDetail(Controller, "System State Initialized...");
     return CHIP_NO_ERROR;
 }
@@ -348,11 +348,6 @@ void DeviceControllerFactory::RetainSystemState()
 void DeviceControllerFactory::ReleaseSystemState()
 {
     mSystemState->Release();
-
-    if (!mSystemState->IsInitialized() && mEnableServerInteractions)
-    {
-        app::DnssdServer::Instance().StopServer();
-    }
 }
 
 DeviceControllerFactory::~DeviceControllerFactory()
@@ -384,6 +379,14 @@ void DeviceControllerSystemState::Shutdown()
 
     ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack");
 
+    if (mTempFabricTable && mEnableServerInteractions)
+    {
+        // The DnssdServer is holding a reference to our temp fabric table,
+        // which we are about to destroy.  Stop it, so that it will stop trying
+        // to use it.
+        app::DnssdServer::Instance().StopServer();
+    }
+
     if (mFabricTableDelegate != nullptr)
     {
         if (mFabrics != nullptr)
diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h
index 739b9f113754d9..e191411d0016fa 100644
--- a/src/controller/CHIPDeviceControllerSystemState.h
+++ b/src/controller/CHIPDeviceControllerSystemState.h
@@ -185,7 +185,11 @@ class DeviceControllerSystemState
     CASESessionManager * CASESessionMgr() const { return mCASESessionManager; }
     Credentials::GroupDataProvider * GetGroupDataProvider() const { return mGroupDataProvider; }
     Crypto::SessionKeystore * GetSessionKeystore() const { return mSessionKeystore; }
-    void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; }
+    void SetTempFabricTable(FabricTable * tempFabricTable, bool enableServerInteractions)
+    {
+        mTempFabricTable          = tempFabricTable;
+        mEnableServerInteractions = enableServerInteractions;
+    }
 
 private:
     DeviceControllerSystemState() {}
@@ -220,6 +224,8 @@ class DeviceControllerSystemState
 
     bool mHaveShutDown = false;
 
+    bool mEnableServerInteractions = false;
+
     void Shutdown();
 };