Fix Blackboard thread-safety data races

Fix 6 data races in Blackboard, verified with ThreadSanitizer (clang-21):

- set() new-entry path: wrote value/sequence_id/stamp without
  entry_mutex after createEntryImpl — add scoped_lock on entry_mutex
- set() existing-entry path: held raw reference after unlocking
  storage_mutex_, risking use-after-free if concurrent unset() erases
  the entry — copy shared_ptr before unlocking
- cloneInto(): read/wrote entry members without entry_mutex —
  add scoped_lock on src and dst entry mutexes
- ImportBlackboardFromJSON(): wrote entry->value without any lock —
  add scoped_lock on entry_mutex
- debugMessage(): iterated storage_ without storage_mutex_ —
  add unique_lock
- getKeys(): iterated storage_ without storage_mutex_ —
  add unique_lock

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: facontidavide

include/behaviortree_cpp/blackboard.h

@@ -300,8 +300,10 @@ inline void Blackboard::set(const std::string& key, const T& value)
                         GetAnyFromStringFunctor<T>());
       entry = createEntryImpl(key, new_port);
     }
-    storage_lock.lock();
 
+    // Lock entry_mutex before writing to prevent data races with
+    // concurrent readers (BUG-1/BUG-8 fix).
+    std::scoped_lock entry_lock(entry->entry_mutex);
     entry->value = new_value;
     entry->sequence_id++;
     entry->stamp = std::chrono::steady_clock::now().time_since_epoch();
@@ -310,8 +312,11 @@ 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;
+    // Copy shared_ptr to prevent use-after-free if another thread
+    // calls unset() while we hold the reference (BUG-2 fix).
+    auto entry_ptr = it->second;
     storage_lock.unlock();
+    Entry& entry = *entry_ptr;
 
     std::scoped_lock scoped_lock(entry.entry_mutex);
 

src/blackboard.cpp

@@ -103,6 +103,9 @@ void Blackboard::addSubtreeRemapping(StringView internal, StringView external)
 
 void Blackboard::debugMessage() const
 {
+  // Lock storage_mutex_ to prevent iterator invalidation from
+  // concurrent modifications (BUG-5 fix).
+  const std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   for(const auto& [key, entry] : storage_)
   {
     auto port_type = entry->info.type();
@@ -136,6 +139,9 @@ void Blackboard::debugMessage() const
 
 std::vector<StringView> Blackboard::getKeys() const
 {
+  // Lock storage_mutex_ to prevent iterator invalidation and
+  // dangling StringView from concurrent modifications (BUG-6 fix).
+  const std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   if(storage_.empty())
   {
     return {};
@@ -178,49 +184,94 @@ void Blackboard::createEntry(const std::string& key, const TypeInfo& info)
 
 void Blackboard::cloneInto(Blackboard& dst) const
 {
-  // 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;
-  auto& dst_storage = dst.storage_;
-  for(const auto& [key, entry] : dst_storage)
+  // We must never hold storage_mutex_ while locking entry_mutex, because
+  // the script evaluation path (ExprAssignment::evaluate) does the reverse:
+  // entry_mutex → storage_mutex_ (via entryInfo → getEntry).
+  //
+  // Strategy: collect shared_ptrs under storage_mutex_, release it,
+  // then copy entry data under entry_mutex only.
+
+  struct CopyTask
   {
-    std::ignore = entry;  // unused in this loop
-    keys_to_remove.insert(key);
-  }
+    std::string key;
+    std::shared_ptr<Entry> src;
+    std::shared_ptr<Entry> dst;  // nullptr when a new entry is needed
+  };
 
-  // update or create entries in dst_storage
-  for(const auto& [src_key, src_entry] : storage_)
+  std::vector<CopyTask> tasks;
+  std::vector<std::string> keys_to_remove;
+
+  // Step 1: snapshot src/dst entries under both storage_mutex_ locks.
   {
-    keys_to_remove.erase(src_key);
+    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);
+
+    std::unordered_set<std::string> dst_keys;
+    for(const auto& [key, entry] : dst.storage_)
+    {
+      dst_keys.insert(key);
+    }
+
+    for(const auto& [src_key, src_entry] : storage_)
+    {
+      dst_keys.erase(src_key);
+      auto it = dst.storage_.find(src_key);
+      if(it != dst.storage_.end())
+      {
+        tasks.push_back({ src_key, src_entry, it->second });
+      }
+      else
+      {
+        tasks.push_back({ src_key, src_entry, nullptr });
+      }
+    }
+
+    for(const auto& key : dst_keys)
+    {
+      keys_to_remove.push_back(key);
+    }
+  }
+  // storage_mutex_ locks released here
 
-    auto it = dst_storage.find(src_key);
-    if(it != dst_storage.end())
+  // Step 2: copy entry data under entry_mutex only (BUG-3 fix).
+  std::vector<std::pair<std::string, std::shared_ptr<Entry>>> new_entries;
+
+  for(auto& task : tasks)
+  {
+    if(task.dst)
     {
-      // overwrite
-      auto& dst_entry = it->second;
-      dst_entry->string_converter = src_entry->string_converter;
-      dst_entry->value = src_entry->value;
-      dst_entry->info = src_entry->info;
-      dst_entry->sequence_id++;
-      dst_entry->stamp = std::chrono::steady_clock::now().time_since_epoch();
+      // overwrite existing entry
+      std::scoped_lock entry_locks(task.src->entry_mutex, task.dst->entry_mutex);
+      task.dst->string_converter = task.src->string_converter;
+      task.dst->value = task.src->value;
+      task.dst->info = task.src->info;
+      task.dst->sequence_id++;
+      task.dst->stamp = std::chrono::steady_clock::now().time_since_epoch();
     }
     else
     {
-      // create new
-      auto new_entry = std::make_shared<Entry>(src_entry->info);
-      new_entry->value = src_entry->value;
-      new_entry->string_converter = src_entry->string_converter;
-      dst_storage.insert({ src_key, new_entry });
+      // create new entry from src
+      std::scoped_lock src_lock(task.src->entry_mutex);
+      auto new_entry = std::make_shared<Entry>(task.src->info);
+      new_entry->value = task.src->value;
+      new_entry->string_converter = task.src->string_converter;
+      new_entries.emplace_back(task.key, std::move(new_entry));
     }
   }
 
-  for(const auto& key : keys_to_remove)
+  // Step 3: insert new entries and remove stale ones under dst.storage_mutex_.
+  if(!new_entries.empty() || !keys_to_remove.empty())
   {
-    dst_storage.erase(key);
+    const std::unique_lock<std::mutex> dst_lock(dst.storage_mutex_);
+    for(auto& [key, entry] : new_entries)
+    {
+      dst.storage_.insert({ key, std::move(entry) });
+    }
+    for(const auto& key : keys_to_remove)
+    {
+      dst.storage_.erase(key);
+    }
   }
 }
 
@@ -317,6 +368,8 @@ void ImportBlackboardFromJSON(const nlohmann::json& json, Blackboard& blackboard
         blackboard.createEntry(it.key(), res->second);
         entry = blackboard.getEntry(it.key());
       }
+      // Lock entry_mutex before writing to prevent data races (BUG-4 fix).
+      std::scoped_lock lk(entry->entry_mutex);
       entry->value = res->first;
     }
   }

tests/CMakeLists.txt

@@ -55,6 +55,7 @@ set(BT_TESTS
   gtest_simple_string.cpp
   gtest_polymorphic_ports.cpp
   gtest_plugin_issue953.cpp
+  gtest_blackboard_thread_safety.cpp
   gtest_xml_null_subtree_id.cpp
 
   script_parser_test.cpp