Share one nesting depth limit between all bounded descents - #5637
Merged
Merged
Conversation
Copying and comparing stopped their descent at basic_json::nesting_depth_limit(), while serializing, hashing and merging used detail::recursion_depth_limit(). Both were 128, but nothing kept them equal. The thread-local count now tests against detail::recursion_depth_limit() as well, and a static_assert keeps the limit small enough for the byte that holds the count. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
added
the
review needed
4 of 6 tasks
gregmarr
approved these changes
Sep 29, 2026
nlohmann
added
🚀 ready to merge
nlohmann
added a commit
that referenced
this pull request
Sep 30, 2026
The comment still named nesting_depth_limit, which #5637 removed on develop in favor of detail::recursion_depth_limit(). Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
added a commit
that referenced
this pull request
Sep 30, 2026
) * Fix stack overflow converting deep values between specializations Constructing a basic_json from another specialization (json to ordered_json or back, also via get<ordered_json>()) converted every container with its range constructor, which calls the converting constructor for each element. The call stack therefore grew with every nesting level, and a value nested some 30,000 levels deep overflowed it. The conversion now bounds its descent the way the copy constructor does since #5387: the first 128 levels are converted exactly as before, and below that convert_iteratively() finishes the value with an explicit stack. It builds each container bottom-up from its converted elements with the container's range constructor, so member order and keys that become equal are handled as before, and it gives a value its type only once its container exists, so an exception leaves nothing behind that cannot be destroyed. Parents (JSON_DIAGNOSTICS) and positions (JSON_DIAGNOSTIC_POSITIONS) are set for every value. Converting a null value no longer resets its positions: the constructor assigned null to a value that already was null, which swapped in the positions of the temporary. Fixes #5650. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Explain why converting null keeps positions and why next is a reference Review feedback on #5723 (gregmarr): clarify in comments that the converting constructor has already copied the positions of val, which the null case keeps like every other case, and that next must be a reference into pending so that ++next advances the stored iterator. Comments only; no code change. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Refer to recursion_depth_limit() in the convert_structured() docs The comment still named nesting_depth_limit, which #5637 removed on develop in favor of detail::recursion_depth_limit(). Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Advance the pending iterator through pending.back() and shorten the null comment Signed-off-by: Niels Lohmann <mail@nlohmann.me> --------- Signed-off-by: Niels Lohmann <mail@nlohmann.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@gregmarr pointed out in #5393 (comment) that there are still two depth limits for the bounded descents (recursing up to a fixed depth, then continuing on an explicit stack):
detail::recursion_depth_limit()(std::size_t): serializing, hashing,merge_patch/update, anddiffin Diff deeply nested values without recursing per nesting level #5548basic_json::nesting_depth_limit()(std::uint8_t): the copy constructor (Fix stack overflow when copying a deeply nested value (#5387) #5389) and the comparison operators (Fix stack overflow and exponential runtime when comparing nested values #5390)Both were 128, but nothing kept them in sync. This PR deletes
nesting_depth_limit(), so the thread-local counter is checked againstdetail::recursion_depth_limit()too.The two mechanisms stay as they are. The copy constructor and the comparison operators have fixed signatures and can't take a depth argument, so they still count depth in a thread-local byte. Only the limit is shared. Because that count is a byte and can go one level past the limit, a
static_assertrequiresrecursion_depth_limit() < 255. Raising the limit to 255 locally trips it.The documentation of
recursion_depth_limit()now says that copying and comparing use it too, and gives the reason for the byte-size restriction.Testing
unit-comparison,unit-constructor1,unit-diagnostics,unit-merge_patch,unit-hash: pass (clang, C++17,-Wall -Wextra -Werror)-DJSON_NO_THREAD_LOCAL -DJSON_DIAGNOSTICS=1: compilesmake amalgamaterunPublic API
No breaking changes.
nesting_depth_limit()was a private member ofbasic_json. The limit stays at 128, so behavior is unchanged.Written by Claude Code.
🤖 Generated with Claude Code