validate ndarray element types in write_bjdata_ndarray (#5301)

* validate ndarray element types in write_bjdata_ndarray

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

* read ndarray elements through get<> instead of a fixed union member

_ArrayType_ names the wire type, not how the value is stored: parsing
keeps a non-negative integer as number_unsigned while the C++ API keeps
an int literal as number_integer. Selecting the union member from the
type marker therefore reads the inactive alternative for one of the two,
so read through get<> instead, which dispatches on the active member.

Also reject a negative _ArraySize_ entry, which is not a usable
dimension, and cover the parse-built path in the tests.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

---------

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

include/nlohmann/detail/output/binary_writer.hpp

@@ -1662,7 +1662,15 @@ class binary_writer
         std::size_t len = (value.at(key).empty() ? 0 : 1);
         for (const auto& el : value.at(key))
         {
-            len *= static_cast<std::size_t>(el.m_data.m_value.number_unsigned);
+            // a dimension is read as an unsigned value below, so anything that
+            // is not a non-negative integer is rejected: a non-integer entry
+            // would pun unrelated bytes as the dimension, and a negative one
+            // would wrap into a nonsensical length
+            if (!el.is_number_integer() || (!el.is_number_unsigned() && el.template get<std::int64_t>() < 0))
+            {
+                return true;
+            }
+            len *= static_cast<std::size_t>(el.template get<std::uint64_t>());
         }
 
         key = "_ArrayData_";
@@ -1671,6 +1679,24 @@ class binary_writer
             return true;
         }
 
+        // every element is written below as the number kind dtype names, so it
+        // has to actually be a number of that category: an element of any other
+        // type would reinterpret unrelated bytes, e.g. a string's heap pointer,
+        // as that number. Such an object falls back to a plain object encoding.
+        // dtype names the wire type, not the storage type: whether an integer
+        // is held as number_integer or number_unsigned depends on how the value
+        // was built (parsing stores non-negative integers as unsigned, the C++
+        // API stores int literals as signed), so both are accepted here and the
+        // writes below go through get<>, which reads the member that is active.
+        const bool ndarray_is_float = (dtype == 'd' || dtype == 'D');
+        for (const auto& el : value.at(key))
+        {
+            if (ndarray_is_float ? !el.is_number_float() : !el.is_number_integer())
+            {
+                return true;
+            }
+        }
+
         oa->write_character('[');
         oa->write_character('$');
         oa->write_character(dtype);
@@ -1684,70 +1710,70 @@ class binary_writer
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint8_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint8_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'i')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int8_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int8_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'u')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint16_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint16_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'I')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int16_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int16_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'm')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint32_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint32_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'l')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int32_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int32_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'M')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint64_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(el.template get<std::uint64_t>(), true);
             }
         }
         else if (dtype == 'L')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int64_t>(el.m_data.m_value.number_integer), true);
+                write_number(el.template get<std::int64_t>(), true);
             }
         }
         else if (dtype == 'd')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<float>(el.m_data.m_value.number_float), true);
+                write_number(static_cast<float>(el.template get<double>()), true);
             }
         }
         else if (dtype == 'D')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<double>(el.m_data.m_value.number_float), true);
+                write_number(el.template get<double>(), true);
             }
         }
         return false;

single_include/nlohmann/json.hpp

@@ -18457,7 +18457,15 @@ class binary_writer
         std::size_t len = (value.at(key).empty() ? 0 : 1);
         for (const auto& el : value.at(key))
         {
-            len *= static_cast<std::size_t>(el.m_data.m_value.number_unsigned);
+            // a dimension is read as an unsigned value below, so anything that
+            // is not a non-negative integer is rejected: a non-integer entry
+            // would pun unrelated bytes as the dimension, and a negative one
+            // would wrap into a nonsensical length
+            if (!el.is_number_integer() || (!el.is_number_unsigned() && el.template get<std::int64_t>() < 0))
+            {
+                return true;
+            }
+            len *= static_cast<std::size_t>(el.template get<std::uint64_t>());
         }
 
         key = "_ArrayData_";
@@ -18466,6 +18474,24 @@ class binary_writer
             return true;
         }
 
+        // every element is written below as the number kind dtype names, so it
+        // has to actually be a number of that category: an element of any other
+        // type would reinterpret unrelated bytes, e.g. a string's heap pointer,
+        // as that number. Such an object falls back to a plain object encoding.
+        // dtype names the wire type, not the storage type: whether an integer
+        // is held as number_integer or number_unsigned depends on how the value
+        // was built (parsing stores non-negative integers as unsigned, the C++
+        // API stores int literals as signed), so both are accepted here and the
+        // writes below go through get<>, which reads the member that is active.
+        const bool ndarray_is_float = (dtype == 'd' || dtype == 'D');
+        for (const auto& el : value.at(key))
+        {
+            if (ndarray_is_float ? !el.is_number_float() : !el.is_number_integer())
+            {
+                return true;
+            }
+        }
+
         oa->write_character('[');
         oa->write_character('$');
         oa->write_character(dtype);
@@ -18479,70 +18505,70 @@ class binary_writer
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint8_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint8_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'i')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int8_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int8_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'u')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint16_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint16_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'I')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int16_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int16_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'm')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint32_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(static_cast<std::uint32_t>(el.template get<std::uint64_t>()), true);
             }
         }
         else if (dtype == 'l')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int32_t>(el.m_data.m_value.number_integer), true);
