Add deprecated from_bon8/from_bjdata(ptr, len) overloads

from_bon8(ptr, len) and from_bjdata(ptr, len) had no overload for a
pointer and a length, unlike from_cbor/from_msgpack/from_ubjson/
from_bson. The call instead bound to from_*(InputType&&, bool strict),
which read ptr as a NUL-terminated C string via strlen and silently
converted len to the strict flag. Data containing a 0x00 byte was cut
off there; data without one was read past the end of the buffer.

Add a deprecated (ptr, len, strict, allow_exceptions) overload for
each function that forwards to (ptr, ptr + len, ...), matching the
existing deprecated overloads of the other four binary readers. Since
neither function ever had this overload, the deprecation is declared
as of version 3.13.0, the next unreleased version, rather than the
version each function was originally added in.

Fixes #5648.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

.clang-tidy

@@ -1,9 +1,11 @@
-# TODO: portability-template-virtual-member-function is only removed to get the CI going. It has to be addressed at some point.
+# TODO: The first three checks are only removed to get the CI going. They have to be addressed at some point.
 # TODO: portability-avoid-pragma-once: should be fixed eventually
 
 Checks: '*,
 
          -portability-template-virtual-member-function,
+         -bugprone-use-after-move,
+         -hicpp-invalid-access-moved,
 
          -altera-id-dependent-backward-branch,
          -altera-struct-pack-align,

docs/mkdocs/docs/api/basic_json/from_bjdata.md

@@ -111,3 +111,12 @@ Linear in the size of the input.
 - Added in version 3.11.0.
 - Extended container support (1) to include types with lvalue-only ADL `begin`/`end` (matching `std::begin`/`std::end` semantics) in version 3.13.0.
 - Extended overload (2) to accept heterogeneous iterator+sentinel pairs (C++20 ranges support) in version 3.13.0.
+
+!!! warning "Deprecation"
+
+    - Overload (2) replaces calls to `from_bjdata` with a pointer and a length as first two parameters, which has been
+      deprecated in version 3.13.0. This overload will be removed in version 4.0.0. Please replace all calls like
+      `#!cpp from_bjdata(ptr, len, ...);` with `#!cpp from_bjdata(ptr, ptr+len, ...);`.
+
+    You should be warned by your compiler with a `-Wdeprecated-declarations` warning if you are using a deprecated
+    function.

docs/mkdocs/docs/api/basic_json/from_bon8.md

@@ -106,3 +106,12 @@ Linear in the size of the input.
 ## Version history
 
 - Added in version 3.13.0.
+
+!!! warning "Deprecation"
+
+    - Overload (2) replaces calls to `from_bon8` with a pointer and a length as first two parameters, which has been
+      deprecated in version 3.13.0. This overload will be removed in version 4.0.0. Please replace all calls like
+      `#!cpp from_bon8(ptr, len, ...);` with `#!cpp from_bon8(ptr, ptr+len, ...);`.
+
+    You should be warned by your compiler with a `-Wdeprecated-declarations` warning if you are using a deprecated
+    function.

docs/mkdocs/docs/api/basic_json/meta.md

@@ -13,7 +13,7 @@ JSON object holding version information
 
 | key         | description                                                                                                                                                                                                                                                                    |
 |-------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `compiler`  | Information on the used compiler. It is an object with the following keys: `c++` (the used C++ standard), `family` (the compiler family; possible values are `clang`, `icc`, `gcc`, `hp`, `ilecpp`, `msvc`, `pgcpp`, `sunpro`, and `unknown`), and `version` (the compiler version). |
