Add ability to assert that chip stack is locked by the current thread. (#6551)

* Initial implementation for tracking chip stack locks. marked as log only for darwin and linux targets

* Fix logging for lock error locations

* Ignore chip stack locking errors if main loop is not yet started

* Restyle fixes

* Remove unused type

* Remove the Platform log module, use DeviceLayer instead

* Add platform buildconfig dependency in inet, to allow for lock tracking management

* Add platform dependency in unit tests: need lock implementation

* Fix bool status lock/unlock ordering

Author: andy31415

src/include/platform/LockTracker.h

@@ -0,0 +1,52 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <platform/CHIPDeviceBuildConfig.h>
+
+/// Defines support for asserting that the chip stack is locked by the current thread via
+/// the macro:
+///
+///   assertChipStackLockedByCurrentThread()
+///
+/// Makes use of the following preprocessor macros:
+///
+///   CHIP_STACK_LOCK_TRACKING_ENABLED     - keeps track of who locks/unlocks the chip stack
+///   CHIP_STACK_LOCK_TRACKING_ERROR_FATAL - lock tracking errors will cause the chip stack to abort/die
+
+namespace chip {
+namespace Platform {
+
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+
+namespace Internal {
+
+void AssertChipStackLockedByCurrentThread(const char * file, int line);
+
+} // namespace Internal
+
+#define assertChipStackLockedByCurrentThread() ::chip::Platform::Internal::AssertChipStackLockedByCurrentThread(__FILE__, __LINE__)
+
+#else
+
+#define assertChipStackLockedByCurrentThread() (void) 0
+
+#endif
+
+} // namespace Platform
+} // namespace chip

src/include/platform/PlatformManager.h

@@ -23,6 +23,7 @@
 
 #pragma once
 
+#include <platform/CHIPDeviceBuildConfig.h>
 #include <platform/CHIPDeviceEvent.h>
 
 namespace chip {
@@ -94,6 +95,10 @@ class PlatformManager
     void UnlockChipStack();
     CHIP_ERROR Shutdown();
 
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    bool IsChipStackLockedByCurrentThread() const;
+#endif
+
 private:
     bool mInitialized = false;
     // ===== Members for internal use by the following friends.
@@ -180,6 +185,13 @@ extern PlatformManagerImpl & PlatformMgrImpl();
 namespace chip {
 namespace DeviceLayer {
 
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+inline bool PlatformManager::IsChipStackLockedByCurrentThread() const
+{
+    return static_cast<const ImplClass *>(this)->_IsChipStackLockedByCurrentThread();
+}
+#endif
+
 inline CHIP_ERROR PlatformManager::InitChipStack()
 {
     // NOTE: this is NOT thread safe and cannot be as the chip stack lock is prepared by

src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp

@@ -82,21 +82,46 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::_LockChipStack()
 {
     int err = pthread_mutex_lock(&mChipStackLock);
     assert(err == 0);
+
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    mChipStackIsLocked        = true;
+    mChipStackLockOwnerThread = pthread_self();
+#endif
 }
 
 template <class ImplClass>
 bool GenericPlatformManagerImpl_POSIX<ImplClass>::_TryLockChipStack()
 {
-    return pthread_mutex_trylock(&mChipStackLock) == 0;
+    bool locked = (pthread_mutex_trylock(&mChipStackLock) == 0);
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    if (locked)
+    {
+        mChipStackIsLocked        = true;
+        mChipStackLockOwnerThread = pthread_self();
+    }
+#endif
+    return locked;
 }
 
 template <class ImplClass>
 void GenericPlatformManagerImpl_POSIX<ImplClass>::_UnlockChipStack()
 {
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    mChipStackIsLocked = false;
+#endif
+
     int err = pthread_mutex_unlock(&mChipStackLock);
     assert(err == 0);
 }
 
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+template <class ImplClass>
+bool GenericPlatformManagerImpl_POSIX<ImplClass>::_IsChipStackLockedByCurrentThread() const
+{
+    return !mMainLoopStarted || (mChipStackIsLocked && (pthread_equal(pthread_self(), mChipStackLockOwnerThread)));
+}
+#endif
+
 template <class ImplClass>
 CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_StartChipTimer(int64_t aMilliseconds)
 {
@@ -210,7 +235,7 @@ 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;
 }

src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

@@ -67,6 +67,12 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl<ImplC
     pthread_attr_t mChipTaskAttr;
     struct sched_param mChipTaskSchedParam;
 
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    bool mMainLoopStarted   = false;
+    bool mChipStackIsLocked = false;
+    pthread_t mChipStackLockOwnerThread;
+#endif
+
     // ===== Methods that implement the PlatformManager abstract interface.
 
     CHIP_ERROR
@@ -80,6 +86,10 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl<ImplC
     CHIP_ERROR _StartChipTimer(int64_t durationMS);
     CHIP_ERROR _Shutdown();
 
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+    bool _IsChipStackLockedByCurrentThread() const;
+#endif
+
     // ===== Methods available to the implementation subclass.
 
 private:

src/inet/BUILD.gn

@@ -99,6 +99,7 @@ static_library("inet") {
   public_deps = [
     ":inet_config_header",
     "${chip_root}/src/lib/support",
+    "${chip_root}/src/platform:platform_buildconfig",
     "${chip_root}/src/system",
     "${nlio_root}:nlio",
   ]

src/inet/InetLayer.cpp

@@ -48,6 +48,8 @@
 
 #include "InetFaultInjection.h"
 
+#include <platform/LockTracker.h>
+
 #include <system/SystemTimer.h>
 
 #include <support/CodeUtils.h>
@@ -535,6 +537,8 @@ INET_ERROR InetLayer::GetLinkLocalAddr(InterfaceId link, IPAddress * llAddr)
  */
 INET_ERROR InetLayer::NewRawEndPoint(IPVersion ipVer, IPProtocol ipProto, RawEndPoint ** retEndPoint)
 {
+    assertChipStackLockedByCurrentThread();
+
     *retEndPoint = nullptr;
 
     VerifyOrReturnError(State == kState_Initialized, INET_ERROR_INCORRECT_STATE);
@@ -573,6 +577,8 @@ INET_ERROR InetLayer::NewRawEndPoint(IPVersion ipVer, IPProtocol ipProto, RawEnd
  */
 INET_ERROR InetLayer::NewTCPEndPoint(TCPEndPoint ** retEndPoint)
 {
+    assertChipStackLockedByCurrentThread();
+
     *retEndPoint = nullptr;
 
     VerifyOrReturnError(State == kState_Initialized, INET_ERROR_INCORRECT_STATE);
@@ -611,6 +617,8 @@ INET_ERROR InetLayer::NewTCPEndPoint(TCPEndPoint ** retEndPoint)
  */
 INET_ERROR InetLayer::NewUDPEndPoint(UDPEndPoint ** retEndPoint)
 {
+    assertChipStackLockedByCurrentThread();
+
     *retEndPoint = nullptr;
 
     VerifyOrReturnError(State == kState_Initialized, INET_ERROR_INCORRECT_STATE);
@@ -777,6 +785,8 @@ INET_ERROR InetLayer::ResolveHostAddress(const char * hostName, uint16_t hostNam
 INET_ERROR InetLayer::ResolveHostAddress(const char * hostName, uint16_t hostNameLen, uint8_t options, uint8_t maxAddrs,
                                          IPAddress * addrArray, DNSResolveCompleteFunct onComplete, void * appState)
 {
+    assertChipStackLockedByCurrentThread();
+
     INET_ERROR err         = INET_NO_ERROR;
     DNSResolver * resolver = nullptr;
 
@@ -867,6 +877,8 @@ INET_ERROR InetLayer::ResolveHostAddress(const char * hostName, uint16_t hostNam
  */
 void InetLayer::CancelResolveHostAddress(DNSResolveCompleteFunct onComplete, void * appState)
 {
+    assertChipStackLockedByCurrentThread();
+
     if (State != kState_Initialized)
         return;
 
@@ -1016,6 +1028,8 @@ void InetLayer::HandleTCPInactivityTimer(chip::System::Layer * aSystemLayer, voi
 chip::System::Error InetLayer::HandleInetLayerEvent(chip::System::Object & aTarget, chip::System::EventType aEventType,
                                                     uintptr_t aArgument)
 {
+    assertChipStackLockedByCurrentThread();
+
     VerifyOrReturnError(INET_IsInetEvent(aEventType), CHIP_SYSTEM_ERROR_UNEXPECTED_EVENT);
 
     // Dispatch the event according to its type.
@@ -1100,6 +1114,8 @@ chip::System::Error InetLayer::HandleInetLayerEvent(chip::System::Object & aTarg
  */
 void InetLayer::PrepareSelect(int & nfds, fd_set * readfds, fd_set * writefds, fd_set * exceptfds, struct timeval & sleepTimeTV)
 {
+    assertChipStackLockedByCurrentThread();
+
     if (State != kState_Initialized)
         return;
 
@@ -1160,6 +1176,8 @@ void InetLayer::PrepareSelect(int & nfds, fd_set * readfds, fd_set * writefds, f
  */
 void InetLayer::HandleSelectResult(int selectRes, fd_set * readfds, fd_set * writefds, fd_set * exceptfds)
 {
+    assertChipStackLockedByCurrentThread();
+
     if (State != kState_Initialized)
         return;
 

src/inet/tests/BUILD.gn

@@ -47,6 +47,7 @@ static_library("helpers") {
   public_deps = [
     "${chip_root}/src/inet",
     "${chip_root}/src/lib/core",
+    "${chip_root}/src/platform",
     "${nlunit_test_root}:nlunit-test",
   ]
 }

src/platform/BUILD.gn

@@ -55,6 +55,9 @@ if (chip_device_platform != "none") {
 
     # Enable adding rotating device id to the additional data.
     chip_enable_rotating_device_id = true
+
+    # lock tracking: none/log/fatal or auto for a platform-dependent choice
+    chip_stack_lock_tracking = "auto"
   }
 
   buildconfig_header("platform_buildconfig") {
@@ -75,6 +78,30 @@ if (chip_device_platform != "none") {
       defines += [ "CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE=${chip_enable_ble}" ]
     }
 
+    stack_log_tracking = chip_stack_lock_tracking
+    if (stack_log_tracking == "auto") {
+      if (chip_device_platform == "linux" || chip_device_platform == "darwin") {
+        # TODO: should be fatal for development. Change once bugs are fixed
+        stack_log_tracking = "log"
+      } else {
+        # TODO: may want to enable at least logging for embedded to find bugs
+        # this needs tuning depending on how many resources various platforms have
+        # available (mainly flash size)
+        stack_log_tracking = "none"
+      }
+    }
+
+    if (stack_log_tracking == "fatal") {
+      defines += [
+        "CHIP_STACK_LOCK_TRACKING_ENABLED=1",
+        "CHIP_STACK_LOCK_TRACKING_ERROR_FATAL=1",
+      ]
+    } else if (stack_log_tracking == "log") {
+      defines += [ "CHIP_STACK_LOCK_TRACKING_ENABLED=1" ]
+    } else {
+      assert(stack_log_tracking == "none")
+    }
+
     if (chip_enable_nfc) {
       defines += [ "CHIP_DEVICE_CONFIG_ENABLE_NFC=1" ]
     }
@@ -224,6 +251,7 @@ if (chip_device_platform != "none" && chip_device_platform != "external") {
       "DeviceControlServer.cpp",
       "GeneralUtils.cpp",
       "Globals.cpp",
+      "LockTracker.cpp",
       "PersistedStorage.cpp",
       "SystemEventSupport.cpp",
       "SystemTimerSupport.cpp",

src/platform/LockTracker.cpp

@@ -0,0 +1,45 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <platform/LockTracker.h>
+
+#if defined(CHIP_STACK_LOCK_TRACKING_ENABLED)
+
+#include <platform/CHIPDeviceLayer.h>
+#include <platform/PlatformManager.h>
+#include <support/CodeUtils.h>
+#include <support/logging/CHIPLogging.h>
+namespace chip {
+namespace Platform {
+namespace Internal {
+
+void AssertChipStackLockedByCurrentThread(const char * file, int line)
+{
+    if (!chip::DeviceLayer::PlatformMgr().IsChipStackLockedByCurrentThread())
+    {
+        ChipLogError(DeviceLayer, "Chip stack locking error at '%s:%d'. Code is unsafe/racy", file, line);
+#if defined(CHIP_STACK_LOCK_TRACKING_ERROR_FATAL)
+        chipDie();
+#endif
+    }
+}
+
+} // namespace Internal
+} // namespace Platform
+} // namespace chip
+
+#endif