Fix Blackboard thread-safety bugs and XML null pointer dereference

Fix 8 bugs found via code analysis and verified with ThreadSanitizer:

Blackboard thread-safety (6 fixes):
- BUG-1/8: set() new-entry path wrote value/sequence_id/stamp without
  entry_mutex after createEntryImpl — add scoped_lock on entry_mutex
- BUG-2: set() existing-entry path held raw reference after unlocking
  storage_mutex_, risking use-after-free if concurrent unset() erases
  the entry — copy shared_ptr before unlocking
- BUG-3: cloneInto() read/wrote entry members without entry_mutex —
  add scoped_lock on src and dst entry mutexes
- BUG-4: ImportBlackboardFromJSON wrote entry->value without any lock —
  add scoped_lock on entry_mutex
- BUG-5: debugMessage() iterated storage_ without storage_mutex_ —
  add unique_lock
- BUG-6: getKeys() iterated storage_ without storage_mutex_ —
  add unique_lock

XML parser (2 fixes):
- BUG-7: loadSubtreeModel crashed on null subtree_id when <SubTree>
  in <TreeNodesModel> was missing ID attribute — add null check with
  descriptive RuntimeError
- BUG-10: Variable name shadowing in loadSubtreeModel inner loop —
  rename to port_name

All fixes include regression tests (TSan-verified for thread-safety bugs).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: facontidavide

CMakeLists.txt

@@ -180,6 +180,7 @@ list(APPEND BT_SOURCE
     src/controls/sequence_node.cpp
     src/controls/sequence_with_memory_node.cpp
     src/controls/switch_node.cpp
+    src/controls/try_catch_node.cpp
     src/controls/while_do_else_node.cpp
 
     src/loggers/bt_cout_logger.cpp

include/behaviortree_cpp/behavior_tree.h

@@ -33,6 +33,7 @@
 #include "behaviortree_cpp/controls/sequence_node.h"
 #include "behaviortree_cpp/controls/sequence_with_memory_node.h"
 #include "behaviortree_cpp/controls/switch_node.h"
+#include "behaviortree_cpp/controls/try_catch_node.h"
 #include "behaviortree_cpp/controls/while_do_else_node.h"
 #include "behaviortree_cpp/decorators/delay_node.h"
 #include "behaviortree_cpp/decorators/force_failure_node.h"

include/behaviortree_cpp/blackboard.h

@@ -300,8 +300,10 @@ inline void Blackboard::set(const std::string& key, const T& value)
                         GetAnyFromStringFunctor<T>());
       entry = createEntryImpl(key, new_port);
     }
-    storage_lock.lock();
 
+    // Lock entry_mutex before writing to prevent data races with
+    // concurrent readers (BUG-1/BUG-8 fix).
+    std::scoped_lock entry_lock(entry->entry_mutex);
     entry->value = new_value;
     entry->sequence_id++;
     entry->stamp = std::chrono::steady_clock::now().time_since_epoch();
