Skip to content

Commit

Permalink
[Rust - bugfix] read_record_from_slice checking wrong size of buffer …
Browse files Browse the repository at this point in the history
…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).
  • Loading branch information
dmweis authored Oct 4, 2023
1 parent 36cea41 commit 7fa4dc1
Showing 1 changed file with 18 additions and 1 deletion.
19 changes: 18 additions & 1 deletion rust/src/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<records::Record<'a>> {
if buf.len() < 5 {
if buf.len() < (size_of::<u64>() + size_of::<u8>()) {
warn!("Malformed MCAP - not enough space for record + length!");
return Err(McapError::UnexpectedEof);
}
Expand Down Expand Up @@ -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: _ })));
}
}

0 comments on commit 7fa4dc1

Please sign in to comment.