Skip to content

Commit

Permalink
Un-revert #22932 + Fix Python Storage lifetime (#22963)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mrjerryjohns authored and pull[bot] committed Oct 27, 2023
1 parent c1fce51 commit ed456f9
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 13 deletions.
1 change: 1 addition & 0 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 6 additions & 3 deletions src/controller/python/chip/ChipStack.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
25 changes: 15 additions & 10 deletions src/controller/python/chip/storage/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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:
Expand All @@ -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()

0 comments on commit ed456f9

Please sign in to comment.