From b081a27f800d394c816560896b5941bcca414739 Mon Sep 17 00:00:00 2001
From: Aysha Afrah Ziya <aysha26@digiscrypt.com>
Date: Fri, 17 Jul 2026 23:23:53 +0530
Subject: [PATCH] fix use-after-free from dangling StringView in
 Blackboard::getKeys

---
 include/behaviortree_cpp/blackboard.h    |  2 +-
 src/blackboard.cpp                       | 17 ++++-----
 tests/gtest_blackboard_thread_safety.cpp | 45 ++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h
index c88924400..ca3221be2 100644
--- a/include/behaviortree_cpp/blackboard.h
+++ b/include/behaviortree_cpp/blackboard.h
@@ -123,7 +123,7 @@ class Blackboard
 
   void debugMessage() const;
 
-  [[nodiscard]] std::vector<StringView> getKeys() const;
+  [[nodiscard]] std::vector<std::string> 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 7858b4655..4e6f7126e 100644
--- a/src/blackboard.cpp
+++ b/src/blackboard.cpp
@@ -137,16 +137,14 @@ void Blackboard::debugMessage() const
   }
 }
 
-std::vector<StringView> Blackboard::getKeys() const
+std::vector<std::string> Blackboard::getKeys() const
 {
-  // Lock storage_mutex_ (shared) to prevent iterator invalidation and
-  // dangling StringView from concurrent modifications (BUG-6 fix).
+  // Return owning copies of the keys, made while holding the shared lock.
+  // Returning StringView into storage_ would dangle as soon as another thread
+  // erased an entry (Blackboard::unset / the UnsetBlackboard action) after the
+  // lock is released but before the caller reads the view.
   const std::shared_lock storage_lock(storage_mutex_);
-  if(storage_.empty())
-  {
-    return {};
-  }
-  std::vector<StringView> out;
+  std::vector<std::string> out;
   out.reserve(storage_.size());
   for(const auto& entry_it : storage_)
   {
@@ -337,9 +335,8 @@ std::shared_ptr<Blackboard::Entry> 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.getKeys())
   {
-    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_thread_safety.cpp b/tests/gtest_blackboard_thread_safety.cpp
index 483720c8a..1eff49def 100644
--- a/tests/gtest_blackboard_thread_safety.cpp
+++ b/tests/gtest_blackboard_thread_safety.cpp
@@ -335,3 +335,48 @@ TEST(BlackboardThreadSafety, GetKeysWhileModifying_Bug6)
 
   SUCCEED();
 }
+
+// BUG-6 (continued): getKeys() used to return StringView pointing into the
+// std::string keys of storage_. The shared lock is released when getKeys()
+// returns, so a concurrent erase (unset() / the UnsetBlackboard action) frees
+// the underlying key while the caller still reads the view. This is exactly
+// what ExportBlackboardToJSON does on the Groot2 server thread.
+// Under ASan/TSan this reports a heap-use-after-free before the fix.
+TEST(BlackboardThreadSafety, GetKeysReadWhileErasing_Bug6)
+{
+  auto bb = Blackboard::create();
+
+  constexpr int kIterations = 200000;
+  std::atomic<bool> stop{ false };
+
+  auto reader = [&]() {
+    while(!stop.load())
+    {
+      for(const auto& key : bb->getKeys())
+      {
+        // Read the key content after getKeys() released the lock.
+        volatile size_t s = std::string(key).size();
+        (void)s;
+      }
+    }
+  };
+
+  auto mutator = [&]() {
+    for(int i = 0; i < kIterations; i++)
+    {
+      const std::string key = "key_" + std::to_string(i % 64);
+      bb->set(key, i);
+      bb->unset(key);  // erase -> frees the map key's std::string
+    }
+    stop.store(true);
+  };
+
+  std::thread t1(reader);
+  std::thread t2(mutator);
+
+  t2.join();
+  stop.store(true);
+  t1.join();
+
+  SUCCEED();
+}