Fix stack overflow in binary format parsers (CBOR/MessagePack/UBJSON/BSON)

[Gooh456]

What

Closes #5104.

The binary format parsers in binary_reader.hpp parse nested arrays/objects
with unbounded recursive descent (unlike the text JSON parser, which is
iterative). A deeply nested but otherwise tiny CBOR/MessagePack/UBJSON/BSON
document therefore exhausts the native call stack and crashes the process
instead of failing gracefully — a DoS vector for anything that parses
untrusted binary input.

This adds a small depth_guard RAII helper that tracks container-nesting
depth on binary_reader and rejects input nested deeper than a fixed
max_depth (1024) with a clean parse_error (new id 116) instead of
recursing further, mirroring the fix suggested in the issue. The guard sits
at each actual recursive entry point:

• parse_cbor_internal
• parse_msgpack_internal
• parse_bson_internal (this also covers nested BSON sub-documents, since
  those recurse back into parse_bson_internal via
  parse_bson_element_internal)
• get_ubjson_value — this is the real recursive entry point for
  UBJSON/BJData, not parse_ubjson_internal (which is a thin wrapper); the
  "optimized homogeneous array" code path in get_ubjson_array/
  get_ubjson_object calls get_ubjson_value directly without going
  through parse_ubjson_internal, so guarding only the wrapper would have
  missed that path.

single_include/nlohmann/json.hpp was regenerated via tools/amalgamate,
and the new parse_error.116 is documented in
docs/mkdocs/docs/home/exceptions.md.

Testing

• Reproduced the crash for real on unpatched develop: a 100k-deep
  CBOR/MessagePack/UBJSON/BSON payload each terminate the process with
  STATUS_STACK_OVERFLOW (Windows exit code 0xC00000FD), matching the
  reported segfault.
• Rebuilt against the patched header: all four formats now throw a clean
  [json.exception.parse_error.116] instead of crashing; process exits 0.
• Added regression tests to unit-cbor.cpp, unit-msgpack.cpp,
  unit-ubjson.cpp, and unit-bson.cpp, one per format, each building a
  deeply-nested payload and asserting the clean parse_error (plus the
  is_discarded() non-throwing-mode path). Verified each test reproduces
  the crash against unpatched code before confirming it passes against the
  fix.
• Compiled and ran the full unit-cbor/unit-msgpack/unit-ubjson/
  unit-bson test binaries (g++ 16, -std=c++17) against both the patched
  tree and a pristine baseline (git stash): the patched build shows the
  exact same pre-existing 8 failing test cases as baseline (all due to the
  external json_test_data fuzz corpus not being present in this
  checkout, unrelated to this change) and zero new failures, with exactly
  the expected 8 additional passing assertions from the new tests
  (3,549,340 passed on patched vs 3,549,332 on baseline).
• Confirmed legitimate moderate nesting (well under the new limit) and a
  normal CBOR round-trip (to_cbor/from_cbor) still parse correctly, so
  the fix doesn't introduce false positives on realistic input.

I'm not a maintainer/regular contributor here — happy to adjust the chosen
max_depth value, error wording, or guard placement if you'd prefer a
different approach (e.g. a configurable limit via a new parameter instead
of a fixed constant).

[github-actions]

🔴 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 amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

@Gooh456 Please read and follow the Contribution Guidelines.

[Gooh456]

Pushed a fix for the amalgamation check: it's exactly the patch the "Check amalgamation" workflow generated as its artifact (pure astyle formatting in a couple of pre-existing regions unrelated to the actual change), applied as-is.

The Windows/Ubuntu CI failures are a more substantive, real finding I want to flag rather than paper over. test-bson_cpp11, test-cbor_cpp11, test-msgpack_cpp11/_cpp17, and test-ubjson_cpp11 are crashing with a genuine SIGSEGV - Stack overflow on the Windows runners (clang-cl and, per the msvc-arm64/vs2026/clang failures on the other job, likely other Windows configs too) — inside the new regression tests themselves, at nesting depths designed to trip max_depth (1024), not below it.

Root cause, from reading the recursion structure: depth_guard only increments once per logical nesting level (in parse_bson_internal/get_cbor_internal/etc.), but each logical level actually costs 3 real native stack frames before recursing again — e.g. for BSON: parse_bson_internal → parse_bson_element_list → parse_bson_element_internal → back into parse_bson_internal. So max_depth = 1024 really means ~3072 real stack frames before the guard fires, and in an unoptimized/Debug build (MSVC Debug, clang-cl Debug — the configs these CI jobs use) each of those frames is apparently large enough that 1024 is enough to exhaust a default ~1MB Windows thread stack before the check ever gets evaluated.

