fix data race in SetBlackboardNode copying a blackboard entry (#1236)

Author: aysha-afrah26

include/behaviortree_cpp/actions/set_blackboard_node.h

@@ -15,6 +15,8 @@
 
 #include "behaviortree_cpp/action_node.h"
 
+#include <mutex>
+
 namespace BT
 {
 /**
@@ -76,13 +78,18 @@ class SetBlackboardNode : public SyncActionNode
       {
         throw RuntimeError("Can't find the port referred by [value]");
       }
+      // Entry::value and Entry::info are shared with any thread that writes
+      // the same entry (Blackboard::set holds entry_mutex while reassigning
+      // them), so they must be copied under the same mutex.
+      std::unique_lock src_lock(src_entry->entry_mutex);
+      out_value = src_entry->value;
+      const TypeInfo src_info = src_entry->info;
+      src_lock.unlock();
       if(!dst_entry)
       {
-        config().blackboard->createEntry(output_key, src_entry->info);
+        config().blackboard->createEntry(output_key, src_info);
         dst_entry = config().blackboard->getEntry(output_key);
       }
-
-      out_value = src_entry->value;
     }
     else
     {
@@ -94,17 +101,24 @@ class SetBlackboardNode : public SyncActionNode
 
     // avoid type issues when port is remapped: current implementation of the set might be a little bit problematic for initialized on the fly values
     // this still does not attack math issues
-    if(dst_entry && dst_entry->info.type() != typeid(std::string) && out_value.isString())
+    if(dst_entry && out_value.isString())
     {
-      try
-      {
-        out_value = dst_entry->info.parseString(out_value.cast<std::string>());
-      }
-      catch(const std::exception& e)
+      // same as above: Entry::info may be reassigned under entry_mutex by a
+      // concurrent Blackboard::set, so take a copy under the same mutex.
+      std::unique_lock dst_lock(dst_entry->entry_mutex);
+      const TypeInfo dst_info = dst_entry->info;
+      dst_lock.unlock();
+      if(dst_info.type() != typeid(std::string))
       {
-        throw LogicError("Can't convert string [", out_value.cast<std::string>(),
-                         "] to type [", BT::demangle(dst_entry->info.type()),
-                         "]: ", e.what());
+        try
+        {
+          out_value = dst_info.parseString(out_value.cast<std::string>());
+        }
+        catch(const std::exception& e)
+        {
+          throw LogicError("Can't convert string [", out_value.cast<std::string>(),
+                           "] to type [", BT::demangle(dst_info.type()), "]: ", e.what());
+        }
       }
     }
 

tests/gtest_blackboard_thread_safety.cpp

@@ -7,6 +7,7 @@
  */
 
 #include "behaviortree_cpp/blackboard.h"
+#include "behaviortree_cpp/bt_factory.h"
 
 #include <atomic>
 #include <condition_variable>
@@ -336,6 +337,45 @@ TEST(BlackboardThreadSafety, GetKeysWhileModifying_Bug6)
   SUCCEED();
 }
 
+// SetBlackboardNode::tick() copied Entry::value (and read Entry::info) of the
+// source entry without locking entry_mutex. A concurrent Blackboard::set() on
+// the same entry reassigns the stored Any under entry_mutex, so the unlocked
+// copy races with the destruction of the previous string buffer.
+// Meaningful with ASan/TSan. The strings are longer than the small string
+// buffer, so that the content lives on the heap.
+TEST(BlackboardThreadSafety, SetBlackboardNodeWhileWriting)
+{
+  static const char* xml_text = R"(
+    <root BTCPP_format="4">
+      <BehaviorTree ID="MainTree">
+        <SetBlackboard value="{src}" output_key="dst"/>
+      </BehaviorTree>
+    </root>)";
+
+  BehaviorTreeFactory factory;
+  auto tree = factory.createTreeFromText(xml_text);
+  auto bb = tree.subtrees.front()->blackboard;
+
+  const std::string prefix = "a_value_that_is_too_long_for_the_small_string_buffer_";
+  bb->set("src", prefix);
+
+  std::atomic_bool stop = false;
+  std::thread writer([&]() {
+    int i = 0;
+    while(!stop)
+    {
+      bb->set("src", prefix + std::to_string(i++ % 8));
+    }
+  });
+
+  for(int i = 0; i < 20000; i++)
+  {
+    tree.tickWhileRunning();
+  }
+  stop = true;
+  writer.join();
+}
+
 // 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,