From 56cc260a5c103c66167f3c330be09da0b3e431ac Mon Sep 17 00:00:00 2001 From: Nathan Fiedler Date: Mon, 12 Jul 2021 17:28:39 -0700 Subject: [PATCH] fix: minimize assertions in non-test code (#56) Initial effort to reduce or eliminate the use of assertions in the production code (i.e. not test code). See issue 43 for an example of how this can cause an application (thread) to panic when reading an unusual file. Not all of the changes were as simple as returning an Err, since some functions did not return a Result. Also, the error type used (InvalidData) is just a catch-all with a message, and in some cases a more refined error type may be in order. cargo test passes --- src/mp4box/dinf.rs | 4 +++- src/mp4box/elst.rs | 3 +-- src/mp4box/hdlr.rs | 4 +++- src/mp4box/mdhd.rs | 13 +++++++------ src/mp4box/mehd.rs | 13 +++++++------ src/mp4box/mvhd.rs | 13 +++++++------ src/mp4box/stsz.rs | 4 +++- src/mp4box/tkhd.rs | 13 +++++++------ src/mp4box/trun.rs | 4 +++- src/reader.rs | 4 +++- src/track.rs | 44 +++++++++++++++++++++++--------------------- src/writer.rs | 4 +++- 12 files changed, 70 insertions(+), 53 deletions(-) diff --git a/src/mp4box/dinf.rs b/src/mp4box/dinf.rs index 8b10848..981c39a 100644 --- a/src/mp4box/dinf.rs +++ b/src/mp4box/dinf.rs @@ -260,7 +260,9 @@ impl ReadBox<&mut R> for UrlBox { reader.read_exact(&mut buf)?; match String::from_utf8(buf) { Ok(t) => { - assert_eq!(t.len(), buf_size as usize); + if t.len() != buf_size as usize { + return Err(Error::InvalidData("string too small")) + } t } _ => String::default(), diff --git a/src/mp4box/elst.rs b/src/mp4box/elst.rs index 7407eee..825b5b5 100644 --- a/src/mp4box/elst.rs +++ b/src/mp4box/elst.rs @@ -30,8 +30,7 @@ impl ElstBox { let mut size = HEADER_SIZE + HEADER_EXT_SIZE + 4; if self.version == 1 { size += self.entries.len() as u64 * 20; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { size += self.entries.len() as u64 * 12; } size diff --git a/src/mp4box/hdlr.rs b/src/mp4box/hdlr.rs index 62410a8..56ba783 100644 --- a/src/mp4box/hdlr.rs +++ b/src/mp4box/hdlr.rs @@ -58,7 +58,9 @@ impl ReadBox<&mut R> for HdlrBox { let handler_string = match String::from_utf8(buf) { Ok(t) => { - assert_eq!(t.len(), buf_size as usize); + if t.len() != buf_size as usize { + return Err(Error::InvalidData("string too small")) + } t } _ => String::from("null"), diff --git a/src/mp4box/mdhd.rs b/src/mp4box/mdhd.rs index c3d24df..135fc36 100644 --- a/src/mp4box/mdhd.rs +++ b/src/mp4box/mdhd.rs @@ -26,8 +26,7 @@ impl MdhdBox { if self.version == 1 { size += 28; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { size += 16; } size += 4; @@ -82,14 +81,15 @@ impl ReadBox<&mut R> for MdhdBox { reader.read_u32::()?, reader.read_u64::()?, ) - } else { - assert_eq!(version, 0); + } else if version == 0 { ( reader.read_u32::()? as u64, reader.read_u32::()? as u64, reader.read_u32::()?, reader.read_u32::()? as u64, ) + } else { + return Err(Error::InvalidData("version must be 0 or 1")); }; let language_code = reader.read_u16::()?; let language = language_string(language_code); @@ -120,12 +120,13 @@ impl WriteBox<&mut W> for MdhdBox { writer.write_u64::(self.modification_time)?; writer.write_u32::(self.timescale)?; writer.write_u64::(self.duration)?; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { writer.write_u32::(self.creation_time as u32)?; writer.write_u32::(self.modification_time as u32)?; writer.write_u32::(self.timescale)?; writer.write_u32::(self.duration as u32)?; + } else { + return Err(Error::InvalidData("version must be 0 or 1")); } let language_code = language_code(&self.language); diff --git a/src/mp4box/mehd.rs b/src/mp4box/mehd.rs index 7a4627b..1b69c4a 100644 --- a/src/mp4box/mehd.rs +++ b/src/mp4box/mehd.rs @@ -21,8 +21,7 @@ impl MehdBox { if self.version == 1 { size += 8; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { size += 4; } size @@ -66,9 +65,10 @@ impl ReadBox<&mut R> for MehdBox { let fragment_duration = if version == 1 { reader.read_u64::()? - } else { - assert_eq!(version, 0); + } else if version == 0 { reader.read_u32::()? as u64 + } else { + return Err(Error::InvalidData("version must be 0 or 1")); }; skip_bytes_to(reader, start + size)?; @@ -89,9 +89,10 @@ impl WriteBox<&mut W> for MehdBox { if self.version == 1 { writer.write_u64::(self.fragment_duration)?; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { writer.write_u32::(self.fragment_duration as u32)?; + } else { + return Err(Error::InvalidData("version must be 0 or 1")); } Ok(size) diff --git a/src/mp4box/mvhd.rs b/src/mp4box/mvhd.rs index 3a96f10..5232019 100644 --- a/src/mp4box/mvhd.rs +++ b/src/mp4box/mvhd.rs @@ -26,8 +26,7 @@ impl MvhdBox { let mut size = HEADER_SIZE + HEADER_EXT_SIZE; if self.version == 1 { size += 28; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { size += 16; } size += 80; @@ -82,14 +81,15 @@ impl ReadBox<&mut R> for MvhdBox { reader.read_u32::()?, reader.read_u64::()?, ) - } else { - assert_eq!(version, 0); + } else if version == 0 { ( reader.read_u32::()? as u64, reader.read_u32::()? as u64, reader.read_u32::()?, reader.read_u32::()? as u64, ) + } else { + return Err(Error::InvalidData("version must be 0 or 1")); }; let rate = FixedPointU16::new_raw(reader.read_u32::()?); @@ -119,12 +119,13 @@ impl WriteBox<&mut W> for MvhdBox { writer.write_u64::(self.modification_time)?; writer.write_u32::(self.timescale)?; writer.write_u64::(self.duration)?; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { writer.write_u32::(self.creation_time as u32)?; writer.write_u32::(self.modification_time as u32)?; writer.write_u32::(self.timescale)?; writer.write_u32::(self.duration as u32)?; + } else { + return Err(Error::InvalidData("version must be 0 or 1")); } writer.write_u32::(self.rate.raw_value())?; diff --git a/src/mp4box/stsz.rs b/src/mp4box/stsz.rs index b76ba86..882b75e 100644 --- a/src/mp4box/stsz.rs +++ b/src/mp4box/stsz.rs @@ -83,7 +83,9 @@ impl WriteBox<&mut W> for StszBox { writer.write_u32::(self.sample_size)?; writer.write_u32::(self.sample_count)?; if self.sample_size == 0 { - assert_eq!(self.sample_count, self.sample_sizes.len() as u32); + if self.sample_count != self.sample_sizes.len() as u32 { + return Err(Error::InvalidData("sample count out of sync")); + } for sample_number in self.sample_sizes.iter() { writer.write_u32::(*sample_number)?; } diff --git a/src/mp4box/tkhd.rs b/src/mp4box/tkhd.rs index 75de65d..b222300 100644 --- a/src/mp4box/tkhd.rs +++ b/src/mp4box/tkhd.rs @@ -73,8 +73,7 @@ impl TkhdBox { let mut size = HEADER_SIZE + HEADER_EXT_SIZE; if self.version == 1 { size += 32; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { size += 20; } size += 60; @@ -125,8 +124,7 @@ impl ReadBox<&mut R> for TkhdBox { reader.read_u32::()?, reader.read_u64::()?, ) - } else { - assert_eq!(version, 0); + } else if version == 0 { ( reader.read_u32::()? as u64, reader.read_u32::()? as u64, @@ -134,6 +132,8 @@ impl ReadBox<&mut R> for TkhdBox { reader.read_u32::()?, reader.read_u32::()? as u64, ) + } else { + return Err(Error::InvalidData("version must be 0 or 1")); }; reader.read_u64::()?; // reserved let layer = reader.read_u16::()?; @@ -188,13 +188,14 @@ impl WriteBox<&mut W> for TkhdBox { writer.write_u32::(self.track_id)?; writer.write_u32::(0)?; // reserved writer.write_u64::(self.duration)?; - } else { - assert_eq!(self.version, 0); + } else if self.version == 0 { writer.write_u32::(self.creation_time as u32)?; writer.write_u32::(self.modification_time as u32)?; writer.write_u32::(self.track_id)?; writer.write_u32::(0)?; // reserved writer.write_u32::(self.duration as u32)?; + } else { + return Err(Error::InvalidData("version must be 0 or 1")); } writer.write_u64::(0)?; // reserved diff --git a/src/mp4box/trun.rs b/src/mp4box/trun.rs index bf6f1cf..b4bec5d 100644 --- a/src/mp4box/trun.rs +++ b/src/mp4box/trun.rs @@ -154,7 +154,9 @@ impl WriteBox<&mut W> for TrunBox { if let Some(v) = self.first_sample_flags { writer.write_u32::(v)?; } - assert_eq!(self.sample_count, self.sample_sizes.len() as u32); + if self.sample_count != self.sample_sizes.len() as u32 { + return Err(Error::InvalidData("sample count out of sync")); + } for i in 0..self.sample_count as usize { if TrunBox::FLAG_SAMPLE_DURATION & self.flags > 0 { writer.write_u32::(self.sample_durations[i])?; diff --git a/src/reader.rs b/src/reader.rs index b610942..4c77e17 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -66,7 +66,9 @@ impl Mp4Reader { let mut tracks = if let Some(ref moov) = moov { let mut tracks = Vec::with_capacity(moov.traks.len()); for (i, trak) in moov.traks.iter().enumerate() { - assert_eq!(trak.tkhd.track_id, i as u32 + 1); + if trak.tkhd.track_id != i as u32 + 1 { + return Err(Error::InvalidData("tracks out of order")); + } tracks.push(Mp4Track::from(trak)); } tracks diff --git a/src/track.rs b/src/track.rs index 46e846c..782af78 100644 --- a/src/track.rs +++ b/src/track.rs @@ -303,19 +303,26 @@ impl Mp4Track { } } - fn stsc_index(&self, sample_id: u32) -> usize { + fn stsc_index(&self, sample_id: u32) -> Result { + if self.trak.mdia.minf.stbl.stsc.entries.is_empty() { + return Err(Error::InvalidData("no stsc entries")); + } for (i, entry) in self.trak.mdia.minf.stbl.stsc.entries.iter().enumerate() { if sample_id < entry.first_sample { - assert_ne!(i, 0); - return i - 1; + return if i == 0 { + Err(Error::InvalidData("sample not found")) + } else { + Ok(i - 1) + }; } } - - assert_ne!(self.trak.mdia.minf.stbl.stsc.entries.len(), 0); - self.trak.mdia.minf.stbl.stsc.entries.len() - 1 + Ok(self.trak.mdia.minf.stbl.stsc.entries.len() - 1) } fn chunk_offset(&self, chunk_id: u32) -> Result { + if self.trak.mdia.minf.stbl.stco.is_none() && self.trak.mdia.minf.stbl.co64.is_none() { + return Err(Error::InvalidData("must have either stco or co64 boxes")); + } if let Some(ref stco) = self.trak.mdia.minf.stbl.stco { if let Some(offset) = stco.entries.get(chunk_id as usize - 1) { return Ok(*offset as u64); @@ -326,21 +333,17 @@ impl Mp4Track { chunk_id, )); } - } else { - if let Some(ref co64) = self.trak.mdia.minf.stbl.co64 { - if let Some(offset) = co64.entries.get(chunk_id as usize - 1) { - return Ok(*offset); - } else { - return Err(Error::EntryInStblNotFound( - self.track_id(), - BoxType::Co64Box, - chunk_id, - )); - } + } else if let Some(ref co64) = self.trak.mdia.minf.stbl.co64 { + if let Some(offset) = co64.entries.get(chunk_id as usize - 1) { + return Ok(*offset); + } else { + return Err(Error::EntryInStblNotFound( + self.track_id(), + BoxType::Co64Box, + chunk_id, + )); } } - - assert!(self.trak.mdia.minf.stbl.stco.is_some() || self.trak.mdia.minf.stbl.co64.is_some()); return Err(Error::Box2NotFound(BoxType::StcoBox, BoxType::Co64Box)); } @@ -436,7 +439,7 @@ impl Mp4Track { )) } } else { - let stsc_index = self.stsc_index(sample_id); + let stsc_index = self.stsc_index(sample_id)?; let stsc = &self.trak.mdia.minf.stbl.stsc; let stsc_entry = stsc.entries.get(stsc_index).unwrap(); @@ -631,7 +634,6 @@ impl Mp4TrackWriter { self.is_fixed_sample_size = true; } } else { - assert!(self.trak.mdia.minf.stbl.stsz.sample_count > 0); if self.is_fixed_sample_size { if self.fixed_sample_size != size { self.is_fixed_sample_size = false; diff --git a/src/writer.rs b/src/writer.rs index 5c946f6..3293a3a 100644 --- a/src/writer.rs +++ b/src/writer.rs @@ -116,7 +116,9 @@ impl Mp4Writer { fn update_mdat_size(&mut self) -> Result<()> { let mdat_end = self.writer.seek(SeekFrom::Current(0))?; let mdat_size = mdat_end - self.mdat_pos; - assert!(mdat_size < std::u32::MAX as u64); + if mdat_size > std::u32::MAX as u64 { + return Err(Error::InvalidData("mdat size too large")); + } self.writer.seek(SeekFrom::Start(self.mdat_pos))?; self.writer.write_u32::(mdat_size as u32)?; self.writer.seek(SeekFrom::Start(mdat_end))?;