From a1de3ca75f32857221ce4b1d91a021b003fac83e Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Thu, 27 Oct 2022 16:50:02 +0200 Subject: [PATCH] Workaround for TSAN false positive reports with glib TSAN thinks that memory accessed before the call to g_source_attach() (which is internally used by e.g. g_main_context_invoke() and g_idle_add()) needs to be guarded by a mutex, otherwise that's a race condition between the thread that is creating shared data (the current thread) and the glib main event loop thread. In fact such race condition does not occur because g_source_attach() acts here as pthread_create() - code is not executed on the other thread before the g_source_attach() is called. --- src/platform/Linux/PlatformManagerImpl.cpp | 85 +++++++++++----------- src/platform/Linux/PlatformManagerImpl.h | 31 +++++++- 2 files changed, 71 insertions(+), 45 deletions(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index 70e576771c437f..9daa120976dfd4 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -33,8 +33,7 @@ #include #include -#include -#include +#include #include #include @@ -57,47 +56,12 @@ PlatformManagerImpl PlatformManagerImpl::sInstance; namespace { #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP - -class CallbackIndirection -{ -public: - CallbackIndirection(GSourceFunc callback, void * userData) : mCallback(callback), mUserData(userData) {} - - void Wait() - { - g_mutex_lock(&mDoneMtx); - while (!mDone) - g_cond_wait(&mDoneCond, &mDoneMtx); - g_mutex_unlock(&mDoneMtx); - } - - static gboolean Callback(CallbackIndirection * self) - { - auto result = self->mCallback(self->mUserData); - - g_mutex_lock(&self->mDoneMtx); - self->mDone = true; - g_mutex_unlock(&self->mDoneMtx); - - g_cond_signal(&self->mDoneCond); - return result; - } - -private: - GMutex mDoneMtx{}; - GCond mDoneCond{}; - bool mDone = false; - GSourceFunc mCallback; - void * mUserData; -}; - void * GLibMainLoopThread(void * loop) { g_main_loop_run(static_cast(loop)); return nullptr; } - -#endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP +#endif #if CHIP_DEVICE_CONFIG_ENABLE_WIFI @@ -229,9 +193,12 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() mGLibMainLoop = g_main_loop_new(nullptr, FALSE); mGLibMainLoopThread = g_thread_new("gmain-matter", GLibMainLoopThread, mGLibMainLoop); - CallbackIndirection startedInd([](void *) { return G_SOURCE_REMOVE; }, nullptr); - g_idle_add(G_SOURCE_FUNC(&CallbackIndirection::Callback), &startedInd); - startedInd.Wait(); + { + std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); + CallbackIndirection startedInd([](void *) { return G_SOURCE_REMOVE; }, nullptr); + g_idle_add(G_SOURCE_FUNC(&CallbackIndirection::Callback), &startedInd); + startedInd.Wait(lock); + } #endif @@ -286,7 +253,34 @@ void PlatformManagerImpl::_Shutdown() #endif } -#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE +#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP + +void PlatformManagerImpl::CallbackIndirection::Wait(std::unique_lock & lock) +{ + mDoneCond.wait(lock, [this]() { return mDone; }); +} + +gboolean PlatformManagerImpl::CallbackIndirection::Callback(CallbackIndirection * self) +{ + // We can not access "self" before acquiring the lock, because TSAN will complain that + // there is a race condition between the thread that created the object and the thread + // that is executing the callback. + std::unique_lock lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); + + auto callback = self->mCallback; + auto userData = self->mUserData; + + lock.unlock(); + auto result = callback(userData); + lock.lock(); + + self->mDone = true; + self->mDoneCond.notify_all(); + + return result; +} + +#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE CHIP_ERROR PlatformManagerImpl::RunOnGLibMainLoopThread(GSourceFunc callback, void * userData, bool wait) { @@ -296,16 +290,19 @@ CHIP_ERROR PlatformManagerImpl::RunOnGLibMainLoopThread(GSourceFunc callback, vo if (wait) { + std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); CallbackIndirection indirection(callback, userData); g_main_context_invoke(context, G_SOURCE_FUNC(&CallbackIndirection::Callback), &indirection); - indirection.Wait(); + indirection.Wait(lock); return CHIP_NO_ERROR; } g_main_context_invoke(context, callback, userData); return CHIP_NO_ERROR; } -#endif +#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE + +#endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Linux/PlatformManagerImpl.h b/src/platform/Linux/PlatformManagerImpl.h index 72925c7137ffc1..bc5bc7c10c4e4a 100644 --- a/src/platform/Linux/PlatformManagerImpl.h +++ b/src/platform/Linux/PlatformManagerImpl.h @@ -23,6 +23,9 @@ #pragma once +#include +#include + #include #include @@ -93,9 +96,35 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener static PlatformManagerImpl sInstance; #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP + + class CallbackIndirection + { + public: + CallbackIndirection(GSourceFunc callback, void * userData) : mCallback(callback), mUserData(userData) {} + void Wait(std::unique_lock & lock); + static gboolean Callback(CallbackIndirection * self); + + private: + GSourceFunc mCallback; + void * mUserData; + // Sync primitives to wait for the callback to be executed. + std::condition_variable mDoneCond; + bool mDone = false; + }; + + // XXX: Mutex for guarding access to glib main event loop callback indirection + // synchronization primitives. This is a workaround to suppress TSAN warnings. + // TSAN does not know that from the thread synchronization perspective the + // g_source_attach() function should be treated as pthread_create(). Memory + // access to shared data before the call to g_source_attach() without mutex + // is not a race condition - the callback will not be executed on glib main + // event loop thread before the call to g_source_attach(). + std::mutex mGLibMainLoopCallbackIndirectionMutex; + GMainLoop * mGLibMainLoop; GThread * mGLibMainLoopThread; -#endif + +#endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP }; /**