+| `compiler`  | Information on the used compiler. It is an object with the following keys: `c++` (the used C++ standard), `family` (the compiler family; possible values are `clang`, `icc`, `gcc`, `ilecpp`, `msvc`, `pgcpp`, `sunpro`, and `unknown`), and `version` (the compiler version). On HP aCC compilers, `compiler` is instead the plain string `hp`. |
 | `copyright` | The copyright line for the library as string.                                                                                                                                                                                                                                  |
 | `name`      | The name of the library as string.                                                                                                                                                                                                                                             |
 | `platform`  | The used platform as string. Possible values are `win32`, `linux`, `apple`, `unix`, and `unknown`.                                                                                                                                                                             |

include/nlohmann/detail/conversions/from_json.hpp

@@ -353,7 +353,7 @@ template < typename BasicJsonType, typename T, std::size_t... Idx >
 std::array<T, sizeof...(Idx)> from_json_inplace_array_impl(BasicJsonType&& j,
                      identity_tag<std::array<T, sizeof...(Idx)>> /*unused*/, index_sequence<Idx...> /*unused*/)
 {
-    return { { j.at(Idx).template get<T>()... } };
+    return { { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... } };
 }
 
 template < typename BasicJsonType, typename T, std::size_t N >
@@ -502,7 +502,7 @@ using tuple_type = std::tuple < decltype(from_json_tuple_get_impl(std::declval<B
 template<std::size_t PTagValue, typename... Args, typename BasicJsonType, std::size_t... Idx>
 tuple_type<PTagValue, BasicJsonType, Args...> from_json_tuple_impl_base(BasicJsonType&& j, index_sequence<Idx...> /*unused*/)
 {
-    return tuple_type<PTagValue, BasicJsonType, Args...>(from_json_tuple_get_impl(j.at(Idx), detail::identity_tag<Args> {}, detail::priority_tag<PTagValue> {})...);
+    return tuple_type<PTagValue, BasicJsonType, Args...>(from_json_tuple_get_impl(std::forward<BasicJsonType>(j).at(Idx), detail::identity_tag<Args> {}, detail::priority_tag<PTagValue> {})...);
 }
 
 template<std::size_t PTagValue, typename BasicJsonType>
@@ -514,8 +514,8 @@ std::tuple<> from_json_tuple_impl_base(BasicJsonType& /*unused*/, index_sequence
 template < typename BasicJsonType, class A1, class A2 >
 std::pair<A1, A2> from_json_tuple_impl(BasicJsonType&& j, identity_tag<std::pair<A1, A2>> /*unused*/, priority_tag<0> /*unused*/)
 {
-    return {j.at(0).template get<A1>(),
-            j.at(1).template get<A2>()};
+    return {std::forward<BasicJsonType>(j).at(0).template get<A1>(),
+            std::forward<BasicJsonType>(j).at(1).template get<A2>()};
 }
 
 template<typename BasicJsonType, typename A1, typename A2>

include/nlohmann/detail/input/input_adapters.hpp

@@ -763,7 +763,6 @@ struct container_input_adapter_factory< ContainerType,
 
            static adapter_type create(ContainerType&& container)
 {
-    // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved) forwarded twice on purpose, so begin() and end() see the same value category and yield matching iterator types
     return input_adapter(begin(std::forward<ContainerType>(container)), end(std::forward<ContainerType>(container)));
 }
        };

include/nlohmann/detail/string_concat.hpp

@@ -39,6 +39,7 @@ inline std::size_t concat_length(const char /*c*/, const Args& ... rest)
 template<typename... Args>
 inline std::size_t concat_length(const char* cstr, const Args& ... rest)
 {
+    // cppcheck-suppress ignoredReturnValue
     return ::strlen(cstr) + concat_length(rest...);
 }
 

include/nlohmann/ordered_map.hpp

@@ -70,41 +70,14 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
         return *this;
     }
 
-private:
-    /// @brief find the entry for @a key, for either constness of @a self
-    /// @note the single place that performs the linear key search
-    template<typename Self, typename KeyType>
-    static auto find_impl(Self& self, KeyType&& key) -> decltype(self.begin())
-    {
-        for (auto it = self.begin(); it != self.end(); ++it)
-        {
-            if (self.m_compare(it->first, key))
-            {
-                return it;
-            }
-        }
-        return self.end();
-    }
-
-    /// @brief remove the entry @a it points to, preserving order
-    /// @note keys are not movable, so the tail is destroyed and re-constructed in place
-    void erase_at(iterator it)
-    {
-        for (auto next = it; ++next != this->end(); ++it)
-        {
-            it->~value_type(); // Destroy but keep allocation
-            new (&*it) value_type{std::move(*next)};
-        }
-        Container::pop_back();
-    }
-
-public:
     std::pair<iterator, bool> emplace(const key_type& key, T&& t)
     {
-        const auto it = find_impl(*this, key);
-        if (it != this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            return {it, false};
+            if (m_compare(it->first, key))
+            {
+                return {it, false};
+            }
         }
         Container::emplace_back(key, std::forward<T>(t));
         return {std::prev(this->end()), true};
@@ -114,10 +87,12 @@ public:
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     std::pair<iterator, bool> emplace(KeyType && key, T && t)
     {
-        const auto it = find_impl(*this, key);
-        if (it != this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            return {it, false};
+            if (m_compare(it->first, key))
+            {
+                return {it, false};
+            }
         }
         Container::emplace_back(std::forward<KeyType>(key), std::forward<T>(t));
         return {std::prev(this->end()), true};
@@ -149,55 +124,75 @@ public:
 
     T& at(const key_type& key)
     {
-        const auto it = find_impl(*this, key);
-        if (it == this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            JSON_THROW(std::out_of_range("key not found"));
+            if (m_compare(it->first, key))
+            {
+                return it->second;
+            }
         }
-        return it->second;
+
+        JSON_THROW(std::out_of_range("key not found"));
     }
 
     template<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     T & at(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        const auto it = find_impl(*this, key);
-        if (it == this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            JSON_THROW(std::out_of_range("key not found"));
+            if (m_compare(it->first, key))
+            {
+                return it->second;
+            }
         }
-        return it->second;
+
+        JSON_THROW(std::out_of_range("key not found"));
     }
 
     const T& at(const key_type& key) const
     {
-        const auto it = find_impl(*this, key);
-        if (it == this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            JSON_THROW(std::out_of_range("key not found"));
+            if (m_compare(it->first, key))
+            {
+                return it->second;
+            }
         }
-        return it->second;
+
+        JSON_THROW(std::out_of_range("key not found"));
     }
 
     template<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     const T & at(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        const auto it = find_impl(*this, key);
-        if (it == this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            JSON_THROW(std::out_of_range("key not found"));
+            if (m_compare(it->first, key))
+            {
+                return it->second;
+            }
         }
-        return it->second;
+
+        JSON_THROW(std::out_of_range("key not found"));
     }
 
     size_type erase(const key_type& key)
     {
-        const auto it = find_impl(*this, key);
-        if (it != this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            erase_at(it);
-            return 1;
+            if (m_compare(it->first, key))
+            {
+                // Since we cannot move const Keys, re-construct them in place
+                for (auto next = it; ++next != this->end(); ++it)
+                {
+                    it->~value_type(); // Destroy but keep allocation
+                    new (&*it) value_type{std::move(*next)};
+                }
+                Container::pop_back();
+                return 1;
+            }
         }
         return 0;
     }
@@ -206,11 +201,19 @@ public:
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     size_type erase(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        const auto it = find_impl(*this, key);
-        if (it != this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            erase_at(it);
-            return 1;
+            if (m_compare(it->first, key))
+            {
+                // Since we cannot move const Keys, re-construct them in place
+                for (auto next = it; ++next != this->end(); ++it)
+                {
+                    it->~value_type(); // Destroy but keep allocation
+                    new (&*it) value_type{std::move(*next)};
+                }
+                Container::pop_back();
+                return 1;
+            }
         }
         return 0;
     }
@@ -275,38 +278,80 @@ public:
 
     size_type count(const key_type& key) const
     {
-        return find_impl(*this, key) != this->end() ? 1 : 0;
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return 1;
+            }
+        }
+        return 0;
     }
 
     template<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     size_type count(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        return find_impl(*this, key) != this->end() ? 1 : 0;
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return 1;
+            }
+        }
+        return 0;
     }
 
     iterator find(const key_type& key)
     {
-        return find_impl(*this, key);
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return it;
+            }
+        }
+        return Container::end();
     }
 
     template<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     iterator find(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        return find_impl(*this, key);
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return it;
+            }
+        }
+        return Container::end();
     }
 
     const_iterator find(const key_type& key) const
     {
-        return find_impl(*this, key);
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return it;
+            }
+        }
+        return Container::end();
     }
 
     template<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
     const_iterator find(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward)
     {
-        return find_impl(*this, key);
+        for (auto it = this->begin(); it != this->end(); ++it)
+        {
+            if (m_compare(it->first, key))
+            {
+                return it;
+            }
+        }
+        return Container::end();
     }
 
     std::pair<iterator, bool> insert( value_type&& value )
