add Blackboard::getKeyNames() and deprecate getKeys() (dangling StringViews)

include/behaviortree_cpp/blackboard.h

@@ -123,7 +123,14 @@
 
   void debugMessage() const;
 
-  [[nodiscard]] std::vector<StringView> getKeys() const;
+  /// The names of all the entries stored in this blackboard (copies).
+  [[nodiscard]] std::vector<std::string> 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<StringView>
+  getKeys() const;
 
   [[deprecated("This command is unsafe. Consider using Backup/Restore instead")]] void
   clear();

src/blackboard.cpp

@@ -38,7 +38,7 @@
   // 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;
 }
 
@@ -49,9 +49,9 @@
 // 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;
@@ -76,7 +76,7 @@
     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.
@@ -85,7 +85,7 @@
   // other entries, i.e. call this function again.
   for(auto* entry : unlocked)
   {
     delete entry;  // NOLINT(cppcoreguidelines-owning-memory)
   }
 }
 
@@ -96,7 +96,7 @@
   // 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);
@@ -229,6 +229,18 @@
   }
 }
 
+std::vector<std::string> Blackboard::getKeyNames() const
+{
+  const std::shared_lock storage_lock(storage_mutex_);
+  std::vector<std::string> out;
+  out.reserve(storage_.size());
+  for(const auto& entry_it : storage_)
+  {
+    out.push_back(entry_it.first);
+  }
+  return out;
+}
+
 std::vector<StringView> Blackboard::getKeys() const
 {
   // Lock storage_mutex_ (shared) to prevent iterator invalidation and
@@ -452,9 +464,8 @@
 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())

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<int>(), 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<int>(), 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<int>(), 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<std::string> 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++)
     {

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();
+}

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<double>(), 8.0);
-  EXPECT_EQ(variables->getKeys().size(), 2);
+  EXPECT_EQ(variables->getKeyNames().size(), 2);
   EXPECT_EQ(variables->get<double>("x"), 3.0);
   EXPECT_EQ(variables->get<double>("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<std::string>("A"), "hello");
   EXPECT_EQ(variables->get<std::string>("B"), " ");
   EXPECT_EQ(variables->get<std::string>("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<std::string>("A"), "   right");
   EXPECT_EQ(variables->get<std::string>("B"), " center ");
   EXPECT_EQ(variables->get<std::string>("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<int>(), 1);