reject non-integer JSON numbers where an integer is expected

include/behaviortree_cpp/json_export.h

@@ -3,6 +3,9 @@
 #include "behaviortree_cpp/basic_types.h"
 #include "behaviortree_cpp/utils/safe_any.hpp"
 
+#include <limits>
+#include <type_traits>
+
 // Use the version of nlohmann::json embedded in BT.CPP,
 // or the system one if BT.CPP was built with USE_VENDORED_JSON=OFF
 #ifdef BTCPP_SYSTEM_JSON
@@ -250,6 +253,45 @@ inline void RegisterJsonDefinition()
   JsonExporter::get().addConverter<T>();
 }
 
+/**
+ * @brief Read a JSON integer that must be representable as T.
+ *
+ * json.get<T>() converts a floating point value or an out-of-range integer
+ * with a plain static_cast, which truncates, wraps, or (for a value outside
+ * the range of T) is undefined behavior. This throws RuntimeError instead.
+ */
+template <typename T>
+[[nodiscard]] inline T JsonToInteger(const nlohmann::json& value)
+{
+  static_assert(std::is_integral_v<T> && !std::is_same_v<T, bool>);
+  if(value.is_number_unsigned())
+  {
+    const auto number = value.get<uint64_t>();
+    if(number <= static_cast<uint64_t>(std::numeric_limits<T>::max()))
+    {
+      return static_cast<T>(number);
+    }
+  }
+  else if(value.is_number_integer())
+  {
+    const auto number = value.get<int64_t>();
+    if constexpr(std::is_signed_v<T>)
+    {
+      if(number >= static_cast<int64_t>(std::numeric_limits<T>::min()) &&
+         number <= static_cast<int64_t>(std::numeric_limits<T>::max()))
+      {
+        return static_cast<T>(number);
+      }
+    }
+    else if(number >= 0 && static_cast<uint64_t>(number) <= std::numeric_limits<T>::max())
+    {
+      return static_cast<T>(number);
+    }
+  }
+  throw RuntimeError(StrCat("JSON value [", value.dump(), "] is not an integer of type ",
+                            BT::demangle(typeid(T))));
+}
+
 }  // namespace BT
 
 //------------------------------------------------

include/behaviortree_cpp/loggers/groot2_protocol.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "behaviortree_cpp/basic_types.h"
+#include "behaviortree_cpp/json_export.h"
 #ifdef BTCPP_SYSTEM_JSON
 #include <nlohmann/json.hpp>
 #else