@@ -316,10 +361,12 @@ public:
 
     std::pair<iterator, bool> insert( const value_type& value )
     {
-        const auto it = find_impl(*this, value.first);
-        if (it != this->end())
+        for (auto it = this->begin(); it != this->end(); ++it)
         {
-            return {it, false};
+            if (m_compare(it->first, value.first))
+            {
+                return {it, false};
+            }
         }
         Container::push_back(value);
         return {--this->end(), true};

tests/src/unit-bjdata.cpp

@@ -4572,3 +4572,31 @@ TEST_CASE("BJData roundtrips" * doctest::skip())
         }
     }
 }
+
+TEST_CASE("issue #5648 - from_bjdata(ptr, len) must read len bytes, not treat ptr as a C string")
+{
+    // to_bjdata() encodes the integer 0 as the two bytes 'i' 0x00 (a BJData
+    // type marker followed by the value byte 0x00), so the packed data
+    // below contains a 0x00 byte before its end.
+    const json j = {{"a", 0}};
+    const std::vector<std::uint8_t> packed = json::to_bjdata(j);
+    bool contains_nul = false;
+    for (const auto byte : packed)
+    {
+        contains_nul |= (byte == 0x00);
+    }
+    REQUIRE(contains_nul);
+
+    // before the fix, from_bjdata had no (ptr, len) overload, so this call
+    // bound to from_bjdata(InputType&&, bool strict) instead: ptr was read
+    // as a NUL-terminated C string (stopping at the embedded 0x00 byte), and
+    // len was silently converted to the strict flag. The deprecated
+    // overload added for this issue forwards to from_bjdata(ptr, ptr + len,
+    // ...) instead, like from_ubjson's deprecated (ptr, len) overload does.
+    json result;
+    CHECK_NOTHROW(result = json::from_bjdata(packed.data(), packed.size()));
+    CHECK(result == j);
+
+    // len must not collapse into the strict flag either
+    CHECK(json::from_bjdata(packed.data(), packed.size(), false) == j);
+}

