From 00cf6cca046feb8b560a9444816dda6b6b37a4ba Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 21 Apr 2023 10:35:41 -0400 Subject: [PATCH] Assert the Matter lock is held in non-threadsafe SystemLayer methods. (#26180) ScheduleWork uses timers, and timers access a timer list member that is not protected against data races in any way. --- src/system/SystemLayer.h | 12 ++++++++---- src/system/SystemLayerImplFreeRTOS.cpp | 7 +++++++ src/system/SystemLayerImplSelect.cpp | 6 ++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/system/SystemLayer.h b/src/system/SystemLayer.h index 632037f45a328f..4321edeb02b883 100644 --- a/src/system/SystemLayer.h +++ b/src/system/SystemLayer.h @@ -96,7 +96,8 @@ class DLL_EXPORT Layer /** * @brief - * This method starts a one-shot timer. + * This method starts a one-shot timer. This method must be called while in the Matter context (from + * the Matter event loop, or while holding the Matter stack lock). * * @note * Only a single timer is allowed to be started with the same @a aComplete and @a aAppState @@ -114,8 +115,9 @@ class DLL_EXPORT Layer virtual CHIP_ERROR StartTimer(Clock::Timeout aDelay, TimerCompleteCallback aComplete, void * aAppState) = 0; /** - * @brief - * This method cancels a one-shot timer, started earlier through @p StartTimer(). + * @brief This method cancels a one-shot timer, started earlier through @p StartTimer(). This method must + * be called while in the Matter context (from the Matter event loop, or while holding the Matter + * stack lock). * * @note * The cancellation could fail silently if the timer specified by the combination of the callback @@ -129,7 +131,9 @@ class DLL_EXPORT Layer /** * @brief - * Schedules a function with a signature identical to `OnCompleteFunct` to be run as soon as possible in the CHIP context. + * Schedules a function with a signature identical to `OnCompleteFunct` to be run as soon as possible in the Matter context. + * This must only be called when already in the Matter context (from the Matter event loop, or while holding the Matter + * stack lock). * * @param[in] aComplete A pointer to a callback function to be called when this timer fires. * @param[in] aAppState A pointer to an application state object to be passed to the callback function as argument. diff --git a/src/system/SystemLayerImplFreeRTOS.cpp b/src/system/SystemLayerImplFreeRTOS.cpp index 1a9692302c9c48..ecc8d9504b0f14 100644 --- a/src/system/SystemLayerImplFreeRTOS.cpp +++ b/src/system/SystemLayerImplFreeRTOS.cpp @@ -22,6 +22,7 @@ */ #include +#include #include #include #include @@ -51,6 +52,8 @@ void LayerImplFreeRTOS::Shutdown() CHIP_ERROR LayerImplFreeRTOS::StartTimer(Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_TimeoutImmediate, delay = Clock::kZero); @@ -75,6 +78,8 @@ CHIP_ERROR LayerImplFreeRTOS::StartTimer(Clock::Timeout delay, TimerCompleteCall void LayerImplFreeRTOS::CancelTimer(TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturn(mLayerState.IsInitialized()); TimerList::Node * timer = mTimerList.Remove(onComplete, appState); @@ -86,6 +91,8 @@ void LayerImplFreeRTOS::CancelTimer(TimerCompleteCallback onComplete, void * app CHIP_ERROR LayerImplFreeRTOS::ScheduleWork(TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); // Ideally we would not use a timer here at all, but if we try to just diff --git a/src/system/SystemLayerImplSelect.cpp b/src/system/SystemLayerImplSelect.cpp index 88883a2a027520..171f14f8a6cfb7 100644 --- a/src/system/SystemLayerImplSelect.cpp +++ b/src/system/SystemLayerImplSelect.cpp @@ -122,6 +122,8 @@ void LayerImplSelect::Signal() CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_TimeoutImmediate, delay = System::Clock::kZero); @@ -167,6 +169,8 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturn(mLayerState.IsInitialized()); TimerList::Node * timer = mTimerList.Remove(onComplete, appState); @@ -194,6 +198,8 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void * appState) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); #if CHIP_SYSTEM_CONFIG_USE_DISPATCH