+                write_number(static_cast<std::int32_t>(el.template get<std::int64_t>()), true);
             }
         }
         else if (dtype == 'M')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::uint64_t>(el.m_data.m_value.number_unsigned), true);
+                write_number(el.template get<std::uint64_t>(), true);
             }
         }
         else if (dtype == 'L')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<std::int64_t>(el.m_data.m_value.number_integer), true);
+                write_number(el.template get<std::int64_t>(), true);
             }
         }
         else if (dtype == 'd')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<float>(el.m_data.m_value.number_float), true);
+                write_number(static_cast<float>(el.template get<double>()), true);
             }
         }
         else if (dtype == 'D')
         {
             for (const auto& el : value.at(key))
             {
-                write_number(static_cast<double>(el.m_data.m_value.number_float), true);
+                write_number(el.template get<double>(), true);
             }
         }
         return false;

tests/src/unit-bjdata.cpp

@@ -2587,6 +2587,69 @@ TEST_CASE("BJData")
                 CHECK(json::to_bjdata(json::from_bjdata(v_B), true, true) == v_B);
             }
 
+            SECTION("ndarray with data not matching _ArrayType_ is written as an object")
+            {
+                // A JData-annotated object is only serialized as an ndarray when
+                // its _ArrayData_ elements are actually stored as the number kind
+                // named by _ArrayType_. Otherwise the writer would read the wrong
+                // union member (e.g. a std::string's heap pointer as a uint64) and
+                // emit it, so such an object falls back to a plain object encoding
+                // that still round-trips.
+
+                // string data declared as a uint64 array
+                json const j_str = json({{"_ArrayType_", "uint64"}, {"_ArraySize_", {1}}, {"_ArrayData_", {"pointer"}}});
+                const auto out_str = json::to_bjdata(j_str);
+                CHECK(out_str.at(0) == '{');
+                CHECK(json::from_bjdata(out_str) == j_str);
+
+                // integer data declared as a double array
+                json const j_float = json({{"_ArrayType_", "double"}, {"_ArraySize_", {2}}, {"_ArrayData_", {1, 2}}});
+                const auto out_float = json::to_bjdata(j_float);
+                CHECK(out_float.at(0) == '{');
+                CHECK(json::from_bjdata(out_float) == j_float);
+
+                // a non-integer shape entry is likewise not treated as an ndarray
+                json const j_size = json({{"_ArrayType_", "uint8"}, {"_ArraySize_", {"x"}}, {"_ArrayData_", {1}}});
+                const auto out_size = json::to_bjdata(j_size);
+                CHECK(out_size.at(0) == '{');
+                CHECK(json::from_bjdata(out_size) == j_size);
+
+                // a negative shape entry is not a usable dimension either
+                json const j_neg = json::parse(R"({"_ArrayType_":"uint8","_ArraySize_":[-1],"_ArrayData_":[1]})");
+                const auto out_neg = json::to_bjdata(j_neg);
+                CHECK(out_neg.at(0) == '{');
+                CHECK(json::from_bjdata(out_neg) == j_neg);
+            }
+
+            SECTION("ndarray parsed from text is written as a typed array")
+            {
+                // json::parse stores a non-negative integer as number_unsigned while
+                // the C++ API stores an int literal as number_integer, so _ArrayType_
+                // names the wire type rather than the storage. Both storages have to
+                // produce the same typed array for every type.
+                for (const char* type :
+                        {"uint8", "int8", "uint16", "int16", "uint32", "int32", "uint64", "int64", "char", "byte"
+                        })
+                {
+                    CAPTURE(type);
+                    const std::string text = std::string(R"({"_ArrayType_":")") + type +
+                                             R"(","_ArraySize_":[2,3],"_ArrayData_":[1,2,3,4,5,6]})";
+                    const auto from_text = json::to_bjdata(json::parse(text));
+                    CHECK(from_text.at(0) == '[');
+                    CHECK(from_text == json::to_bjdata(json({{"_ArrayType_", type}, {"_ArraySize_", {2, 3}}, {"_ArrayData_", {1, 2, 3, 4, 5, 6}}})));
+                }
+
+                // negative values under a signed type behave the same way
+                const auto from_neg = json::to_bjdata(json::parse(R"({"_ArrayType_":"int32","_ArraySize_":[2],"_ArrayData_":[-5,7]})"));
+                CHECK(from_neg.at(0) == '[');
+                CHECK(from_neg == json::to_bjdata(json({{"_ArrayType_", "int32"}, {"_ArraySize_", {2}}, {"_ArrayData_", {-5, 7}}})));
+
+                // and so do the floating point types
+                const auto from_float = json::to_bjdata(json::parse(R"({"_ArrayType_":"double","_ArraySize_":[2],"_ArrayData_":[1.5,2.5]})"));
+                CHECK(from_float.at(0) == '[');
+                CHECK(from_float == json::to_bjdata(json({{"_ArrayType_", "double"}, {"_ArraySize_", {2}}, {"_ArrayData_", {1.5, 2.5}}})));
+            }
+
             SECTION("optimized ndarray (type and vector-size as 1D array)")
             {
                 // create vector with two elements of the same type