tests/src/unit-bon8.cpp

@@ -1010,6 +1010,34 @@ TEST_CASE("BON8 roundtrips" * doctest::skip())
     }
 }
 
+TEST_CASE("issue #5648 - from_bon8(ptr, len) must read len bytes, not treat ptr as a C string")
+{
+    // to_bon8() encodes the integer 40 as the two bytes 0xC2 0x00 (a BON8
+    // lead byte followed by a continuation byte of 0x00), so the packed data
+    // below contains a 0x00 byte before its end.
+    const json j = {{"a", 40}, {"b", "x"}};
+    const std::vector<std::uint8_t> packed = json::to_bon8(j);
+    bool contains_nul = false;
+    for (const auto byte : packed)
+    {
+        contains_nul |= (byte == 0x00);
+    }
+    REQUIRE(contains_nul);
+
+    // before the fix, from_bon8 had no (ptr, len) overload, so this call
+    // bound to from_bon8(InputType&&, bool strict) instead: ptr was read as
+    // a NUL-terminated C string (stopping at the embedded 0x00 byte), and
+    // len was silently converted to the strict flag. The deprecated
+    // overload added for this issue forwards to from_bon8(ptr, ptr + len,
+    // ...) instead, like from_cbor's deprecated (ptr, len) overload does.
+    json result;
+    CHECK_NOTHROW(result = json::from_bon8(packed.data(), packed.size()));
+    CHECK(result == j);
+
+    // len must not collapse into the strict flag either
+    CHECK(json::from_bon8(packed.data(), packed.size(), false) == j);
+}
+
 #ifdef JSON_HAS_CPP_17
 TEST_CASE("BON8 with std::byte")
 {

tests/src/unit-class_parser.cpp

@@ -2525,10 +2525,12 @@ TEST_CASE("diagnostic positions: value lifetime, input adapters, and SAX")
 
         SECTION("move constructor resets the moved-from value to npos")
         {
-            // basic_json(basic_json&&) (json.hpp, around line 1944) copies
+            // basic_json(basic_json&&) (json.hpp, around line 1265) copies
             // other's start_position/end_position into *this and then resets
-            // other's to npos. Only the top-level moved-from value is
-            // affected; its (moved-away) children are gone along with it.
+            // other's to npos (see the cppcheck-suppress[accessForwarded]
+            // annotation there, which flags this reset as worth a second
+            // look). Only the top-level moved-from value is affected; its
+            // (moved-away) children are gone along with it.
             const std::string s = R"({"a":1,"b":[1,2,3]})";
             json a = json::parse(s);
             const auto a_start = a.start_pos();

tests/src/unit-modifiers.cpp

@@ -618,49 +618,6 @@ TEST_CASE("modifiers")
             }
         }
 
