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

Author: aysha-afrah26

include/behaviortree_cpp/utils/locked_reference.hpp

@@ -2,6 +2,7 @@
 
 #include "behaviortree_cpp/utils/safe_any.hpp"
 
+#include <memory>
 #include <mutex>
 
 namespace BT
@@ -19,7 +20,15 @@ class LockedPtr
 public:
   LockedPtr() = default;
 
-  LockedPtr(T* obj, std::mutex* obj_mutex) : ref_(obj), mutex_(obj_mutex)
+  /**
+   * @param obj        the object to be protected
+   * @param obj_mutex  the mutex protecting obj
+   * @param owner      optional shared ownership of whatever holds obj and obj_mutex.
+   *                   It is kept alive until this instance is destroyed, so that
+   *                   neither the object nor the mutex can be freed while locked.
+   */
+  LockedPtr(T* obj, std::mutex* obj_mutex, std::shared_ptr<const void> owner = {})
+    : ref_(obj), mutex_(obj_mutex), owner_(std::move(owner))
   {
     mutex_->lock();
   }
@@ -39,12 +48,14 @@ class LockedPtr
   {
     std::swap(ref_, other.ref_);
     std::swap(mutex_, other.mutex_);
+    std::swap(owner_, other.owner_);
   }
 
   LockedPtr& operator=(LockedPtr&& other) noexcept
   {
     std::swap(ref_, other.ref_);
     std::swap(mutex_, other.mutex_);
+    std::swap(owner_, other.owner_);
     return *this;
   }
 
@@ -108,6 +119,7 @@ class LockedPtr
 private:
   T* ref_ = nullptr;
   std::mutex* mutex_ = nullptr;
+  std::shared_ptr<const void> owner_ = nullptr;
 };
 
 }  // namespace BT

src/blackboard.cpp

@@ -25,7 +25,9 @@ AnyPtrLocked Blackboard::getAnyLocked(const std::string& key)
 {
   if(auto entry = getEntry(key))
   {
-    return AnyPtrLocked(&entry->value, &entry->entry_mutex);
+    // hand over the shared_ptr: unset()/clear() only take storage_mutex_, so the
+    // entry can be erased while entry_mutex is still locked here.
+    return AnyPtrLocked(&entry->value, &entry->entry_mutex, entry);
   }
   return {};
 }
@@ -34,7 +36,8 @@ AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) const
 {
   if(auto entry = getEntry(key))
   {
-    return AnyPtrLocked(&entry->value, const_cast<std::mutex*>(&entry->entry_mutex));
+    return AnyPtrLocked(&entry->value, const_cast<std::mutex*>(&entry->entry_mutex),
+                        entry);
   }
   return {};
 }

tests/gtest_blackboard.cpp

@@ -303,6 +303,21 @@ TEST(BlackboardTest, AnyPtrLocked)
 }
 #endif
 
+TEST(BlackboardTest, AnyPtrLockedSurvivesUnset)
+{
+  auto blackboard = Blackboard::create();
+  blackboard->set("value", 42);
+
+  auto locked = blackboard->getAnyLocked("value");
+  ASSERT_TRUE(bool(locked));
+
+  // unset() takes storage_mutex_ but not entry_mutex, so it can drop the last
+  // reference to the entry we are holding the lock on.
+  blackboard->unset("value");
+
+  ASSERT_EQ(locked.get()->cast<int>(), 42);
+}
+
 TEST(BlackboardTest, SetStringView)
 {
   auto bb = Blackboard::create();