Skip to content

serde_bser/mincode: validate wire-supplied lengths before indexing - #1420

Open
rootkiller6788 wants to merge 1 commit into
facebook:mainfrom
rootkiller6788:fix-bser-mincode-unchecked-lengths
Open

serde_bser/mincode: validate wire-supplied lengths before indexing#1420
rootkiller6788 wants to merge 1 commit into
facebook:mainfrom
rootkiller6788:fix-bser-mincode-unchecked-lengths

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

Closes #1417.

Both serde_bser (Watchman's BSER codec) and mincode take a length from the encoded data and use it to index into a buffer without validating it against the remaining input, so malformed/truncated payloads panic instead of returning a deserialization error.

serde_bser

  • Bunser::read_bytes casts a signed wire length straight to usize. A negative length (e.g. BSER_INT8 = -1) becomes usize::MAX, making the slice reader overflow on index + len (debug: attempt to add with overflow; release: inverted-range slice panic) and making the stream reader call scratch.resize(huge, 0) (capacity overflow). The fix rejects len < 0 in read_bytes with a new Error::DeNegativeLength variant.
  • IoRead::next_bytes grew the scratch buffer to the full wire length up front. The fix grows it in bounded 4 KiB chunks, so a declared length far larger than the stream yields an EOF error instead of a capacity-overflow panic.

mincode

  • Deserializer::read_slice called split_at(len) with a VLQ length read from the data; a length larger than the remaining buffer panicked with mid > len. It now returns an error.
  • deserialize_char indexed self.bytes[0] without checking for an empty buffer and sliced &self.bytes[..width] with width taken from the UTF-8 lead byte without checking the remaining length. Both paths now return an error.

Test plan

  • cargo test in watchman/rust/serde_bser (15 pre-existing + 2 new tests pass, debug and release).
  • cargo test in eden/scm/lib/mincode (1 pre-existing roundtrip + 3 new tests pass).
  • New tests cover: negative bytestring length (slice + stream readers), oversized string length, empty char, truncated multi-byte char.

BSER and mincode deserializers take lengths directly from the encoded
data and use them to index into buffers without validation, panicking
on malformed input instead of returning an error.

serde_bser:
- Reject negative bytestring/utf8string lengths in read_bytes instead of
  casting them to a huge usize.
- Grow the IoRead scratch buffer in bounded chunks instead of
  pre-allocating the wire length, avoiding a capacity-overflow panic when
  the declared length is far larger than the stream.

mincode:
- Reject string/bytes lengths that exceed the remaining input before
  calling split_at.
- Reject empty or truncated chars instead of indexing out of bounds.

Add regression tests for each panic.
@meta-cla meta-cla Bot added the CLA Signed label Aug 21, 2026
@meta-codesync

meta-codesync Bot commented Aug 21, 2026

Copy link
Copy Markdown

This pull request has been imported. If you are a Meta employee, you can view this in D116931726. (Because this pull request was imported automatically, there will not be any future comments.)

@rootkiller6788
rootkiller6788 marked this pull request as ready for review August 22, 2026 14:52
@rootkiller6788

Copy link
Copy Markdown
Author

The PR has been imported internally as D116931726.
Please let me know if any changes are needed, I will update my branch accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

serde_bser/mincode: wire-supplied lengths are used unchecked, panicking on malformed input

1 participant