-        SECTION("rvalue at position moves rather than copies")
-        {
-            // regression test: insert(pos, basic_json&&) used to forward to
-            // insert(pos, const basic_json&) because the named rvalue
-            // reference parameter is itself an lvalue, so it always
-            // deep-copied its argument instead of moving it
-            json j_big = std::string(1000, 'x');
-            const auto* const original_buffer = j_big.get_ref<const std::string&>().data();
-
-            auto it = j_array.insert(j_array.begin(), std::move(j_big));
-            CHECK(j_array.size() == 5);
-            CHECK(*it == json(std::string(1000, 'x')));
-            CHECK((*it).get_ref<const std::string&>().data() == original_buffer);
-
-            // the moved-from value is null, the same as after push_back(&&)
-            CHECK(j_big.is_null()); // NOLINT(bugprone-use-after-move,hicpp-invalid-access-moved)
-        }
-
-        SECTION("self-aliasing insertion")
-        {
-            SECTION("without reallocation")
-            {
-                json j_self = {1, 2, 3, 4};
-                j_self.get_ref<json::array_t&>().reserve(j_self.size() + 1);
-
-                auto it = j_self.insert(j_self.begin(), std::move(j_self[1]));
-                CHECK(j_self.size() == 5);
-                CHECK(*it == json(2));
-                CHECK(j_self == json({2, 1, nullptr, 3, 4}));
-            }
-
-            SECTION("with reallocation")
-            {
-                json j_self = {1, 2, 3, 4};
-                j_self.get_ref<json::array_t&>().shrink_to_fit();
-
-                auto it = j_self.insert(j_self.begin(), std::move(j_self[1]));
-                CHECK(j_self.size() == 5);
-                CHECK(*it == json(2));
-                CHECK(j_self == json({2, 1, nullptr, 3, 4}));
-            }
-        }
-
         SECTION("copies at position")
         {
             SECTION("insert before begin()")