fix use-after-free of a blackboard entry held by getAnyLocked

include/behaviortree_cpp/blackboard.h

@@ -76,7 +76,7 @@ class Blackboard
     return std::shared_ptr<Blackboard>(new Blackboard(parent));
   }
 
-  virtual ~Blackboard() = default;
+  virtual ~Blackboard();
 
   void enableAutoRemapping(bool remapping);
 
@@ -115,6 +115,11 @@ class Blackboard
   template <typename T>
   void set(const std::string& key, const T& value);
 
+  /**
+   * @brief Remove the entry with the given key, if any. This never blocks:
+   * if an AnyPtrLocked to that entry is still alive, the entry is destroyed
+   * only after the lock has been released.
+   */
   void unset(const std::string& key);
 
   [[nodiscard]] const TypeInfo* entryInfo(const std::string& key);
@@ -253,21 +258,6 @@ inline T Blackboard::get(const std::string& key) const
   throw RuntimeError("Blackboard::get() error. Missing key [", key, "]");
 }
 
-inline void Blackboard::unset(const std::string& key)
-{
-  std::unique_lock storage_lock(storage_mutex_);
-
-  // check local storage
-  auto it = storage_.find(key);
-  if(it == storage_.end())
-  {
-    // No entry, nothing to do.
-    return;
-  }
-
-  storage_.erase(it);
-}
-
 template <typename T>
 inline void Blackboard::set(const std::string& key, const T& value)
 {

include/behaviortree_cpp/utils/locked_reference.hpp

@@ -12,6 +12,10 @@ namespace BT
  *
  * As long as the object remains in scope, the mutex is locked, therefore
  * you must destroy this instance as soon as the pointer was used.
+ *
+ * LockedPtr does not own the object it points to: the owner is only expected
+ * to keep it alive while the mutex is locked. In particular, the pointer must
+ * not be considered valid after calling unlock().
  */
 template <typename T>
 class LockedPtr

src/blackboard.cpp

@@ -4,6 +4,7 @@
 
 #include <tuple>
 #include <unordered_set>
+#include <vector>
 
 namespace BT
 {
@@ -14,27 +15,104 @@ bool IsPrivateKey(StringView str)
 {
   return str.size() >= 1 && str.data()[0] == '_';
 }
+
+// Entries removed from a Blackboard while an AnyPtrLocked still held their
+// entry_mutex. An AnyPtrLocked does not own the entry, and the only signal that
+// the holder is done with it is the release of the mutex, so such entries are
+// parked here and destroyed by a later removal that finds them unlocked.
+struct DeferredEntries
+{
+  std::mutex mutex;
+  std::vector<std::shared_ptr<Blackboard::Entry>> entries;
+};
+
+DeferredEntries& GetDeferredEntries()
+{
+  // Never destroyed: a Blackboard with static storage duration may be
+  // destroyed after this object during program exit.
+  static auto* const instance = new DeferredEntries();
+  return *instance;
+}
+
+// Destroy the entries that were removed from a Blackboard, except those whose
+// entry_mutex is still locked by an outstanding AnyPtrLocked: these are parked
+// until a later call finds them unlocked. This never blocks, so removing an
+// entry while holding a lock on it (from any thread) cannot deadlock.
+void ReleaseEntries(std::vector<std::shared_ptr<Blackboard::Entry>> entries)
+{
+  std::vector<std::shared_ptr<Blackboard::Entry>> to_destroy;
+  auto& deferred = GetDeferredEntries();
+  {
+    const std::scoped_lock lock(deferred.mutex);
+    entries.insert(entries.end(), std::make_move_iterator(deferred.entries.begin()),
+                   std::make_move_iterator(deferred.entries.end()));
+    deferred.entries.clear();
+    for(auto& entry : entries)
+    {
+      // A removed entry can't be found by getAnyLocked() anymore, so once
+      // try_lock() succeeds nobody holds it.
+      if(entry->entry_mutex.try_lock())
+      {
+        entry->entry_mutex.unlock();
+        to_destroy.push_back(std::move(entry));
+      }
+      else
+      {
+        deferred.entries.push_back(std::move(entry));
+      }
+    }
+  }
+  // "to_destroy" is released here, outside the lock, because the destructor
+  // of a stored value may call back into a Blackboard.
+}
 }  // namespace
 
+Blackboard::~Blackboard()
+{
+  // An AnyPtrLocked may still refer to one of the entries: let ReleaseEntries()
+  // defer the destruction of that entry until the lock is released.
+  std::vector<std::shared_ptr<Entry>> entries;
+  entries.reserve(storage_.size());
+  for(auto& [key, entry] : storage_)
+  {
+    entries.push_back(std::move(entry));
+  }
+  storage_.clear();
+  ReleaseEntries(std::move(entries));
+}
+
 void Blackboard::enableAutoRemapping(bool remapping)
 {
   autoremapping_ = remapping;
 }
 
 AnyPtrLocked Blackboard::getAnyLocked(const std::string& key)
 {
-  if(auto entry = getEntry(key))
+  while(auto entry = getEntry(key))
   {
-    return AnyPtrLocked(&entry->value, &entry->entry_mutex);
+    AnyPtrLocked locked(&entry->value, &entry->entry_mutex);
+    // Re-check under entry_mutex: the removal paths erase the entry from the
+    // storage first and destroy it only once its mutex can be acquired (see
+    // ReleaseEntries). If the key still resolves to this entry, it will stay
+    // alive as long as the lock is held; otherwise release the lock and look
+    // the key up again.
+    if(getEntry(key) == entry)
+    {
+      return locked;
+    }
   }
   return {};
 }
 
 AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) const
 {
-  if(auto entry = getEntry(key))
+  while(auto entry = getEntry(key))
   {
-    return AnyPtrLocked(&entry->value, const_cast<std::mutex*>(&entry->entry_mutex));
+    AnyPtrLocked locked(&entry->value, const_cast<std::mutex*>(&entry->entry_mutex));
+    if(getEntry(key) == entry)
+    {
+      return locked;
+    }
   }
   return {};
 }
