From e1f6903af392fe8a1ab6720f29e19463d6979123 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 + src/controller/python/chip/ChipStack.py | 9 ++++--- .../python/chip/storage/__init__.py | 25 +++++++++++-------- 3 files changed, 22 insertions(+), 13 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/ChipStack.py b/src/controller/python/chip/ChipStack.py index c6b5ebac7fbaf1..77d36b39d511c0 100644 --- a/src/controller/python/chip/ChipStack.py +++ b/src/controller/python/chip/ChipStack.py @@ -328,11 +328,14 @@ def setLogFunct(self, logFunct): self._ChipStackLib.pychip_Stack_SetLogFunct(logFunct) def Shutdown(self): - # Make sure PersistentStorage is destructed before chipStack - # to avoid accessing builtins.chipStack after destruction. + self.Call(lambda: self._ChipStackLib.pychip_DeviceController_StackShutdown()) + + # + # We only shutdown the persistent storage layer AFTER we've shut down the stack, + # since there is a possibility of interactions with the storage layer during shutdown. + # self._persistentStorage.Shutdown() self._persistentStorage = None - self.Call(lambda: self._ChipStackLib.pychip_DeviceController_StackShutdown()) # # Stack init happens in native, but shutdown happens here unfortunately. 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()