Skip to content

Stop binary writers overflowing the stack on deep values - #5781

Merged
nlohmann merged 2 commits into
developfrom
binary-writer-iterative
Oct 7, 2026
Merged

nlohmann merged 2 commits into
developfrom
binary-writer-iterative

Conversation

@nlohmann

@nlohmann nlohmann commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Takes over #5518 by @ayush-singh-0601 (thank you!), rebased on develop and with the open review comments addressed.

to_cbor, to_msgpack, to_ubjson and to_bjdata recursed once per nesting level. Parsing doesn't recurse, so a value the library happily builds could still overflow the stack when serialized. This is the crash in OSS-Fuzz issue 566583014 (parse_cbor_fuzzer, stack overflow in binary_writer::write_cbor).

Each writer now uses the same bounded descent as dump(): it recurses as before up to detail::recursion_depth_limit() levels and writes anything deeper with an explicit heap stack. The output is the same bytes as before.

Commits

  1. Stop binary writers overflowing the stack on deep values. The author's commit cf65ac4, cherry-picked with authorship kept and conflicts with develop resolved (write_cbor_head from Remove unreachable branches from the binary writer #5583, to_msgpack_length from Throw instead of writing MessagePack lengths beyond UINT32_MAX #5584, is_bjdata_excluded_type_marker from Stop allocating the BJData excluded-marker list per container #5555). The author's second commit 7301b06 isn't included because it only changed CBOR prefix helpers that write_cbor_head replaces.
  2. Redesign the iterative binary writers around a shared recursion depth limit. Addresses the open review comments:
    • Uses detail::recursion_depth_limit() instead of a new limit, and write_cbor_head instead of the CBOR prefix helpers.
    • MessagePack array and map headers are written by one helper per container type, shared by both paths, through to_msgpack_length (out_of_range.412 above UINT32_MAX).
    • Frames hold only containers, and a container's header is written when its frame is pushed, like bson_frame. There is no started flag. The UBJSON frame stores only the value, prefix_required and the iterator.
    • is_bjdata_ndarray() is shared by the recursive path and the iterative path.
    • Discarded values throw type_error.321 on both paths. Strict UTF-8 key checks report the same diagnostics context on both paths.
    • The doc comments refer to the public to_* functions instead of to themselves.
    • Tests: round trips instead of size checks. Byte-exact output at depths limit−1 to limit+2 for CBOR, MessagePack and UBJSON. A deep object, a deep BJData ND-array and a deep discarded value. The OSS-Fuzz regression (200,000 nested arrays) for CBOR and MessagePack.

BSON isn't in this PR. Its writer stopped recursing in #5553.

Verification

  • Output is byte-identical to develop on random json and ordered_json values nested 120–140 levels deep, for CBOR, MessagePack, UBJSON and BJData in every use_size/use_type combination.
  • 1,000,000-deep arrays and objects serialize with all four writers on a 1 MiB stack.
  • unit-large_json.cpp passes in C++11 and C++17 on a 1 MiB stack. No -Weverything warnings in binary_writer.hpp.

Public API

No breaking changes. Serialization output is unchanged. Values nested deeper than the recursion limit no longer crash.

CI on this PR is expected to fail until #5779 fixes develop.

Closes #5392
Closes #5518


🤖 Generated with Claude Code

@github-actions github-actions Bot added aspect: binary formats BSON, CBOR, MessagePack, UBJSON L tests labels Oct 7, 2026
ayush-singh-0601 and others added 2 commits October 7, 2026 16:40
to_cbor, to_msgpack, and to_ubjson recurse once per nesting level.
The parser is iterative, so a value the library accepts can crash on
the way back out.

Keep the existing recursive path for the first 128 levels and finish
anything deeper on a heap stack. Output is unchanged. BSON is left
alone because its extra size walk is a separate change.

Rebased onto the value-type output sink. The heap frames now initialize
every member, which is what -Weffc++ was rejecting.

See #5392.

Signed-off-by: ayush-singh-0601 <singhayush062006@gmail.com>
(cherry picked from commit cf65ac4)
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
… limit

