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

Blackboard::getAnyLocked() builds the returned AnyPtrLocked from a local
shared_ptr<Entry>, so the caller holds a raw Any* and mutex* into an entry
that nothing keeps alive. unset() and clear() take storage_mutex_ but never
entry_mutex, so erasing the key destroys the entry while its mutex may still
be locked by the caller.

AnyPtrLocked is returned by value from exported functions, so it can't grow
an owner handle without breaking the ABI of existing binaries. Instead, the
removal paths (unset(), clear(), the stale-key removal in cloneInto() and
~Blackboard()) take the entry out of the storage and destroy it only if its
entry_mutex can be acquired with try_lock(); otherwise the entry is parked
in a process-wide list and destroyed by a later removal that finds it
unlocked. getAnyLocked() re-checks the key under the lock, so an entry can't
be dropped between lookup and lock. Nothing blocks, so removing an entry
while holding a lock on it, from the same or another thread, can't deadlock.

unset() and the destructor move out of the header, so that future changes
to the removal logic reach callers through the library.

The regression tests cover unset(), clear(), cloneInto() and destruction of
the blackboard while an AnyPtrLocked is alive, two threads each removing the
entry the other one holds, and the deferred destruction of a released entry.

Author: aysha-afrah26

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();