fix thread safety issues

Author: facontidavide

cmake/sanitizers.cmake

@@ -15,16 +15,17 @@ if(BTCPP_ENABLE_ASAN)
     message(STATUS "Address Sanitizer enabled")
     add_compile_options(-fsanitize=address)
     add_link_options(-fsanitize=address)
-endif()
+    endif()
 
-if(BTCPP_ENABLE_UBSAN)
+    if(BTCPP_ENABLE_UBSAN)
     message(STATUS "Undefined Behavior Sanitizer enabled")
     add_compile_options(-fsanitize=undefined)
     add_link_options(-fsanitize=undefined)
-endif()
+    endif()
 
-if(BTCPP_ENABLE_TSAN)
+    if(BTCPP_ENABLE_TSAN)
     message(STATUS "Thread Sanitizer enabled")
     add_compile_options(-fsanitize=thread)
     add_link_options(-fsanitize=thread)
+    add_compile_definitions(USE_SANITIZE_THREAD)
 endif()

include/behaviortree_cpp/blackboard.h

@@ -141,7 +141,7 @@ class Blackboard
   const Blackboard* rootBlackboard() const;
 
 private:
-  mutable std::mutex mutex_;
+  mutable std::mutex storage_mutex_;
   mutable std::recursive_mutex entry_mutex_;
   std::unordered_map<std::string, std::shared_ptr<Entry>> storage_;
   std::weak_ptr<Blackboard> parent_bb_;
@@ -186,7 +186,7 @@ inline T Blackboard::get(const std::string& key) const
 
 inline void Blackboard::unset(const std::string& key)
 {
-  std::unique_lock lock(mutex_);
+  std::unique_lock storage_lock(storage_mutex_);
 
   // check local storage
   auto it = storage_.find(key);
@@ -207,15 +207,15 @@ inline void Blackboard::set(const std::string& key, const T& value)
     rootBlackboard()->set(key.substr(1, key.size() - 1), value);
     return;
   }
-  std::unique_lock lock(mutex_);
+  std::unique_lock storage_lock(storage_mutex_);
 
   // check local storage
   auto it = storage_.find(key);
   if(it == storage_.end())
   {
     // create a new entry
     Any new_value(value);
-    lock.unlock();
+    storage_lock.unlock();
     std::shared_ptr<Blackboard::Entry> entry;
     // if a new generic port is created with a string, it's type should be AnyTypeAllowed
     if constexpr(std::is_same_v<std::string, T>)
@@ -228,7 +228,7 @@ inline void Blackboard::set(const std::string& key, const T& value)
                         GetAnyFromStringFunctor<T>());
       entry = createEntryImpl(key, new_port);
     }
-    lock.lock();
+    storage_lock.lock();
 
     entry->value = new_value;
     entry->sequence_id++;
@@ -239,6 +239,8 @@ inline void Blackboard::set(const std::string& key, const T& value)
     // this is not the first time we set this entry, we need to check
     // if the type is the same or not.
     Entry& entry = *it->second;
+    storage_lock.unlock();
+
     std::scoped_lock scoped_lock(entry.entry_mutex);
 
     Any& previous_any = entry.value;

include/behaviortree_cpp/utils/timer_queue.h

@@ -23,10 +23,7 @@ class Semaphore
 
   void notify()
   {
-    {
-      std::lock_guard<std::mutex> lock(m_mtx);
-      m_count++;
-    }
+    m_count.fetch_add(1);
     m_cv.notify_one();
   }
 
@@ -38,8 +35,15 @@ class Semaphore
     {
       return false;
     }
-    m_count--;
+    // Only decrement if there is a real count. If we woke because of manualUnlock,
+    // m_count may be zero and we must not decrement it.
+    if(m_count > 0)
+    {
+      m_count.fetch_sub(1);
+    }
+    // Clear the manual unlock flag
     m_unlock = false;
