Skip to content

Fix allocation failure during JSON destruction - #5239

Open
michaelsam94 wants to merge 1 commit into
nlohmann:developfrom
michaelsam94:codex/fix-destructor-allocation-5135
Open

Fix allocation failure during JSON destruction#5239
michaelsam94 wants to merge 1 commit into
nlohmann:developfrom
michaelsam94:codex/fix-destructor-allocation-5135

Conversation

@michaelsam94

Copy link
Copy Markdown

This PR fixes #5135 by preventing allocation failures from the temporary destruction stack from escaping basic_json's noexcept destructor.

The existing iterative destruction path is kept unchanged for the normal case. It is now wrapped with the library's JSON_TRY / JSON_CATCH macros; if growing the heap-allocated stack throws, destruction falls back to the regular array/object cleanup below instead of allowing the exception to cross the destructor boundary and call std::terminate.

A regression test in unit-regression2.cpp injects a one-shot global allocation failure while destroying a structured JSON value, verifying that the destructor consumes the failing stack allocation path and returns normally.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

Local verification:

/usr/bin/c++ -std=c++11 -Iinclude -Itests/src -Itests/thirdparty/doctest tests/src/unit.cpp tests/src/unit-regression2.cpp -o build-local/unit-regression2 && ./build-local/unit-regression2 --test-case='*5135*'
./build-local/unit-regression2
/usr/bin/c++ -std=c++11 -Iinclude -Itests/src -Itests/thirdparty/doctest tests/src/unit.cpp tests/src/unit-allocator.cpp -o build-local/unit-allocator && ./build-local/unit-allocator
printf '#include <nlohmann/json.hpp>\nint main(){ nlohmann::json j = nlohmann::json::array({1,2,3}); return j.size() == 3 ? 0 : 1; }\n' | /usr/bin/c++ -std=c++11 -Isingle_include -x c++ - -o build-local/single-smoke && ./build-local/single-smoke
printf '#include <nlohmann/json.hpp>\nint main(){ nlohmann::json j = nlohmann::json::array({1,2,3}); return j.size() == 3 ? 0 : 1; }\n' | /usr/bin/c++ -std=c++11 -fno-exceptions -DJSON_NOEXCEPTION -Iinclude -x c++ - -o build-local/noexceptions-smoke && ./build-local/noexceptions-smoke
git diff --check

I could not run the full CMake suite locally because cmake is not available on this machine's PATH.

@michaelsam94
michaelsam94 requested a review from nlohmann as a code owner July 6, 2026 10:40
@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Jul 6, 2026
@nlohmann

nlohmann commented Jul 6, 2026

Copy link
Copy Markdown
Owner

I just merged a test which caused a conflict. Sorry, please rebase.

Signed-off-by: Michael Sam <michaelsam94@users.noreply.github.com>
@michaelsam94
michaelsam94 force-pushed the codex/fix-destructor-allocation-5135 branch from 877084d to d428f79 Compare July 6, 2026 10:59
@michaelsam94

Copy link
Copy Markdown
Author

Rebased onto current develop (acf076a) and resolved the test conflict by keeping the newly merged #4320 regression and placing the #5135 regression after it.\n\nRe-ran local checks:\n\n- unit-regression2: 9 test cases, 128 assertions passed\n- unit-allocator: 3 test cases, 17 assertions passed\n- single-header smoke compile\n- no-exceptions smoke compile\n- git diff --check

@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Jul 6, 2026
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions!

@github-actions github-actions Bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 6, 2026
@nlohmann

Copy link
Copy Markdown
Owner

Thanks for the fix — the JSON_TRY/JSON_CATCH wrapper does address the crash from #5135 in the common case. A review turned up a few things worth a look before merging:

Correctness

  1. New dangling-pointer risk in external_constructor<>::construct() (to_json.hpp, e.g. external_constructor<value_t::string>::construct). Every overload does destroy(old_type); m_type = new_type; m_value = new_value;. Before this PR, if destroy() threw (OOM), execution never reached the m_type reassignment, so j stayed consistent (leaked, but not corrupted). Now destroy() never throws, so m_type is always updated — and if the second allocation (for the new value) then also fails under the same sustained memory pressure, j ends up with m_type and m_value referring to different types, i.e. a dangling pointer. Reproduction sketch: j holds a large/deeply-nested array, then j = someString; under persistent OOM — destroy() swallows the failure and frees the old array, m_type becomes string, then the string allocation also fails, leaving m_value.string pointing at freed memory while m_type == string.

  2. Fix is a no-op under JSON_NOEXCEPTION alone. JSON_TRY/JSON_CATCH only expand to real try/catch when exceptions are compiler-enabled and JSON_NOEXCEPTION is undefined; otherwise they expand to if(true)/if(false). A build with -DJSON_NOEXCEPTION but without -fno-exceptions (a supported, documented combination) still gets a genuine std::bad_alloc from std::vector, but the new catch compiles away — so it still escapes the noexcept destructor and calls std::terminate(), reproducing basic_json destructor allocates memory, violating noexcept semantics #5135 exactly in that configuration. The new regression test is itself guarded with !defined(JSON_NOEXCEPTION), so this gap isn't covered.

  3. JSON_CATCH(...) swallows more than allocation failures. It's a blanket catch, so a custom AllocatorType that throws something other than std::bad_alloc gets silently discarded and funneled into the same fallback path, even though the comment describes only the "heap-allocated stack cannot grow" case.

Design tradeoff worth a second look

  1. The fallback (swallow, then let the container's normal allocator-based destroy run) falls through to ordinary recursive destruction for whatever wasn't flattened yet. Since every nested level's destroy() independently hits the same catch, under a sustained (not just transient) low-memory condition, each level fails its own flatten attempt too — so potentially the entire remaining tree ends up destroyed via plain recursion, which is exactly the stack-overflow-prone pattern the iterative flatten-to-heap-stack algorithm was originally introduced to avoid, and it kicks in specifically under memory pressure. Might be worth a comment (or a depth-based test) documenting that this residual risk was considered and accepted.

Minor

  • The new regression test only injects a single one-shot failure on a 2–3 level deep structure, so it doesn't exercise any of the scenarios above.
  • tests/src/unit-regression2.cpp: the global operator new/delete override is active for the whole unit-regression2 binary rather than scoped to just the new TEST_CASE.
  • const bool allocation_failure_was_injected = !fail_next_global_allocation; ... CHECK(allocation_failure_was_injected); could just be CHECK_FALSE(fail_next_global_allocation);.

(Comment drafted with the help of Claude Code.)

@nlohmann nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 19, 2026

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix conflicts and check out #5239 (comment)

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L please rebase Please rebase your branch to origin/develop tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

basic_json destructor allocates memory, violating noexcept semantics

2 participants