From 5575bf277f9380ce275ad04db52394feaa533d75 Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Thu, 29 Sep 2022 14:36:18 -0700 Subject: [PATCH] Un-revert #22932 + Fix Python Storage lifetime 22932 shuts down the fabric table as part of shutting down the stack. This entails interactions with the persistent storage layer to update some keys. In Python however, the storage layer has already been shutdown by the time we get to stack shutdown, causing a use-after-free. Flip the order in which we shutdown the storage adapter relative to shutting down the rest of the stack. --- .../CHIPDeviceControllerFactory.cpp | 1 + .../python/chip/storage/__init__.py | 25 +++++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 45ab785e508640..84a7c4388525bd 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -463,6 +463,7 @@ void DeviceControllerSystemState::Shutdown() if (mTempFabricTable != nullptr) { + mTempFabricTable->Shutdown(); chip::Platform::Delete(mTempFabricTable); mTempFabricTable = nullptr; // if we created a temp fabric table, then mFabrics points to it. diff --git a/src/controller/python/chip/storage/__init__.py b/src/controller/python/chip/storage/__init__.py index ad51c75754f9f4..926b7e12855849 100644 --- a/src/controller/python/chip/storage/__init__.py +++ b/src/controller/python/chip/storage/__init__.py @@ -90,6 +90,8 @@ class PersistentStorage: This interfaces with a C++ adapter that implements the PersistentStorageDelegate interface and can be passed into C++ logic that needs an instance of that interface. + + Object must be resident before the Matter stack starts up and last past its shutdown. ''' @classmethod def logger(cls): @@ -241,15 +243,21 @@ def DeleteSdkKey(self, key: str): def Shutdown(self): ''' Shuts down the object by free'ing up the associated adapter instance. - You cannot interact with this object there-after. + + This should only be called after the CHIP stack has shutdown (i.e + after calling pychip_DeviceController_StackShutdown()). ''' - self._handle.pychip_Storage_ShutdownAdapter.argtypes = [c_void_p] - builtins.chipStack.Call( - lambda: self._handle.pychip_Storage_ShutdownAdapter(self._closure) - ) + if (self._isActive): + self._handle.pychip_Storage_ShutdownAdapter.argtypes = [c_void_p] - self._isActive = False + # + # Since the stack is not running at this point, we can safely call + # C++ method directly on the current execution context without worrying + # about race conditions. + # + self._handle.pychip_Storage_ShutdownAdapter(self._closure) + self._isActive = False @property def jsonData(self) -> Dict: @@ -258,7 +266,4 @@ def jsonData(self) -> Dict: return copy.deepcopy(self._jsonData) def __del__(self): - if (self._isActive): - builtins.chipStack.Call( - lambda: self._handle.pychip_Storage_ShutdownAdapter() - ) + self.Shutdown()