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

[renz011tzar]

Two of Sapling's hand-rolled binary deserializers take a length from the encoded data and use it without validating it against the remaining buffer. Malformed or truncated input panics instead of returning a deserialization error.

Verified at 303c8f0b05d749fe542698fa1c75599b343d21d8 (current main). Both crates build standalone from a clean checkout, so the reproductions below run without building the full tree.

---

1. serde_bser — a negative wire length casts to usize::MAX

watchman/rust/serde_bser parses BSER, Watchman's binary wire protocol. The length of a bytestring/utf8string is read as a signed integer:

// src/de.rs:167 (and :179 for utf8string)
let len = self.bunser.check_next_int()?;   // -> i64
match self.bunser.read_bytes(len)? { … }

check_next_int accepts BSER_INT8/16/32/64 and returns Result<i64> with no sign check. It is then cast without validation:

// src/de/bunser.rs:92-93
pub fn read_bytes<'s>(&'s mut self, len: i64) -> Result<Reference<'de, 's, [u8]>> {
    let len = len as usize;      // -1i64 -> 18446744073709551615

Both Read implementations use that value directly:

// src/de/read.rs:155  (slice reader)
if self.index + len > self.slice.len() {

// src/de/read.rs:213  (stream reader)
scratch.resize(len, 0);

Reproduction

// tests/negative_length.rs
#[test]
fn negative_bytestring_length_is_rejected() {
    let mut msg: Vec<u8> = vec![0x00, 0x02];           // BSER magic
    msg.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]);  // capabilities
    msg.extend_from_slice(&[0x03, 0x03]);              // PDU length: INT8 = 3
    msg.extend_from_slice(&[0x02, 0x03, 0xFF]);        // BYTESTRING, INT8 length = -1

    let r: Result<String, _> = serde_bser::de::from_slice(&msg);
    assert!(r.is_err(), "expected a deserialization error");
}

Both profiles panic, in different places:

$ cargo test --test negative_length
thread '…' panicked at src/de/read.rs:155:12:
attempt to add with overflow

$ cargo test --release --test negative_length
thread '…' panicked at src/de/read.rs:158:35:
slice index starts at 11 but ends at 10

In release the addition wraps instead of trapping, so the bounds check on :155 passes for an out-of-range length and :158 is reached with an inverted range.

Which lengths are affected

Worth stating precisely, because it is not all of them. For a wire length v < 0 the cast yields v + 2^64, so self.index + len overflows exactly when self.index >= -v:

• INT8 = -1 overflows as soon as one byte has been consumed — always true by the time a bytestring is read, since the magic, capabilities and PDU length are already behind it. That is the case above.
• Large-magnitude values such as i64::MIN do not overflow this addition; they fail the bounds check and return an error correctly.

The stream reader has no such condition: scratch.resize(len, 0) at :213 requests at least 8 EiB for every negative length.

---

2. mincode — length and width used to slice without a bounds check

eden/scm/lib/mincode has the same shape without the sign issue:

// eden/scm/lib/mincode/src/de.rs:35-41
fn read_slice(&mut self) -> Result<&'de [u8]> {
    let len = Deserialize::deserialize(&mut *self)?;   // length from the data
    let (slice, rest) = self.bytes.split_at(len);      // panics if len > bytes.len()
    self.bytes = rest;
    Ok(slice)
}

deserialize_char has two further unchecked accesses — self.bytes[0] at :106/:108 with no emptiness check, and &self.bytes[..width] at :113, where width comes from the UTF-8 lead byte rather than from the remaining input.

Every other read path in that file goes through the checked helpers read_u8()? and read_vlq()?, which return Err on EOF. These three index directly.

Reproduction

// tests/truncated.rs
#[test]
fn truncated_string_returns_err() {
    let bytes: Vec<u8> = vec![10, b'a', b'b', b'c'];   // VLQ length 10, 3 bytes follow
    let r: mincode::Result<String> = mincode::deserialize(&bytes);
    assert!(r.is_err());
}

#[test]
fn char_on_empty_buffer_returns_err() {
    let bytes: Vec<u8> = vec![];
    let r: mincode::Result<char> = mincode::deserialize(&bytes);
    assert!(r.is_err());
}

#[test]
fn truncated_multibyte_char_returns_err() {
    let bytes: Vec<u8> = vec![0xF0];                   // 4-byte lead byte, 1 byte present
    let r: mincode::Result<char> = mincode::deserialize(&bytes);
    assert!(r.is_err());
}

All three panic:

$ cargo test --test truncated
thread '…' panicked at src/de.rs:37:40:  mid > len
thread '…' panicked at src/de.rs:106:37: index out of bounds: the len is 0 but the index is 0
thread '…' panicked at src/de.rs:113:51: range end index 4 out of range for slice of length 1

mincode is used by dag, metalog, revisionstore, filters, zstore and commands for on-disk repository data, so this is reachable from a corrupted or truncated store file.

---

Scope

Filing this as a correctness/robustness report rather than a security one. watchman_client connects over a UnixStream or a Windows named pipe — there is no TCP path, so no remote peer — and mincode reads repository files the process already trusts. In both cases the fix is the same: return Err instead of panicking.

Suggested fix

For serde_bser, validate before the cast — reusing the existing DeCustom variant, or adding a dedicated one:

pub fn read_bytes<'s>(&'s mut self, len: i64) -> Result<Reference<'de, 's, [u8]>> {
    let len: usize = len.try_into().map_err(|_| Error::DeCustom {
        msg: format!("negative length: {len}"),
    })?;
    …
}

and use a non-overflowing bounds check in the slice reader:

if len > self.slice.len() - self.index {
    bail!("eof while parsing bytes/string");
}

For mincode, return an error rather than slicing past the end:

if len > self.bytes.len() {
    return Err(Error::new("unexpected end of input"));
}
let (slice, rest) = self.bytes.split_at(len);

with equivalent guards on self.bytes[0] and ..width in deserialize_char.

I applied both fixes locally to check them: all four reproductions above turn into clean Err returns — serde_bser in debug and release — and the existing suites still pass (15 tests in serde_bser, the roundtrip test in mincode).

How this was found

Found while evaluating Rust verification tooling. Working the serde_bser case through the midas-lex guidance workflow identifies the spec owner here as a precondition: the decoded length must be non-negative and within the remaining buffer before it is used. Stating that obligation in Verus proves both halves — that the cast admits lengths no buffer can satisfy, and that a try_into() sign check plus the rearranged bounds check makes both read paths total.

That model is also where the "which lengths are affected" section came from: an earlier version of it asserted that any negative length overflows :155, and the verifier rejected that as too strong.

Happy to send a PR for either or both.