Repository navigation
fix(decode): MessageLen handles ext markers instead of panicking - #391
Open
prostomarkeloff wants to merge 1 commit into
Open
prostomarkeloff wants to merge 1 commit into
prostomarkeloff wants to merge 1 commit into
Conversation
MessageLen::read_one_item / read_marker_with_len hit todo!() for every ext marker (fixext1..16, ext8/16/32). Since MessageLen parses incoming, possibly untrusted stream bytes to find a message boundary, three bytes (e.g. a fixext1) panic len_of / incremental_len. Handle ext like the other length-prefixed types; fixext* skip the type byte plus fixed data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found with find-dup-defs using its
convergepass, which reports behavioural divergences between near-duplicate functions. It flaggedMessageLen's marker-dispatch (read_one_item/read_marker_with_len) as near-duplicates whoseextarms diverge from the rest — they aretodo!()while every other marker decodes a length.MessageLen::{len_of, incremental_len}therefore panic viatodo!()on any ext marker: both functions haveMarker::Ext8 | Ext16 | Ext32 => todo!()andFixExt1..=FixExt16 => todo!().extis a normal MessagePack type (timestamps arefixext4/fixext8), so e.g. the three bytesd4 01 2a(afixext1) panic the parser — in every build, not only with overflow checks.MessageLenis documented as a way to find a message boundary while reading a stream, i.e. it runs on incoming bytes that may be untrusted, so this is a denial of service on malformed input rather than just a missing feature.Fix
Route the length-carrying ext markers (
ext8/ext16/ext32) through the same length decode asStr/Bin/Array/Map—MarkerLen::size()already returns 1/2/4 for them, so resumable parsing keeps working — and skiptype byte + datafor the fixed-sizefixext*markers. Forext8/16/32the data skip islen + 1(the extra type byte);len < max_len <= u32::MAX, so the+ 1cannot overflow.Tests
Added
rmp/tests/repro_message_len_ext.rs: afixext1, anext8inside an array, a truncatedext16(expectsTruncated), and a mixed-size round-trip built withwrite_ext_metawhose length matches byte-for-byte both vialen_ofand via chunkedincremental_len. The fullrmptest suite (181 unit + 4 new + 13 doctests) passes.Disclosure: I used an AI-assisted tool while investigating and preparing this change; I have reviewed the fix and tests and can explain each line.