Address the open review on the non-recursive CBOR/MessagePack/UBJSON/BJData
writers (#5518):

- Delete the CBOR array/object prefix helpers; both the recursive and
  iterative paths call write_cbor_head(), which already existed on develop.
- MessagePack: share one write_msgpack_array_prefix()/write_msgpack_object_prefix()
  helper per container kind between the recursive and iterative paths, both
  going through to_msgpack_length() so an over-long container throws
  out_of_range.412 identically either way.
- Reuse detail::recursion_depth_limit() instead of a separate constant, the
  same bound serializer::dump() and write_bson_document() already use.
- Redesign the frames after bson_frame/dump_frame: only a container with
  elements is ever pushed, its header is written at the point it is pushed,
  and the iterator is set in the frame's constructor instead of a
  default-then-assign two-step with a since-removed "started" flag. The
  UBJSON frame keeps only the value pointer, the per-element prefix_required
  flag, and the iterator; write_closer and is_object are no longer stored,
  since the former is always !use_count (use_count is constant for the whole
  document) and the latter follows from value->is_object().
- Factor the BJData ND-array shape check into is_bjdata_ndarray(), used by
  both the recursive object case and the iterative pushing logic.
- Give the frame classes the GCC -Weffc++ treatment already used for
  diff_frame: a noexcept converting constructor plus the five special members
  defaulted with no explicit noexcept.
- Fix two @ref self-references in write_cbor/write_msgpack/write_ubjson's own
  doc comments to point at the public to_cbor/to_msgpack/to_ubjson/to_bjdata
  API instead.
- The iterative object-key write for CBOR/MessagePack now runs the same
  strict-mode check_utf8() against the parent object as diagnostics context
  that the recursive path already ran, so the two paths raise identical
  diagnostics across the switch-over.
- Rewrite the tests: round trips instead of a bare size check, byte-exact
  comparisons against the recursive output at depths around the bound, a
  deep object and a BJData ND-array past the bound, a deep discarded value
  (type_error.321), and the OSS-Fuzz 566583014 CBOR/MessagePack regression.

BSON is unaffected by this change; it already walks its documents
iteratively and is covered separately by #5553.

Co-authored-by: ayush-singh-0601 <179524189+ayush-singh-0601@users.noreply.github.com>
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the binary-writer-iterative branch from b8d7d56 to 3f8fee0 Compare October 7, 2026 14:44
@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Oct 7, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 7, 2026
@nlohmann
nlohmann merged commit 43fe892 into develop Oct 7, 2026
57 of 171 checks passed
@nlohmann
nlohmann deleted the binary-writer-iterative branch October 7, 2026 17:18
nlohmann added a commit that referenced this pull request Oct 7, 2026
Resolve the conflicts with #5781 (iterative binary writers) and the
BJData ND-array change by taking develop's code, then name its exception
ids and use invalid_byte() for the moved _ArrayType_ check.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 8, 2026
Two test problems from #5781 broke CI on develop: CAPTURE(depth); trips
clang's -Wextra-semi-stmt, and the type_error.321 messages did not
account for the diagnostics path prefix.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 8, 2026
* Use the with_*_t aliases in tests, examples, and docs

Replace spelled-out basic_json<...> instantiations that only change one
or two template parameters with nlohmann::json::with_*_t (or
ordered_json::with_*_t when the object type is ordered_map). Types that
change all three number types chain with_integers_t and with_float_t.

The raw basic_json<...> spelling stays where the template parameter
list itself is the subject: the alias tests in unit-udt.cpp, explicit
instantiations, and the ordered_json/compile-time docs.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Fix unit-large_json for clang and JSON_DIAGNOSTICS

Two test problems from #5781 broke CI on develop: CAPTURE(depth); trips
clang's -Wextra-semi-stmt, and the type_error.321 messages did not
account for the diagnostics path prefix.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Use static_cast in unit-hash for clang-tidy

#5772 added functional casts that clang-tidy reports as C-style casts
(google-readability-casting). Also append a char instead of a
one-character string in unit-large_json.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Declare the expected message prefix const in unit-large_json

Without JSON_DIAGNOSTICS the prefix was never modified, which
clang-tidy reports (misc-const-correctness).

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 11, 2026
* Stop the BON8 writer overflowing the stack on deep values

to_bon8() recursed once per nesting level, so a value the iterative
BON8 reader accepts (e.g. ~24k nested one-element arrays) crashed on
the way back out. #5781 bounded the CBOR, MessagePack, and UBJSON/BJData
writers, but BON8 was merged before it and was not covered.

Apply the same scheme: recurse for the first recursion_depth_limit()
levels, then finish the value with write_bon8_iterative, which keeps
the open containers on a heap stack (reusing binary_container_frame)
and writes the 0xFE closer when it leaves a container with more than
four elements. The output is byte-for-byte unchanged.

Fixes https://issues.oss-fuzz.com/issues/572238015

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Merge the array and object branches of the BON8 iterative writer

write_bon8_iterative and write_bon8_value_or_push handled arrays and
objects in separate branches that repeated the end-of-container check,
the 0xFE closer and the marker computation. Share those parts and branch
only where arrays and objects really differ. The output is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hz7VJi1FTKr6gpseLErbbS
Signed-off-by: Niels Lohmann <mail@nlohmann.me>

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aspect: binary formats BSON, CBOR, MessagePack, UBJSON L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stack overflow in binary format writers (CBOR/MessagePack/UBJSON/BSON) due to unbounded recursion

3 participants