Keep converted object keys alive while writing UBJSON and BJData (#5791)

* Keep converted object keys alive while writing UBJSON and BJData

Since #5746, write_ubjson and write_ubjson_iterative pass each object key
to sanitize_utf8_for_write and keep the returned reference. When
object_t::key_type is not string_t but converts to it, the argument is a
temporary that is destroyed at the end of the statement, and the
function returns a reference to it in every case but a sanitized copy,
so the key bytes are read from a dead object (AddressSanitizer:
stack-use-after-scope). Default json and ordered_json are unaffected.

Bind the key to a named object_key_string_t first: a reference when
key_type is string_t, so no copy is added there, and a converted copy
otherwise. A deleted overload of sanitize_utf8_for_write for anything
other than string_t turns a recurrence into a compile error.

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

* Suppress -Wunused-member-function for the converting_key test type

converting_key::data() is only called when JSON_DIAGNOSTICS is enabled.

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

* Fix clang-tidy findings in the UBJSON/BJData converted-key fix

Suppress hicpp/modernize-use-equals-delete on the deleted
sanitize_utf8_for_write overload: it guards a private helper and must stay
private. Replace the C-style array in the new test with std::array.

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

---------

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

include/nlohmann/detail/output/binary_writer.hpp

@@ -85,6 +85,12 @@ template<typename BasicJsonType, typename CharType, typename OutputSinkType = ou
 class binary_writer
 {
     using string_t = typename BasicJsonType::string_t;
+
+    /// an object key as string_t: a reference when object_t::key_type already is
+    /// string_t, otherwise a converted copy that outlives sanitize_utf8_for_write's result
+    using object_key_string_t = typename std::conditional <
+                                std::is_same<typename BasicJsonType::object_t::key_type, string_t>::value,
+                                const string_t&, string_t >::type;
     using binary_t = typename BasicJsonType::binary_t;
     using number_float_t = typename BasicJsonType::number_float_t;
 
@@ -776,8 +782,10 @@ class binary_writer
 
                 for (const auto& el : *j.m_data.m_value.object)
                 {
+                    // a converted key must outlive the reference returned by sanitize_utf8_for_write
+                    const object_key_string_t key_string = el.first;
                     string_t storage;
-                    const string_t& key = sanitize_utf8_for_write(el.first, j, storage);
+                    const string_t& key = sanitize_utf8_for_write(key_string, j, storage);
                     write_number_with_ubjson_prefix(key.size(), true, use_bjdata);
                     oa.write_characters(
                           reinterpret_cast<const CharType*>(key.data()),
@@ -1317,8 +1325,10 @@ class binary_writer
                     continue;
                 }
 
+                // a converted key must outlive the reference returned by sanitize_utf8_for_write
+                const object_key_string_t key_string = current.object_it->first;
                 string_t storage;
-                const string_t& key = sanitize_utf8_for_write(current.object_it->first, j, storage);
+                const string_t& key = sanitize_utf8_for_write(key_string, j, storage);
                 write_number_with_ubjson_prefix(key.size(), true, use_bjdata);
                 oa.write_characters(
                       reinterpret_cast<const CharType*>(key.data()),
@@ -2760,6 +2770,11 @@ class binary_writer
     itself in every case but a sanitized `replace`/`ignore` one, so @a
     storage must outlive the returned reference only then.
 
+    @a s must be an lvalue that outlives the returned reference. An object key
+    whose `key_type` is not @ref string_t must therefore first be converted
+    into a named string_t (see @ref object_key_string_t); the deleted overload
+    below enforces this at compile time.
+
     @param[in] s        the string (value or object key) to write
     @param[in] context  the value @a s belongs to (for diagnostics)
     @param[out] storage  backing storage for a sanitized copy
@@ -2789,6 +2804,10 @@ class binary_writer
         }
     }
 
+    /// deleted: anything but a string_t would bind a temporary that dies before the returned reference is used
+    template < typename T, enable_if_t < !std::is_same<T, string_t>::value, int > = 0 >
+    const string_t& sanitize_utf8_for_write(const T& /*s*/, const BasicJsonType& /*context*/, string_t& /*storage*/) const = delete; // NOLINT(hicpp-use-equals-delete,modernize-use-equals-delete): a private helper's guard, not part of the interface
+
     /*!
     @brief write an integer in the shortest encoding
 

single_include/nlohmann/json.hpp

@@ -21590,6 +21590,12 @@ template<typename BasicJsonType, typename CharType, typename OutputSinkType = ou
 class binary_writer
 {
     using string_t = typename BasicJsonType::string_t;
+
+    /// an object key as string_t: a reference when object_t::key_type already is
+    /// string_t, otherwise a converted copy that outlives sanitize_utf8_for_write's result
+    using object_key_string_t = typename std::conditional <
+                                std::is_same<typename BasicJsonType::object_t::key_type, string_t>::value,
+                                const string_t&, string_t >::type;
     using binary_t = typename BasicJsonType::binary_t;
     using number_float_t = typename BasicJsonType::number_float_t;
 
@@ -22281,8 +22287,10 @@ class binary_writer
 
                 for (const auto& el : *j.m_data.m_value.object)
                 {
+                    // a converted key must outlive the reference returned by sanitize_utf8_for_write
+                    const object_key_string_t key_string = el.first;
                     string_t storage;
-                    const string_t& key = sanitize_utf8_for_write(el.first, j, storage);
+                    const string_t& key = sanitize_utf8_for_write(key_string, j, storage);
                     write_number_with_ubjson_prefix(key.size(), true, use_bjdata);
                     oa.write_characters(
                           reinterpret_cast<const CharType*>(key.data()),
@@ -22822,8 +22830,10 @@ class binary_writer
                     continue;
                 }
 
+                // a converted key must outlive the reference returned by sanitize_utf8_for_write
+                const object_key_string_t key_string = current.object_it->first;
                 string_t storage;
-                const string_t& key = sanitize_utf8_for_write(current.object_it->first, j, storage);
+                const string_t& key = sanitize_utf8_for_write(key_string, j, storage);
                 write_number_with_ubjson_prefix(key.size(), true, use_bjdata);
                 oa.write_characters(
                       reinterpret_cast<const CharType*>(key.data()),
@@ -24265,6 +24275,11 @@ class binary_writer
     itself in every case but a sanitized `replace`/`ignore` one, so @a
     storage must outlive the returned reference only then.
 
+    @a s must be an lvalue that outlives the returned reference. An object key
+    whose `key_type` is not @ref string_t must therefore first be converted
+    into a named string_t (see @ref object_key_string_t); the deleted overload
+    below enforces this at compile time.
+
     @param[in] s        the string (value or object key) to write
     @param[in] context  the value @a s belongs to (for diagnostics)
     @param[out] storage  backing storage for a sanitized copy
@@ -24294,6 +24309,10 @@ class binary_writer
         }
     }
 
+    /// deleted: anything but a string_t would bind a temporary that dies before the returned reference is used
+    template < typename T, enable_if_t < !std::is_same<T, string_t>::value, int > = 0 >
+    const string_t& sanitize_utf8_for_write(const T& /*s*/, const BasicJsonType& /*context*/, string_t& /*storage*/) const = delete; // NOLINT(hicpp-use-equals-delete,modernize-use-equals-delete): a private helper's guard, not part of the interface
+
     /*!
     @brief write an integer in the shortest encoding
 

tests/src/unit-binary_utf8_error_handler.cpp

@@ -12,7 +12,11 @@
 #include <nlohmann/json.hpp>
 using nlohmann::json;
 
+#include <array>
+#include <map>
+#include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace
@@ -55,6 +59,53 @@ std::string dump_and_parse(const std::string& raw, eh error_handler)
     return json::parse(json(raw).dump(-1, ' ', false, error_handler)).get<std::string>();
 }
 
+// an object key type that is not string_t, but converts implicitly to it;
+// data() is only used when JSON_DIAGNOSTICS is enabled
+DOCTEST_CLANG_SUPPRESS_WARNING_PUSH
+DOCTEST_CLANG_SUPPRESS_WARNING("-Wunused-member-function")
+class converting_key
+{
+  public:
+    converting_key(const char* s) : m_value(s) {} // NOLINT(google-explicit-constructor,hicpp-explicit-conversions)
+    converting_key(std::string s) : m_value(std::move(s)) {} // NOLINT(google-explicit-constructor,hicpp-explicit-conversions)
+
+    // the conversion yields a temporary string_t
+    operator std::string() const // NOLINT(google-explicit-constructor,hicpp-explicit-conversions)
+    {
+        return m_value;
+    }
+
+    // read by the exception messages when JSON_DIAGNOSTICS is enabled
+    const char* data() const noexcept
+    {
+        return m_value.data();
+    }
+
+    friend bool operator<(const converting_key& lhs, const converting_key& rhs)
+    {
+        return lhs.m_value < rhs.m_value;
+    }
+
+  private:
+    std::string m_value;
+};
+DOCTEST_CLANG_SUPPRESS_WARNING_POP
+
+// ObjectType using converting_key; the Key template argument is ignored
+template<typename Key, typename Value, typename Compare, typename Allocator>
+class converting_key_object : public std::map<converting_key, Value, std::less<converting_key>, // NOLINT(modernize-use-transparent-functors)
+    typename std::allocator_traits<Allocator>::template rebind_alloc<std::pair<const converting_key, Value>>>
+{
+    using base_type = std::map<converting_key, Value, std::less<converting_key>, // NOLINT(modernize-use-transparent-functors)
+                               typename std::allocator_traits<Allocator>::template rebind_alloc<std::pair<const converting_key, Value>>>;
+
+                             public:
+                               using base_type::base_type;
+                               using base_type::operator=;
+};
+
+using converting_key_json = nlohmann::basic_json<converting_key_object>;
+
 } // namespace
 
 TEST_CASE("UTF-8 error_handler for the binary readers and writers")
@@ -370,3 +421,109 @@ TEST_CASE("UTF-8 error_handler for the binary readers and writers")
         CHECK(json::from_bson(bson_bytes)["k"].get<std::string>() == ill_formed_cases()[0].bytes);
     }
 }
+
+// The UBJSON and BJData writers bind the (possibly sanitized) key to a const
+// string_t&. If key_type is not string_t but converts to it, the converted
+// temporary must outlive that reference; this was a use-after-scope found by
+// AddressSanitizer. Keys exceed the small string optimization on purpose.
+TEST_CASE("UBJSON and BJData writers with an object_t whose key_type is not string_t")
+{
+    const std::string long_prefix(70, 'k');
+
+    SECTION("well-formed keys, every error_handler")
+    {
+        const std::string key1 = long_prefix + "-first";
+        const std::string key2 = long_prefix + "-second";
+
+        converting_key_json::object_t o;
+        o.emplace(converting_key(key1), 1);
+        o.emplace(converting_key(key2), "value");
+        const converting_key_json v(std::move(o));
+
+        json expected;
+        expected[key1] = 1;
+        expected[key2] = "value";
+
+        const std::array<std::pair<bool, bool>, 3> combos = {{{false, false}, {true, false}, {true, true}}};
+        for (const auto h : all_handlers())
+        {
+            CAPTURE(static_cast<int>(h))
+            for (const auto& combo : combos)
+            {
+                const bool use_count = combo.first;
+                const bool use_type = combo.second;
+                CAPTURE(use_count)
+                CAPTURE(use_type)
+
+                CHECK(json::from_ubjson(converting_key_json::to_ubjson(v, use_count, use_type, h)) == expected);
+                CHECK(json::from_bjdata(converting_key_json::to_bjdata(v, use_count, use_type, json::bjdata_version_t::draft2, h)) == expected);
+                CHECK(json::from_bjdata(converting_key_json::to_bjdata(v, use_count, use_type, json::bjdata_version_t::draft3, h)) == expected);
+            }
+        }
+    }
+
+    SECTION("ill-formed keys")
+    {
+        for (const auto& c : ill_formed_cases())
+        {
+            CAPTURE(c.name)
+            const std::string key = long_prefix + c.bytes;
+
+            converting_key_json::object_t o;
+            o.emplace(converting_key(key), 1);
+            const converting_key_json v(std::move(o));
+
+            CHECK_THROWS_AS(converting_key_json::to_ubjson(v, false, false, eh::strict), converting_key_json::type_error&);
+            CHECK_THROWS_AS(converting_key_json::to_bjdata(v, false, false, json::bjdata_version_t::draft2, eh::strict), converting_key_json::type_error&);
+
+            for (const auto h :
+                    {
+                        eh::replace, eh::ignore
+                    })
+            {
+                CAPTURE(static_cast<int>(h))
+                const std::string expected = dump_and_parse(key, h);
+
+                CHECK(json::from_ubjson(converting_key_json::to_ubjson(v, false, false, h)).begin().key() == expected);
+                CHECK(json::from_bjdata(converting_key_json::to_bjdata(v, false, false, json::bjdata_version_t::draft2, h)).begin().key() == expected);
+            }
+
+            CHECK(json::from_ubjson(converting_key_json::to_ubjson(v, false, false, eh::keep)).begin().key() == key);
+            CHECK(json::from_bjdata(converting_key_json::to_bjdata(v, false, false, json::bjdata_version_t::draft2, eh::keep)).begin().key() == key);
+        }
+    }
+
+    SECTION("nested deeper than the recursion limit")
+    {
+        // wrap the previous value, innermost first
+        converting_key_json v = 42;
+        json expected = 42;
+        for (int i = 199; i >= 0; --i)
+        {
+            const std::string key = "level-" + std::to_string(i) + "-" + std::string(64, 'x');
+
+            converting_key_json::object_t o;
+            o.emplace(converting_key(key), std::move(v));
+            v = converting_key_json(std::move(o));
+
+            json e;
+            e[key] = std::move(expected);
+            expected = std::move(e);
+        }
+
+        for (const auto h : all_handlers())
+        {
+            CAPTURE(static_cast<int>(h))
+            for (const bool use_count :
+                    {
+                        false, true
+                    })
+            {
+                CAPTURE(use_count)
+
+                CHECK(json::from_ubjson(converting_key_json::to_ubjson(v, use_count, false, h)) == expected);
+                CHECK(json::from_bjdata(converting_key_json::to_bjdata(v, use_count, false, json::bjdata_version_t::draft2, h)) == expected);
+            }
+        }
+    }
+}