@@ -310,8 +312,11 @@ inline void Blackboard::set(const std::string& key, const T& value)
   {
     // this is not the first time we set this entry, we need to check
     // if the type is the same or not.
-    Entry& entry = *it->second;
+    // Copy shared_ptr to prevent use-after-free if another thread
+    // calls unset() while we hold the reference (BUG-2 fix).
+    auto entry_ptr = it->second;
     storage_lock.unlock();
+    Entry& entry = *entry_ptr;
 
     std::scoped_lock scoped_lock(entry.entry_mutex);
 

include/behaviortree_cpp/controls/try_catch_node.h

@@ -0,0 +1,69 @@
+/* Copyright (C) 2018-2025 Davide Faconti, Eurecat -  All Rights Reserved
+*
+*   Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"),
+*   to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense,
+*   and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
+*   The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+*
+*   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+*   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+*   WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+*/
+
+#pragma once
+
+#include "behaviortree_cpp/control_node.h"
+
+namespace BT
+{
+/**
+ * @brief The TryCatch node executes children 1..N-1 as a Sequence ("try" block).
+ *
+ * If all children in the try-block succeed, this node returns SUCCESS.
+ *
+ * If any child in the try-block fails, the last child N is executed as a
+ * "catch" (cleanup) action, and this node returns FAILURE regardless of
+ * the catch child's result.
+ *
+ * - If a try-child returns RUNNING, this node returns RUNNING.
+ * - If a try-child returns SUCCESS, continue to the next try-child.
+ * - If a try-child returns FAILURE, enter catch mode and tick the last child.
+ * - If the catch child returns RUNNING, this node returns RUNNING.
+ * - When the catch child finishes (SUCCESS or FAILURE), this node returns FAILURE.
+ * - SKIPPED try-children are skipped over (not treated as failure).
+ *
+ * Port "catch_on_halt" (default false): if true, the catch child is also
+ * executed when the TryCatch node is halted while the try-block is RUNNING.
+ *
+ * Requires at least 2 children.
+ */
+class TryCatchNode : public ControlNode
+{
+public:
+  TryCatchNode(const std::string& name, const NodeConfig& config);
+
+  ~TryCatchNode() override = default;
+
+  TryCatchNode(const TryCatchNode&) = delete;
+  TryCatchNode& operator=(const TryCatchNode&) = delete;
+  TryCatchNode(TryCatchNode&&) = delete;
+  TryCatchNode& operator=(TryCatchNode&&) = delete;
+
+  static PortsList providedPorts()
+  {
+    return { InputPort<bool>("catch_on_halt", false,
+                             "If true, execute the catch child when "
+                             "the node is halted during the try-block") };
+  }
+
+  virtual void halt() override;
+
+private:
+  size_t current_child_idx_;
+  size_t skipped_count_;
+  bool in_catch_;
+
+  virtual BT::NodeStatus tick() override;
+};
+
+}  // namespace BT

src/blackboard.cpp

@@ -103,6 +103,9 @@ void Blackboard::addSubtreeRemapping(StringView internal, StringView external)
 
 void Blackboard::debugMessage() const
 {
+  // Lock storage_mutex_ to prevent iterator invalidation from
+  // concurrent modifications (BUG-5 fix).
+  const std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   for(const auto& [key, entry] : storage_)
   {
     auto port_type = entry->info.type();
@@ -136,6 +139,9 @@ void Blackboard::debugMessage() const
 
 std::vector<StringView> Blackboard::getKeys() const
 {
+  // Lock storage_mutex_ to prevent iterator invalidation and
+  // dangling StringView from concurrent modifications (BUG-6 fix).
+  const std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   if(storage_.empty())
   {
     return {};
@@ -200,8 +206,10 @@ void Blackboard::cloneInto(Blackboard& dst) const
     auto it = dst_storage.find(src_key);
     if(it != dst_storage.end())
     {
-      // overwrite
+      // overwrite — lock both entry mutexes to prevent data races
+      // with concurrent readers that hold shared_ptr<Entry> (BUG-3 fix).
       auto& dst_entry = it->second;
+      std::scoped_lock entry_locks(src_entry->entry_mutex, dst_entry->entry_mutex);
       dst_entry->string_converter = src_entry->string_converter;
       dst_entry->value = src_entry->value;
       dst_entry->info = src_entry->info;
@@ -210,7 +218,8 @@ void Blackboard::cloneInto(Blackboard& dst) const
     }
     else
     {
-      // create new
+      // create new — lock src entry to read its members safely
+      std::scoped_lock src_lock(src_entry->entry_mutex);
       auto new_entry = std::make_shared<Entry>(src_entry->info);
       new_entry->value = src_entry->value;
       new_entry->string_converter = src_entry->string_converter;
@@ -317,6 +326,8 @@ void ImportBlackboardFromJSON(const nlohmann::json& json, Blackboard& blackboard
         blackboard.createEntry(it.key(), res->second);
         entry = blackboard.getEntry(it.key());
       }
+      // Lock entry_mutex before writing to prevent data races (BUG-4 fix).
+      std::scoped_lock lk(entry->entry_mutex);
       entry->value = res->first;
     }
   }

src/bt_factory.cpp

@@ -131,6 +131,7 @@ BehaviorTreeFactory::BehaviorTreeFactory() : _p(new PImpl)
   registerNodeType<ReactiveFallback>("ReactiveFallback");
   registerNodeType<IfThenElseNode>("IfThenElse");
   registerNodeType<WhileDoElseNode>("WhileDoElse");
+  registerNodeType<TryCatchNode>("TryCatch");
 
   registerNodeType<InverterNode>("Inverter");
 

src/controls/try_catch_node.cpp

@@ -0,0 +1,137 @@
+/* Copyright (C) 2018-2025 Davide Faconti, Eurecat -  All Rights Reserved
+*
+*   Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"),
+*   to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense,
+*   and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
+*   The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+*
+*   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+*   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+*   WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+*/
+
+#include "behaviortree_cpp/controls/try_catch_node.h"
+
+namespace BT
+{
+TryCatchNode::TryCatchNode(const std::string& name, const NodeConfig& config)
+  : ControlNode::ControlNode(name, config)
+  , current_child_idx_(0)
+  , skipped_count_(0)
+  , in_catch_(false)
+{
+  setRegistrationID("TryCatch");
+}
+
+void TryCatchNode::halt()
+{
+  bool catch_on_halt = false;
+  getInput("catch_on_halt", catch_on_halt);
+
+  // If catch_on_halt is enabled and we were in the try-block (not already in catch),
+  // execute the catch child synchronously before halting.
+  if(catch_on_halt && !in_catch_ && isStatusActive(status()) &&
+     children_nodes_.size() >= 2)
+  {
+    // Halt all try-block children first
+    for(size_t i = 0; i < children_nodes_.size() - 1; i++)
+    {
+      haltChild(i);
+    }
+
+    // Tick the catch child. If it returns RUNNING, halt it too
+    // (best-effort cleanup during halt).
+    TreeNode* catch_child = children_nodes_.back();
+    const NodeStatus catch_status = catch_child->executeTick();
+    if(catch_status == NodeStatus::RUNNING)
+    {
+      haltChild(children_nodes_.size() - 1);
+    }
+  }
+
+  current_child_idx_ = 0;
+  skipped_count_ = 0;
+  in_catch_ = false;
+  ControlNode::halt();
+}
+
+NodeStatus TryCatchNode::tick()
+{
+  const size_t children_count = children_nodes_.size();
+
+  if(children_count < 2)
+  {
+    throw LogicError("[", name(), "]: TryCatch requires at least 2 children");
+  }
+
+  if(!isStatusActive(status()))
+  {
+    skipped_count_ = 0;
+    in_catch_ = false;
+  }
+
+  setStatus(NodeStatus::RUNNING);
+
+  const size_t try_count = children_count - 1;
+
+  // If we are in catch mode, tick the last child (cleanup)
+  if(in_catch_)
+  {
+    TreeNode* catch_child = children_nodes_.back();
+    const NodeStatus catch_status = catch_child->executeTick();
+
+    if(catch_status == NodeStatus::RUNNING)
+    {
+      return NodeStatus::RUNNING;
+    }
+
+    // Catch child finished (SUCCESS or FAILURE): return FAILURE
+    resetChildren();
+    current_child_idx_ = 0;
+    in_catch_ = false;
+    return NodeStatus::FAILURE;
+  }
+
+  // Try-block: execute children 0..N-2 as a Sequence
+  while(current_child_idx_ < try_count)
+  {
+    TreeNode* current_child_node = children_nodes_[current_child_idx_];
+    const NodeStatus child_status = current_child_node->executeTick();
+
+    switch(child_status)
+    {
+      case NodeStatus::RUNNING: {
+        return NodeStatus::RUNNING;
+      }
+      case NodeStatus::FAILURE: {
+        // Enter catch mode: halt try-block children, then tick catch child
+        resetChildren();
+        current_child_idx_ = 0;
+        in_catch_ = true;
+        return tick();  // re-enter to tick the catch child
+      }
+      case NodeStatus::SUCCESS: {
+        current_child_idx_++;
+      }
+      break;
+      case NodeStatus::SKIPPED: {
+        current_child_idx_++;
+        skipped_count_++;
+      }
+      break;
+      case NodeStatus::IDLE: {
+        throw LogicError("[", name(), "]: A child should not return IDLE");
+      }
+    }
+  }
+
+  // All try-children completed successfully (or were skipped)
+  const bool all_skipped = (skipped_count_ == try_count);
+  resetChildren();
+  current_child_idx_ = 0;
+  skipped_count_ = 0;
+
+  return all_skipped ? NodeStatus::SKIPPED : NodeStatus::SUCCESS;
+}
+
+}  // namespace BT

src/xml_parsing.cpp

@@ -293,6 +293,11 @@ void BT::XMLParser::PImpl::loadSubtreeModel(const XMLElement* xml_root)
         sub_node = sub_node->NextSiblingElement("SubTree"))
     {
       auto subtree_id = sub_node->Attribute("ID");
+      if(subtree_id == nullptr)
+      {
+        throw RuntimeError("Missing attribute 'ID' in SubTree element "
+                           "within TreeNodesModel");
+      }
       auto& subtree_model = subtree_models[subtree_id];
 
       const std::pair<const char*, BT::PortDirection> port_types[3] = {
@@ -307,13 +312,13 @@ void BT::XMLParser::PImpl::loadSubtreeModel(const XMLElement* xml_root)
             port_node = port_node->NextSiblingElement(name))
         {
           BT::PortInfo port(direction);
-          auto name = port_node->Attribute("name");
-          if(name == nullptr)
+          auto port_name = port_node->Attribute("name");
+          if(port_name == nullptr)
           {
             throw RuntimeError("Missing attribute [name] in port (SubTree model)");
           }
           // Validate port name
-          validatePortName(name, port_node->GetLineNum());
+          validatePortName(port_name, port_node->GetLineNum());
           if(auto default_value = port_node->Attribute("default"))
           {
             port.setDefaultValue(default_value);
@@ -322,7 +327,7 @@ void BT::XMLParser::PImpl::loadSubtreeModel(const XMLElement* xml_root)
           {
             port.setDescription(description);
           }
-          subtree_model.ports[name] = std::move(port);
+          subtree_model.ports[port_name] = std::move(port);
         }
       }
     }
@@ -627,6 +632,11 @@ void VerifyXML(const std::string& xml_text,
           ThrowError(line_number, std::string("The node '") + registered_name +
                                       "' must have 1 or more children");
         }
+        if(registered_name == "TryCatch" && children_count < 2)
+        {
+          ThrowError(line_number, std::string("The node 'TryCatch' must have "
+                                              "at least 2 children"));
+        }
         if(registered_name == "ReactiveSequence")
         {
           size_t async_count = 0;

tests/CMakeLists.txt

@@ -46,6 +46,7 @@ set(BT_TESTS
   gtest_subtree.cpp
   gtest_switch.cpp
   gtest_tree.cpp
+  gtest_try_catch.cpp
   gtest_exception_tracking.cpp
   gtest_updates.cpp
   gtest_wakeup.cpp
@@ -54,6 +55,8 @@ set(BT_TESTS
   gtest_simple_string.cpp
   gtest_polymorphic_ports.cpp
   gtest_plugin_issue953.cpp
+  gtest_blackboard_thread_safety.cpp
+  gtest_xml_null_subtree_id.cpp
 
   script_parser_test.cpp
   test_helper.hpp