From d1d5ecf6950a6c6c5cb27ca910dd611e9af98e86 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 15:00:29 +0200 Subject: [PATCH] add Blackboard::getKeyNames() and deprecate getKeys() getKeys() returns StringViews into the keys of the storage map, valid only while the storage lock is held, i.e. never for the caller. ExportBlackboardToJSON built a std::string from them while another thread could unset() the entry: heap-use-after-free (Groot2 thread vs UnsetBlackboard on the tick thread). getKeyNames() returns copies made under the lock. getKeys() is unchanged but deprecated, so the change is purely additive (no ABI/API break). Co-Authored-By: Claude Fable 5.1 --- include/behaviortree_cpp/blackboard.h | 9 ++++++- src/blackboard.cpp | 15 +++++++++-- tests/gtest_blackboard.cpp | 12 ++++----- tests/gtest_blackboard_thread_safety.cpp | 32 ++++++++++++++++++++++-- tests/script_parser_test.cpp | 10 ++++---- 5 files changed, 62 insertions(+), 16 deletions(-) diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index 651c05a5d..890eeb217 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -123,7 +123,14 @@ class Blackboard void debugMessage() const; - [[nodiscard]] std::vector getKeys() const; + /// The names of all the entries stored in this blackboard (copies). + [[nodiscard]] std::vector getKeyNames() const; + + // The views point into the storage: they dangle as soon as another thread + // removes the entry. + [[deprecated( + "The views may dangle: use getKeyNames")]] [[nodiscard]] std::vector + getKeys() const; [[deprecated("This command is unsafe. Consider using Backup/Restore instead")]] void clear(); diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 412e263f7..5f313a6f0 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -229,6 +229,18 @@ void Blackboard::debugMessage() const } } +std::vector Blackboard::getKeyNames() const +{ + const std::shared_lock storage_lock(storage_mutex_); + std::vector out; + out.reserve(storage_.size()); + for(const auto& entry_it : storage_) + { + out.push_back(entry_it.first); + } + return out; +} + std::vector Blackboard::getKeys() const { // Lock storage_mutex_ (shared) to prevent iterator invalidation and @@ -452,9 +464,8 @@ std::shared_ptr Blackboard::createEntryImpl(const std::string nlohmann::json ExportBlackboardToJSON(const Blackboard& blackboard) { nlohmann::json dest; - for(auto entry_name : blackboard.getKeys()) + for(const auto& name : blackboard.getKeyNames()) { - const std::string name(entry_name); if(auto any_ref = blackboard.getAnyLocked(name)) { if(auto any_ptr = any_ref.get()) diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index fd2c2769e..693b63c9e 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -320,7 +320,7 @@ TEST(BlackboardTest, AnyPtrLockedSurvivesUnset) ASSERT_TRUE(bool(locked)); blackboard->unset("value"); - ASSERT_TRUE(blackboard->getKeys().empty()); + ASSERT_TRUE(blackboard->getKeyNames().empty()); // the entry must stay alive as long as we hold the lock ASSERT_EQ(locked.get()->cast(), 42); @@ -338,7 +338,7 @@ TEST(BlackboardTest, AnyPtrLockedSurvivesClear) ASSERT_TRUE(bool(locked)); blackboard->clear(); - ASSERT_TRUE(blackboard->getKeys().empty()); + ASSERT_TRUE(blackboard->getKeyNames().empty()); ASSERT_EQ(locked.get()->cast(), 42); } @@ -353,7 +353,7 @@ TEST(BlackboardTest, AnyPtrLockedSurvivesCloneInto) // "stale" doesn't exist in src, so cloneInto() removes it from dst src->cloneInto(*dst); - ASSERT_TRUE(dst->getKeys().empty()); + ASSERT_TRUE(dst->getKeyNames().empty()); ASSERT_EQ(locked.get()->cast(), 42); } @@ -397,7 +397,7 @@ TEST(BlackboardTest, AnyPtrLockedCrossUnsetDoesNotDeadlock) ASSERT_EQ(values[0], 1); ASSERT_EQ(values[1], 2); - ASSERT_TRUE(blackboard->getKeys().empty()); + ASSERT_TRUE(blackboard->getKeyNames().empty()); } TEST(BlackboardTest, AnyPtrLockedDeferredEntryIsDestroyed) @@ -674,7 +674,7 @@ TEST(BlackboardTest, BlackboardBackup) for(const auto& sub : tree.subtrees) { std::vector keys; - for(const auto& str_view : sub->blackboard->getKeys()) + for(const auto& str_view : sub->blackboard->getKeyNames()) { keys.push_back(std::string(str_view)); } @@ -691,7 +691,7 @@ TEST(BlackboardTest, BlackboardBackup) for(size_t i = 0; i < tree.subtrees.size(); i++) { - const auto keys = tree.subtrees[i]->blackboard->getKeys(); + const auto keys = tree.subtrees[i]->blackboard->getKeyNames(); ASSERT_EQ(expected_keys[i].size(), keys.size()); for(size_t a = 0; a < keys.size(); a++) { diff --git a/tests/gtest_blackboard_thread_safety.cpp b/tests/gtest_blackboard_thread_safety.cpp index 483720c8a..e23cc8966 100644 --- a/tests/gtest_blackboard_thread_safety.cpp +++ b/tests/gtest_blackboard_thread_safety.cpp @@ -301,7 +301,7 @@ TEST(BlackboardThreadSafety, DebugMessageWhileModifying_Bug5) SUCCEED(); } -// BUG-6: getKeys() iterates storage_ without holding storage_mutex_. +// BUG-6: getKeyNames() iterates storage_ without holding storage_mutex_. // Also returns StringView into map keys which can dangle if entries are erased. TEST(BlackboardThreadSafety, GetKeysWhileModifying_Bug6) { @@ -320,7 +320,7 @@ TEST(BlackboardThreadSafety, GetKeysWhileModifying_Bug6) auto key_reader = [&]() { for(int i = 0; i < kIterations; i++) { - auto keys = bb->getKeys(); + auto keys = bb->getKeyNames(); // Just access the keys to detect any race volatile size_t count = keys.size(); (void)count; @@ -335,3 +335,31 @@ TEST(BlackboardThreadSafety, GetKeysWhileModifying_Bug6) SUCCEED(); } + +// ExportBlackboardToJSON used the StringViews returned by getKeyNames() after the +// storage lock was released: a concurrent unset() frees the key they point to. +// Meaningful with ASan/TSan. The keys are longer than the small string buffer, +// so that they live on the heap. +TEST(BlackboardThreadSafety, ExportToJsonWhileKeysAreRemoved) +{ + auto bb = Blackboard::create(); + std::atomic_bool stop = false; + + std::thread exporter([&]() { + while(!stop) + { + const auto json = ExportBlackboardToJSON(*bb); + ASSERT_LE(json.size(), 8U); + } + }); + + const std::string prefix = "a_key_that_is_too_long_for_the_small_string_buffer_"; + for(int i = 0; i < 20000; i++) + { + const std::string key = prefix + std::to_string(i % 8); + bb->set(key, i); + bb->unset(key); + } + stop = true; + exporter.join(); +} diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index b9af10071..775b506ec 100644 --- a/tests/script_parser_test.cpp +++ b/tests/script_parser_test.cpp @@ -130,7 +130,7 @@ TEST(ParserTest, Equations) //------------------- const auto& variables = environment.vars; EXPECT_EQ(GetResult("x:= 3; y:=5; x+y").cast(), 8.0); - EXPECT_EQ(variables->getKeys().size(), 2); + EXPECT_EQ(variables->getKeyNames().size(), 2); EXPECT_EQ(variables->get("x"), 3.0); EXPECT_EQ(variables->get("y"), 5.0); @@ -170,7 +170,7 @@ TEST(ParserTest, Equations) "o " "worl" "d"); - EXPECT_EQ(variables->getKeys().size(), 5); + EXPECT_EQ(variables->getKeyNames().size(), 5); EXPECT_EQ(variables->get("A"), "hello"); EXPECT_EQ(variables->get("B"), " "); EXPECT_EQ(variables->get("C"), "world"); @@ -181,7 +181,7 @@ TEST(ParserTest, Equations) "C= 'left ' ") .empty()); - EXPECT_EQ(variables->getKeys().size(), 5); + EXPECT_EQ(variables->getKeyNames().size(), 5); EXPECT_EQ(variables->get("A"), " right"); EXPECT_EQ(variables->get("B"), " center "); EXPECT_EQ(variables->get("C"), "left "); @@ -196,9 +196,9 @@ TEST(ParserTest, Equations) EXPECT_ANY_THROW(GetResult(" 'hello' = 2.0 ")); EXPECT_ANY_THROW(GetResult(" 3.0 = 2.0 ")); - size_t prev_size = variables->getKeys().size(); + size_t prev_size = variables->getKeyNames().size(); EXPECT_ANY_THROW(GetResult("new_var=69")); - EXPECT_EQ(variables->getKeys().size(), prev_size); // shouldn't increase + EXPECT_EQ(variables->getKeyNames().size(), prev_size); // shouldn't increase // check comparisons EXPECT_EQ(GetResult("x < y").cast(), 1);