commit 1a907b374ba62736f980524fda43dbfc3a2cd3a1
author Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Wed Apr 10 00:48:14 2024 +1200
committer GitHub <noreply@github.com> Tue Apr 09 12:48:14 2024 +0000
tree 659e92849d4a6a2aa43f59f5f9241e9d65210021
parent f29ccbec98ea3c648c8dfd6f2a04f736972dec06

Avoid zombie DeviceControllerSystemState (#32903)

Make sure a dead system state can't be revived by raising the retain count
above 0 again.

Also ensure the retain count is read and updated in a single atomic operation.
Return a bool from Release so the caller can know if the release resulted in
state shutdown, and add an IsShutdown method.

src/controller/CHIPDeviceControllerFactory.cpp

diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp
index 84a00d4..e4e4199 100644
--- a/src/controller/CHIPDeviceControllerFactory.cpp
+++ b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -392,9 +392,9 @@
     (void) mSystemState->Retain();
 }
 
-void DeviceControllerFactory::ReleaseSystemState()
+bool DeviceControllerFactory::ReleaseSystemState()
 {
-    mSystemState->Release();
+    return mSystemState->Release();
 }
 
 DeviceControllerFactory::~DeviceControllerFactory()

src/controller/CHIPDeviceControllerFactory.h

diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h
index 2602731..19b19e5 100644
--- a/src/controller/CHIPDeviceControllerFactory.h
+++ b/src/controller/CHIPDeviceControllerFactory.h
@@ -205,7 +205,9 @@
     //
     // This should only be invoked if a matching call to RetainSystemState() was called prior.
     //
-    void ReleaseSystemState();
+    // Returns true if stack was shut down in response to this call, or false otherwise.
+    //
+    bool ReleaseSystemState();
 
     //
     // Retrieve a read-only pointer to the system state object that contains pointers to key stack

src/controller/CHIPDeviceControllerSystemState.h

diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h
index bc89d6c..707f1be 100644
--- a/src/controller/CHIPDeviceControllerSystemState.h
+++ b/src/controller/CHIPDeviceControllerSystemState.h
@@ -167,8 +167,9 @@
     // should be called to release the reference once it is no longer needed.
     DeviceControllerSystemState * Retain()
     {
-        VerifyOrDie(mRefCount < std::numeric_limits<uint32_t>::max());
-        ++mRefCount;
+        auto count = mRefCount++;
+        VerifyOrDie(count < std::numeric_limits<decltype(count)>::max()); // overflow
+        VerifyOrDie(!IsShutdown());                                       // avoid zombie
         return this;
     };
 
@@ -178,14 +179,15 @@
     //
     // NB: The system state is owned by the factory; Relase() will not free it
     // but will free its members (Shutdown()).
-    void Release()
+    //
+    // Returns true if the system state was shut down in response to this call.
+    bool Release()
     {
-        VerifyOrDie(mRefCount > 0);
-
-        if (--mRefCount == 0)
-        {
-            Shutdown();
-        }
+        auto count = mRefCount--;
+        VerifyOrDie(count > 0); // underflow
+        VerifyOrReturnValue(count == 1, false);
+        Shutdown();
+        return true;
     };
     bool IsInitialized()
     {
@@ -195,6 +197,7 @@
             mGroupDataProvider != nullptr && mReportScheduler != nullptr && mTimerDelegate != nullptr &&
             mSessionKeystore != nullptr && mSessionResumptionStorage != nullptr && mBDXTransferServer != nullptr;
     };
+    bool IsShutdown() { return mHaveShutDown; }
 
     System::Layer * SystemLayer() const { return mSystemLayer; };
     Inet::EndPointManager<Inet::TCPEndPoint> * TCPEndPointManager() const { return mTCPEndPointManager; };