fix use-after-free of a blackboard entry held by getAnyLocked (alternative to #1181)

include/behaviortree_cpp/blackboard.h

@@ -255,6 +255,8 @@ inline T Blackboard::get(const std::string& key) const
 
 inline void Blackboard::unset(const std::string& key)
 {
+  // the entry (i.e. the stored value) is destroyed outside the lock
+  std::shared_ptr<Entry> removed;
   std::unique_lock storage_lock(storage_mutex_);
 
   // check local storage
@@ -265,6 +267,7 @@ inline void Blackboard::unset(const std::string& key)
     return;
   }
 
+  removed = std::move(it->second);
   storage_.erase(it);
 }
 

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

@@ -2,8 +2,12 @@
 
 #include "behaviortree_cpp/json_export.h"
 
+#include <algorithm>
+#include <atomic>
+#include <mutex>
 #include <tuple>
 #include <unordered_set>
+#include <vector>
 
 namespace BT
 {
@@ -14,6 +18,94 @@
 {
   return str.size() >= 1 && str.data()[0] == '_';
 }
+
+// Blackboard::getAnyLocked() returns an AnyPtrLocked: raw pointers to the value
+// and to the (locked) entry_mutex of an Entry. It does not own the Entry and its
+// layout can not change (ABI), so the only sign that the holder is done is the
+// release of the mutex. Therefore an Entry whose last shared_ptr is dropped
+// (unset(), clear(), cloneInto(), ~Blackboard) while its mutex is still locked
+// is parked here, and destroyed later by the first RetireEntry() that finds it
+// unlocked.
+struct RetiredEntries
+{
+  std::mutex mutex;
+  std::vector<Blackboard::Entry*> entries;
+  std::atomic_size_t count = 0;
+};
+
+RetiredEntries& GetRetiredEntries()
+{
+  // Never destroyed: a Blackboard with static storage duration may be
+  // destroyed after this object during program exit.
+  // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
+  static auto* const instance = new RetiredEntries();
+  return *instance;
+}
+
+// No shared_ptr is left when this is called, so nobody can lock the entry
+// anymore: if try_lock() succeeds, the entry is unreachable.
+// Note: the calling thread may be the one holding the AnyPtrLocked. A
+// try_lock() on a std::mutex owned by the caller is formally undefined, but
+// it simply fails on all the supported platforms (pthread: EBUSY).
+bool IsUnlocked(Blackboard::Entry* entry)
+{
+  if(entry->entry_mutex.try_lock())
+  {
+    entry->entry_mutex.unlock();
+    return true;
+  }
+  return false;
+}
+
+// Destroy the parked entries that have been unlocked in the meantime.
+// If "retired" is not null, it is parked first.
+void SweepRetiredEntries(Blackboard::Entry* retired = nullptr) noexcept
+{
+  auto& parked = GetRetiredEntries();
+  std::vector<Blackboard::Entry*> unlocked;
+  try
+  {
+    const std::scoped_lock lock(parked.mutex);
+    if(retired != nullptr)
+    {
+      parked.entries.push_back(retired);
+    }
+    const auto it = std::partition(parked.entries.begin(), parked.entries.end(),
+                                   [](auto* entry) { return !IsUnlocked(entry); });
+    unlocked.assign(it, parked.entries.end());
+    parked.entries.erase(it, parked.entries.end());
+    parked.count = parked.entries.size();
+  }
+  catch(...)  // NOLINT(bugprone-empty-catch)
+  {
+    // Out of memory. Whatever could not be moved to "unlocked" remains parked
+    // (or leaks, in the case of "retired"), which is safe.
+  }
+  // Destroyed outside the lock: the destructor of a stored value may remove
+  // other entries, i.e. call this function again.
+  for(auto* entry : unlocked)
+  {
+    delete entry;  // NOLINT(cppcoreguidelines-owning-memory)
+  }
+}
+
+// Deleter of every shared_ptr<Entry>. Being attached at allocation time, it
+// covers all the removal paths, including those inlined in the user's binary.
+void RetireEntry(Blackboard::Entry* retired) noexcept
+{
+  // Common case: nothing is parked and nobody holds this entry
+  if(GetRetiredEntries().count == 0 && IsUnlocked(retired))
+  {
+    delete retired;  // NOLINT(cppcoreguidelines-owning-memory)
+    return;
+  }
+  SweepRetiredEntries(retired);
+}
+
+std::shared_ptr<Blackboard::Entry> MakeEntry(const TypeInfo& info)
+{
+  return { new Blackboard::Entry(info), &RetireEntry };
+}
 }  // namespace
 
 void Blackboard::enableAutoRemapping(bool remapping)
