From 21a69230bdaabd05c9b8b1427302f35b11771caf Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Tue, 6 Oct 2026 22:59:34 +0200 Subject: [PATCH] Create a value before giving it its type (#5585) * Create a value before giving it its type Squashed onto develop from: - Create a value before giving it its type - Skip the failed-allocation test when exceptions are disabled - Keep the created pointer rather than an uninitialized json_value - Skip the vector failed-allocation check for VS 2015 with iterator debugging - Test the remaining to_json overloads with a failing allocation - Skip the to_json allocation-failure section on VS 2015 Debug - Fix false GCC -Warray-bounds error with JSON_DIAGNOSTICS at -O3 (#5744) Signed-off-by: Niels Lohmann * Store the new value with a helper in all to_json constructors Every external_constructor<>::construct now creates the new value first and hands it to basic_json::replace_value(), which destroys the old value, stores the new one before setting its type (as elsewhere in this PR), sets the parents, and checks the invariant. The std::vector and std::valarray overloads use array_t's range constructor, which the other array overloads already rely on. Range views keep their loop, as begin() and end() of a view may have different types. Signed-off-by: Niels Lohmann --------- Signed-off-by: Niels Lohmann --- .../nlohmann/detail/conversions/to_json.hpp | 139 +++++--------- include/nlohmann/json.hpp | 29 ++- single_include/nlohmann/json.hpp | 168 +++++++---------- tests/CMakeLists.txt | 5 + tests/src/unit-allocator.cpp | 175 ++++++++++++++++++ tests/src/unit-diagnostics-optimized.cpp | 80 ++++++++ 6 files changed, 392 insertions(+), 204 deletions(-) create mode 100644 tests/src/unit-diagnostics-optimized.cpp diff --git a/include/nlohmann/detail/conversions/to_json.hpp b/include/nlohmann/detail/conversions/to_json.hpp index f3b4994f1..8edaee2fe 100644 --- a/include/nlohmann/detail/conversions/to_json.hpp +++ b/include/nlohmann/detail/conversions/to_json.hpp @@ -13,7 +13,6 @@ #include // optional #endif -#include // copy #include // begin, end #include // allocator_traits #include // basic_string, char_traits @@ -39,10 +38,14 @@ namespace detail ////////////////// /* - * Note all external_constructor<>::construct functions need to call - * j.m_data.m_value.destroy(j.m_data.m_type) to avoid a memory leak in case j contains an - * allocated value (e.g., a string). See bug issue + * Note all external_constructor<>::construct functions need to store the new + * value with j.replace_value(), which destroys the old one to avoid a memory + * leak in case j contains an allocated value (e.g., a string). See bug issue * https://github.com/nlohmann/json/issues/2865 for more information. + * + * A value that has to be allocated is created before the old one is destroyed: + * were it the other way around, an exception while creating the new value would + * leave j with the type of the new value, but the pointer to the destroyed old one. */ template struct external_constructor; @@ -53,10 +56,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::boolean; - j.m_data.m_value = b; - j.assert_invariant(); + j.replace_value(value_t::boolean, b); } }; @@ -66,19 +66,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::string_t& s) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value = s; - j.assert_invariant(); + const typename BasicJsonType::json_value value(s); + j.replace_value(value_t::string, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::string_t&& s) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value = std::move(s); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(s)); + j.replace_value(value_t::string, value); } template < typename BasicJsonType, typename CompatibleStringType, @@ -86,10 +82,8 @@ struct external_constructor int > = 0 > static void construct(BasicJsonType& j, const CompatibleStringType& str) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value.string = j.template create(str); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(str)); + j.replace_value(value_t::string, value); } }; @@ -99,19 +93,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::binary_t& b) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::binary; - j.m_data.m_value = typename BasicJsonType::binary_t(b); - j.assert_invariant(); + const typename BasicJsonType::json_value value(b); + j.replace_value(value_t::binary, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::binary_t&& b) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::binary; - j.m_data.m_value = typename BasicJsonType::binary_t(std::move(b)); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(b)); + j.replace_value(value_t::binary, value); } }; @@ -121,10 +111,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_float_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_float; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_float, val); } }; @@ -134,10 +121,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_unsigned_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_unsigned; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_unsigned, val); } }; @@ -147,10 +131,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_integer_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_integer; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_integer, val); } }; @@ -160,21 +141,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::array_t& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = arr; - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(arr); + j.replace_value(value_t::array, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::array_t&& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = std::move(arr); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(arr)); + j.replace_value(value_t::array, value); } template < typename BasicJsonType, typename CompatibleArrayType, @@ -188,39 +163,23 @@ struct external_constructor using std::begin; using std::end; - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value.array = j.template create(begin(arr), end(arr)); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(begin(arr), end(arr))); + j.replace_value(value_t::array, value); } template static void construct(BasicJsonType& j, const std::vector& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; - j.m_data.m_value.array->reserve(arr.size()); - for (const bool x : arr) - { - j.m_data.m_value.array->push_back(x); - j.set_parent(j.m_data.m_value.array->back()); - } - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(arr.begin(), arr.end())); + j.replace_value(value_t::array, value); } template::value, int> = 0> static void construct(BasicJsonType& j, const std::valarray& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; - j.m_data.m_value.array->resize(arr.size()); - std::copy(std::begin(arr), std::end(arr), j.m_data.m_value.array->begin()); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(std::begin(arr), std::end(arr))); + j.replace_value(value_t::array, value); } #if JSON_HAS_RANGE_VIEW_CONVERSION @@ -228,18 +187,15 @@ struct external_constructor enable_if_t>::value, int> = 0> static void construct(BasicJsonType& j, CompatibleArrayType && arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; + // no range constructor: a view's begin() and end() may have different + // types, and the view may only be iterable once + typename BasicJsonType::array_t elements; for (auto&& x : std::forward(arr)) { - j.m_data.m_value.array->push_back(x); + elements.push_back(x); } - // set the parents only once all elements are in place: a push_back - // that reallocates moves the earlier elements, which does not keep - // their parent pointers - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(elements)); + j.replace_value(value_t::array, value); } #endif }; @@ -250,21 +206,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::object_t& obj) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value = obj; - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(obj); + j.replace_value(value_t::object, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::object_t&& obj) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value = std::move(obj); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(obj)); + j.replace_value(value_t::object, value); } template < typename BasicJsonType, typename CompatibleObjectType, @@ -274,11 +224,8 @@ struct external_constructor using std::begin; using std::end; - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value.object = j.template create(begin(obj), end(obj)); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(begin(obj), end(obj))); + j.replace_value(value_t::object, value); } }; diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 4eeb326b8..788cc999c 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -676,6 +676,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec /// constructor for rvalue binary arrays (internal type) json_value(binary_t&& value) : binary(create(std::move(value))) {} + /// constructors taking ownership of an already created value + explicit json_value(string_t* value) noexcept : string(value) {} + explicit json_value(object_t* value) noexcept : object(value) {} + explicit json_value(array_t* value) noexcept : array(value) {} + private: // raw, allocation-free transfer of m_data from src to dst: no // set_parents()/assert_invariant() (the former is O(#children) per @@ -1006,6 +1011,18 @@ public: #endif } + /// @brief replace the stored value with an already created one + /// The new value must be created before calling this function: if its + /// creation throws, the current value is left untouched. + void replace_value(value_t t, const json_value& v) noexcept + { + m_data.m_value.destroy(m_data.m_type); + m_data.m_value = v; + m_data.m_type = t; + set_parents(); + assert_invariant(); + } + iterator set_parents(iterator it, std::ptrdiff_t count_set_parents) { #if JSON_DIAGNOSTICS @@ -2265,8 +2282,8 @@ public: if (is_an_object) { // the initializer list is a list of pairs -> create an object - m_data.m_type = value_t::object; m_data.m_value = value_t::object; + m_data.m_type = value_t::object; for (auto& element_ref : init) { @@ -2288,8 +2305,8 @@ public: } #endif // the initializer list describes an array -> create an array - m_data.m_type = value_t::array; m_data.m_value.array = create(init.begin(), init.end()); + m_data.m_type = value_t::array; } set_parents(); @@ -2302,8 +2319,8 @@ public: static basic_json binary(const typename binary_t::container_type& init) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = init; + res.m_data.m_type = value_t::binary; return res; } @@ -2313,8 +2330,8 @@ public: static basic_json binary(const typename binary_t::container_type& init, typename binary_t::subtype_type subtype) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = binary_t(init, subtype); + res.m_data.m_type = value_t::binary; return res; } @@ -2324,8 +2341,8 @@ public: static basic_json binary(typename binary_t::container_type&& init) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = std::move(init); + res.m_data.m_type = value_t::binary; return res; } @@ -2335,8 +2352,8 @@ public: static basic_json binary(typename binary_t::container_type&& init, typename binary_t::subtype_type subtype) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = binary_t(std::move(init), subtype); + res.m_data.m_type = value_t::binary; return res; } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index be32959d5..7d19b53af 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -6193,7 +6193,6 @@ NLOHMANN_JSON_NAMESPACE_END #include // optional #endif -#include // copy #include // begin, end #include // allocator_traits #include // basic_string, char_traits @@ -6833,10 +6832,14 @@ namespace detail ////////////////// /* - * Note all external_constructor<>::construct functions need to call - * j.m_data.m_value.destroy(j.m_data.m_type) to avoid a memory leak in case j contains an - * allocated value (e.g., a string). See bug issue + * Note all external_constructor<>::construct functions need to store the new + * value with j.replace_value(), which destroys the old one to avoid a memory + * leak in case j contains an allocated value (e.g., a string). See bug issue * https://github.com/nlohmann/json/issues/2865 for more information. + * + * A value that has to be allocated is created before the old one is destroyed: + * were it the other way around, an exception while creating the new value would + * leave j with the type of the new value, but the pointer to the destroyed old one. */ template struct external_constructor; @@ -6847,10 +6850,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::boolean; - j.m_data.m_value = b; - j.assert_invariant(); + j.replace_value(value_t::boolean, b); } }; @@ -6860,19 +6860,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::string_t& s) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value = s; - j.assert_invariant(); + const typename BasicJsonType::json_value value(s); + j.replace_value(value_t::string, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::string_t&& s) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value = std::move(s); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(s)); + j.replace_value(value_t::string, value); } template < typename BasicJsonType, typename CompatibleStringType, @@ -6880,10 +6876,8 @@ struct external_constructor int > = 0 > static void construct(BasicJsonType& j, const CompatibleStringType& str) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::string; - j.m_data.m_value.string = j.template create(str); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(str)); + j.replace_value(value_t::string, value); } }; @@ -6893,19 +6887,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::binary_t& b) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::binary; - j.m_data.m_value = typename BasicJsonType::binary_t(b); - j.assert_invariant(); + const typename BasicJsonType::json_value value(b); + j.replace_value(value_t::binary, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::binary_t&& b) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::binary; - j.m_data.m_value = typename BasicJsonType::binary_t(std::move(b)); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(b)); + j.replace_value(value_t::binary, value); } }; @@ -6915,10 +6905,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_float_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_float; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_float, val); } }; @@ -6928,10 +6915,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_unsigned_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_unsigned; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_unsigned, val); } }; @@ -6941,10 +6925,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_integer_t val) noexcept { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::number_integer; - j.m_data.m_value = val; - j.assert_invariant(); + j.replace_value(value_t::number_integer, val); } }; @@ -6954,21 +6935,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::array_t& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = arr; - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(arr); + j.replace_value(value_t::array, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::array_t&& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = std::move(arr); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(arr)); + j.replace_value(value_t::array, value); } template < typename BasicJsonType, typename CompatibleArrayType, @@ -6982,39 +6957,23 @@ struct external_constructor using std::begin; using std::end; - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value.array = j.template create(begin(arr), end(arr)); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(begin(arr), end(arr))); + j.replace_value(value_t::array, value); } template static void construct(BasicJsonType& j, const std::vector& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; - j.m_data.m_value.array->reserve(arr.size()); - for (const bool x : arr) - { - j.m_data.m_value.array->push_back(x); - j.set_parent(j.m_data.m_value.array->back()); - } - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(arr.begin(), arr.end())); + j.replace_value(value_t::array, value); } template::value, int> = 0> static void construct(BasicJsonType& j, const std::valarray& arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; - j.m_data.m_value.array->resize(arr.size()); - std::copy(std::begin(arr), std::end(arr), j.m_data.m_value.array->begin()); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(std::begin(arr), std::end(arr))); + j.replace_value(value_t::array, value); } #if JSON_HAS_RANGE_VIEW_CONVERSION @@ -7022,18 +6981,15 @@ struct external_constructor enable_if_t>::value, int> = 0> static void construct(BasicJsonType& j, CompatibleArrayType && arr) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::array; - j.m_data.m_value = value_t::array; + // no range constructor: a view's begin() and end() may have different + // types, and the view may only be iterable once + typename BasicJsonType::array_t elements; for (auto&& x : std::forward(arr)) { - j.m_data.m_value.array->push_back(x); + elements.push_back(x); } - // set the parents only once all elements are in place: a push_back - // that reallocates moves the earlier elements, which does not keep - // their parent pointers - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(elements)); + j.replace_value(value_t::array, value); } #endif }; @@ -7044,21 +7000,15 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::object_t& obj) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value = obj; - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(obj); + j.replace_value(value_t::object, value); } template static void construct(BasicJsonType& j, typename BasicJsonType::object_t&& obj) { - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value = std::move(obj); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(std::move(obj)); + j.replace_value(value_t::object, value); } template < typename BasicJsonType, typename CompatibleObjectType, @@ -7068,11 +7018,8 @@ struct external_constructor using std::begin; using std::end; - j.m_data.m_value.destroy(j.m_data.m_type); - j.m_data.m_type = value_t::object; - j.m_data.m_value.object = j.template create(begin(obj), end(obj)); - j.set_parents(); - j.assert_invariant(); + const typename BasicJsonType::json_value value(j.template create(begin(obj), end(obj))); + j.replace_value(value_t::object, value); } }; @@ -27921,6 +27868,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec /// constructor for rvalue binary arrays (internal type) json_value(binary_t&& value) : binary(create(std::move(value))) {} + /// constructors taking ownership of an already created value + explicit json_value(string_t* value) noexcept : string(value) {} + explicit json_value(object_t* value) noexcept : object(value) {} + explicit json_value(array_t* value) noexcept : array(value) {} + private: // raw, allocation-free transfer of m_data from src to dst: no // set_parents()/assert_invariant() (the former is O(#children) per @@ -28251,6 +28203,18 @@ public: #endif } + /// @brief replace the stored value with an already created one + /// The new value must be created before calling this function: if its + /// creation throws, the current value is left untouched. + void replace_value(value_t t, const json_value& v) noexcept + { + m_data.m_value.destroy(m_data.m_type); + m_data.m_value = v; + m_data.m_type = t; + set_parents(); + assert_invariant(); + } + iterator set_parents(iterator it, std::ptrdiff_t count_set_parents) { #if JSON_DIAGNOSTICS @@ -29510,8 +29474,8 @@ public: if (is_an_object) { // the initializer list is a list of pairs -> create an object - m_data.m_type = value_t::object; m_data.m_value = value_t::object; + m_data.m_type = value_t::object; for (auto& element_ref : init) { @@ -29533,8 +29497,8 @@ public: } #endif // the initializer list describes an array -> create an array - m_data.m_type = value_t::array; m_data.m_value.array = create(init.begin(), init.end()); + m_data.m_type = value_t::array; } set_parents(); @@ -29547,8 +29511,8 @@ public: static basic_json binary(const typename binary_t::container_type& init) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = init; + res.m_data.m_type = value_t::binary; return res; } @@ -29558,8 +29522,8 @@ public: static basic_json binary(const typename binary_t::container_type& init, typename binary_t::subtype_type subtype) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = binary_t(init, subtype); + res.m_data.m_type = value_t::binary; return res; } @@ -29569,8 +29533,8 @@ public: static basic_json binary(typename binary_t::container_type&& init) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = std::move(init); + res.m_data.m_type = value_t::binary; return res; } @@ -29580,8 +29544,8 @@ public: static basic_json binary(typename binary_t::container_type&& init, typename binary_t::subtype_type subtype) { auto res = basic_json(); - res.m_data.m_type = value_t::binary; res.m_data.m_value = binary_t(std::move(init), subtype); + res.m_data.m_type = value_t::binary; return res; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f98ab7593..de415897f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -138,6 +138,11 @@ json_test_set_test_options(test-disabled_exceptions # only the #972 regression test needs thirdparty/fifo_map on its include path json_test_set_test_options(test-regression1 LINK_LIBRARIES fifo_map_include) +# GCC's false -Warray-bounds error with JSON_DIAGNOSTICS only shows up when optimizing (#5742) +json_test_set_test_options(test-diagnostics-optimized + COMPILE_OPTIONS $<$:-O3 -Werror=array-bounds> +) + ############################################################################# # add unit tests ############################################################################# diff --git a/tests/src/unit-allocator.cpp b/tests/src/unit-allocator.cpp index d2e23c1a7..07eb21c4e 100644 --- a/tests/src/unit-allocator.cpp +++ b/tests/src/unit-allocator.cpp @@ -12,6 +12,11 @@ #include using nlohmann::json; +#include +#if JSON_HAS_RANGES + #include +#endif + namespace { // special test case to check if memory is leaked if constructor throws @@ -671,3 +676,173 @@ TEST_CASE("destructor performs no allocation, only deallocation") CHECK(counting_allocator_deallocations > deallocations_before); } } + +// the no-exceptions CI job skips every CHECK_THROWS_AS, which would leave +// next_construct_fails set for the next allocation outside a check +#if !defined(JSON_NOEXCEPTION) +TEST_CASE("a failed allocation leaves the value unchanged") +{ + // create JSON type using the throwing allocator + using my_json = nlohmann::basic_json; + + // Each of these creates a string, array, object, or binary value. The + // value must be created before the type is changed: otherwise, a failed + // creation left a value of the new type without anything behind it (an + // assertion in its destructor, a null pointer everywhere else) or, when + // an old value was destroyed first, with a pointer to that destroyed one. + + SECTION("creating a binary value") + { + const std::vector bytes = {1, 2, 3}; + my_json _; + + next_construct_fails = true; + CHECK_THROWS_AS(_ = my_json::binary(bytes), std::bad_alloc&); + next_construct_fails = true; + CHECK_THROWS_AS(_ = my_json::binary(bytes, 42), std::bad_alloc&); + next_construct_fails = true; + CHECK_THROWS_AS(_ = my_json::binary(std::vector(bytes)), std::bad_alloc&); + next_construct_fails = true; + CHECK_THROWS_AS(_ = my_json::binary(std::vector(bytes), 42), std::bad_alloc&); + next_construct_fails = false; + } + + SECTION("turning a null value into an array or object") + { + my_json j; + + next_construct_fails = true; + CHECK_THROWS_AS(j[0], std::bad_alloc&); + CHECK(j.is_null()); + + next_construct_fails = true; + CHECK_THROWS_AS(j["key"], std::bad_alloc&); + CHECK(j.is_null()); + +#ifdef JSON_HAS_CPP_17 + next_construct_fails = true; + CHECK_THROWS_AS(j[std::string_view("key")], std::bad_alloc&); + CHECK(j.is_null()); +#endif + + next_construct_fails = true; + CHECK_THROWS_AS(j.push_back(my_json(1)), std::bad_alloc&); + CHECK(j.is_null()); + + const my_json one = 1; + next_construct_fails = true; + CHECK_THROWS_AS(j.push_back(one), std::bad_alloc&); + CHECK(j.is_null()); + + next_construct_fails = true; + CHECK_THROWS_AS(j.push_back(my_json::object_t::value_type("key", 1)), std::bad_alloc&); + CHECK(j.is_null()); + + next_construct_fails = true; + CHECK_THROWS_AS(j.emplace_back(1), std::bad_alloc&); + CHECK(j.is_null()); + + next_construct_fails = true; + CHECK_THROWS_AS(j.emplace("key", 1), std::bad_alloc&); + CHECK(j.is_null()); + + const my_json object = {{"key", 1}}; + next_construct_fails = true; + CHECK_THROWS_AS(j.update(object), std::bad_alloc&); + CHECK(j.is_null()); + + next_construct_fails = false; + } + + // With iterator debugging, VS 2015's containers construct a proxy with the + // allocator in constructors that cannot report its failure, so a failing + // allocator crashes this section there (SIGSEGV with VS 2015 Debug x86). +#if !(defined(_MSC_VER) && _MSC_VER < 1910 && defined(_ITERATOR_DEBUG_LEVEL) && _ITERATOR_DEBUG_LEVEL > 0) + SECTION("converting into an existing value") + { + // to_json replaces the value it is given; the old one must survive a + // failed creation of the new one + my_json j = "old"; + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::string("new")), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::vector {1, 2}), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::vector {true, false}), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::map {{"a", 1}}), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, my_json::binary_t({1, 2})), std::bad_alloc&); + CHECK(j == "old"); + + // the overloads for lvalues of the value types, for the value types + // themselves, and for the remaining compatible types + const std::string string = "new"; + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, string), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, "new"), std::bad_alloc&); + CHECK(j == "old"); + + // to_json only moves a binary value that it converted from another + // container type, which my_json's std::vector is not + using binary_constructor = nlohmann::detail::external_constructor; + next_construct_fails = true; + CHECK_THROWS_AS(binary_constructor::construct(j, my_json::binary_t({1, 2})), std::bad_alloc&); + CHECK(j == "old"); + + my_json::array_t array = {1, 2}; + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, array), std::bad_alloc&); + CHECK(j == "old"); + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::move(array)), std::bad_alloc&); + CHECK(j == "old"); + + my_json::object_t object = {{"a", 1}}; + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, object), std::bad_alloc&); + CHECK(j == "old"); + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::move(object)), std::bad_alloc&); + CHECK(j == "old"); + + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, std::valarray {1, 2}), std::bad_alloc&); + CHECK(j == "old"); + +#if JSON_HAS_RANGES && !defined(__MINGW32__) + const std::vector numbers = {1, 2}; + next_construct_fails = true; + CHECK_THROWS_AS(nlohmann::to_json(j, numbers | std::views::filter([](int /*unused*/) + { + return true; + })), std::bad_alloc&); + CHECK(j == "old"); +#endif + + next_construct_fails = false; + nlohmann::to_json(j, std::vector {1, 2}); + CHECK(j == my_json({1, 2})); + } +#endif +} +#endif diff --git a/tests/src/unit-diagnostics-optimized.cpp b/tests/src/unit-diagnostics-optimized.cpp new file mode 100644 index 000000000..a020c2fe9 --- /dev/null +++ b/tests/src/unit-diagnostics-optimized.cpp @@ -0,0 +1,80 @@ +// __ _____ _____ _____ +// __| | __| | | | JSON for Modern C++ (supporting code) +// | | |__ | | | | | | version 3.12.0 +// |_____|_____|_____|_|___| https://github.com/nlohmann/json +// +// SPDX-FileCopyrightText: 2013-2026 Niels Lohmann +// SPDX-License-Identifier: MIT + +// Regression test for https://github.com/nlohmann/json/issues/5742: with +// JSON_DIAGNOSTICS, GCC (12 to at least 16) reported a false -Warray-bounds +// error in the inlined set_parents() at -O3. The type of a new string was set +// before the string was allocated, so GCC had to assume that operator new +// could change it again and checked the object branch of set_parents() +// against the string's allocation. Setting the type after creating the value +// avoids this. The warning depends on GCC's inlining decisions, so the +// sections cover two patterns that trigger it on different GCC versions +// (#4819 and #5742). +// On GCC, this file is compiled with -O3 -Werror=array-bounds (see +// tests/CMakeLists.txt), so the test fails to build if the warning returns. + +#include "doctest_compatibility.h" + +#ifdef JSON_DIAGNOSTICS + #undef JSON_DIAGNOSTICS +#endif + +#define JSON_DIAGNOSTICS 1 + +#include +using nlohmann::json; + +#include +#include +#include +#include + +namespace +{ +enum class diag_color +{ + red, + green, + blue +}; + +void to_json(json& j, const diag_color& c) +{ + static const std::pair m[] = // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) + { + {diag_color::red, "r"}, + {diag_color::green, "g"}, + {diag_color::blue, "b"}, + }; + const auto* it = std::find_if(std::begin(m), std::end(m), [c](const std::pair& p) + { + return p.first == c; + }); + j = it->second; +} +} // namespace + +TEST_CASE("diagnostics with optimization") +{ + SECTION("issue #4819 - object in vector") + { + std::vector jsons{}; + jsons.emplace_back(json({{"key", "value"}})); + CHECK(jsons.back()["key"] == "value"); + } + + SECTION("issue #5742 - string values from a static table") + { + json j = json::array(); + j.push_back(diag_color::red); + j.push_back(diag_color::green); + j.push_back(diag_color::blue); + CHECK(j.dump() == R"(["r","g","b"])"); + CHECK_THROWS_WITH_AS(j[1].get(), "[json.exception.type_error.302] (/1) type must be number, but is string", json::type_error); + } +}