Skip to content

Throw type_error.321 when serializing discarded values to binary formats - #5761

Merged
nlohmann merged 2 commits into
developfrom
fix/discarded-binary-4714
Oct 6, 2026
Merged

nlohmann merged 2 commits into
developfrom
fix/discarded-binary-4714

Conversation

@nlohmann

@nlohmann nlohmann commented Oct 4, 2026

Copy link
Copy Markdown
Owner

The CBOR, MessagePack, UBJSON, BJData and BSON writers skip the payload of a discarded value nested in an array or object, but they still count it in the container size. For BSON they also write its entry header. The result is corrupt binary output that cannot be parsed back (#4714).

All five writers now throw type_error.321 ("cannot serialize discarded value to ") for a discarded value anywhere in the tree. For CBOR, MessagePack, UBJSON and BJData this includes a discarded value at the top level. For BSON, a top-level discarded value keeps throwing the existing type_error.317 ("top-level type must be object").

  • binary_writer.hpp: one JSON_HEDLEY_NO_RETURN helper, throw_on_discarded(), called from the discarded cases of write_cbor, write_msgpack, write_ubjson (UBJSON and BJData, including the optimized-array path) and the BSON value writer and size calculation. For BSON this case was wrongly treated as unreachable under LCOV_EXCL.
  • Docs: new type_error.321 entry in exceptions.md. The to_cbor/to_msgpack/to_ubjson/to_bjdata/to_bson pages list the new exception.
  • Tests in all five suites: a discarded value in an array, as an object value, nested deeper, and at the top level. Plus the optimized UBJSON/BJData arrays, and BSON. The old tests that expected the silent behavior now expect the exception.

This takes over #5382 by @ameliabarnabyhub, which went stale. The binary writers have changed a lot since then, so their change was redone on current develop, with them as the commit author. Review points from #5382:

The exception id is 321 because 318 is used on develop, and 319 and 320 are used by open PRs (#5630, #5632).

This will conflict with #5518 (non-recursive binary writers). Whichever PR lands second needs a small rebase.

Closes #4714. Supersedes #5382. Thanks @ameliabarnabyhub!

Public API

This changes behavior but no signatures. Serializing a discarded value to a binary format used to silently produce corrupt output. It now throws type_error.321 (type_error.317 for a top-level value in BSON). Code that serializes only regular values is not affected.

🤖 Generated with Claude Code

@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Oct 4, 2026
@nlohmann nlohmann assigned nlohmann and unassigned nlohmann Oct 4, 2026
@gregmarr

gregmarr commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

The CBOR, MessagePack, UBJSON, BJData and BSON writers skip the payload of a discarded value nested in an array or object, but they still count it in the container size. For BSON they also write its entry header. The result is corrupt binary output that cannot be parsed back (#4714).

Would writing null instead of discarded fix the issue without blocking otherwise-valid documents from being written?

Original issue says better to throw an error than to write null and mask an error:
#4714 (comment)

@nlohmann

nlohmann commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

As you found in #4714, writing null would silently produce a different document than the one the caller has. A discarded value only exists after a parse callback rejected it or after parse(..., false) failed, so serializing it is almost always a bug in the calling code. Throwing makes that visible instead of emitting something that round-trips to a value nobody asked for. Callers who really want null there can replace discarded values before serializing. The text serializer also treats discarded values as something that should not reach output (it writes <discarded>, which is not valid JSON either), so throwing is the consistent choice for the binary formats, where there is no such placeholder.

(Written by Claude Code.)

@gregmarr

gregmarr commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

so serializing it is almost always a bug in the calling code

I'll accept that.

ameliabarnabyhub and others added 2 commits October 6, 2026 07:25
The CBOR, MessagePack, UBJSON, BJData, and BSON writers silently
skipped the payload of a value_t::discarded value nested in an array
or object, while still writing its slot in the element/member count
(and, for BSON, its entry header), producing a binary document whose
declared size does not match what was actually written.

Throw type_error.321 instead, for a discarded value anywhere in the
tree, including at the top level.

Rewritten from the original PR against the current (non-recursive
option aside) binary_writer.hpp, which has changed substantially since
this was first proposed; the out_of_range.412 MessagePack size check
and unrelated test reformatting from that PR are dropped as out of
scope here.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Add docs for the new exception (home/exceptions.md and the Exceptions
sections of to_cbor/to_msgpack/to_ubjson/to_bjdata/to_bson) and test
coverage for a discarded value nested in an array or object, nested
deeper, and (for UBJSON/BJData) inside an optimized same-type array,
for each of CBOR, MessagePack, UBJSON, BJData, and BSON. Adjust the
three pre-existing "discarded" tests that asserted the old silent
behavior (empty/short output) to expect type_error.321 instead.

Co-authored-by: ameliabarnabyhub <312084480+ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the fix/discarded-binary-4714 branch from 94c5635 to d44d93e Compare October 6, 2026 05:25
@nlohmann nlohmann removed the review needed It would be great if someone could review the proposed changes. label Oct 6, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 6, 2026
@nlohmann
nlohmann merged commit 5379e04 into develop Oct 6, 2026
4 of 164 checks passed
@nlohmann
nlohmann deleted the fix/discarded-binary-4714 branch October 6, 2026 05:33
nlohmann added a commit that referenced this pull request Oct 7, 2026
With JSON_NOEXCEPTION, JSON_THROW expands to std::abort(), so Clang's
-Wunused-parameter breaks test-disabled_exceptions (since #5761).

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 7, 2026
* Fix CI on develop after #5585

- test-diagnostics-optimized: -O3 makes GCC's -Winline and
  -Wsuggest-attribute=pure/const warnings fire with the ci_test_gcc flag
  set; turn them off for this test.
- test-diagnostics-optimized: suppress Clang's -Wexit-time-destructors for
  the static table in to_json.
- Infer: raise pulse-max-disjuncts from 20 to 40. With the default,
  Pulse loses the stored type in basic_json::replace_value() and reports
  false null dereferences of get_ptr() results in unit-pointer_access.cpp.

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

* Ignore Infer's false USE_AFTER_DELETE in ordered_map::erase

Infer's std::string model keeps the buffer of a moved-from string, so the
destroy-and-reconstruct loop in erase(first, last) looks like it destroys a
buffer twice.

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

* Mark throw_on_discarded()'s parameters as used without exceptions

With JSON_NOEXCEPTION, JSON_THROW expands to std::abort(), so Clang's
-Wunused-parameter breaks test-disabled_exceptions (since #5761).

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

* Skip the span_input_adapter sax_parse checks with deleted deprecated functions

The #5676 regression test (#5740) calls the deprecated
sax_parse(span_input_adapter&&, ...), which JSON_DELETE_DEPRECATED_FUNCTIONS
deletes, so ci_test_delete_deprecated_functions failed to build.

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

* Fall back to the first entry in test-diagnostics-optimized's to_json

clang-tidy (clang-analyzer-security.ArrayBound) flagged it->second for a
value not in the table. Use the same fallback as
NLOHMANN_JSON_SERIALIZE_ENUM; the test still fails with -Werror=array-bounds
on the headers from before #5585.

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

* Fix clang-tidy findings in tests from #5762 and #5774

- unit-regression2.cpp (#5762): const/auto for the destroy() test values;
  NOLINT the intended copy in check_destroy_edge_case().
- unit-serialization.cpp (#5774): build the expected strings with += instead
  of chained operator+ (performance-inefficient-string-concatenation).

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

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Binary formats invalid encoding for <discarded> values in arrays and objects

3 participants