From a348c9b00334ebed7a2f1f1c95c013d439e397d7 Mon Sep 17 00:00:00 2001 From: rootkiller6788 Date: Fri, 21 Aug 2026 16:54:37 +0800 Subject: [PATCH] serde_bser/mincode: validate wire-supplied lengths before indexing 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. --- eden/scm/lib/mincode/src/de.rs | 17 +++++++ eden/scm/lib/mincode/src/tests.rs | 36 ++++++++++++++ watchman/rust/serde_bser/src/de/bunser.rs | 5 ++ watchman/rust/serde_bser/src/de/read.rs | 30 +++++++----- watchman/rust/serde_bser/src/errors.rs | 3 ++ .../rust/serde_bser/tests/negative_length.rs | 48 +++++++++++++++++++ 6 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 watchman/rust/serde_bser/tests/negative_length.rs diff --git a/eden/scm/lib/mincode/src/de.rs b/eden/scm/lib/mincode/src/de.rs index bc89f6dc23dab..54744f7470d57 100644 --- a/eden/scm/lib/mincode/src/de.rs +++ b/eden/scm/lib/mincode/src/de.rs @@ -34,6 +34,13 @@ impl<'de> Deserializer<'de> { #[inline] fn read_slice(&mut self) -> Result<&'de [u8]> { let len = Deserialize::deserialize(&mut *self)?; + if len > self.bytes.len() { + return Err(Error::new(format!( + "length {} exceeds remaining input of {} bytes", + len, + self.bytes.len() + ))); + } let (slice, rest) = self.bytes.split_at(len); self.bytes = rest; Ok(slice) @@ -103,6 +110,9 @@ impl<'de, 'a> serde::Deserializer<'de> for &'a mut Deserializer<'de> { where V: Visitor<'de>, { + if self.bytes.is_empty() { + return Err(Error::new("EOF while deserializing char")); + } let width = utf8_char_width(self.bytes[0]); if width == 1 { return visitor.visit_char(self.bytes[0] as char); @@ -110,6 +120,13 @@ impl<'de, 'a> serde::Deserializer<'de> for &'a mut Deserializer<'de> { if width == 0 { return Err(Error::new("invalid char")); } + if width > self.bytes.len() { + return Err(Error::new(format!( + "truncated char: expected {} bytes, only {} remain", + width, + self.bytes.len() + ))); + } let res = match str::from_utf8(&self.bytes[..width]) { Ok(s) => s.chars().next().unwrap(), Err(err) => { diff --git a/eden/scm/lib/mincode/src/tests.rs b/eden/scm/lib/mincode/src/tests.rs index e8b76d8f788b4..97547b8d020ce 100644 --- a/eden/scm/lib/mincode/src/tests.rs +++ b/eden/scm/lib/mincode/src/tests.rs @@ -37,3 +37,39 @@ quickcheck! { foo == foo_deserialized } } + +#[test] +fn string_length_longer_than_input_is_rejected() { + // VLQ length 5 followed by a single byte: the declared string is longer + // than the remaining input. + let bytes = [0x05, b'a']; + let r: Result = crate::deserialize(&bytes); + assert!( + r.is_err(), + "expected an error for an oversized string length, got {:?}", + r + ); +} + +#[test] +fn empty_input_for_char_is_rejected() { + let r: Result = crate::deserialize(&[]); + assert!( + r.is_err(), + "expected an error for an empty char, got {:?}", + r + ); +} + +#[test] +fn truncated_multibyte_char_is_rejected() { + // 0xC3 is the lead byte of a two-byte UTF-8 sequence; the continuation + // byte is missing. + let bytes = [0xC3]; + let r: Result = crate::deserialize(&bytes); + assert!( + r.is_err(), + "expected an error for a truncated char, got {:?}", + r + ); +} diff --git a/watchman/rust/serde_bser/src/de/bunser.rs b/watchman/rust/serde_bser/src/de/bunser.rs index d42fb634663ac..6fafd9c004bb3 100644 --- a/watchman/rust/serde_bser/src/de/bunser.rs +++ b/watchman/rust/serde_bser/src/de/bunser.rs @@ -90,6 +90,11 @@ where /// Return a borrowed or copied version of the next n bytes. #[inline] pub fn read_bytes<'s>(&'s mut self, len: i64) -> Result> { + if len < 0 { + // A negative wire-supplied length would cast to a huge `usize` + // below and panic in the underlying reader. + return Err(Error::DeNegativeLength { len }); + } let len = len as usize; self.read .next_bytes(len, &mut self.scratch) diff --git a/watchman/rust/serde_bser/src/de/read.rs b/watchman/rust/serde_bser/src/de/read.rs index 47d32d956c6b4..546d66fad9cb7 100644 --- a/watchman/rust/serde_bser/src/de/read.rs +++ b/watchman/rust/serde_bser/src/de/read.rs @@ -10,8 +10,8 @@ use std::fmt; use std::io; use std::result; -use anyhow::Context as _; use anyhow::bail; +use anyhow::Context as _; use byteorder::ByteOrder; use byteorder::NativeEndian; @@ -210,18 +210,24 @@ where len: usize, scratch: &'s mut Vec, ) -> anyhow::Result> { - scratch.resize(len, 0); - let mut idx = 0; - if self.peeked.is_some() { - idx += 1; - } - if idx < len { - self.reader.read_exact(&mut scratch[idx..len])?; - debug_bytes!("{:x}", ByteBuf(&scratch[idx..len])); - self.read_count += len - idx; - } + // Grow the scratch buffer in bounded chunks instead of pre-allocating + // `len` bytes, so a wire-supplied length that is far larger than the + // stream can ever provide yields an EOF error rather than a capacity + // overflow panic. + scratch.clear(); if let Some(peeked) = self.peeked.take() { - scratch[0] = peeked; + scratch.push(peeked); + } + let mut buf = [0u8; 4096]; + while scratch.len() < len { + let want = (len - scratch.len()).min(buf.len()); + let n = self.reader.read(&mut buf[..want])?; + if n == 0 { + bail!("eof while reading bytes/string"); + } + debug_bytes!("{:x}", ByteBuf(&buf[..n])); + scratch.extend_from_slice(&buf[..n]); + self.read_count += n; } Ok(Reference::Copied(&scratch[0..len])) } diff --git a/watchman/rust/serde_bser/src/errors.rs b/watchman/rust/serde_bser/src/errors.rs index f391027fa95b7..bc0bcbb35d0e7 100644 --- a/watchman/rust/serde_bser/src/errors.rs +++ b/watchman/rust/serde_bser/src/errors.rs @@ -29,6 +29,9 @@ pub enum Error { #[error("Expected {} bytes read, but only read {} bytes", .expected, .read)] DeEof { expected: usize, read: usize }, + #[error("while deserializing BSER: negative length {}", .len)] + DeNegativeLength { len: i64 }, + #[error("Invalid magic header: {:?}", .magic)] DeInvalidMagic { magic: Vec }, diff --git a/watchman/rust/serde_bser/tests/negative_length.rs b/watchman/rust/serde_bser/tests/negative_length.rs new file mode 100644 index 0000000000000..7d66a6afce287 --- /dev/null +++ b/watchman/rust/serde_bser/tests/negative_length.rs @@ -0,0 +1,48 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +//! Regression tests for deserializing BSER payloads with a negative +//! bytestring/utf8string length. The wire length is a signed integer and used +//! to index into the buffer, so a negative value must be rejected instead of +//! being reinterpreted as a huge `usize`. + +use std::io::Cursor; + +use serde_bser::de::from_reader; +use serde_bser::de::from_slice; + +fn negative_len_bytestring() -> Vec { + // BSER magic + capabilities + PDU length (INT8 = 3) + a bytestring whose + // length is INT8 = -1 (0xFF). + let mut msg = vec![0x00, 0x02]; + msg.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]); + msg.extend_from_slice(&[0x03, 0x03]); + msg.extend_from_slice(&[0x02, 0x03, 0xFF]); + msg +} + +#[test] +fn negative_bytestring_length_is_rejected() { + let msg = negative_len_bytestring(); + let r: Result = from_slice(&msg); + assert!( + r.is_err(), + "expected a deserialization error for a negative bytestring length, got {:?}", + r + ); +} + +#[test] +fn negative_bytestring_length_is_rejected_from_reader() { + let msg = negative_len_bytestring(); + let r: Result = from_reader(Cursor::new(msg)); + assert!( + r.is_err(), + "expected a deserialization error for a negative bytestring length, got {:?}", + r + ); +}