+
     return true;
   }
 
@@ -52,7 +56,7 @@ class Semaphore
 private:
   std::mutex m_mtx;
   std::condition_variable m_cv;
-  unsigned m_count = 0;
+  std::atomic_uint m_count = 0;
   std::atomic_bool m_unlock = false;
 };
 }  // namespace details
@@ -74,15 +78,19 @@ class TimerQueue
 public:
   TimerQueue()
   {
-    m_th = std::thread([this] { run(); });
+    m_finish.store(false);
+    m_thread = std::thread([this]() { run(); });
   }
 
   ~TimerQueue()
   {
-    m_finish = true;
+    m_finish.store(true);
     cancelAll();
-    m_checkWork.manualUnlock();
-    m_th.join();
+
+    if(m_thread.joinable())
+    {
+      m_thread.join();
+    }
   }
 
   //! Adds a new timer
@@ -174,7 +182,7 @@ class TimerQueue
 
   void run()
   {
-    while(!m_finish)
+    while(!m_finish.load())
     {
       auto end = calcWaitTime();
       if(end.first)
@@ -239,8 +247,8 @@ class TimerQueue
   }
 
   details::Semaphore m_checkWork;
-  std::thread m_th;
-  bool m_finish = false;
+  std::thread m_thread;
+  std::atomic_bool m_finish = false;
   uint64_t m_idcounter = 0;
 
   struct WorkItem

src/blackboard.cpp

@@ -52,11 +52,13 @@ Blackboard::getEntry(const std::string& key) const
     return rootBlackboard()->getEntry(key.substr(1, key.size() - 1));
   }
 
-  std::unique_lock<std::mutex> lock(mutex_);
-  auto it = storage_.find(key);
-  if(it != storage_.end())
   {
-    return it->second;
+    std::unique_lock<std::mutex> storage_lock(storage_mutex_);
+    auto it = storage_.find(key);
+    if(it != storage_.end())
+    {
+      return it->second;
+    }
   }
   // not found. Try autoremapping
   if(auto parent = parent_bb_.lock())
@@ -130,7 +132,7 @@ std::vector<StringView> Blackboard::getKeys() const
 
 void Blackboard::clear()
 {
-  std::unique_lock<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   storage_.clear();
 }
 
@@ -157,8 +159,10 @@ void Blackboard::createEntry(const std::string& key, const TypeInfo& info)
 
 void Blackboard::cloneInto(Blackboard& dst) const
 {
-  std::unique_lock lk1(mutex_);
-  std::unique_lock lk2(dst.mutex_);
+  // Lock both mutexes without risking lock-order inversion.
+  std::unique_lock<std::mutex> lk1(storage_mutex_, std::defer_lock);
+  std::unique_lock<std::mutex> lk2(dst.storage_mutex_, std::defer_lock);
+  std::lock(lk1, lk2);
 
   // keys that are not updated must be removed.
   std::unordered_set<std::string> keys_to_remove;
@@ -212,7 +216,7 @@ Blackboard::Ptr Blackboard::parent()
 std::shared_ptr<Blackboard::Entry> Blackboard::createEntryImpl(const std::string& key,
                                                                const TypeInfo& info)
 {
-  std::unique_lock<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   // This function might be called recursively, when we do remapping, because we move
   // to the top scope to find already existing  entries
 

tests/gtest_blackboard.cpp

@@ -294,6 +294,8 @@ TEST(BlackboardTest, CheckTypeSafety)
   ASSERT_TRUE(is);
 }
 
+#ifndef USE_SANITIZE_THREAD
+
 TEST(BlackboardTest, AnyPtrLocked)
 {
   auto blackboard = Blackboard::create();
@@ -346,6 +348,7 @@ TEST(BlackboardTest, AnyPtrLocked)
     ASSERT_NE(cycles, value);
   }
 }
+#endif
 
 TEST(BlackboardTest, SetStringView)
 {