@@ -256,10 +257,10 @@ inline void to_json(nlohmann::json& js, const Hook& bp)
 inline void from_json(const nlohmann::json& js, Hook& bp)
 {
   js.at("enabled").get_to(bp.enabled);
-  js.at("uid").get_to(bp.node_uid);
+  bp.node_uid = JsonToInteger<uint16_t>(js.at("uid"));
   js.at("once").get_to(bp.remove_when_done);
-  bp.mode = static_cast<Hook::Mode>(js.at("mode").get<int>());
-  bp.position = static_cast<Hook::Position>(js.at("position").get<int>());
+  bp.mode = static_cast<Hook::Mode>(JsonToInteger<int>(js.at("mode")));
+  bp.position = static_cast<Hook::Position>(JsonToInteger<int>(js.at("position")));
 
   const std::string desired_value = js.at("desired_status").get<std::string>();
   bp.desired_status = convertFromString<NodeStatus>(desired_value);

src/basic_types.cpp

@@ -309,7 +309,18 @@ std::vector<int> convertFromString<std::vector<int>>(StringView str)
   if(StartWith(str, "json:"))
   {
     str.remove_prefix(5);
-    return nlohmann::json::parse(str).get<std::vector<int>>();
+    const auto json = nlohmann::json::parse(str);
+    if(!json.is_array())
+    {
+      throw RuntimeError(StrCat("Can't convert string [", str, "] to std::vector<int>"));
+    }
+    std::vector<int> output;
+    output.reserve(json.size());
+    for(const auto& element : json)
+    {
+      output.push_back(JsonToInteger<int>(element));
+    }
+    return output;
   }
   auto parts = splitString(str, ';');
   std::vector<int> output;

src/bt_factory.cpp

@@ -14,6 +14,7 @@
 
 #include "tinyxml2.h"
 
+#include "behaviortree_cpp/json_export.h"
 #include "behaviortree_cpp/utils/shared_library.h"
 #include "behaviortree_cpp/utils/wildcards.hpp"
 #include "behaviortree_cpp/xml_parsing.h"
@@ -569,7 +570,7 @@ void BehaviorTreeFactory::loadSubstitutionRuleFromJSON(const std::string& json_t
       if(test_config.contains("async_delay"))
       {
         config.async_delay =
-            std::chrono::milliseconds(test_config["async_delay"].get<int>());
+            std::chrono::milliseconds(JsonToInteger<int>(test_config["async_delay"]));
       }
       if(test_config.contains("post_script"))
       {

src/json_export.cpp

@@ -116,8 +116,14 @@ JsonExporter::ExpectedEntry JsonExporter::fromJson(const nlohmann::json& source)
     }
     if(first_element.is_number_integer())
     {
-      return Entry{ BT::Any(source.get<std::vector<int>>()),
-                    BT::TypeInfo::Create<std::vector<int>>() };
+      // the type comes from the first element only: check every element
+      std::vector<int> values;
+      values.reserve(source.size());
+      for(const auto& element : source)
+      {
+        values.push_back(JsonToInteger<int>(element));
+      }
+      return Entry{ BT::Any(values), BT::TypeInfo::Create<std::vector<int>>() };
     }
     if(first_element.is_number_float())
     {

src/loggers/groot2_publisher.cpp

@@ -352,8 +352,9 @@ void Groot2Publisher::serverLoop()
           }
 
           auto InsertHook = [this](nlohmann::json const& json) {
-            uint16_t const node_uid = json.at("uid").get<uint16_t>();
-            Position const pos = static_cast<Position>(json.at("position").get<int>());
+            uint16_t const node_uid = JsonToInteger<uint16_t>(json.at("uid"));
+            int const position = JsonToInteger<int>(json.at("position"));
+            Position const pos = static_cast<Position>(position);
 
             if(auto hook = getHook(pos, node_uid))
             {
@@ -403,9 +404,9 @@ void Groot2Publisher::serverLoop()
           }
 
           auto json = nlohmann::json::parse(requestMsg[1].to_string());
-          const uint16_t node_uid = json.at("uid").get<uint16_t>();
+          const uint16_t node_uid = JsonToInteger<uint16_t>(json.at("uid"));
           const std::string status_str = json.at("desired_status").get<std::string>();
-          auto position = static_cast<Position>(json.at("position").get<int>());
+          auto position = static_cast<Position>(JsonToInteger<int>(json.at("position")));
           const bool remove = json.at("remove_when_done").get<bool>();
 
           NodeStatus desired_status = NodeStatus::SKIPPED;
@@ -444,8 +445,8 @@ void Groot2Publisher::serverLoop()
           }
 
           auto json = nlohmann::json::parse(requestMsg[1].to_string());
-          const uint16_t node_uid = json.at("uid").get<uint16_t>();
-          auto position = static_cast<Position>(json.at("position").get<int>());
+          const uint16_t node_uid = JsonToInteger<uint16_t>(json.at("uid"));
+          auto position = static_cast<Position>(JsonToInteger<int>(json.at("position")));
 
           if(!removeHook(position, node_uid))
           {

tests/gtest_groot2_publisher_integration.cpp

@@ -161,6 +161,40 @@ TEST(Groot2PublisherIntegration, PostHookRunsAfterNodeAndCanBeRemoved)
   EXPECT_EQ(tick_count, 2);
 }
 
+TEST(Groot2PublisherIntegration, HookRequestRejectsNonIntegerUid)
+{
+  BT::BehaviorTreeFactory factory;
+  factory.registerSimpleAction("CountAction",
+                               [](BT::TreeNode&) { return BT::NodeStatus::SUCCESS; });
+  auto tree = factory.createTreeFromText(kHookTreeXml);
+  auto [publisher, port] = Groot2Test::makePublisher(tree);
+  Groot2Test::Client client(port);
+  const auto uid = tree.rootNode()->UID();
+
+  // get<uint16_t>() used to wrap uid + 65536 and truncate uid + 0.5 to the uid
+  // of the node, and to cast 1e300 out of range (undefined behavior).
+  for(const nlohmann::json bad_uid :
+      { nlohmann::json(uid + 65536), nlohmann::json(uid + 0.5), nlohmann::json(1e300) })
+  {
+    auto hook = Groot2Test::makeHook(uid, BT::Monitor::Hook::Mode::REPLACE,
+                                     BT::Monitor::Hook::Position::PRE);
+    hook["uid"] = bad_uid;
+    auto reply = client.rawRequest(BT::Monitor::RequestType::HOOK_INSERT, hook.dump());
+    ASSERT_EQ(reply.size(), 2);
+    EXPECT_EQ(reply[0].to_string(), "error") << bad_uid;
+
+    const auto remove = nlohmann::json{ { "uid", bad_uid }, { "position", 0 } };
+    reply = client.rawRequest(BT::Monitor::RequestType::HOOK_REMOVE, remove.dump());
+    ASSERT_EQ(reply.size(), 2);
+    EXPECT_EQ(reply[0].to_string(), "error") << bad_uid;
+  }
+
+  auto dump_reply = client.request(BT::Monitor::RequestType::HOOKS_DUMP);
+  ASSERT_EQ(dump_reply.size(), 2);
+  EXPECT_TRUE(nlohmann::json::parse(dump_reply[1].to_string()).empty());
+  EXPECT_EQ(tree.tickExactlyOnce(), BT::NodeStatus::SUCCESS);
+}
+
 TEST(Groot2PublisherIntegration, MalformedRequestDoesNotTerminateServer)
 {
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";

tests/gtest_json.cpp

@@ -271,6 +271,52 @@ TEST_F(JsonTest, VectorInteger)
   }
 }
 
+TEST_F(JsonTest, IntegerValuesAreRangeChecked)
+{
+  using nlohmann::json;
+  BT::JsonExporter& exporter = BT::JsonExporter::get();
+
+  ASSERT_EQ(BT::JsonToInteger<int>(json(-7)), -7);
+  ASSERT_EQ(BT::JsonToInteger<uint16_t>(json(65535)), 65535);
+  ASSERT_EQ(BT::JsonToInteger<int64_t>(json(INT64_MIN)), INT64_MIN);
+  ASSERT_EQ(BT::JsonToInteger<uint64_t>(json(UINT64_MAX)), UINT64_MAX);
+  ASSERT_THROW(std::ignore = BT::JsonToInteger<uint16_t>(json(65536)), BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::JsonToInteger<uint16_t>(json(-1)), BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::JsonToInteger<int>(json(2.5)), BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::JsonToInteger<int>(json(1e300)), BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::JsonToInteger<int>(json(true)), BT::RuntimeError);
+
+  // The type of the array is taken from the first element. The other elements
+  // used to be truncated, wrapped, or cast out of range (undefined behavior).
+  for(const char* text :
+      { "[1, 2.5]", "[1, 1e300]", "[1, 5000000000]", "[1, -3000000000]" })
+  {
+    ASSERT_THROW(std::ignore = exporter.fromJson(json::parse(text)), BT::RuntimeError)
+        << text;
+    auto bb = BT::Blackboard::create();
+    ASSERT_THROW(BT::ImportBlackboardFromJSON(
+                     json::parse(R"({"v":)" + std::string(text) + "}"), *bb),
+                 BT::RuntimeError)
+        << text;
+  }
+  auto result = exporter.fromJson(json::parse("[1, -2, 2147483647]"));
+  ASSERT_TRUE(result) << result.error();
+  ASSERT_EQ(result->first.cast<std::vector<int>>(),
+            (std::vector<int>{ 1, -2, 2147483647 }));
+
+  // Same for the "json:" prefix of a std::vector<int> port
+  ASSERT_EQ(BT::convertFromString<std::vector<int>>("json:[3, -4]"),
+            (std::vector<int>{ 3, -4 }));
+  ASSERT_THROW(std::ignore = BT::convertFromString<std::vector<int>>("json:[1e300]"),
+               BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::convertFromString<std::vector<int>>("json:[1.5]"),
+               BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::convertFromString<std::vector<int>>("json:[4294967297]"),
+               BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::convertFromString<std::vector<int>>("json:{\"a\":1}"),
+               BT::RuntimeError);
+}
+
 TEST_F(JsonTest, VectorSring)
 {
   BT::JsonExporter& exporter = BT::JsonExporter::get();

tests/gtest_substitution.cpp

@@ -133,6 +133,26 @@ TEST(Substitution, ParserRejectsMissingReturnStatusAndScript)
   ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(invalid_json), RuntimeError);
 }
 
+TEST(Substitution, ParserRejectsNonIntegerAsyncDelay)
+{
+  // async_delay used to be read with get<int>(), which truncates a real
+  // number and casts an out-of-range one to int (undefined behavior).
+  for(const char* delay : { "1e300", "2.5", "5000000000" })
+  {
+    const std::string json_text = std::string(R"(
+    {
+      "TestNodeConfigs": {
+        "Delayed": { "return_status": "SUCCESS", "async_delay": )") +
+                                  delay + R"( }
+      },
+      "SubstitutionRules": { "actionA": "Delayed" }
+    })";
+
+    BehaviorTreeFactory factory;
+    ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(json_text), RuntimeError) << delay;
+  }
+}
+
 TEST(Substitution, ScriptedReturnStatusFromJson)
 {
   static const char* xml_text = R"(