Repository navigation
Give a deep copy its type only after its container exists - #5721
Conversation
When copying a value nested deeper than 128 levels, and an allocation fails while an inner array or object is being copied, the partially built copy ended up with an element typed array/object but holding a null pointer. That element was already a fully constructed member of its parent's container, so destroying the parent during stack unwinding dereferenced the null pointer (release builds) or failed assert_invariant() (debug builds), instead of letting std::bad_alloc reach the caller. copy_iteratively() set a pending worklist element's type right after popping it, before the next loop iteration created its container in copy_array_level()/copy_object_level(). Move that type assignment into those two functions, right after the container is successfully created, and drop the premature one in copy_iteratively(), so a half-built element stays a null value - as copy_shallow()'s comment already promised - until it can safely hold one. Add a regression test to tests/src/unit-allocator.cpp that copies a value nested 130 levels deep (both arrays and objects, with a std::map- and an ordered_map-backed object_t) and fails every allocation of the copy in turn: each attempt must throw std::bad_alloc without crashing, and the source must stay unchanged. Fixes #5640. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
gregmarr
left a comment
There was a problem hiding this comment.
I was very confused about why moving the setting of the type earlier fixed a bug that it was set too early, and then I realized that the current code was setting the type after dst_value was updated to point to the new value which hasn't been processed yet. If it had been before if (worklist.empty()) then it would have been correct.
Yes, I also had to read the diff several times... |
Conflicts: - include/nlohmann/json.hpp: copy_array_level() keeps develop's #5721 "set the array type only once the container exists" and then builds the elements with the PR's copy_construct_tag emplace_back loop - single_include/nlohmann/json.hpp: regenerated with make amalgamate Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Summary
When the copy constructor copies a value nested deeper than 128 levels and an allocation fails while it is
copying an inner array or object, the partially built copy ends up with an element typed
array/objectbut holding a null pointer. That element is already a fully constructed member of its parent's container,
so destroying the parent during stack unwinding dereferences the null pointer (release builds) or fails
assert_invariant()(debug builds), instead of lettingstd::bad_allocreach the caller. This also happensat any depth when
JSON_NO_THREAD_LOCALis defined, since every copy then takes the same iterative path.Root cause:
copy_iteratively()gave a pending worklist element its type right after popping it, before thenext loop iteration created its container in
copy_array_level()/copy_object_level(). Ifcreate<array_t>()or
create<object_t>()then threw, the element was left typed but empty.Changes
copy_iteratively()and intocopy_array_level()/copy_object_level(),right after the container is successfully created, so a half-built element stays a null value - as
copy_shallow()'s comment already promised - until it can safely hold one.single_include/nlohmann/json.hpp(make amalgamate).Open PR #5585 ("Create a value before giving it its type") fixes the same before-the-value-exists-type
pattern at other places in
json.hppand into_json.hpp, but does not touchcopy_iteratively(),copy_array_level()orcopy_object_level(). This PR is independent of it and fixes the pattern in thosethree functions only.
Tests
Added
tests/src/unit-allocator.cpp/TEST_CASE("copy of a deeply nested value survives a failing allocation (#5640)"). It builds a value nested 130 levels deep (both arrays and objects, with astd::map- and anordered_map-backedobject_t), then, for n = 0, 1, 2, ..., lets the n-th allocation ofthe copy fail: each such attempt must throw
std::bad_alloc(CHECK_THROWS_AS) and leave the sourceunchanged (compared against a
dump()taken before), stopping once the copy succeeds.develop's headers: an assertion failure in debug builds(
assert_invariant,json.hpp:740); confirmed for both the default build and with-DJSON_NO_THREAD_LOCAL(which fails after only a few allocations, since every copy takes theiterative path).
-fsanitize=address,undefined -fno-sanitize-recover=undefined, plain and with-DJSON_NO_THREAD_LOCAL.Public API
No breaking changes. This only changes the order of two internal, private member functions
(
copy_array_level(),copy_object_level(),copy_iteratively()); observable behavior changes only inthat a previously-crashing case (allocation failure during a deep copy) now correctly throws
std::bad_alloc, matching the documented exception safety of the copy constructor.Fixes #5640
This PR was written by Claude Code on behalf of @nlohmann.
🤖 Generated with Claude Code