From 7fa4dc146e33c394a913ccdae83352a00cc345fc Mon Sep 17 00:00:00 2001 From: David Weis Date: Wed, 4 Oct 2023 05:13:18 +0100 Subject: [PATCH] [Rust - bugfix] read_record_from_slice checking wrong size of buffer wrong (#961) ### Public-Facing Changes There should be no changes in public API caused by this change. ### Description `read_record_from_slice` was checking that buffer size is bigger than 5 but was trying to read a `u8` and `u64` which are in total 9 bytes. This was causing panics when reading malformed mcap files or otherwise malformed streams. I added some simple tests as well but they aren't very robust (I don't check parsing of records). --- rust/src/read.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/rust/src/read.rs b/rust/src/read.rs index 43a6821054..689375e5a3 100644 --- a/rust/src/read.rs +++ b/rust/src/read.rs @@ -118,7 +118,7 @@ impl<'a> Iterator for LinearReader<'a> { /// Read a record and advance the slice fn read_record_from_slice<'a>(buf: &mut &'a [u8]) -> McapResult> { - if buf.len() < 5 { + if buf.len() < (size_of::() + size_of::()) { warn!("Malformed MCAP - not enough space for record + length!"); return Err(McapError::UnexpectedEof); } @@ -1193,4 +1193,21 @@ mod test { LinearReader::new_with_options(MAGIC, enum_set!(Options::IgnoreEndMagic)).unwrap(); assert!(reader.next().is_none()); } + + #[test] + fn test_read_record_from_slice_fails_on_too_short_chunks() { + let res = read_record_from_slice(&mut [0_u8; 4].as_slice()); + assert!(matches!(res, Err(McapError::UnexpectedEof))); + + let res = read_record_from_slice(&mut [0_u8; 8].as_slice()); + assert!(matches!(res, Err(McapError::UnexpectedEof))); + } + + #[test] + fn test_read_record_from_slice_parses_for_big_enough_records() { + let res = read_record_from_slice(&mut [0_u8; 9].as_slice()); + assert!(res.is_ok()); + // Not a very strong test, but we are only testing that it checks the buffer size correctly + assert!(matches!(res, Ok(Record::Unknown { opcode: _, data: _ }))); + } }