reject binary subtypes MessagePack and BSON cannot represent - #5366
reject binary subtypes MessagePack and BSON cannot represent#5366Angadi56 wants to merge 2 commits into
Conversation
Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>
nlohmann
left a comment
There was a problem hiding this comment.
Apologies that this is an AI-written review — it's from Claude Code. I don't have time for a deeper look myself at the moment, so please weigh it accordingly.
Thanks for this — the diagnosis is right and the fix is in the right place.
I checked every subtype() use in binary_writer.hpp, and the two you changed (binary_writer.hpp:691 and :1181) were indeed the only unguarded narrowing sites; the CBOR ladder at :310-330 correctly already covers the full range. Exception id 413 is unused (412 was the highest), value_in_range_of and <limits> are already in the TU, the helper mirrors to_bson_length's shape and sits in the right section, and the doc pages follow the existing conventions. make amalgamate produces no diff, so single_include is in sync.
unit-msgpack and unit-bson were built and run locally against the modular headers — the new sections pass. (test_data.hpp was stubbed to run offline, so the only failures seen were the file-reading cases the stub can't serve, not anything from this change.) Note the Actions matrix hasn't started yet, so the green checks so far are only the linters plus DCO.
One substantive point, on the MessagePack ceiling.
MessagePack's ext type is a signed int8: 0–127 are application-specific, −1 to −128 are reserved, and −1 is the predefined timestamp extension. So subtypes 128–255 land in reserved space too. Checking what we actually emit:
subtype 255 -> d6 ff ca fe ba be (ext type as int8 = -1)
That is the same timestamp collision you cite as the harm in truncating 511 — just reached directly instead of by truncation. So the patch stops a subtype from silently becoming a reserved type, but still permits one to be written as one.
That probably shouldn't change the ceiling here. Tightening MessagePack to 127 would break anyone using 128–255 today, and that's a wider decision than this PR. Scoping to uint8_t is defensible. But two bits of wording are worth adjusting so neither reads as an endorsement of 128–255:
- the test comment "the largest subtype the ext type field can hold still round-trips" — true inside this library, but a conformant reader sees a timestamp;
- the corresponding sentence in
to_msgpack.md.
to_binary_subtype already takes a format name, so a per-format maximum would be a small follow-up if we ever want it.
Minor: the check runs after the control byte and length are written, so the streaming to_msgpack(j, o) / to_bson(j, o) overloads emit partial output before throwing. That matches the existing 412 behavior, so it's not a regression — but for MessagePack the check could move up beside use_ext at :623 at no cost.
To be explicit for the changelog: this isn't an API break, but it is a behavior break — to_msgpack/to_bson now throw where they previously emitted truncated bytes. Same shape as 412 this cycle, and the 3.13.0 note covers it.
Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>
|
Good point on the signed ext type, I'd been thinking only about truncation and missed that 128..255 already sit in reserved space when written directly. Agreed the ceiling should stay at 255 here, since tightening to 127 would break anyone using those subtypes today, and to_binary_subtype already takes the format name so a per-format max stays a small follow-up if it's ever wanted. I reworded the test comment and the to_msgpack.md sentence so neither reads as blessing that range; both now say those subtypes round-trip within this library but a conforming reader sees a predefined extension (255 reads as the timestamp). Also moved the MessagePack check up beside use_ext, so the streaming overload no longer emits the control byte and length before throwing; I left the BSON site as-is since it matches the existing 412 placement. |
binary_tkeeps its subtype in astd::uint64_t, and the CBOR reader fills it straight from a tag (0xD8..0xDB) whencbor_tag_handler_t::storeis used, so values well past 255 arrive from ordinary input andto_cborround-trips them exactly. MessagePack ext types and BSON binary values only have a single byte for the subtype, and both writers narrowed the stored value with a bare cast instead of saying it does not fit, so the byte that came out was whatever the low eight bits happened to be. That is worse than a lossy conversion, because the truncated value lands on encodings the two formats reserve:from_cboron tag 511 followed by a four-byte string leavesto_msgpackas ext type -1, which MessagePack assigns to its predefined timestamp extension, and tag 258 leavesto_bsonas subtype 0x02, the "Binary (Old)" subtype whose payload is specified to carry its own int32 length, so a reader on the far side reframes bytes the sender picked. I noticed it lining the three writers up against each other, since CBOR spends four tag widths to keep the whole range while the other two quietly dropped the high bits. The fix followsto_bson_length, which already guards the length field of that same BSON element rather than truncating it: a small shared helper checks the subtype and raisesout_of_range.413, and both narrowing sites go through it. Subtypes 0 through 255 encode exactly as before in both formats, and CBOR and UBJSON are untouched, since one already covers the full range and the other does not write subtypes at all.make amalgamate.