Repository navigation
Prevent stack overflow in json_pointer::flatten - #5517
dajiaohuang wants to merge 8 commits into
Conversation
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly. 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. |
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
|
Applied the amalgamation-patch artifact from run 34485452558. Commit fe2f71e updates only include/nlohmann/detail/json_pointer.hpp and single_include/nlohmann/json.hpp to match the generated formatting, and is pushed to fix/5393-iterative-flatten. Validation passed: Release C++11 and C++20 issue #5393 regression selections (1 test, 3 assertions each), Release single_detail_json_pointer build and smoke run, and git diff --check. |
|
The GCC C++26 standards jobs exposed a -Werror=noexcept failure for the iterative flatten stack when instantiated with the alternative string type: latten_task's move construction was inferred as potentially throwing. Commit 7121dfd marks that constructor |
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
7121dfd to
0eabb21
Compare
|
Amended the latest fix commit with the repository-required DCO sign-off and force-updated the fork branch. New head: 0eabb21. No source changes beyond the Signed-off-by trailer; the previous C++26 noexcept fix remains intact. |
|
There are build failures related to the new type having pointers but not implementing or deleting the copy/move assignment operators. |
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
50a5908 to
1d819ec
Compare
|
Declared the flatten_task copy/move special members in the source and generated headers in commit 1d819ec to satisfy GCC -Weffc++ while preserving the existing non-owning pointer semantics. git diff --check passes; local C++ toolchain is unavailable on Windows, so hosted CI is the remaining compile validation. |
nlohmann
left a comment
nlohmann
left a comment
There was a problem hiding this comment.
Thanks, this does exactly what #5393 asks for flatten(), and the work stack keeps the original traversal order.
I verified it locally, merged into current develop (the branch is 44 commits behind):
- Identical output. I compared
flatten()andunflatten(flatten(x))againstdevelopon 10,010 random values:jsonandordered_json, keys containing/and~(so escaping is exercised), empty arrays and objects, and chains 100-200 levels deep with siblings. All 20,020 outputs match exactly. On the branch as is, someunflattenresults differ, but that comes from #5443 landing ondevelopsince your base, not from this PR. Merged, it is identical. - Deep values. It handles 1,000,000 levels of nested arrays and of nested objects on a 1 MiB stack, where
developcrashes at 100,000. Memory improves too: 5 MB instead of 466 MB at 20,000 levels, because the recursive version kept every level's reference string alive.
Two CI failures block it, and one change fixes both:
- GCC 4.8 (
ci_test_compilers_gcc_old (4.8)) rejectsflatten_task& operator=(flatten_task&&) noexcept = default;because it is "defaulted ... with an exception-specification that differs from the implicit declaration". The instantiation that trips it isunit-alt-string.cpp, whosealt_stringhas a move assignment that is notnoexcept. For such string types the explicitnoexceptis not merely rejected by old compilers, it is wrong. - clang-tidy (
cppcoreguidelines-special-member-functions) complains that the struct declares the copy/move members but no destructor.
Dropping all four explicitly defaulted members fixes both (rule of zero). The implicit ones are generated with the right exception specifications, so moves stay noexcept wherever string_t's are. I'd also drop the noexcept on the converting constructor: it moves a string_t that may throw, and noexcept would turn that into std::terminate. See the suggestion. I ran clang-tidy locally with the repository's .clang-tidy on the suggested version, and the special-member finding is gone.
Two smaller things:
- Please move the test from
unit-regression2.cpptounit-large_json.cpp, where the other deep-nesting tests live.unit-regression2.cppis at the size the MinGW linker still copes with; it already had to be split once (#5511). - Please rebase onto
developso CI runs against the current code.
Optional: a newer clang-tidy than CI's (23) additionally suggests const auto current = std::move(stack.back()); (misc-const-correctness). It's harmless either way.
This review was written by Claude Code on behalf of @nlohmann.
| CHECK(mixed == json({{"keep", {{"a", 1}, {"b", 2}}}, {"replace", {{"x", 2}}}})); | ||
| } | ||
|
|
||
| TEST_CASE("issue #5393 - flatten deeply nested values without overflowing the stack") |
There was a problem hiding this comment.
Could this move to tests/src/unit-large_json.cpp? That's where the other deep-nesting tests live, and unit-regression2.cpp is at the size the MinGW linker still copes with (it had to be split in #5511).
GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
|
@dajiaohuang Are you willing to continue working on this? |
* Diff deeply nested values without recursing per nesting level diff() descended into both values once per nesting level, and compared them with operator== on every level on the way, which recurses as well. Values nested deeply enough - 25,000 levels on an 8 MiB stack - exhausted the call stack and terminated the process, although parse() accepts them without complaint. On such a chain the per-level comparisons and path strings also made diff() quadratic in time and memory. Both the recursion and operator== only descend as far as the source is nested. So diff() first checks, recursing at most diff_depth_limit() (128) levels, whether the source is nested more deeply than that. If not - all but a vanishing minority of values - the recursive algorithm diffs it exactly as before, now as diff_recursively(). Otherwise diff_iteratively() walks the two values on an explicit stack, emitting the same operations in the same order. It does not compare arrays and objects with operator== up front (equal ones yield no operations anyway), keeps the path in one buffer instead of a new string per level, and hands every subtree that is not nested too deeply back to diff_recursively(), so equal parts are still skipped quickly. The check costs one pass over the source. On a 3,000-object document that is about 30% of diffing two equal values (which is just an operator== call), about 10% of diffing values that differ in a few places, and noise when arrays change length. Once operator== no longer recurses (#5390), the check can go. Tests check that the patch reproduces the target at every depth up to 300, for json and ordered_json, including reordered members. They also check the exact operation for a difference deep inside, and diff values nested 100,000 levels deep. Fixes #5393 for diff(). Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Make diff_frame a member struct that declares its special members GCC's -Weffc++ (an error in CI) asks a class with pointer members, a user constructor and a non-trivial destructor to declare its copy constructor and copy assignment; diff_frame's vector and basic_json members make its destructor non-trivial. Declare all five as defaulted, which also satisfies clang-tidy's special-member-functions check. Leave their exception specifications implicit: GCC 4.8 rejects an explicit one that differs from the implicit one, as it does for flatten_task in #5517. The converting constructor cannot throw, and is now declared noexcept for GCC's -Wnoexcept, which flags the emplace_back() under C++26 otherwise. The struct also moves from diff_iteratively() into the class, like dump_frame in the serializer. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Use the shared recursion limit in diff() diff_depth_limit() is gone in favor of detail::recursion_depth_limit(). Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Diff fewer nesting depths so the test does not time out under Valgrind Checking every depth up to 300 made test-json_patch exceed the 1500 s ctest timeout in ci_test_valgrind. Check the depths up to 16, those around the recursion limit of 128, and 300 instead. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Mark the diff frame's value-initialized members for clang-tidy The braces are kept for GCC's -Weffc++, as in json_sax.hpp. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Bound diff()'s descent with a depth count instead of scanning the source Now that operator== no longer recurses (#5390), diff() can keep its per-level equality shortcut all the way down. It diffs recursively for the first detail::recursion_depth_limit() levels, as merge_patch() does, and hands anything deeper to diff_iteratively(). The nesting_exceeds() scan, which cost about 30% on equal documents, is gone, and diff() is on par with develop again. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Note that the diff frame reference is invalidated by pop_back() too Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Keep diff()'s recursive levels small and its result elided diff_recursively built every patch operation in place from initializer lists. Unoptimized builds give each of those temporaries its own stack slot, so every level of the bounded descent cost kilobytes of stack (about 6 KB with clang -O0), and the 128 recursive levels overflowed the 1 MB stack of MSVC Debug in the "deeply nested values" test. The operations and the key comparison of two objects are now built by separate functions, which diff_iteratively shares, and both diff functions append to one result instead of returning a patch per level that the caller copies. With clang -O0, diffing values nested 300 levels deep now peaks at about 190 KB of stack instead of 880 KB. Since diff() now owns the only returned value, clang's -Wnrvo no longer reports the returns of diff_recursively, which alternated between the local patch and diff_iteratively's result. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Copy the diff frame's members instead of holding a reference to it The loop in diff_iteratively held a reference to the top frame, which enter() invalidates when it pushes and the end of the loop invalidates when it pops. Nothing used it afterwards, but a later change could. As in the other iterative walks, the members the loop reads are now copied out as constants and the ones it advances are changed through stack.back(). The frame as a whole is not copied: it holds the common keys and the "add" operations of an object. Signed-off-by: Niels Lohmann <mail@nlohmann.me> --------- Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
This is not true, and I thought there was a later conversation about that. Maybe that was a different PR. The correct fix is to add a defaulted destructor as well. |
|
You're right — that suggestion was wrong, and I retracted it in the review thread on 09-23 (#5517 (comment)). Rule of zero brings back the @dajiaohuang could you replace the struct body with the corrected version: all five special members Written by Claude Code on behalf of @nlohmann. |
|
Thank you for the detailed feedback and correction, @nlohmann and @gregmarr! I understand the issues now: I need to declare all five special members as defaulted (including ~flatten_task()), remove explicit noexcept on the defaulted moves, keep noexcept on the converting constructor, and merge develop to address the unrelated CI failures. I will apply these changes shortly. |
Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
|
Applied the latest correction in f8c4838: the constructor remains noexcept; all five special members are explicitly defaulted without exception specifications. The test is already in unit-large_json.cpp, and the single-header output is regenerated. The existing PR history already includes develop commit 11e75a5 as the merge parent, so I preserved history with a fast-forward update. GCC 13.3 C++11 builds and runs pass in both multi-header and single-header configurations: large_json (3043 assertions) and alt-string (57 assertions). GCC 4.8 and C++26 remain for CI. |
nlohmann
left a comment
nlohmann
left a comment
There was a problem hiding this comment.
Thanks for applying the struct correction. The code looks right to me now, and two small things are left before this can be merged.
What I checked (on the PR merged into current develop a5f5d30; the merge is clean):
flatten_taskmatches what we discussed: the converting constructor isnoexcept, and all five special members are= defaultwithout exception specifications.- GCC 16 with
-Wall -Wextra -Weffc++ -Wnoexcept -Werror, C++11 and C++26:unit-large_jsonandunit-alt-stringcompile cleanly (no-Weffc++/-Wnoexcepthits). - clang with ASan and UBSan:
unit-large_json,unit-alt-string, andunit-json_pointerpass, including the 100,000-level regression. make amalgamatewith astyle 3.4.13 gives no diff on the PR head.
Requested changes
- The PR description is out of date. It says the explicitly defaulted copy/move members and the constructor's
noexceptwere removed, but the final commit (f8c4838) does the opposite. Please update it to describe the final state, since it becomes the squash commit message. - Docs:
docs/mkdocs/docs/community/assurance_case.md(the "Bounded recursion" bullet, around line 59) still listsflattenamong the operations that "still recurse once per nesting level". Please removeflattenfrom that sentence. (The binary writers mentioned there were made iterative in #5781, so they can go too, leaving onlydiff.)
There is one optional nit inline.
(Review written by Claude Code on behalf of @nlohmann.)
| else | ||
| { | ||
| // iterate object and use keys as reference string | ||
| std::vector<flatten_task> children; |
There was a problem hiding this comment.
Nit (optional): this allocates a temporary vector for every non-empty object, just to reverse the order. You could push the children straight onto stack and reverse the newly added range instead:
const auto old_size = stack.size();
for (const auto& element : *current.value->m_data.m_value.object)
{
stack.emplace_back(detail::concat<string_t>(current.reference_string, '/', detail::escape(element.first)),
&element.second);
}
// reverse the new entries so the LIFO stack preserves object iteration order
std::reverse(stack.begin() + static_cast<std::ptrdiff_t>(old_size), stack.end());This saves the extra allocation and still only needs forward iterators on object_t. (<algorithm> may need to be included.) Not a blocker.
|
Updated the description to match the final struct and corrected the assurance-case bounded-recursion paragraph in the latest commit. This follow-up changes documentation only; git diff --check passes and the unchanged implementation validation is retained. |
|
Thank you for this, @dajiaohuang, and for working through several rounds of review, one of them caused by my own wrong rule-of-zero suggestion. In the meantime, #5792 made (Comment written by Claude Code on behalf of @nlohmann.) |
3 participants
Replace recursive
json_pointer::flatten()with an explicit stack to avoid call-stack exhaustion on deeply nested values. Preserve traversal order, escaped pointer names, and empty-container behavior. This addresses the flatten portion of #5393.flatten_taskretains itsnoexceptconverting constructor and all five defaulted special members without explicit exception specifications. The 100,000-level regression is inunit-large_json.cpp, and the current task is const. Develop was integrated with a history-preserving merge, and the single header was regenerated with pinned AStyle 3.4.13. The assurance case now lists onlydiffas still recursing per nesting level.Validation: unchanged implementation previously passed GCC 13.3.0 Linux
test-large_json,test-alt-string, andtest-json_pointerunder C++11 and C++20, including the 100,000-level regression. The documentation follow-up passesgit diff --check; implementation tests were not rerun for this prose-only change. GCC 4.8, clang-tidy and the full CI matrix remain unverified locally.