This is exactly why local verification can miss this: I verified the fix against an optimized MinGW g++ build on Windows, where frames are small enough that even ~100k logical levels didn't overflow — a materially different stack-cost-per-frame than what upstream's Debug-mode Windows CI matrix hits. Good catch by your CI, basically.

I don't currently have an MSVC/clang-cl Debug toolchain available to compile-and-verify a corrected max_depth (or an alternative mitigation — e.g. checking chars_read isn't enough, but something like a much lower default, or a real stack-space probe) before pushing it, and I'd rather not guess at a fix and hope it's right. Flagging this precisely so it's visible instead of silently sitting red — happy to follow up with an actual verified fix once I can test against a comparable Debug/Windows configuration, or if a maintainer wants to just pick a conservative constant directly, I'm not attached to 1024.

[Gooh456]

Follow-up on the CI failure flagged above: confirmed it's a real stack overflow (SIGSEGV) in test-bson_cpp11 under MSVC Debug (msvc Debug x64, msvc-arm64 Debug, msvc-vs2026 Debug x64 all hit it, which triggered fail-fast and cancelled the rest of the matrix). Root cause: recursing to depth=1024 itself already overflows a 1 MiB default thread stack in unoptimized/Debug builds, before the guard can reject it.

Pushed ce8906d lowering max_depth to 128 (a 4x margin under the depth that crashed). No MSVC available here, so rather than guess a "safe enough" number, I kept it conservative and I'm relying on this branch's next CI run (real Windows/MSVC runners, which reproduced the original crash) to confirm it's actually fixed rather than asserting that myself. Locally verified with g++/C++17 that the four depth-limit regression tests still pass with their expected byte offsets updated to match the new limit, and that nothing else in the suite regressed.

[Gooh456]

My last fix for the Debug crash was too aggressive - I picked 128 without checking whether it was actually high enough for real data, and it wasn't. It broke the library's own roundtrip fixture (sample.json, 458 levels deep), which is why CI just lit up across basically every job instead of just the Debug ones.

Pushed a fix: 600 instead of 128, which covers that fixture with real margin and stays well clear of the 1024 that caused the original overflow. Downloaded the actual fixture this time and measured its real depth instead of guessing (458), so this number's grounded in something concrete rather than a second guess.

Sorry for the noise on this one - should've checked what depth real input needs before picking a number the first time around.

[Gooh456]