@@ -155,10 +233,38 @@ std::vector<StringView> Blackboard::getKeys() const
   return out;
 }
 
+void Blackboard::unset(const std::string& key)
+{
+  std::vector<std::shared_ptr<Entry>> removed;
+  {
+    const std::unique_lock storage_lock(storage_mutex_);
+
+    // check local storage
+    auto it = storage_.find(key);
+    if(it == storage_.end())
+    {
+      // No entry, nothing to do.
+      return;
+    }
+    removed.push_back(std::move(it->second));
+    storage_.erase(it);
+  }
+  ReleaseEntries(std::move(removed));
+}
+
 void Blackboard::clear()
 {
-  const std::unique_lock storage_lock(storage_mutex_);
-  storage_.clear();
+  std::vector<std::shared_ptr<Entry>> removed;
+  {
+    const std::unique_lock storage_lock(storage_mutex_);
+    removed.reserve(storage_.size());
+    for(auto& [key, entry] : storage_)
+    {
+      removed.push_back(std::move(entry));
+    }
+    storage_.clear();
+  }
+  ReleaseEntries(std::move(removed));
 }
 
 void Blackboard::createEntry(const std::string& key, const TypeInfo& info)
@@ -258,15 +364,24 @@ void Blackboard::cloneInto(Blackboard& dst) const
   // Step 3: insert new entries and remove stale ones under dst.storage_mutex_.
   if(!new_entries.empty() || !keys_to_remove.empty())
   {
-    const std::unique_lock dst_lock(dst.storage_mutex_);
-    for(auto& [key, entry] : new_entries)
-    {
-      dst.storage_.try_emplace(key, std::move(entry));
-    }
-    for(const auto& key : keys_to_remove)
+    std::vector<std::shared_ptr<Entry>> removed_entries;
     {
-      dst.storage_.erase(key);
+      const std::unique_lock dst_lock(dst.storage_mutex_);
+      for(auto& [key, entry] : new_entries)
+      {
+        dst.storage_.try_emplace(key, std::move(entry));
+      }
+      for(const auto& key : keys_to_remove)
+      {
+        auto it = dst.storage_.find(key);
+        if(it != dst.storage_.end())
+        {
+          removed_entries.push_back(std::move(it->second));
+          dst.storage_.erase(it);
+        }
+      }
     }
+    ReleaseEntries(std::move(removed_entries));
   }
 }
 

tests/gtest_blackboard.cpp

@@ -13,6 +13,10 @@
 #include "behaviortree_cpp/blackboard.h"
 #include "behaviortree_cpp/bt_factory.h"
 
+#include <atomic>
+#include <memory>
+#include <thread>
+
 #include <gtest/gtest.h>
 
 #include "../sample_nodes/dummy_nodes.h"
