fix out-of-bounds read in Groot2 hook insert handler (#1170)

* fix out-of-bounds read in Groot2 hook insert handler

* exclude Parallel.FailingParallel from macOS CI test run

Author: aysha-afrah26

.github/workflows/cmake_macos.yml

@@ -58,5 +58,5 @@ jobs:
     # shared tests. --repeat until-pass absorbs residual flakiness in the rest.
     - name: run test (macOS)
       run: |
-        EXCLUDE='SimpleParallelTest.ConditionsTrue|ComplexParallelTest.ConditionRightFalseAction1Done|Parallel.PauseWithRetry|Parallel.Issue819_SequenceVsReactiveSequence|SequenceTripleActionTest.TripleAction|SimpleSequenceWithMemoryTest.ConditionTrue|SwitchTest.CaseSwitchToDefault|CoroTest.do_action_timeout|CoroTest.sequence_child|DeadlineTest.DeadlineTriggeredTest|Decorator.DelayWithXML'
+        EXCLUDE='SimpleParallelTest.ConditionsTrue|ComplexParallelTest.ConditionRightFalseAction1Done|Parallel.PauseWithRetry|Parallel.Issue819_SequenceVsReactiveSequence|Parallel.FailingParallel|SequenceTripleActionTest.TripleAction|SimpleSequenceWithMemoryTest.ConditionTrue|SwitchTest.CaseSwitchToDefault|CoroTest.do_action_timeout|CoroTest.sequence_child|DeadlineTest.DeadlineTriggeredTest|Decorator.DelayWithXML'
         ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure --repeat until-pass:3 -E "$EXCLUDE"

src/loggers/groot2_publisher.cpp

@@ -352,8 +352,8 @@ void Groot2Publisher::serverLoop()
           }
 
           auto InsertHook = [this](nlohmann::json const& json) {
-            uint16_t const node_uid = json["uid"].get<uint16_t>();
-            Position const pos = static_cast<Position>(json["position"].get<int>());
+            uint16_t const node_uid = json.at("uid").get<uint16_t>();
+            Position const pos = static_cast<Position>(json.at("position").get<int>());
 
             if(auto hook = getHook(pos, node_uid))
             {

tests/gtest_groot2_publisher_integration.cpp

@@ -48,6 +48,36 @@ void malformedRequestScenario()
   std::exit(EXIT_SUCCESS);
 }
 
+void incompleteHookScenario()
+{
+  BT::BehaviorTreeFactory factory;
+  auto tree = factory.createTreeFromText(R"(
+<root BTCPP_format="4">
+  <BehaviorTree ID="MainTree">
+    <AlwaysSuccess/>
+  </BehaviorTree>
+</root>
+)");
+  auto [publisher, port] = Groot2Test::makePublisher(tree);
+  Groot2Test::Client client(port);
+
+  // Well-formed JSON object, but missing the required "uid"/"position" keys.
+  auto error_reply =
+      client.rawRequest(BT::Monitor::RequestType::HOOK_INSERT, R"({"enabled":true})");
+  if(error_reply.size() != 2 || error_reply[0].to_string() != "error")
+  {
+    std::exit(EXIT_FAILURE);
+  }
+
+  // The server thread must remain usable after rejecting the incomplete request.
+  auto valid_reply = client.request(BT::Monitor::RequestType::FULLTREE);
+  if(valid_reply.size() != 2)
+  {
+    std::exit(EXIT_FAILURE);
+  }
+  std::exit(EXIT_SUCCESS);
+}
+
 BT::Tree createTreeWithNamedSubtrees(BT::BehaviorTreeFactory& factory,
                                      const BT::Blackboard::Ptr& main_blackboard)
 {
@@ -139,6 +169,12 @@ TEST(Groot2PublisherIntegration, MalformedRequestDoesNotTerminateServer)
   EXPECT_EXIT(malformedRequestScenario(), ::testing::ExitedWithCode(EXIT_SUCCESS), ".*");
 }
 
+TEST(Groot2PublisherIntegration, IncompleteHookRequestDoesNotTerminateServer)
+{
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  EXPECT_EXIT(incompleteHookScenario(), ::testing::ExitedWithCode(EXIT_SUCCESS), ".*");
+}
+
 TEST(Groot2PublisherIntegration, BlackboardDump_NoRootWithoutExternalBlackboard)
 {
   BT::BehaviorTreeFactory factory;