fix use-after-free of the timer node mutex on destruction (#1180)

* fix use-after-free of the timer node mutex on destruction

* add regression test for tree destruction with pending timers

Without the member reorder in sleep_node.h, delay_node.h and
timeout_node.h, this test aborts on libc++ with
"mutex lock failed: Invalid argument": the canceled timer handler
locks the node mutex after its destructor has already run.

Author: aysha-afrah26

include/behaviortree_cpp/actions/sleep_node.h

@@ -40,11 +40,13 @@ class SleepNode : public StatefulActionNode
   }
 
 private:
-  TimerQueue<> timer_;
   uint64_t timer_id_ = 0;
 
   std::atomic_bool timer_waiting_ = false;
   std::mutex delay_mutex_;
+  // Keep last: ~TimerQueue() joins the worker thread, so this must be destroyed
+  // before delay_mutex_ and timer_waiting_, which the timer handler touches.
+  TimerQueue<> timer_;
 };
 
 }  // namespace BT

include/behaviortree_cpp/decorators/delay_node.h

@@ -65,7 +65,6 @@ class DelayNode : public DecoratorNode
   void halt() override;
 
 private:
-  TimerQueue<> timer_;
   uint64_t timer_id_;
 
   virtual BT::NodeStatus tick() override;
@@ -76,6 +75,9 @@ class DelayNode : public DecoratorNode
   unsigned msec_;
   bool read_parameter_from_ports_;
   std::mutex delay_mutex_;
+  // Keep last: ~TimerQueue() joins the worker thread, so this must be destroyed
+  // before delay_mutex_, which the timer handler locks.
+  TimerQueue<> timer_;
 };
 
 }  // namespace BT

include/behaviortree_cpp/decorators/timeout_node.h

@@ -77,14 +77,16 @@ class TimeoutNode : public DecoratorNode
 
   void halt() override;
 
-  TimerQueue<> timer_;
   std::atomic_bool child_halted_ = false;
   uint64_t timer_id_;
 
   unsigned msec_;
   bool read_parameter_from_ports_;
   std::atomic_bool timeout_started_ = false;
   std::mutex timeout_mutex_;
+  // Keep last: ~TimerQueue() joins the worker thread, so this must be destroyed
+  // before timeout_mutex_, which the timer handler locks.
+  TimerQueue<> timer_;
 };
 
 }  // namespace BT

tests/gtest_decorator.cpp

@@ -434,6 +434,45 @@ TEST(Decorator, Inverter_InSequence)
   ASSERT_EQ(status, NodeStatus::SUCCESS);
 }
 
+// Regression test: Sleep, Delay and Timeout own a TimerQueue whose worker
+// thread locks a mutex belonging to the node. The TimerQueue member must be
+// destroyed (joining that thread) before the mutex, otherwise destroying a
+// tree with a pending timer lets the handler lock an already-destroyed mutex.
+// On libc++ this aborts with "mutex lock failed: Invalid argument".
+TEST(Decorator, DestroyTreeWhileTimerIsPending)
+{
+  BT::BehaviorTreeFactory factory;
+
+  const std::string xml_text = R"(
+    <root BTCPP_format="4">
+       <BehaviorTree ID="PendingTimers">
+          <Parallel success_count="6" failure_count="1">
+            <Sleep msec="500"/>
+            <Sleep msec="500"/>
+            <Sleep msec="500"/>
+            <Sleep msec="500"/>
+            <Delay delay_msec="500">
+              <AlwaysSuccess/>
+            </Delay>
+            <Timeout msec="500">
+              <Sleep msec="500"/>
+            </Timeout>
+          </Parallel>
+       </BehaviorTree>
+    </root>)";
+
+  factory.registerBehaviorTreeFromText(xml_text);
+
+  for(int i = 0; i < 1000; i++)
+  {
+    auto tree = factory.createTree("PendingTimers");
+    ASSERT_EQ(tree.tickOnce(), NodeStatus::RUNNING);
+    // The tree is destroyed here while all the timers are still pending:
+    // each canceled handler runs on its TimerQueue worker thread, racing
+    // with the destruction of the node members.
+  }
+}
+
 // Test KeepRunningUntilFailure decorator
 TEST(Decorator, KeepRunningUntilFailure)
 {