@@ -157,8 +249,10 @@
 
 void Blackboard::clear()
 {
+  // the entries (i.e. the stored values) are destroyed outside the lock
+  decltype(storage_) removed;
   const std::unique_lock storage_lock(storage_mutex_);
-  storage_.clear();
+  removed.swap(storage_);
 }
 
 void Blackboard::createEntry(const std::string& key, const TypeInfo& info)
@@ -248,7 +342,7 @@
     {
       // create new entry from src
       std::scoped_lock src_lock(task.src->entry_mutex);
-      auto new_entry = std::make_shared<Entry>(task.src->info);
+      auto new_entry = MakeEntry(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));
@@ -258,14 +352,20 @@
   // Step 3: insert new entries and remove stale ones under dst.storage_mutex_.
   if(!new_entries.empty() || !keys_to_remove.empty())
   {
+    // the stale entries are destroyed outside the lock
+    std::vector<std::shared_ptr<Entry>> removed;
     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)
     {
-      dst.storage_.erase(key);
+      if(auto it = dst.storage_.find(key); it != dst.storage_.end())
+      {
+        removed.push_back(std::move(it->second));
+        dst.storage_.erase(it);
+      }
     }
   }
 }
@@ -291,6 +391,12 @@
     return rootBlackboard()->createEntryImpl(key.substr(1, key.size() - 1), info);
   }
 
+  // Bound the lifetime of the entries that were removed while locked
+  if(GetRetiredEntries().count != 0)
+  {
+    SweepRetiredEntries();
+  }
+
   const std::unique_lock 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
@@ -336,7 +442,7 @@
   }
   // not remapped, not found. Create locally.
 
-  auto entry = std::make_shared<Entry>(info);
+  auto entry = MakeEntry(info);
   // even if empty, let's assign to it a default type
   entry->value = Any(info.type());
   storage_.insert({ key, entry });

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,162 @@ 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 when the next one is
+  // created (or removed)
+  blackboard->set("other", 1);
+  ASSERT_TRUE(weak_value.expired());
+}
+
+// Stress test, meaningful with ASan/TSan: readers hold an AnyPtrLocked while
+// another thread keeps removing and re-creating the same entry.
+TEST(BlackboardTest, AnyPtrLockedConcurrentUnsetStress)
+{
+  auto blackboard = Blackboard::create();
+  std::atomic_bool stop = false;
+  std::atomic_int reads = 0;
+
+  auto reader = [&]() {
+    while(!stop)
+    {
+      if(auto locked = blackboard->getAnyLocked("value"))
+      {
+        // set() inserts the entry before assigning the value: it may be empty.
+        // Otherwise, read it while the writer may unset the key.
+        if(!locked->empty())
+        {
+          ASSERT_EQ(locked->cast<int>(), 42);
+          reads++;
+        }
+      }
+    }
+  };
+  std::thread reader_a(reader);
+  std::thread reader_b(reader);
+
+  for(int i = 0; i < 5000; i++)
+  {
+    blackboard->set("value", 42);
+    blackboard->unset("value");
+  }
+  // make sure that the readers had the chance to run
+  blackboard->set("value", 42);
+  while(reads == 0)
+  {
+    std::this_thread::yield();
+  }
+  stop = true;
+  reader_a.join();
+  reader_b.join();
+}
+
 TEST(BlackboardTest, SetStringView)
 {
   auto bb = Blackboard::create();