Fix stack overflow when copying a deeply nested value (#5387) - #5389
Fix stack overflow when copying a deeply nested value (#5387)#5389nlohmann wants to merge 12 commits into
Conversation
basic_json's copy constructor copied objects and arrays by handing the container to its own copy constructor, which copy-constructs every element and so reaches this constructor again, once per nesting level. A value nested deeply enough exhausted the call stack and terminated the process with a segmentation fault - no exception, nothing the caller could catch. Parsing such a value works, as the parser is iterative, and so does destroying one, as #1436 made destruction iterative. Bound how far the copy descends rather than take the call stack away from it. The first levels are copied exactly as they were - the containers copy their own elements, which is by far the fastest way to fill them - and only once the copy has descended 128 levels is the value below it finished without the call stack, through an explicit worklist. Copying can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound - all but a vanishing minority - is copied by the very same code as before and pays only for one counter. That counter lives in thread_local storage, as one shared between threads would be raced. JSON_NO_THREAD_LOCAL switches it off for toolchains without thread_local; copying then goes through the worklist right away, which yields the same values but is measurably slower. The deferred values are completed before the copy they belong to returns, so a value copied while another copy is going on - by a custom base class, say - is unaffected by the copy it is nested in. operator= takes its argument by value, so copy assignment is fixed as well. Copying is as fast as it was, within measurement noise (medians of 9 interleaved runs, clang -O3): -1.3% for an array of strings, +0.0% for a flat object, +0.1% for a flat array of numbers, +0.3% for nested arrays, +0.6% for nested objects and +1.2% for a twitter-like document. Copying a three-key object costs about ten nanoseconds more, the counter. Deferring every level instead, rather than only those below the bound, measured between 3% and 9% slower depending on the shape of the value. This fixes #5387 for the copy constructor. dump() is still recursive. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Serializing a container serializes its elements, so dump() descended into one call per nesting level. A value nested deeply enough exhausted the call stack and terminated the process with a segmentation fault - no exception, nothing the caller could catch. Parsing such a value works, as the parser is iterative, and so does destroying one, as #1436 made destruction iterative. Bound how far the descent goes rather than take the call stack away from it. The first 128 levels are written by exactly the code that always wrote them, and only below that does dump_iteratively write out what is left, keeping the containers it has entered on an explicit stack. Serializing can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound pays only for one comparison per container. Writing every value that way instead measured between 2% and 20% slower - 20% on object-heavy documents - which is why the descent is kept for all but the values that cannot afford it. The bound costs nothing measurable: between -1.4% and +1.2% across compact and pretty output of number, integer, string, object-heavy, wide-object and deeply nested documents. The output is unchanged for every value. Both ways of writing a container emit the separator in front of every element but the first, rather than after every element but the last, which puts exactly one between each pair and none at the end. This fixes #5387 for dump(). The copy constructor is fixed in #5389.
Serializing a container serializes its elements, so dump() descended into one call per nesting level. A value nested deeply enough exhausted the call stack and terminated the process with a segmentation fault - no exception, nothing the caller could catch. Parsing such a value works, as the parser is iterative, and so does destroying one, as #1436 made destruction iterative. Bound how far the descent goes rather than take the call stack away from it. The first 128 levels are written by exactly the code that always wrote them, and only below that does dump_iteratively write out what is left, keeping the containers it has entered on an explicit stack. Serializing can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound pays only for one comparison per container. Writing every value that way instead measured between 2% and 20% slower - 20% on object-heavy documents - which is why the descent is kept for all but the values that cannot afford it. The bound costs nothing measurable: between -1.4% and +1.2% across compact and pretty output of number, integer, string, object-heavy, wide-object and deeply nested documents. The output is unchanged for every value. Both ways of writing a container emit the separator in front of every element but the first, rather than after every element but the last, which puts exactly one between each pair and none at the end. This fixes #5387 for dump(). The copy constructor is fixed in #5389. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Serializing a container serializes its elements, so dump() descended into one call per nesting level. A value nested deeply enough exhausted the call stack and terminated the process with a segmentation fault - no exception, nothing the caller could catch. Parsing such a value works, as the parser is iterative, and so does destroying one, as #1436 made destruction iterative. Bound how far the descent goes rather than take the call stack away from it. The first 128 levels are written by exactly the code that always wrote them, and only below that does dump_iteratively write out what is left, keeping the containers it has entered on an explicit stack. Serializing can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound pays only for one comparison per container. Writing every value that way instead measured between 2% and 20% slower - 20% on object-heavy documents - which is why the descent is kept for all but the values that cannot afford it. The bound costs nothing measurable: between -1.4% and +1.2% across compact and pretty output of number, integer, string, object-heavy, wide-object and deeply nested documents. The output is unchanged for every value. Both ways of writing a container emit the separator in front of every element but the first, rather than after every element but the last, which puts exactly one between each pair and none at the end. This fixes #5387 for dump(). The copy constructor is fixed in #5389. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The copy constructor descends into 128 levels before it finishes a value without the call stack, so the iterative path is otherwise only reached by the few tests that nest deeper than that. JSON_NO_THREAD_LOCAL switches the descent off, which sends every value down that path. Running the whole test suite that way covers it with every object type, string type, allocator, and base class the suite already exercises. The new ci_test_no_thread_local target does that; the macro had no build coverage at all before. Copying a nested value also has to carry over what the element-wise copy constructor would have copied: the parents that JSON_DIAGNOSTICS relies on, and the positions that JSON_DIAGNOSTIC_POSITIONS reports. Both are now checked on either side of the descent bound, for objects and arrays. Neither was tested before, and dropping either one makes the new tests fail. Also quantify what JSON_NO_THREAD_LOCAL costs a copy instead of calling it "measurably slower". Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Linking test-regression2 fails with "relocation truncated to fit: IMAGE_REL_AMD64_REL32 against `.rdata'" once its object grows past what the MinGW linker copes with, and the copy constructor's helpers push it over: the object grows by 6.3%, from 4,654,128 to 4,944,920 bytes at -O0, and develop links at the smaller of the two. Building the tests optimized shrinks the object enough to link, but the binaries clang 11.0.1 and clang 18.1.8 then produce crash before doctest prints its first line - 39 of 102 tests on clang 18 - so the objects have to become smaller rather than denser. Moving the test cases that follow "regression tests 2" into a file of their own brings that object to 4,687,888 bytes, which is 0.7% above the size that links today rather than 6.3%. Both files still build for C++11, C++17 and C++20, and run the same 9 test cases and 135 assertions as before, now spread over two binaries. New regression tests belong in unit-regression3.cpp from here on, which is what CONTRIBUTING.md now says. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
d0fe4fe to
6305cc9
Compare
Every test that copies a value segfaults there - 42 of 105 on clang 11.0.1, 39 of 102 on clang 18.1.8 - while the same tests pass with GCC targeting MinGW, with Clang targeting MSVC, and with every other toolchain the library is tested on. The counter that bounds the copy constructor's descent is the library's first use of thread_local, so that job had never exercised it before. JSON_NO_THREAD_LOCAL already covers toolchains without thread_local storage, and copying yields the same values with it, only more slowly. Define it for this one automatically. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
unit-regression2.cpp opens a DOCTEST_CLANG_SUPPRESS_WARNING_PUSH block at the top and closed it at the very bottom, which the split moved into unit-regression3.cpp: one file was left with a push and no pop, the other with a pop and no push, which clang reports as an error. Give each file the pair it needs. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
clang-tidy rejects the array the two shapes were iterated over (cppcoreguidelines-avoid-c-arrays). The array only existed because astyle reformats a range-for over a braced initializer list into something unreadable; naming the two cases avoids both. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The first split left unit-regression2.cpp 0.7% below the size develop links at, which the comparison change in the follow-up immediately used up: the MinGW linker fails on test-regression2_cpp20 again, naming copy_shallow and to_partial_ordering among the relocations it cannot fit. Move the sections from "issue #2067" on, and the helper types they use, so that the file stops being the one that decides whether the tests can be linked at all. At -O0 and C++20, unit-regression2.cpp is now 2,964,944 bytes against develop's 4,708,248, and 3,070,568 bytes with the follow-up applied - roughly a third smaller either way, rather than a fraction of a percent larger. The 135 assertions are the same ones as before, now spread over three test cases in two files. Also silence the clang-tidy findings the deep-nesting tests draw: the copies they make are what is being tested, and the reserve() computation gets its parentheses. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The split left the json_4804 alias behind in unit-regression2.cpp while the test case that uses it went to unit-regression3.cpp, which does not build for C++17 and C++20 as a result. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The #2546 test case guards itself with __has_include(<span>), but the include itself sat in unit-regression2.cpp's preamble and stayed behind, so the section compiled without a declaration wherever the guard passed - which nvhpc reported and libc++ builds do not, as they skip the section altogether. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Serializing a container serializes its elements, so dump() descended into one call per nesting level. A value nested deeply enough exhausted the call stack and terminated the process with a segmentation fault - no exception, nothing the caller could catch. Parsing such a value works, as the parser is iterative, and so does destroying one, as #1436 made destruction iterative. Bound how far the descent goes rather than take the call stack away from it. The first 128 levels are written by exactly the code that always wrote them, and only below that does dump_iteratively write out what is left, keeping the containers it has entered on an explicit stack. Serializing can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound pays only for one comparison per container. Writing every value that way instead measured between 2% and 20% slower - 20% on object-heavy documents - which is why the descent is kept for all but the values that cannot afford it. The bound costs nothing measurable: between -1.4% and +1.2% across compact and pretty output of number, integer, string, object-heavy, wide-object and deeply nested documents. The output is unchanged for every value. Both ways of writing a container emit the separator in front of every element but the first, rather than after every element but the last, which puts exactly one between each pair and none at the end. This fixes #5387 for dump(). The copy constructor is fixed in #5389. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
| } | ||
| #endif | ||
|
|
||
| #ifndef JSON_NO_THREAD_LOCAL |
There was a problem hiding this comment.
why close and reopen the ifndef?
| ++m_depth; | ||
| } | ||
|
|
||
| ~copy_depth_guard() noexcept |
There was a problem hiding this comment.
noexcept is the default for destructors.
| break; | ||
| } | ||
|
|
||
| src_value = worklist.back().first; |
There was a problem hiding this comment.
should this only call .back() once for performance reasons?
| #ifndef JSON_NO_THREAD_LOCAL | ||
| /// the number of levels the copy constructor descends into before it | ||
| /// finishes the value below without the call stack | ||
| static constexpr std::size_t copy_depth_limit() |
There was a problem hiding this comment.
Would it be better to use a smaller size variable here, such as unsigned char to keep down the amount of thread storage used?
|
|
||
| const copy_depth_guard guard(depth); | ||
| copy_level(src); | ||
| #endif |
There was a problem hiding this comment.
If you invert things a little, the no-thread-local and too-deep are the same code.
#ifndef JSON_NO_THREAD_LOCAL
std::size_t& depth = copy_depth();
if (JSON_HEDLEY_LIKELY(depth < copy_depth_limit()))
{
const copy_depth_guard guard(depth);
copy_level(src);
return;
}
#endif
copy_iteratively(src);
You could also have copy_depth_guard do all the access and testing.
class copy_depth_guard
{
public:
explicit copy_depth_guard() noexcept
: m_okay(copy_depth() < copy_depth_limit())
{
++copy_depth();
}
~copy_depth_guard()
{
--copy_depth();
}
bool okay() const { return m_okay; }
copy_depth_guard(const copy_depth_guard&) = delete;
copy_depth_guard& operator=(const copy_depth_guard&) = delete;
copy_depth_guard(copy_depth_guard&&) = delete;
copy_depth_guard& operator=(copy_depth_guard&&) = delete;
private:
bool m_okay;
};
void copy_structured(const basic_json& src)
{
#ifndef JSON_NO_THREAD_LOCAL
const copy_depth_guard guard;
if (JSON_HEDLEY_LIKELY(guard.okay()))
{
copy_level(src);
return;
}
#endif
copy_iteratively(src);
}
There was a problem hiding this comment.
#5390 already does the proposed change for copy_depth_guard except that it does it in a separate class instead of reusing copy_depth_guard, but it also renames copy_depth_guard to make it more general and then doesn't use it more generally.
8ee5372 to
7bfb3aa
Compare
Copying carried a depth count, a depth limit and a guard of its own, and the comparison in the follow-up added a second set beside them. Neither operation needs its own: they are never nested inside one another by the library - copying a value does not compare one, and comparing two values does not copy them - and where user code nests them anyway, sharing the count only ends a descent sooner than it had to. So there is now one nesting_depth(), one nesting_depth_limit() and one nesting_depth_guard, which the follow-up uses instead of adding its own. Inverting the test in copy_structured leaves the too-deep case and the no-thread-local case as the same code. The guard takes the count rather than looking it up, because the caller has looked it up already to test it against the limit, and reaching thread-local storage twice on the path that is taken almost every time is worth avoiding. The switch that copies the value of anything that is not an object or an array was written twice - once in the copy constructor, once in copy_shallow - so that adding a value_t meant editing both, and missing one would have been silent. It is copy_leaf_value now, and inlined: both callers have already sorted the containers out, and folding that test into the switch is what keeps a value made mostly of numbers copying as fast as it did. Copying canada.json, citm_catalog.json and twitter.json is within 0.6% of what it was before, measured as a paired ratio over 18 interleaved rounds against a run-to-run spread of 0.3%. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Copying a value without the call stack builds the copy from the top down, and every value whose own copy has not been made yet stays a null value until it is. That is what lets a copy be abandoned half-built: the destructor finds nothing but complete values and null ones. Nothing tested it. Failing an allocation part-way through a copy of a deeply nested value does, with the allocator the file already has for exactly this kind of test. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The code scanning job reports CWE-362 - "check when opening files" - for a test that opens no files: Flawfinder matched a local variable called open. Rename it and its partner. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
7bfb3aa to
bc01db0
Compare
|
|
||
| /// the number of levels an operation descends into before it finishes the | ||
| /// value below it without the call stack | ||
| static constexpr std::size_t nesting_depth_limit() |
There was a problem hiding this comment.
Isn't this only needed when JSON_NO_THREAD_LOCAL is not defined?
Also, I just realized that most of the other defines like this are #if 1/0 not "ifdef/ifndef`. Is that intentional? I have a feeling that is for ones that are detected but the user can override.
| @brief counts one level of a bounded descent for as long as it runs | ||
|
|
||
| The count is taken rather than looked up here, because the caller has looked | ||
| it up already to test it against the limit: reaching thread-local storage is |
There was a problem hiding this comment.
It is not required that the caller look it up first.
|
|
||
| if (JSON_HEDLEY_LIKELY(depth < nesting_depth_limit())) | ||
| { | ||
| const nesting_depth_guard guard(depth); |
There was a problem hiding this comment.
You say that nesting_depth_guard takes the depth because the caller has already looked it up, but that isn't necessary as I showed in my last comment below.
#ifndef JSON_NO_THREAD_LOCAL
const copy_depth_guard guard;
if (JSON_HEDLEY_LIKELY(guard.okay()))
{
copy_level(src);
return;
}
#endif
What & why
basic_json's copy constructor copies an object or array by handing the container to its own copy constructor, which copy-constructs every element and so reaches the copy constructor again — once per nesting level. A value nested deeply enough exhausts the call stack and terminates the process withSIGSEGV: no exception, nothing the caller can catch. Parsing such a value works (the parser is iterative), and so does destroying one (#1436 made destruction iterative) — copying never got the same treatment.This is reachable from untrusted input wherever a parsed value is copied, which is what makes it worth fixing rather than documenting: it is currently crashing llama.cpp's HTTP server (ggml-org/llama.cpp#27434), which copies client-controlled JSON.
Fixes #5387 for the copy constructor.
dump()is still recursive — that half is being handled in #5285, which already owns and restructuresserializer::dump().The approach: bound the descent, don't remove it
The obvious fix — copy every level through an explicit worklist — makes every copy pay for a problem only pathologically deep values have. I measured that version: 3% to 9% slower depending on the shape of the value, because it has to inspect the elements before it can pick a strategy.
So instead of taking the call stack away from the copy, this bounds how far it descends:
copy_depth_limit()(128) levels are copied exactly as they were — the containers copy their own elements, by far the fastest way to fill them.copy_iteratively).Copying can therefore no longer exhaust the stack, however deeply a value is nested, while a value nested less deeply than the bound — all but a vanishing minority — runs the very same code as before and pays only for one counter.
operator=takes its argument by value, so copy assignment is fixed along with it.The counter, and
JSON_NO_THREAD_LOCALThe level counter lives in
thread_localstorage, as one shared between threads would be raced. This is the library's first use ofthread_local, so it can be switched off: withJSON_NO_THREAD_LOCALdefined, objects and arrays go through the worklist right away. That yields the same values and is just as safe against deep nesting, but is measurably slower (see below). Documented as a macro page, with nav and index entries.Values copied while a copy is going on
The deferred values are completed before the copy they belong to returns. An earlier version of this deferred them to the outermost copy's worklist, which is slightly faster but means a value copied while another copy is in progress — by a custom base class, say — could be left as a null value when its constructor returns, and if it were a temporary, the outer copy would later write through a dangling pointer. The version here is not affected by the copy it is nested in; there is a test for it.
Measured
Medians of 9 interleaved runs, clang
-O3, versusdevelop:JSON_NO_THREAD_LOCALCopying a three-key object costs about ten nanoseconds more — the counter.
Public API impact
No breaking changes. No public signature, type, or exception changes;
copy_structuredand its helpers are private members. The one addition is the opt-out macroJSON_NO_THREAD_LOCAL, which is not defined by default, so existing builds are unaffected — except that they now usethread_localstorage, which is the one thing to weigh in review.No new requirements are placed on
ArrayType/ObjectType: the worklist path uses only the fill constructor and range constructor the library already relies on (json.hpp:4288,1153,1160).Verification
develop, byte-identical: 8000 randomized values (jsonandordered_json) across three dump modes and both equality operators; plus a second differential walking every node of a parsed corpus comparing type,start_pos()/end_pos(), diagnostic path and value. Both run under the default build,JSON_DIAGNOSTICS,JSON_DIAGNOSTIC_POSITIONS, both together, and withJSON_NO_THREAD_LOCAL— identical in every combination.developand pass here.JSON_DIAGNOSTICSand underJSON_NO_THREAD_LOCAL; ASan + UBSan clean; warning-clean under the CI clang flag set (-Weverything -Werrorminus the project's exclusions).make amalgamaterun; re-amalgamation is stable.Note that the deep-nesting tests deliberately compare
copy.dump()/walk the values by hand rather than usingoperator==: comparison recurses through the containers (json.hpp:3669) and would overflow the stack itself.JSON_NO_THREAD_LOCALswitches the descent off entirely, so every value goes down it - which is how theordered_mapkey-comparator bug in the follow-up was caught in the first place. The newci_test_no_thread_localtarget runs the full suite that way (109/109 tests pass); the macro had no build coverage at all before.JSON_DIAGNOSTICSreports paths from, and the positionsJSON_DIAGNOSTIC_POSITIONSreports. Neither was tested anywhere before this PR - and the positions were broken in an earlier revision of it. Both are now checked on either side of the bound, for objects and arrays. Removing either fix makes the new tests fail: positions at exactlydepth=129(with 1/127/128 still passing, pinning it to the iterative path), and the diagnostic pointer collapsing from/a/a/.../ato empty.Why this PR also splits a test file
tests/src/unit-regression2.cppis at the size limit the MinGW linker copes with, and the copy helpers push it over:its object grows 6.3% (4,654,128 -> 4,944,920 bytes at
-O0), and theclangjob on Windows then fails withrelocation truncated to fit: IMAGE_REL_AMD64_REL32 against '.rdata'.developlinks at the smaller size, so this iscaused by the change - but the file was already at the edge, and the job already carries a comment about the same test
hitting the same class of failure against
.debug_line.I first tried shrinking the object with
-O1in that job. It links - and then 39 of 102 tests segfault before doctestprints its first line, on both clang 11.0.1 and clang 18.1.8. That approach is reverted; a comment in the workflow now
records it so nobody tries it again.
The fix is to make the object smaller instead: the test cases after
TEST_CASE("regression tests 2")move to a newunit-regression3.cpp, which brings the object to 4,687,888 bytes - 0.7% above the size that links today rather than6.3%. Both files still build for C++11/17/20, and the same 9 test cases and 135 assertions run as before, just in two
binaries.
CONTRIBUTING.mdnow points new regression tests atunit-regression3.cpp.What still recurses after this PR
This PR fixes copying, and nothing else. These still recurse once per nesting level on user-controlled data, so #5387 should not be closed as fully fixed when this merges:
operator==,operator<,operator<=>json.hpp:3669operator<dump()serializer.hppto_cbor/to_msgpack/to_ubjson/to_bsonbinary_writer.hppbasic_json::diffjson.hpp:5089basic_json::merge_patchjson.hpp:5231json_pointer::flattenjson_pointer.hpp:861How other libraries handle this
Worth knowing where this sits, because the trade-off taken here is deliberately not the common one.
Most libraries reject deeply nested input rather than support it — a hard depth limit at parse time, which then protects everything downstream:
parse_options::max_depth= 32RecursionLimitExceededmaxNestingDepth1000 (500 in 3.0)System.Text.JsonJackson's limit exists because of CVE-2025-52999, and CVE-2026-29062 covers a parser that bypassed it; PostgreSQL patched its OAuth JSON parser the same way. Only a few libraries are iterative throughout instead: yyjson advertises "unlimited JSON nesting levels" as a headline feature, and miniserde gives its value type a non-recursive
Drop. A third approach is to grow the stack on demand, as serde_stacker does.This library sits in the worst spot of the three. Its parser is iterative, so it accepts input nested arbitrarily deeply — and then hands it to operations that cannot cope. lovasoa/bad_json_parsers, which measures exactly this, buckets it under "unlimited → segfault". A library with a parse limit never builds such a value in the first place.
The closest C++ peer has the same split and has fixed none of it. RapidJSON's
kParseIterativeFlagmakes only parsing iterative, and is opt-in because the recursive parser is faster; its destructor is still recursive (#2217, open, asking for exactly what #1436 did here in 2019),CopyFromstill overflows on deep values, andValue::Accept()— its serializer — still recurses.Two things that supports about the approach taken here: RapidJSON keeps recursion as the default because it is faster, which matches the measurement that writing every value iteratively costs up to 20%; and a bound of 128 sits in the same range as everyone else's hard limits — except that nothing is rejected at it, so this library stays strictly more permissive than all of them while no longer crashing.
JSON_NO_THREAD_LOCALmacro page, nav entry, and supported-macros overview)make amalgamate.Written by Claude Code.