From 0a365865f9338ee7a35b3f7d22ed7f808b75385d Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Tue, 6 Oct 2026 07:40:39 +0200 Subject: [PATCH] Make basic_json destruction allocation-free and non-recursive (#5762) * Use the provided allocator in destroy() (#4842) Uses the provided allocator to allocate the stack used to avoid recursion in the destroy() implementation used by ~basic_json. Signed-off-by: Niels Lohmann * Test that the destructor uses the provided allocator Adds a regression test for #4842: destroying a nested array or object must allocate its temporary stack through the basic_json allocator, not std::allocator. Signed-off-by: Niels Lohmann * Fix allocation failure during JSON destruction Signed-off-by: Michael Sam Signed-off-by: Niels Lohmann * Make json_value::destroy() non-recursive and allocation-free destroy() used to flatten a nested array/object into a heap-allocated std::vector to avoid recursing per nesting level. That vector could itself throw bad_alloc under memory pressure, and since it now used the basic_json's own allocator (#4842), a failing allocator supplied by the caller made this more likely, not less. An exception thrown from inside ~basic_json(), which is noexcept, terminates the program (#5135). Replace the vector-based stack with a pointer-reversal walk that visits the tree without recursing per level and without allocating anything: cur is the array/object currently being emptied, prev is its parent (or null at the top). A parent's last child slot doubles as storage for that parent's own parent link while we are below it, so no extra memory is needed. A child is only ever removed once it is a scalar or an empty array/object, which neither allocates nor recurses more than one level deep. take() moves m_data between these locals directly, bypassing set_parents()/assert_invariant() (the former is O(#children) per call under JSON_DIAGNOSTICS, which would make the walk quadratic otherwise). This also removes the std::vector stack added by #4842, so the extra allocations it introduced disappear along with it. Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com> Signed-off-by: Niels Lohmann * Test that destroy() performs no allocation, even under memory pressure Update the #4842 regression test: it used to check that destroying a nested array/object made at least one allocation through the provided allocator (the old flattening stack). Now that destroy() does not allocate at all, assert the opposite: zero allocations, deallocations only. Rework the #5135 regression test to use a dedicated failing/counting allocator instead of overriding the process-wide ::operator new and ::operator delete, which affected every allocation in the whole unit-regression2 binary rather than just the values under test. Keep the original small repro as one case, and add deep (100000 levels) and wide-and-deep nested array/object/ordered_json cases, all destroyed while every further allocation is made to fail: the destructor must complete without allocating, without throwing, and without leaking. Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com> Signed-off-by: Niels Lohmann * Refactor destroy() for readability and add edge-case tests Apply review feedback from Greg Marr on the json_value::destroy() non-recursive, allocation-free destruction walk (#5135): - last_child() now uses object->rbegin()->second instead of std::prev(object->end())->second; pop_last_child() keeps std::prev(end()) since erase() needs a forward iterator. - is_empty_container() becomes has_no_children(), a switch that returns true for every non-container type as well as empty array/object, simplifying the "scalar or already-empty child" check at the call site. The local variable `last` is renamed to `cur_last_ref` for clarity. - free_container() asserts the array/object is already empty before freeing it, and the object branches assert the expected type. - destroy(value_t t) is now a thin dispatcher to destroy_string(), destroy_binary(), and destroy_container(t), each handling its own "not initialized" check and sharing the simple cases first in the switch. - destroy_container() moves the top-level container into the local stand-in via a plain swap of the json_value union, instead of a manual copy plus clearing array/object by hand. - The "cur has no children and there is no parent" case now frees cur and returns immediately, so the main loop is a plain while (true) with no trailing code after it. Also adds edge-case tests for both json and ordered_json (mixes of empty/non-empty arrays and objects, container children in first/last position, single-element chains, top-level empty containers, and destruction via erase()/assignment), plus a mixed-tree case in the "destructor performs no allocation" test. Signed-off-by: Niels Lohmann * Make the destroy() walk helpers private They modify basic_json internals without maintaining its invariants and are only meant for destroy_container(), so they no longer need to be reachable from the rest of basic_json. Signed-off-by: Niels Lohmann --------- Signed-off-by: Niels Lohmann Signed-off-by: Michael Sam Co-authored-by: Vesko Karaganev Co-authored-by: Michael Sam Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com> --- include/nlohmann/json.hpp | 254 +++++++++++++++++++-------- single_include/nlohmann/json.hpp | 254 +++++++++++++++++++-------- tests/src/unit-allocator.cpp | 85 +++++++++ tests/src/unit-regression2.cpp | 287 +++++++++++++++++++++++++++++++ 4 files changed, 734 insertions(+), 146 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 6f6bb8d04..aa8b1a6c6 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -612,100 +612,210 @@ 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))) {} - void destroy(value_t t) +private: + // raw, allocation-free transfer of m_data from src to dst: no + // set_parents()/assert_invariant() (the former is O(#children) per + // call under JSON_DIAGNOSTICS, which would make the walk below + // quadratic); dst takes ownership, src is left as value_t::null. + static void take(basic_json& dst, basic_json& src) noexcept + { + dst.m_data.m_type = src.m_data.m_type; + dst.m_data.m_value = src.m_data.m_value; + src.m_data.m_type = value_t::null; + } + + // true if v is not an array/object, or is an already-empty one + static bool has_no_children(const basic_json& v) noexcept + { + switch (v.m_data.m_type) + { + case value_t::array: + return v.m_data.m_value.array->empty(); + case value_t::object: + return v.m_data.m_value.object->empty(); + default: + return true; + } + } + + static basic_json& last_child(basic_json& v) + { + if (v.m_data.m_type == value_t::array) + { + return v.m_data.m_value.array->back(); + } + JSON_ASSERT(v.m_data.m_type == value_t::object); + return v.m_data.m_value.object->rbegin()->second; + } + + // removes the last child of a non-empty array/object v; this never + // allocates, and since it is only ever called when that child is a + // scalar or an already-empty array/object, destroying it never + // recurses more than one level deep (see destroy() below) + static void pop_last_child(basic_json& v) + { + if (v.m_data.m_type == value_t::array) + { + v.m_data.m_value.array->pop_back(); + } + else + { + JSON_ASSERT(v.m_data.m_type == value_t::object); + // erase() needs a forward iterator, so std::prev(end()) is + // used here rather than rbegin() (see last_child() above) + v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end())); + } + } + + // deallocates the (already empty) array/object held by v; this is + // the same allocator-based free the old recursive implementation + // used, just factored out so every level of the walk in destroy() + // can share it + static void free_container(basic_json& v) noexcept + { + if (v.m_data.m_type == value_t::array) + { + JSON_ASSERT(v.m_data.m_value.array->empty()); + AllocatorType alloc; + std::allocator_traits::destroy(alloc, v.m_data.m_value.array); + std::allocator_traits::deallocate(alloc, v.m_data.m_value.array, 1); + } + else + { + JSON_ASSERT(v.m_data.m_type == value_t::object); + JSON_ASSERT(v.m_data.m_value.object->empty()); + AllocatorType alloc; + std::allocator_traits::destroy(alloc, v.m_data.m_value.object); + std::allocator_traits::deallocate(alloc, v.m_data.m_value.object, 1); + } + v.m_data.m_type = value_t::null; // avoid a double free if v is later destructed + } + +public: + void destroy_string() noexcept + { + if (string == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, string); + std::allocator_traits::deallocate(alloc, string, 1); + } + + void destroy_binary() noexcept + { + if (binary == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, binary); + std::allocator_traits::deallocate(alloc, binary, 1); + } + + // t must be value_t::array or value_t::object + void destroy_container(value_t t) noexcept { if ( (t == value_t::object && object == nullptr) || - (t == value_t::array && array == nullptr) || - (t == value_t::string && string == nullptr) || - (t == value_t::binary && binary == nullptr) + (t == value_t::array && array == nullptr) ) { // not initialized (e.g., due to exception in the ctor) return; } - if (t == value_t::array || t == value_t::object) + + // Destroy the tree without recursing per nesting level and + // without any heap allocation: a heap-allocated flattening + // stack (the previous implementation) can itself throw + // bad_alloc, which would escape this noexcept destructor and + // terminate the program (#5135). + // + // Instead, walk down the "last child" chain, reversing links + // as we go: cur is the container currently being emptied, + // and prev is its parent (value_t::null when there is none). + // Each parent's last child slot doubles as storage for that + // parent's own parent link while we are below it, so no + // extra memory is needed. We only ever remove a child once + // it is a scalar or an empty array/object, which neither + // allocates nor recurses more than one level deep. + // + // This json_value is not itself a basic_json, so the + // top-level container is first moved into a local stand-in + // ("cur"); a default-constructed basic_json has a null + // pointer in its m_value (see data::m_value's initializer), + // so swapping it with *this leaves this union's own pointer + // null, and it is never looked at or freed a second time. + basic_json cur; + cur.m_data.m_type = t; + using std::swap; + swap(cur.m_data.m_value, *this); + + basic_json prev; // value_t::null: no parent + + while (true) { - // flatten the current json_value to a heap-allocated stack - std::vector stack; - - // move the top-level items to stack - if (t == value_t::array) + if (has_no_children(cur)) { - stack.reserve(array->size()); - std::move(array->begin(), array->end(), std::back_inserter(stack)); - } - else - { - stack.reserve(object->size()); - for (auto&& it : *object) + if (prev.m_data.m_type == value_t::null) { - stack.push_back(std::move(it.second)); - } - } - - while (!stack.empty()) - { - // move the last item to a local variable to be processed - basic_json current_item(std::move(stack.back())); - stack.pop_back(); - - // if current_item is array/object, move - // its children to the stack to be processed later - if (current_item.is_array()) - { - std::move(current_item.m_data.m_value.array->begin(), current_item.m_data.m_value.array->end(), std::back_inserter(stack)); - - current_item.m_data.m_value.array->clear(); - } - else if (current_item.is_object()) - { - for (auto&& it : *current_item.m_data.m_value.object) - { - stack.push_back(std::move(it.second)); - } - - current_item.m_data.m_value.object->clear(); + free_container(cur); + return; // back at the top with nothing left to do } - // it's now safe that current_item gets destructed - // since it doesn't have any children + // ascend: detach the grandparent link from prev's + // last slot, drop that (now null) slot, free cur + // (it is empty), then move up one level + basic_json gp; + take(gp, last_child(prev)); + pop_last_child(prev); + + free_container(cur); + + take(cur, prev); + take(prev, gp); + continue; } + + basic_json& cur_last_ref = last_child(cur); + + if (has_no_children(cur_last_ref)) + { + // scalar, or already-empty array/object + pop_last_child(cur); + continue; + } + + // descend into the non-empty last child, reversing the + // link: its slot takes over prev, and the child becomes + // the new cur + basic_json tmp; + take(tmp, cur_last_ref); + take(cur_last_ref, prev); + take(prev, cur); + take(cur, tmp); } + } + void destroy(value_t t) + { switch (t) { - case value_t::object: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, object); - std::allocator_traits::deallocate(alloc, object, 1); - break; - } - - case value_t::array: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, array); - std::allocator_traits::deallocate(alloc, array, 1); - break; - } - case value_t::string: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, string); - std::allocator_traits::deallocate(alloc, string, 1); + destroy_string(); break; - } case value_t::binary: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, binary); - std::allocator_traits::deallocate(alloc, binary, 1); + destroy_binary(); + break; + + case value_t::object: + case value_t::array: + destroy_container(t); break; - } case value_t::null: case value_t::boolean: @@ -714,9 +824,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::number_float: case value_t::discarded: default: - { break; - } } } }; diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 3c7b9704d..86321a36e 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -27812,100 +27812,210 @@ 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))) {} - void destroy(value_t t) +private: + // raw, allocation-free transfer of m_data from src to dst: no + // set_parents()/assert_invariant() (the former is O(#children) per + // call under JSON_DIAGNOSTICS, which would make the walk below + // quadratic); dst takes ownership, src is left as value_t::null. + static void take(basic_json& dst, basic_json& src) noexcept + { + dst.m_data.m_type = src.m_data.m_type; + dst.m_data.m_value = src.m_data.m_value; + src.m_data.m_type = value_t::null; + } + + // true if v is not an array/object, or is an already-empty one + static bool has_no_children(const basic_json& v) noexcept + { + switch (v.m_data.m_type) + { + case value_t::array: + return v.m_data.m_value.array->empty(); + case value_t::object: + return v.m_data.m_value.object->empty(); + default: + return true; + } + } + + static basic_json& last_child(basic_json& v) + { + if (v.m_data.m_type == value_t::array) + { + return v.m_data.m_value.array->back(); + } + JSON_ASSERT(v.m_data.m_type == value_t::object); + return v.m_data.m_value.object->rbegin()->second; + } + + // removes the last child of a non-empty array/object v; this never + // allocates, and since it is only ever called when that child is a + // scalar or an already-empty array/object, destroying it never + // recurses more than one level deep (see destroy() below) + static void pop_last_child(basic_json& v) + { + if (v.m_data.m_type == value_t::array) + { + v.m_data.m_value.array->pop_back(); + } + else + { + JSON_ASSERT(v.m_data.m_type == value_t::object); + // erase() needs a forward iterator, so std::prev(end()) is + // used here rather than rbegin() (see last_child() above) + v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end())); + } + } + + // deallocates the (already empty) array/object held by v; this is + // the same allocator-based free the old recursive implementation + // used, just factored out so every level of the walk in destroy() + // can share it + static void free_container(basic_json& v) noexcept + { + if (v.m_data.m_type == value_t::array) + { + JSON_ASSERT(v.m_data.m_value.array->empty()); + AllocatorType alloc; + std::allocator_traits::destroy(alloc, v.m_data.m_value.array); + std::allocator_traits::deallocate(alloc, v.m_data.m_value.array, 1); + } + else + { + JSON_ASSERT(v.m_data.m_type == value_t::object); + JSON_ASSERT(v.m_data.m_value.object->empty()); + AllocatorType alloc; + std::allocator_traits::destroy(alloc, v.m_data.m_value.object); + std::allocator_traits::deallocate(alloc, v.m_data.m_value.object, 1); + } + v.m_data.m_type = value_t::null; // avoid a double free if v is later destructed + } + +public: + void destroy_string() noexcept + { + if (string == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, string); + std::allocator_traits::deallocate(alloc, string, 1); + } + + void destroy_binary() noexcept + { + if (binary == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, binary); + std::allocator_traits::deallocate(alloc, binary, 1); + } + + // t must be value_t::array or value_t::object + void destroy_container(value_t t) noexcept { if ( (t == value_t::object && object == nullptr) || - (t == value_t::array && array == nullptr) || - (t == value_t::string && string == nullptr) || - (t == value_t::binary && binary == nullptr) + (t == value_t::array && array == nullptr) ) { // not initialized (e.g., due to exception in the ctor) return; } - if (t == value_t::array || t == value_t::object) + + // Destroy the tree without recursing per nesting level and + // without any heap allocation: a heap-allocated flattening + // stack (the previous implementation) can itself throw + // bad_alloc, which would escape this noexcept destructor and + // terminate the program (#5135). + // + // Instead, walk down the "last child" chain, reversing links + // as we go: cur is the container currently being emptied, + // and prev is its parent (value_t::null when there is none). + // Each parent's last child slot doubles as storage for that + // parent's own parent link while we are below it, so no + // extra memory is needed. We only ever remove a child once + // it is a scalar or an empty array/object, which neither + // allocates nor recurses more than one level deep. + // + // This json_value is not itself a basic_json, so the + // top-level container is first moved into a local stand-in + // ("cur"); a default-constructed basic_json has a null + // pointer in its m_value (see data::m_value's initializer), + // so swapping it with *this leaves this union's own pointer + // null, and it is never looked at or freed a second time. + basic_json cur; + cur.m_data.m_type = t; + using std::swap; + swap(cur.m_data.m_value, *this); + + basic_json prev; // value_t::null: no parent + + while (true) { - // flatten the current json_value to a heap-allocated stack - std::vector stack; - - // move the top-level items to stack - if (t == value_t::array) + if (has_no_children(cur)) { - stack.reserve(array->size()); - std::move(array->begin(), array->end(), std::back_inserter(stack)); - } - else - { - stack.reserve(object->size()); - for (auto&& it : *object) + if (prev.m_data.m_type == value_t::null) { - stack.push_back(std::move(it.second)); - } - } - - while (!stack.empty()) - { - // move the last item to a local variable to be processed - basic_json current_item(std::move(stack.back())); - stack.pop_back(); - - // if current_item is array/object, move - // its children to the stack to be processed later - if (current_item.is_array()) - { - std::move(current_item.m_data.m_value.array->begin(), current_item.m_data.m_value.array->end(), std::back_inserter(stack)); - - current_item.m_data.m_value.array->clear(); - } - else if (current_item.is_object()) - { - for (auto&& it : *current_item.m_data.m_value.object) - { - stack.push_back(std::move(it.second)); - } - - current_item.m_data.m_value.object->clear(); + free_container(cur); + return; // back at the top with nothing left to do } - // it's now safe that current_item gets destructed - // since it doesn't have any children + // ascend: detach the grandparent link from prev's + // last slot, drop that (now null) slot, free cur + // (it is empty), then move up one level + basic_json gp; + take(gp, last_child(prev)); + pop_last_child(prev); + + free_container(cur); + + take(cur, prev); + take(prev, gp); + continue; } + + basic_json& cur_last_ref = last_child(cur); + + if (has_no_children(cur_last_ref)) + { + // scalar, or already-empty array/object + pop_last_child(cur); + continue; + } + + // descend into the non-empty last child, reversing the + // link: its slot takes over prev, and the child becomes + // the new cur + basic_json tmp; + take(tmp, cur_last_ref); + take(cur_last_ref, prev); + take(prev, cur); + take(cur, tmp); } + } + void destroy(value_t t) + { switch (t) { - case value_t::object: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, object); - std::allocator_traits::deallocate(alloc, object, 1); - break; - } - - case value_t::array: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, array); - std::allocator_traits::deallocate(alloc, array, 1); - break; - } - case value_t::string: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, string); - std::allocator_traits::deallocate(alloc, string, 1); + destroy_string(); break; - } case value_t::binary: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, binary); - std::allocator_traits::deallocate(alloc, binary, 1); + destroy_binary(); + break; + + case value_t::object: + case value_t::array: + destroy_container(t); break; - } case value_t::null: case value_t::boolean: @@ -27914,9 +28024,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::number_float: case value_t::discarded: default: - { break; - } } } }; diff --git a/tests/src/unit-allocator.cpp b/tests/src/unit-allocator.cpp index a358e75f9..c716b3a5b 100644 --- a/tests/src/unit-allocator.cpp +++ b/tests/src/unit-allocator.cpp @@ -607,3 +607,88 @@ TEST_CASE("bad my_allocator::construct") j["test"].push_back("should not leak"); } } + +namespace +{ +std::size_t counting_allocator_allocations = 0; +std::size_t counting_allocator_deallocations = 0; + +template +struct counting_allocator : std::allocator +{ + using std::allocator::allocator; + + T* allocate(std::size_t n) + { + ++counting_allocator_allocations; + return std::allocator::allocate(n); + } + + void deallocate(T* p, std::size_t n) + { + ++counting_allocator_deallocations; + std::allocator::deallocate(p, n); + } + + template + struct rebind + { + using other = counting_allocator; + }; +}; +} // namespace + +TEST_CASE("destructor performs no allocation, only deallocation") +{ + // see https://github.com/nlohmann/json/issues/4842 and + // https://github.com/nlohmann/json/issues/5135: destroying nested + // arrays/objects used to allocate a temporary stack (first with + // std::allocator, later - after #4842 - with the provided allocator). + // Since that stack could itself throw bad_alloc from inside the + // noexcept destructor (#5135), destroy() no longer allocates anything: + // it only ever frees what is already there. + using counting_json = nlohmann::basic_json; + + SECTION("array") + { + auto* j = new counting_json({1, {2, {3, 4}}, 5}); // NOLINT(cppcoreguidelines-owning-memory) + const auto allocations_before = counting_allocator_allocations; + const auto deallocations_before = counting_allocator_deallocations; + delete j; // NOLINT(cppcoreguidelines-owning-memory) + CHECK(counting_allocator_allocations == allocations_before); + CHECK(counting_allocator_deallocations > deallocations_before); + } + + SECTION("object") + { + auto* j = new counting_json({{"a", {{"b", {1, 2}}}}, {"c", 3}}); // NOLINT(cppcoreguidelines-owning-memory) + const auto allocations_before = counting_allocator_allocations; + const auto deallocations_before = counting_allocator_deallocations; + delete j; // NOLINT(cppcoreguidelines-owning-memory) + CHECK(counting_allocator_allocations == allocations_before); + CHECK(counting_allocator_deallocations > deallocations_before); + } + + SECTION("mixed tree of empty/non-empty arrays and objects") + { + auto* j = new counting_json( // NOLINT(cppcoreguidelines-owning-memory) + { + {"empty_obj", counting_json::object()}, + {"empty_arr", counting_json::array()}, + {"nested", {{"a", counting_json::array({1, 2, counting_json::object()})}, {"b", 3}}}, + {"tail", counting_json::array({counting_json::array({1}), 2, counting_json::array({3})})} + }); + const auto allocations_before = counting_allocator_allocations; + const auto deallocations_before = counting_allocator_deallocations; + delete j; // NOLINT(cppcoreguidelines-owning-memory) + CHECK(counting_allocator_allocations == allocations_before); + CHECK(counting_allocator_deallocations > deallocations_before); + } +} diff --git a/tests/src/unit-regression2.cpp b/tests/src/unit-regression2.cpp index 1a8c73d4e..eb140f157 100644 --- a/tests/src/unit-regression2.cpp +++ b/tests/src/unit-regression2.cpp @@ -40,7 +40,9 @@ using ordered_json = nlohmann::ordered_json; #endif #include +#include #include +#include #include #include #include @@ -107,6 +109,84 @@ DOCTEST_CLANG_SUPPRESS_WARNING("-Wexit-time-destructors") using float_json = nlohmann::basic_json; +#if (defined(__cpp_exceptions) || defined(__EXCEPTIONS) || defined(_CPPUNWIND)) && !defined(JSON_NOEXCEPTION) +namespace +{ +// An allocator whose allocate() can be told to fail on demand, so tests can +// check that ~basic_json() tolerates - in fact, after #5135, never even +// triggers - an allocation failure. This replaces an earlier version of +// this test that overrode the process-wide ::operator new/::operator +// delete, which affected every allocation in the whole unit-regression2 +// binary rather than just the values under test. +std::size_t failing_allocator_allocations = 0; +std::size_t failing_allocator_deallocations = 0; +bool fail_next_allocation = false; + +template +struct failing_allocator : std::allocator +{ + using std::allocator::allocator; + + failing_allocator() noexcept = default; + template + failing_allocator(const failing_allocator& /*unused*/) noexcept {} // NOLINT(google-explicit-constructor) + + T* allocate(std::size_t n) + { + if (fail_next_allocation) + { + fail_next_allocation = false; + throw std::bad_alloc(); + } + ++failing_allocator_allocations; + return std::allocator::allocate(n); + } + + void deallocate(T* p, std::size_t n) + { + ++failing_allocator_deallocations; + std::allocator::deallocate(p, n); + } + + template + struct rebind + { + using other = failing_allocator; + }; +}; + +using failing_json = nlohmann::basic_json; +using failing_ordered_json = nlohmann::basic_json; + +// builds `depth` levels of nesting around a scalar, iteratively (never +// recursing: each wrap only moves the previous, already-built value, which +// is O(1)), each level an array or an object depending on `nest_objects` +template +BasicJsonType make_deep_nest(std::size_t depth, bool nest_objects) +{ + BasicJsonType v = 0; + for (std::size_t i = 0; i < depth; ++i) + { + if (nest_objects) + { + BasicJsonType wrapper = BasicJsonType::object(); + wrapper["x"] = std::move(v); + v = std::move(wrapper); + } + else + { + BasicJsonType wrapper = BasicJsonType::array(); + wrapper.push_back(std::move(v)); + v = std::move(wrapper); + } + } + return v; +} +} // namespace +#endif + ///////////////////////////////////////////////////////////////////// // for #1647 ///////////////////////////////////////////////////////////////////// @@ -940,4 +1020,211 @@ TEST_CASE("regression test - excessive binary container size honors allow_except CHECK(json::from_cbor(std::vector {0x9b, 0, 0, 0, 0, 0, 0, 0, 0x02}, true, false).is_discarded()); } +#if (defined(__cpp_exceptions) || defined(__EXCEPTIONS) || defined(_CPPUNWIND)) && !defined(JSON_NOEXCEPTION) +TEST_CASE("regression test #5135 - destructor never allocates, even under memory pressure") +{ + // Before the fix, ~basic_json() flattened a nested array/object into a + // heap-allocated std::vector to avoid recursing; that allocation could + // itself throw bad_alloc, which escapes a noexcept destructor and + // terminates the program. destroy() no longer allocates anything, so + // none of the sections below ever observe fail_next_allocation being + // consumed: CHECK(fail_next_allocation) confirms it was never touched. + + SECTION("the original report: a small, mixed array/object nest") + { + failing_allocator_allocations = 0; + failing_allocator_deallocations = 0; + { + failing_json j = failing_json::array( + { + failing_json::array({1, 2}), + failing_json::object({{"key", failing_json::array({3})}}) + }); + fail_next_allocation = true; + } // j is destroyed here, with every further allocation set to fail + + CHECK(fail_next_allocation); + fail_next_allocation = false; + CHECK(failing_allocator_deallocations > 0); + } + + SECTION("100000-deep nested array") + { + std::size_t allocations_before = 0; + { + failing_json j = make_deep_nest(100000, false); + allocations_before = failing_allocator_allocations; + fail_next_allocation = true; + } + + CHECK(fail_next_allocation); + fail_next_allocation = false; + CHECK(failing_allocator_allocations == allocations_before); + } + + SECTION("100000-deep nested object") + { + std::size_t allocations_before = 0; + { + failing_json j = make_deep_nest(100000, true); + allocations_before = failing_allocator_allocations; + fail_next_allocation = true; + } + + CHECK(fail_next_allocation); + fail_next_allocation = false; + CHECK(failing_allocator_allocations == allocations_before); + } + + SECTION("100000-deep nested ordered_json") + { + std::size_t allocations_before = 0; + { + failing_ordered_json j = make_deep_nest(100000, true); + allocations_before = failing_allocator_allocations; + fail_next_allocation = true; + } + + CHECK(fail_next_allocation); + fail_next_allocation = false; + CHECK(failing_allocator_allocations == allocations_before); + } + + SECTION("wide and deep: 1000 arrays of 1000 elements, each a small nested object") + { + std::size_t allocations_before = 0; + { + failing_json wide = failing_json::array(); + for (std::size_t i = 0; i < 1000; ++i) + { + failing_json inner = failing_json::array(); + for (std::size_t k = 0; k < 1000; ++k) + { + inner.push_back(failing_json::object({{"a", 1}, {"b", failing_json::array({1, 2, 3})}})); + } + wide.push_back(std::move(inner)); + } + + allocations_before = failing_allocator_allocations; + fail_next_allocation = true; + } + + CHECK(fail_next_allocation); + fail_next_allocation = false; + CHECK(failing_allocator_allocations == allocations_before); + } +} +#endif + +namespace +{ +// a single-element chain of `depth` arrays, built iteratively (never +// recursing: each wrap only moves the previous, already-built value) +template +BasicJsonType make_single_chain(std::size_t depth) +{ + BasicJsonType v = 1; + for (std::size_t i = 0; i < depth; ++i) + { + BasicJsonType wrapper = BasicJsonType::array(); + wrapper.push_back(std::move(v)); + v = std::move(wrapper); + } + return v; +} + +// copies value first, to make sure nothing was corrupted by building it, +// then lets both the copy and the original destruct via normal scope exit +template +void check_destroy_edge_case(const BasicJsonType& value) +{ + const BasicJsonType copy = value; + CHECK(copy == value); +} +} // namespace + +TEST_CASE_TEMPLATE("regression test #5135 - destroy() edge cases", BasicJsonType, json, ordered_json) +{ + using binary_t = typename BasicJsonType::binary_t; + + SECTION("mix of empty objects, empty arrays, non-empty containers, and scalars") + { + BasicJsonType root = BasicJsonType::array(); + root.push_back(BasicJsonType::object()); + root.push_back(BasicJsonType::array()); + root.push_back(BasicJsonType::object({{"k", 1}})); + root.push_back(BasicJsonType::array({1, 2, 3})); + root.push_back(nullptr); + root.push_back(true); + root.push_back(42); + root.push_back(3.14); + root.push_back("a string"); + root.push_back(BasicJsonType(binary_t({1, 2, 3}))); + check_destroy_edge_case(root); + } + + SECTION("container child in first position only") + { + BasicJsonType root = BasicJsonType::array({BasicJsonType::array({1, 2}), 3, 4, 5}); + check_destroy_edge_case(root); + } + + SECTION("container child in last position only") + { + BasicJsonType root = BasicJsonType::array({1, 2, 3, BasicJsonType::array({4, 5})}); + check_destroy_edge_case(root); + } + + SECTION("container children in first and last position") + { + BasicJsonType root = BasicJsonType::array({BasicJsonType::array({1}), 2, 3, BasicJsonType::array({4})}); + check_destroy_edge_case(root); + } + + SECTION("single-element chain, 1000 levels deep") + { + BasicJsonType root = make_single_chain(1000); + check_destroy_edge_case(root); + } + + SECTION("top-level empty array") + { + BasicJsonType root = BasicJsonType::array(); + check_destroy_edge_case(root); + } + + SECTION("top-level empty object") + { + BasicJsonType root = BasicJsonType::object(); + check_destroy_edge_case(root); + } + + SECTION("object whose last child is a non-empty array whose last child is an empty object") + { + BasicJsonType inner_array = BasicJsonType::array({1, 2, BasicJsonType::object()}); + BasicJsonType root = BasicJsonType::object({{"a", 1}, {"b", inner_array}}); + check_destroy_edge_case(root); + } + + SECTION("destruction via erase() on a deeply nested child") + { + BasicJsonType root = BasicJsonType::array(); + root.push_back(make_single_chain(500)); + root.push_back(BasicJsonType::object({{"k", BasicJsonType::array({1, 2, 3})}})); + // erase() must destroy the removed subtree without recursing or + // allocating beyond what erase() itself needs + root.erase(0); + CAPTURE(root.size()) + CHECK(root.size() == 1); + } + + SECTION("destruction via assignment on a deep tree") + { + BasicJsonType root = make_single_chain(2000); + // assigning a new value destroys the old one in place + root = nullptr; + CHECK(root.is_null()); + } +} + DOCTEST_CLANG_SUPPRESS_WARNING_POP