Skip to content

Fix ext32 unpacking of the maximum UINT32_MAX-byte payload - #1184

Open
afonsojanu wants to merge 1 commit into
msgpack:c_masterfrom
afonsojanu:fix/ext32-uint32-max-length-overflow
Open

Fix ext32 unpacking of the maximum UINT32_MAX-byte payload#1184
afonsojanu wants to merge 1 commit into
msgpack:c_masterfrom
afonsojanu:fix/ext32-uint32-max-length-overflow

Conversation

@afonsojanu

Copy link
Copy Markdown

Fixes #1086.

The C unpacker tracks how many bytes are left to read for a variable-length value ("trail") by adding 1 to the wire-format length whenever it's an ext (the extra byte is the ext type tag). For ext8 and ext16 that stays comfortably inside a 32-bit range, but ext32's length field can legitimately be the full UINT32_MAX, and tmp + 1 computed in 32-bit arithmetic wraps around to 0 in that one case. The parser then treats the object as a zero-length ext and bails out immediately through template_callback_ext's l == 0 guard, so an ext sitting exactly at the wire format's own maximum size fails to unpack with MSGPACK_UNPACK_PARSE_ERROR, even though it's a perfectly valid encoding.

Fix: widen the internal trail field (and the length parameter the str/bin/ext callbacks receive it through) from unsigned int to size_t, and cast the ext32 length to size_t before adding 1 so the addition itself doesn't wrap on a 64-bit build. This is scoped to the C library specifically (src/unpack.c / unpack_template.h) - unpack_template.h is only ever included from that one file. On a 32-bit build the same boundary case is still unreachable, but that's the same limitation the C++ side already documents as intentional, and a 32-bit process couldn't address a buffer that size in the first place.

Verified with a small standalone reproducer (pack + unpack an ext with exactly UINT32_MAX bytes of data): fails with MSGPACK_UNPACK_PARSE_ERROR on the current code, succeeds with the correct type/size/contents after this change. Also ran a broader manual check across fixext/ext8/ext16/ext32 and str8/16/32 at several boundary sizes (0, 1, 16, 17, 255, 256, 65535, 65536 bytes) under both -Wall -Wextra -Werror and -fsanitize=undefined to make sure the wider type didn't change behavior anywhere else - all green.

Added a regression test (simple_buffer_ext_maxlen in test/msgpack_c.cpp) matching the repro from the issue. It needs roughly 8GB of free memory to allocate the UINT32_MAX-byte source buffer plus the packed copy, so it calls GTEST_SKIP() instead of failing if calloc comes back empty rather than assuming every machine running the suite has that much to spare.

The C unpacker's internal "trail" bookkeeping combines an ext value's
type byte with its data into a single byte count by adding 1 to the
length read off the wire. For ext8/ext16 that addition stays well
inside a 32-bit range, but ext32's length field can legitimately be
the full UINT32_MAX, and tmp + 1 done in 32-bit arithmetic wraps
around to 0. The parser then treats the object as a zero-length ext
and immediately errors out through template_callback_ext's l == 0
check, so any ext exactly at the wire format's own maximum size fails
to unpack with MSGPACK_UNPACK_PARSE_ERROR even though it's a
perfectly valid encoding.

Widened the trail field (and the str/bin/ext callback length
parameter that ends up receiving it) from unsigned int to size_t, and
cast the ext32 length to size_t before adding 1 so the addition
itself doesn't wrap on a 64-bit build. On a 32-bit build the same
edge case is still out of reach, but that matches the existing
intentional limitation on the C++ side and a 32-bit process can't
address a buffer that size anyway.

Added a regression test that packs and unpacks an ext with exactly
UINT32_MAX bytes of data; it skips itself if the host can't spare the
roughly 8GB of memory the test needs rather than failing outright.

Fixes msgpack#1086
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.

1 participant