Skip to content

Commit 23b0e1f

Browse files
rust: fix int underflow when chunk wasn't closed (#1480)
### Changelog <!-- Write a one-sentence summary of the user-impacting change (API, UI/UX, performance, etc) that could appear in a changelog. Write "None" if there is no user-facing change --> - rust: fix int underflow on chunk with invalid size ### Docs <!-- Link to a Docs PR, tracking ticket in Linear, OR write "None" if no documentation changes are needed. --> None ### Description When a chunk doesn't close correctly (loss of power, crash, etc.) the size that was written will be `u64::MAX`. If you try and read a file like this with the Rust reader it breaks because the size of the chunk is expected to be valid. In debug mode this will panic due to underflow, in release mode this value will wrap and break something else down the track. This change explicitly uses checked calculations for the chunk size and returns an `UnexpectedEoc` error if any of the calculations overflow. Fixes #1479. Fixes FIRE-202.
1 parent f5a53e9 commit 23b0e1f

File tree

2 files changed

+40
-1
lines changed

2 files changed

+40
-1
lines changed

rust/src/sans_io/linear_reader.rs

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,8 +509,15 @@ impl LinearReader {
509509
&mut self.decompressors,
510510
&header.compression
511511
));
512+
513+
let chunk_data_len = check!(len
514+
.checked_sub(header_len as u64)
515+
.ok_or(McapError::UnexpectedEoc));
516+
512517
let padding_after_compressed_data = check!(check_len(
513-
len - (header_len as u64) - header.compressed_size,
518+
check!(chunk_data_len
519+
.checked_sub(header.compressed_size)
520+
.ok_or(McapError::UnexpectedEoc)),
514521
self.options.record_length_limit
515522
)
516523
.ok_or(McapError::ChunkTooLarge(len)));
@@ -706,6 +713,7 @@ impl LinearReader {
706713
}
707714

708715
/// Events emitted by the linear reader.
716+
#[derive(Debug)]
709717
pub enum LinearReadEvent<'a> {
710718
/// The reader needs more data to provide the next record. Call [`LinearReader::insert`] then
711719
/// [`LinearReader::notify_read`] to load more data. The value provided here is a hint for how
@@ -985,6 +993,7 @@ mod tests {
985993
);
986994
Ok(())
987995
}
996+
use assert_matches::assert_matches;
988997
use paste::paste;
989998

990999
macro_rules! test_chunked_parametrized {
@@ -1206,4 +1215,31 @@ mod tests {
12061215
}
12071216
assert_eq!(message_count, 12);
12081217
}
1218+
1219+
#[test]
1220+
fn test_handles_failed_to_close_chunks() {
1221+
let mut f =
1222+
std::fs::File::open("tests/data/chunk_not_closed.mcap").expect("failed to open file");
1223+
let mut output = vec![];
1224+
f.read_to_end(&mut output).expect("failed to read");
1225+
1226+
let mut reader = LinearReader::new();
1227+
reader.insert(output.len()).copy_from_slice(&output[..]);
1228+
reader.notify_read(output.len());
1229+
1230+
// the first record is the header;
1231+
let next = reader
1232+
.next_event()
1233+
.expect("there should be one event")
1234+
.expect("first record should be header");
1235+
1236+
let LinearReadEvent::Record { opcode: 1, .. } = next else {
1237+
panic!("expected first record to be header");
1238+
};
1239+
1240+
let next = reader.next_event().expect("there should be one event");
1241+
1242+
// test fails with unexpected EOC because sizes are u64::max
1243+
assert_matches!(next, Err(McapError::UnexpectedEoc));
1244+
}
12091245
}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
version https://git-lfs.github.com/spec/v1
2+
oid sha256:7b49182be8db86f194a34617cab44720d50460d9caa7ec54cbcf5c19700b8578
3+
size 92

0 commit comments

Comments
 (0)