From 73064ada5df91dcda636c9954eb5162345db8af5 Mon Sep 17 00:00:00 2001 From: Aysha Afrah Ziya Date: Wed, 29 Jul 2026 23:39:36 +0530 Subject: [PATCH] fix use-after-free of a blackboard entry held by getAnyLocked Blackboard::getAnyLocked() builds the returned AnyPtrLocked from a local shared_ptr, 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. --- include/behaviortree_cpp/blackboard.h | 22 +-- .../utils/locked_reference.hpp | 4 + src/blackboard.cpp | 141 ++++++++++++++++-- tests/gtest_blackboard.cpp | 118 +++++++++++++++ 4 files changed, 256 insertions(+), 29 deletions(-) diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index c88924400..aac7592a6 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -76,7 +76,7 @@ class Blackboard return std::shared_ptr(new Blackboard(parent)); } - virtual ~Blackboard() = default; + virtual ~Blackboard(); void enableAutoRemapping(bool remapping); @@ -115,6 +115,11 @@ class Blackboard template 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 inline void Blackboard::set(const std::string& key, const T& value) { diff --git a/include/behaviortree_cpp/utils/locked_reference.hpp b/include/behaviortree_cpp/utils/locked_reference.hpp index 674908dfe..2374b493b 100644 --- a/include/behaviortree_cpp/utils/locked_reference.hpp +++ b/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 class LockedPtr diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 7858b4655..6bf7889cd 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -4,6 +4,7 @@ #include #include +#include namespace BT { @@ -14,8 +15,72 @@ 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> 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> entries) +{ + std::vector> 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> 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; @@ -23,18 +88,31 @@ void Blackboard::enableAutoRemapping(bool 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(&entry->entry_mutex)); + AnyPtrLocked locked(&entry->value, const_cast(&entry->entry_mutex)); + if(getEntry(key) == entry) + { + return locked; + } } return {}; } @@ -155,10 +233,38 @@ std::vector Blackboard::getKeys() const return out; } +void Blackboard::unset(const std::string& key) +{ + std::vector> 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> 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> 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)); } } diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index f4ce45689..c645fe466 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -13,6 +13,10 @@ #include "behaviortree_cpp/blackboard.h" #include "behaviortree_cpp/bt_factory.h" +#include +#include +#include + #include #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(), 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(), 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(), 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(), 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 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() : 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(42); + std::weak_ptr 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();