Assert that holders are added to sessions only with the Matter lock h…

…eld. Adding/removing holders manipulates circular lists, and if we end up with data races on these manipulations we can end up in bad states. Since sessions are somewhat singleton resources, and the "hold on to a session" operation is pretty hidden in many cases, it's easy to end up with a situation where a session is being pointed to by objects being manipulated on multiple threads and hard to catch this via manual code inspection.
project-chip · Jul 10, 2022 · 692ba08 · 692ba08
1 parent 776c06d
commit 692ba08
Show file tree

Hide file tree

Showing 3 changed files with 4 additions and 3 deletions.
diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h
@@ -77,7 +77,6 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl<ImplC
     struct sched_param mChipTaskSchedParam;
 
 #if CHIP_STACK_LOCK_TRACKING_ENABLED
-    bool mMainLoopStarted   = false;
     bool mChipStackIsLocked = false;
     pthread_t mChipStackLockOwnerThread;
 #endif

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp
@@ -117,7 +117,7 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::_UnlockChipStack()
 template <class ImplClass>
 bool GenericPlatformManagerImpl_POSIX<ImplClass>::_IsChipStackLockedByCurrentThread() const
 {
-    return !mMainLoopStarted || (mChipStackIsLocked && (pthread_equal(pthread_self(), mChipStackLockOwnerThread)));
+    return !mHasValidChipTask || (mChipStackIsLocked && (pthread_equal(pthread_self(), mChipStackLockOwnerThread)));
 }
 #endif
 
@@ -201,7 +201,6 @@ template <class ImplClass>
 void * GenericPlatformManagerImpl_POSIX<ImplClass>::EventLoopTaskMain(void * arg)
 {
     ChipLogDetail(DeviceLayer, "CHIP task running");
-    static_cast<GenericPlatformManagerImpl_POSIX<ImplClass> *>(arg)->Impl()->mMainLoopStarted = true;
     static_cast<GenericPlatformManagerImpl_POSIX<ImplClass> *>(arg)->Impl()->RunEventLoop();
     return nullptr;
 }

diff --git a/src/transport/Session.h b/src/transport/Session.h
@@ -21,6 +21,7 @@
 #include <lib/core/PeerId.h>
 #include <lib/core/ScopedNodeId.h>
 #include <messaging/ReliableMessageProtocolConfig.h>
+#include <platform/LockTracker.h>
 #include <transport/SessionHolder.h>
 #include <transport/raw/PeerAddress.h>
 
@@ -55,12 +56,14 @@ class Session
 
     void AddHolder(SessionHolder & holder)
     {
+        assertChipStackLockedByCurrentThread();
         VerifyOrDie(!holder.IsInList());
         mHolders.PushBack(&holder);
     }
 
     void RemoveHolder(SessionHolder & holder)
     {
+        assertChipStackLockedByCurrentThread();
         VerifyOrDie(mHolders.Contains(&holder));
         mHolders.Remove(&holder);
     }