fix: throw when MessagePack size exceeds uint32 limit (#5320) - #5381
ameliabarnabyhub wants to merge 2 commits into
Conversation
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
| */ | ||
| void assert_msgpack_size(const std::size_t size, const BasicJsonType* const context = nullptr) const | ||
| { | ||
| if (JSON_HEDLEY_UNLIKELY(!value_in_range_of<std::uint32_t>(size))) |
There was a problem hiding this comment.
Why do you need this test when this is only called when this is known to be true?
| /*! | ||
| @throw out_of_range.412 if @a size exceeds the MessagePack uint32 length limit | ||
| */ | ||
| void assert_msgpack_size(const std::size_t size, const BasicJsonType* const context = nullptr) const |
There was a problem hiding this comment.
Since this is always passed a pointer, don't need the default argument.
🔴 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 git apply amalgamation.patchThis does not require installing astyle yourself. |
nlohmann
left a comment
There was a problem hiding this comment.
Critical: this doesn't compile. tests/src/unit-msgpack.cpp line ~1991 — the new test does CHECK_THROWS_WITH_AS(_ = huge_json::to_msgpack(j), ...), but unlike every other use of this discard-variable pattern in the file, no json _; is declared in this TEST_CASE. Compiling a minimal repro against the PR's headers reproduces:
error: use of undeclared identifier '_'
This matches what's happening on the PR itself — every CI check is currently failing (gcc, clang, MSVC, macOS, sanitizers, fuzzing, etc.), all traceable to this one build break. Fix: add json _; (or reuse the pattern from the other test cases in this file) before the CHECK_THROWS_WITH_AS call.
Minor: test coverage gap. The fix in binary_writer.hpp adds the same assert_msgpack_size guard to four independent branches (string, array, binary, object), but the new test only exercises the array case. Consider adding cases for the other three so a future regression in one branch doesn't slip through untested.
The core fix logic itself looks correct — it throws out_of_range.412 exactly when the size exceeds UINT32_MAX, matching the existing BSON pattern (to_bson_length). I verified against develop that oversized values previously fell through silently with no length byte written at all (malformed MessagePack output), so this does fix a real bug — it just needs the test to actually compile.
Written by Claude Code on behalf of @nlohmann.
Fixes #5320
to_msgpack()now throwsout_of_range.412when a string, array, binary, or object reports a size aboveUINT32_MAX, matching the documented MessagePack limit and the BSON fix pattern from #5314.Adds a regression test using a container type that reports an oversized
size()without allocating multi-gigabyte data.