Fix to_msgpack() reading the inactive number union member - #5694
Conversation
basic_json stores number_integer and number_unsigned in a union, and number_unsigned_t only has to be at least as wide as number_integer_t (with the default types, both are 64-bit and have the same representation). When number_integer_t is narrower, write_msgpack() read the wrong union member in two places: - The number_unsigned case wrote number_integer's bits instead of number_unsigned's, silently writing the wrong value whenever it did not fit in number_integer_t. - The number_integer case (non-negative branch) picked the encoded width by comparing number_unsigned's bits, which is undefined behavior, though the value written was still number_integer's, so at worst a too-wide encoding was chosen. Read the active member in both cases, like the other binary writers (CBOR, UBJSON, BJData, BSON, BON8) already do. Fixes #5644. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
| // signed integers and unsigned integers. Therefore, we used | ||
| // the code from the value_t::number_unsigned case here. | ||
| if (j.m_data.m_value.number_unsigned < 128) | ||
| if (static_cast<typename BasicJsonType::number_unsigned_t>(j.m_data.m_value.number_integer) < 128) |
There was a problem hiding this comment.
Is it worth only casting once?
auto value_as_unsigned = static_cast<typename BasicJsonType::number_unsigned_t>(j.m_data.m_value.number_integer) ;
if (value_as_unsigned < 128)
Addresses review comment by @gregmarr. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
| // signed integers and unsigned integers. Therefore, we used | ||
| // the code from the value_t::number_unsigned case here. | ||
| if (j.m_data.m_value.number_unsigned < 128) | ||
| const auto value_as_unsigned = static_cast<typename BasicJsonType::number_unsigned_t>(j.m_data.m_value.number_integer); |
There was a problem hiding this comment.
Does this need to be static_cast<std::make_unsigned<typename BasicJsonType::number_integer_t>> to prevent truncation if number_unsigned_t is smaller than number_integer_t?
There was a problem hiding this comment.
Good thought, but that configuration can no longer be instantiated: since #5443, basic_json has static_assert(sizeof(NumberUnsignedType) >= sizeof(NumberIntegerType), ...), so every non-negative number_integer_t value fits in number_unsigned_t and the cast can't truncate. std::make_unsigned<number_integer_t> would be the same width or narrower, so it wouldn't add anything here. I verified this by trying to add a test with number_integer_t = std::int64_t, number_unsigned_t = std::uint32_t — it fails to compile on that assertion.
(Reply written by Claude Code on behalf of @nlohmann.)
There was a problem hiding this comment.
Good thought, but that configuration can no longer be instantiated:
Perfect.
Conflicts: - include/nlohmann/detail/output/binary_writer.hpp: kept write_msgpack_unsigned() for both msgpack integer cases; it already reads the active union member, which is what develop's #5694 fixes in the old ladders - single_include/nlohmann/json.hpp: regenerated with make amalgamate Signed-off-by: Niels Lohmann <mail@nlohmann.me>
* Fix CI configuration broken by recent merges and tool updates - gcc_flags.cmake: drop -Wexperimental-fmv-target, which GCC 16 accepts only on aarch64; amd64 rejects it, so every GCC job failed while checking the compiler. - ci_get_cmake: add VERBATIM so the checksum pipeline is passed to the shell intact (the unescaped `$'` broke the generated Makefile and build.ninja, failing ci_cmake_flags and ci_module_cpp20); match the SHA-256 entry case-insensitively, as CMake 3.5.0 lists the archive as "Linux-x86_64"; and unpack with --strip-components, as that archive's top-level directory is spelled "Linux" too. - ci_single_binaries: compile json.hpp's TU without IWYU's --error, as the comment above the gate already intends. - tests: restore -Wno-deprecated-declarations for all non-MSVC compilers (#5737 kept it for GCC only), as several tests call deprecated functions on purpose; include thirdparty/fifo_map as SYSTEM. - .clang-tidy: set misc-use-internal-linkage.AnalyzeTypes to false; clang-tidy 22.1 extended the check to classes and enums and flagged 100 test helper types. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Fix library warnings and a JSON_DIAGNOSTICS parent bug - binary_reader: pass integers to sax->number_integer() through conditional_static_cast<number_integer_t>, making the existing narrowing for a narrow number_integer_t explicit (MSVC C4244 and GCC -Wconversion/-Warith-conversion with the int16_t test from #5694); mark two Infer DEAD_STORE false positives with @infer-ignore. - to_json: set the parents of an array built from a C++20 range view after all elements are in place; a reallocating push_back moved the earlier elements and left their parent pointers stale, failing the JSON_DIAGNOSTICS invariant assertion. - json.hpp: suppress MSVC C4127 for the new is_ordered_map check in diff(), like the three existing ones; spell out std::formatter::parse's return and iterator types for clang-tidy 22.1. - number_parse: make the Eisel-Lemire digit counter unsigned (GCC -Wstrict-overflow). - string_utils: take encode_utf8's callable by const reference (cppcoreguidelines-missing-std-forward) and drop a \u from its doc comment (-Wdocumentation-unknown-command). - ordered_map: include <memory> for std::allocator (cpplint). Ran make amalgamate. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Fix tests failing in CI on develop - unit-allocator: skip the #5640 test under MSVC STL iterator debugging, where containers allocate a debug proxy in noexcept move constructors and a failing allocation terminates; move a decrement out of an if condition (bugprone-inc-dec-in-conditions). - unit-conversions: expect the "(/0)" path with JSON_DIAGNOSTICS; compare strict enums via get<>() rather than through the noexcept operator==(ScalarType, json), which bugprone-exception-escape flags. - unit-alt-string: suppress -Wexit-time-destructors for the strict enum macro and misc-use-internal-linkage for its enum. - unit-bjdata: call the static lookup functions through the type and pass unsigned char (-Wsign-conversion on amd64). - Mark Infer false positives with @infer-ignore in unit-diagnostics, unit-pointer_access, unit-udt, and unit-conversions. - Smaller clang-tidy 22.1 findings in unit-class_parser, unit-constructor2, unit-custom-base-class, unit-locale-cpp, and unit-noexcept. Signed-off-by: Niels Lohmann <mail@nlohmann.me> --------- Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Summary
to_msgpack()reads the wrong member of thenumber_integer/number_unsignedunion inwrite_msgpack()whennumber_integer_tis narrower thannumber_unsigned_t(e.g.basic_json<..., std::int32_t, std::uint64_t, ...>).In the
number_unsignedcase, every branch wrotenumber_integer's (inactive, narrower) bits instead ofnumber_unsigned's, silently producing the wrong value whenever it did not fit innumber_integer_t. In thenumber_integercase (non-negative branch), the encoded width was chosen by comparingnumber_unsigned's(inactive) bits, which is undefined behavior, though the value actually written was still
number_integer's.With the default
std::int64_t/std::uint64_tpair both members have the same width and representation, so theoutput was unaffected there.
Changes
include/nlohmann/detail/output/binary_writer.hpp: inwrite_msgpack(), read the active union member in boththe
number_integerandnumber_unsignedcases (matching CBOR, UBJSON, BJData, BSON, and BON8, which alreadydo).
docs/mkdocs/docs/api/basic_json/to_msgpack.md: add a "Version history" entry for the fix.single_include/nlohmann/json.hpp: regenerated viamake amalgamate.Tests
Added
tests/src/unit-msgpack.cpp/TEST_CASE("MessagePack numbers use the active union member (see #5644)"),using
basic_json<std::map, std::vector, std::string, bool, std::int32_t, std::uint64_t, double>(and anstd::int16_tvariant) to reproduce the issue's two examples (6442450944and4294967496) plus the issue's16-bit example (
98304): each value must encode to the documented correct bytes and round-trip throughfrom_msgpack(). A fourth section checks a default-type (std::int64_t/std::uint64_t) value is unaffected.include/fromdevelop@633de8e44, with thesame three assertions failing with exactly the corrupted values the issue reports:
18446744071562067968,200, and4294934528).tests/src/unit-msgpack.cppwith the fix for C++11 and C++17(
clang++ -O1 -g -Wall -Wextra -Werror -fsanitize=address,undefined -fno-sanitize-recover=undefined): all testcases pass, no sanitizer reports.
Public API
No breaking changes. This only fixes the bytes
to_msgpack()writes forbasic_jsoninstantiations wherenumber_integer_tis narrower thannumber_unsigned_t; no signatures or types changed.Fixes #5644
This PR was written by Claude Code on behalf of @nlohmann.
🤖 Generated with Claude Code