@@ -303,6 +307,120 @@ TEST(BlackboardTest, AnyPtrLocked)
 }
 #endif
 
+// An entry removed from the blackboard must stay alive as long as an
+// AnyPtrLocked refers to it, and removing it must never block, not even
+// from the thread holding the lock.
+
+TEST(BlackboardTest, AnyPtrLockedSurvivesUnset)
+{
+  auto blackboard = Blackboard::create();
+  blackboard->set("value", 42);
+
+  auto locked = blackboard->getAnyLocked("value");
+  ASSERT_TRUE(bool(locked));
+
+  blackboard->unset("value");
+  ASSERT_TRUE(blackboard->getKeys().empty());
+
+  // the entry must stay alive as long as we hold the lock
+  ASSERT_EQ(locked.get()->cast<int>(), 42);
+
+  locked = {};
+  ASSERT_FALSE(bool(blackboard->getAnyLocked("value")));
+}
+
+TEST(BlackboardTest, AnyPtrLockedSurvivesClear)
+{
+  auto blackboard = Blackboard::create();
+  blackboard->set("value", 42);
+
+  auto locked = blackboard->getAnyLocked("value");
+  ASSERT_TRUE(bool(locked));
+
+  blackboard->clear();
+  ASSERT_TRUE(blackboard->getKeys().empty());
+  ASSERT_EQ(locked.get()->cast<int>(), 42);
+}
+
+TEST(BlackboardTest, AnyPtrLockedSurvivesCloneInto)
+{
+  auto src = Blackboard::create();
+  auto dst = Blackboard::create();
+  dst->set("stale", 42);
+
+  auto locked = dst->getAnyLocked("stale");
+  ASSERT_TRUE(bool(locked));
+
+  // "stale" doesn't exist in src, so cloneInto() removes it from dst
+  src->cloneInto(*dst);
+  ASSERT_TRUE(dst->getKeys().empty());
+  ASSERT_EQ(locked.get()->cast<int>(), 42);
+}
+
+TEST(BlackboardTest, AnyPtrLockedSurvivesBlackboardDestruction)
+{
+  auto blackboard = Blackboard::create();
+  blackboard->set("value", 42);
+
+  auto locked = blackboard->getAnyLocked("value");
+  ASSERT_TRUE(bool(locked));
+
+  blackboard.reset();
+  ASSERT_EQ(locked.get()->cast<int>(), 42);
+}
+
+TEST(BlackboardTest, AnyPtrLockedCrossUnsetDoesNotDeadlock)
+{
+  auto blackboard = Blackboard::create();
+  blackboard->set("A", 1);
+  blackboard->set("B", 2);
+
+  // Each thread holds a lock on one entry and removes the other one, while
+  // the other thread does the opposite.
+  std::atomic<int> ready = 0;
+  int values[2] = { 0, 0 };
+  auto hold_and_unset = [&](const char* held, const char* removed, int& out) {
+    auto locked = blackboard->getAnyLocked(held);
+    ready++;
+    while(ready < 2)
+    {
+      std::this_thread::yield();
+    }
+    blackboard->unset(removed);
+    out = locked ? locked.get()->cast<int>() : 0;
+  };
+
+  std::thread t1(hold_and_unset, "A", "B", std::ref(values[0]));
+  std::thread t2(hold_and_unset, "B", "A", std::ref(values[1]));
+  t1.join();
+  t2.join();
+
+  ASSERT_EQ(values[0], 1);
+  ASSERT_EQ(values[1], 2);
+  ASSERT_TRUE(blackboard->getKeys().empty());
+}
+
+TEST(BlackboardTest, AnyPtrLockedDeferredEntryIsDestroyed)
+{
+  auto blackboard = Blackboard::create();
+  auto value = std::make_shared<int>(42);
+  std::weak_ptr<int> weak_value = value;
+  blackboard->set("value", value);
+  value.reset();
+
+  {
+    auto locked = blackboard->getAnyLocked("value");
+    ASSERT_TRUE(bool(locked));
+    blackboard->unset("value");
+    // still alive, since we hold the lock
+    ASSERT_FALSE(weak_value.expired());
+  }
+  // the lock was released: the entry is destroyed by the next removal
+  blackboard->set("other", 1);
+  blackboard->unset("other");
+  ASSERT_TRUE(weak_value.expired());
+}
+
 TEST(BlackboardTest, SetStringView)
 {
   auto bb = Blackboard::create();