Pushed a fix for the clang-cl-12 (x64) Windows failure (test-ubjson_cpp11 crashing with a stack overflow on the #5104 regression test), plus rebased all commits to add DCO sign-off.

On the stack overflow: I want to be clear this wasn't a missing or bypassed depth check. I went through every recursive call site in the UBJSON/BJData path (get_ubjson_array, get_ubjson_object, and the optimized $type#count container path) and depth_guard/max_depth are applied correctly everywhere - CBOR, MessagePack and BSON all pass their equivalent depth-limit test at the same max_depth=600, so the guard itself is fine.

The actual problem is that reaching the same depth of 600 costs UBJSON meaningfully more real stack than it costs the other three formats. CBOR and MessagePack fold "fetch+dispatch the next value" and the depth guard into a single function (parse_cbor_internal/parse_msgpack_internal), so their recursive chain is two stack frames per nesting level (that function plus get_cbor_array/get_msgpack_array). UBJSON splits this into two separate functions - parse_ubjson_internal() (fetch the byte) calling get_ubjson_value() (the depth guard + the switch) - because get_ubjson_value() also has to be callable directly with an already-known type marker from the optimized $type#count array/object path, which the other formats don't have. That leaves parse_ubjson_internal() sitting on the stack as a genuine extra frame on top of get_ubjson_value() and get_ubjson_array()/get_ubjson_object() - three frames per nesting level instead of two.

I measured this with g++ -O0 -fstack-usage as a stand-in for MSVC Debug's similarly unoptimized frames: the UBJSON chain costs about 1488 bytes/level pre-fix vs. CBOR's ~1088 bytes/level - roughly 37% more stack for the same max_depth. That's enough to explain overflowing the 4 MB stack the test binaries already link with (/STACK:4000000, from the earlier #2955 fix) at depth 600 specifically for UBJSON.

Fix is just marking parse_ubjson_internal() JSON_HEDLEY_ALWAYS_INLINE so it collapses into its caller in every build, including unoptimized Debug builds where the compiler wouldn't otherwise bother inlining something this small. Checked with objdump that this actually eliminates the calls (2 real call instructions to it before, 0 after) rather than just hinting at it. max_depth itself is untouched, so the 458-level-deep legitimate roundtrip fixture that made 128 too low earlier is still fine.

No MSVC available in my environment, so I can't reproduce the exact CI crash, but I rebuilt and ran test-ubjson/test-cbor/test-msgpack/test-bson locally (g++ 16, Debug, both the multi-header and the regenerated single_include build) and everything passes, including the #5104 regression test and the legitimate deep-nesting roundtrip test. Only failures are the pre-existing external-test-corpus-not-downloaded gap and one unrelated vector::reserve failure in a CBOR RFC 7049 test that reproduces identically with or without this change.

[nlohmann]

The more I think about it - maybe a non-recursive parser would be a better fix. WDYT?

[Gooh456]

Agreed, and I've pushed that now.

I went back through the last few CI failures with that lens and yeah - every one of them was the same underlying problem wearing a different hat: max_depth was never actually a stack-safety mechanism, it was a proxy for one, and the proxy kept breaking because the real cost (native stack frames per nesting level) depends on the compiler, the optimization level, and incidental things like whether a helper function happens to get inlined. Tuning the number just moves where it breaks.

So: the container-parsing loops for CBOR, MessagePack, UBJSON/BJData, and BSON are no longer recursive. Each format's parser now runs one iterative loop with an explicit, heap-allocated stack of small "resume this container" frames - descending into a nested array/object pushes a frame and loops back around to parse its first child instead of calling back into itself, and finishing a container pops the frame and resumes the parent. Scalars (numbers/strings/binary) don't recurse into container parsing at all, so they're untouched. depth_guard and the per-call recursion counter are gone - there's nothing left for them to guard - and max_depth (raised to 100000) is now just a sanity/DoS cap checked against the explicit stack's size, not a stack-safety number. Native call-stack depth for parsing is now O(1) regardless of how deeply the input nests.

One CBOR-specific thing worth calling out: the tag-unwrapping path (0xD8-style tagged items) was also recursing through the same depth-guarded function, so a long chain of tags had the identical failure mode as nested arrays/objects, just less obviously. That's a plain loop now too.

While I was in there I also noticed BSON's depth guard only ever applied to nested objects (record type 0x03) - nested arrays (0x04) went through a separate path that never checked depth at all. The unified loop applies the cap to both now; added a regression test for that specifically since it wasn't caught by anything upstream.

What I verified this against:

• The full existing test suite (g++ 16, Debug, MinGW, on Windows) - all four binary-format suites plus BJData plus everything else, 94 test binaries - diffed assertion-by-assertion against a baseline build of the tree before this commit. Every binary matches the baseline's pass/fail counts exactly, aside from two new BSON assertions (for the array-nesting gap) which pass. The pre-existing gaps (external fuzz corpus not downloaded in this environment, one unrelated vector::reserve failure in a CBOR RFC 7049 test) reproduce identically in both, so nothing regressed and nothing was silently papered over.
• A throwaway smoke test at 500,000 levels of nesting for all four formats (~830x the old max_depth, and deep enough that no version of the recursive parser could have survived it at any max_depth) using a minimal SAX consumer - all four reject it cleanly via parse_error.116, no crash. Separately confirmed 50,000 levels of legitimate nesting parses to completion correctly, not just that deep input gets rejected.
• Regenerated single_include/nlohmann/json.hpp and reformatted with the pinned astyle version, matching the amalgamation-check workflow's exact steps.

Honest caveat: I don't have real clang-cl or MSVC available in this environment, so I can't reproduce the exact CI toolchain locally - that's exactly the failure mode this rewrite is meant to stop mattering, but I haven't watched it pass on the actual Debug clang-cl/MSVC jobs with my own eyes yet. CI on this PR needs a maintainer approval to run (standard for external PRs, it looks like), so I'd treat this as "strong local evidence, not yet independently confirmed on the real matrix" until those jobs actually go green.

[nlohmann]

👋 Heads up: I'm on vacation, so this review was produced with the help of an AI assistant on my behalf and I haven't hand-verified every detail yet. Apologies for the impersonal reply — I wanted to get you actionable feedback rather than leave the PR sitting. I'll go over it properly when I'm back.

Thanks a lot for the iterative rewrite and for the very transparent write-ups on each CI round — this is the right direction.

One substantive concern: the DOM API can still overflow on tiny nested input

The parser itself is now genuinely non-recursive (nicely done — a 500k-deep payload is rejected cleanly for all four formats). But the crash appears to just move one frame later, into the return path of the from_* entry points:

return res ? result : basic_json(value_t::discarded);

That ternary produces a prvalue, so result is copy-constructed, and basic_json's copy constructor is recursive. A validly-nested document below max_depth parses fine and then overflows the native stack on return.

Measured locally (Linux, 8 MB stack, -O2):

Input	Result
from_cbor, 40 000-deep arrays	OK
from_cbor, 60 000-deep arrays	SIGSEGV — recursive basic_json copy ctor, on return
json::parse, 200 000-deep text	OK (returns result directly, no copy)

So a ~60 KB CBOR file still crashes the process through from_cbor. On the Windows test binaries (linked /STACK:4000000) the threshold is roughly half that, and max_depth = 100000 sits comfortably above it — so the new guard never gets a chance to protect the DOM path.

Why current CI stays green: the regression tests here all exceed max_depth, so they throw/discard during parsing and the partial tree unwinds through the (iterative) destructor. The case that's still exposed is exactly the one #5104 is about: a valid, accepted, deeply-nested document that parses successfully and is returned.

Possible fixes:

1. Return by move — return res ? std::move(result) : basic_json(value_t::discarded); in from_cbor/from_msgpack/from_ubjson/from_bson/from_bjdata. I tested this and it raised the safe from_cbor depth from ~60k up to the full max_depth cap, matching json::parse.
2. And/or choose a max_depth that stays below the library's safe recursion depth on the smallest supported (4 MB) stack, treating it as a genuine sanity cap rather than 100000.

Caveat worth noting: even with the move, a caller copying / comparing / dump-ing a 100000-deep returned value hits the same recursive copy constructor. That's a pre-existing, library-wide limitation (also reachable via json::parse), so not this PR's job to solve — but it's another reason to lean toward a more conservative cap.

What looks good

• Iterative parser verified: native stack is O(1); 500k-deep input rejected cleanly, no crash, all four formats.
• Correctness spot-checks against this branch's header all passed: indefinite CBOR arrays/maps, empty indefinite containers, mixed definite/indefinite nesting, chained tags in ignore mode, half-float, UBJSON optimized $type#count arrays/objects, BJData ND-array (JData wrapping) and $B binary — plus ~20k random-structure round-trips across all six format variants (0 mismatches) and ~200k random-byte inputs (0 crashes / no non-json exceptions).
• Closing the BSON nested-array (0x04) depth gap, with a dedicated test, is a real bonus.

Minor

• The clang 20.1.8 CI red is a MinGW relocation truncated to fit linker error in unit-regression2 — a known large-object-file toolchain issue, unrelated to this diff.

Bottom line: great approach and the parser change is solid, but I'd hold off on merging it as "closes #5104" until the return-path copy overflow is handled — otherwise from_cbor and friends still crash on a small nested file.

---

Generated by Claude Code

[Gooh456]

Appreciate the thorough pass, and no worries about the AI-assisted disclaimer — the finding's real either way.

The move fix is right, and it's an obvious miss on my end — result is an lvalue in that ternary so it's a straight copy, and copying a deeply-nested basic_json is exactly as recursive as the thing this PR is supposed to kill. Pushing std::move(result) across all 14 from_cbor/from_msgpack/from_ubjson/from_bjdata/from_bson overloads that have this pattern. Agreed a caller doing their own copy/dump/compare on the returned value afterward is a separate, pre-existing, library-wide issue — not scoping that into this PR.

On "the clang 20.1.8 red is unrelated to this diff" — I don't think that's quite right, and wanted to flag it before it gets waved off. Checked develop's last several Windows CI runs and clang 20.1.8 is clean there, every time. The thing that's actually failing is a linker error (relocation truncated to fit: IMAGE_REL_AMD64_REL32) linking test-regression2_cpp17/_cpp20 — and unit-regression2.cpp is already your single biggest test file (~52KB, split off from regression1 before for size reasons) and it does exercise CBOR/BSON/MessagePack/UBJSON parsing. This PR's rewrite is a ~3.7k line diff to that exact code path. My read: regression2 was already sitting close to whatever size MinGW/clang's COFF backend can address, and the extra instantiated code from making the parser non-recursive tipped it over on some clang versions. Which version fails isn't even consistent build to build (20.1.8 twice, 14.0.6 once, clang-cl-12 once) — that's a "right on the edge" signature, not a stable pre-existing bug.

Pushed a shot at a CI-only mitigation: -fuse-ld=lld for test-regression2 under Clang+MinGW specifically, same pattern as the existing MSVC /STACK:4000000 override. lld lays out COFF sections differently and is the standard fix for this class of error elsewhere. Being upfront: I don't have a matching Windows/clang-mingw toolchain here to actually confirm this links, so treat it as a hypothesis backed by the log, not a verified fix — if it doesn't clear it, the fallback is splitting unit-regression2.cpp into a third file, which is more invasive but guaranteed to work.