From 374dfe4f0f3f259b1ccaab879a4ab706dbf97e4c Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Fri, 9 Oct 2026 17:05:36 +0200 Subject: [PATCH] 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 * 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 * 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 --------- Signed-off-by: Niels Lohmann --- .../nlohmann/detail/output/binary_writer.hpp | 23 ++- single_include/nlohmann/json.hpp | 23 ++- tests/src/unit-binary_utf8_error_handler.cpp | 157 ++++++++++++++++++ 3 files changed, 199 insertions(+), 4 deletions(-) diff --git a/include/nlohmann/detail/output/binary_writer.hpp b/include/nlohmann/detail/output/binary_writer.hpp index 4af3ff9fb..212d2658f 100644 --- a/include/nlohmann/detail/output/binary_writer.hpp +++ b/include/nlohmann/detail/output/binary_writer.hpp @@ -85,6 +85,12 @@ template::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(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(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::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 diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 428a6abbf..2e6838bbc 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -21590,6 +21590,12 @@ template::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(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(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::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 diff --git a/tests/src/unit-binary_utf8_error_handler.cpp b/tests/src/unit-binary_utf8_error_handler.cpp index c3f2435fd..114c92885 100644 --- a/tests/src/unit-binary_utf8_error_handler.cpp +++ b/tests/src/unit-binary_utf8_error_handler.cpp @@ -12,7 +12,11 @@ #include using nlohmann::json; +#include +#include +#include #include +#include #include 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(); } +// 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 +class converting_key_object : public std::map, // NOLINT(modernize-use-transparent-functors) + typename std::allocator_traits::template rebind_alloc>> +{ + using base_type = std::map, // NOLINT(modernize-use-transparent-functors) + typename std::allocator_traits::template rebind_alloc>>; + + public: + using base_type::base_type; + using base_type::operator=; +}; + +using converting_key_json = nlohmann::basic_json; + } // 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() == 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, 3> combos = {{{false, false}, {true, false}, {true, true}}}; + for (const auto h : all_handlers()) + { + CAPTURE(static_cast(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(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(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); + } + } + } +}