OGG: Properly handle large packets

.github/workflows/ci.yml

@@ -21,7 +21,8 @@ jobs:
     steps:
       - uses: actions/checkout@v2
       - name: Dependencies
-        run: sudo apt-get update && sudo apt-get install -y ffmpeg # Need ffprobe for issue #37
+      - run: sudo apt-get update && sudo apt-get install -y ffmpeg # Need ffprobe for issue #37
+      - run: sudo apt-get install -y opus-tools # Need opusinfo for issue #130
       - uses: actions-rs/toolchain@v1
         with:
           profile: minimal

Cargo.toml

@@ -24,7 +24,7 @@ lofty_attr = { path = "lofty_attr" }
 # Debug logging
 log        = "0.4.17"
 # OGG Vorbis/Opus
-ogg_pager  = "0.4.0"
+ogg_pager  = { path = "ogg_pager" }
 # Key maps
 once_cell  = "1.16.0"
 paste      = "1.0.11"

ogg_pager/src/header.rs

@@ -4,8 +4,10 @@ use std::io::{Read, Seek};
 
 use byteorder::{LittleEndian, ReadBytesExt};
 
+pub const PAGE_HEADER_SIZE: usize = 27;
+
 /// An OGG page header
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[derive(Clone, PartialEq, Eq, Debug)]
 pub struct PageHeader {
 	/// The position in the stream the page started at
 	pub start: u64,
@@ -16,6 +18,7 @@ pub struct PageHeader {
 	pub stream_serial: u32,
 	/// The page's sequence number
 	pub sequence_number: u32,
+	pub(crate) segments: Vec<u8>,
 	pub(crate) checksum: u32,
 }
 
@@ -34,6 +37,7 @@ impl PageHeader {
 			abgp,
 			stream_serial,
 			sequence_number,
+			segments: Vec::new(),
 			checksum: 0,
 		}
 	}
@@ -46,7 +50,7 @@ impl PageHeader {
 	/// * [`PageError::InvalidVersion`]
 	/// * [`PageError::BadSegmentCount`]
 	/// * Reader does not have enough data
-	pub fn read<R>(data: &mut R) -> Result<(Self, Vec<u8>)>
+	pub fn read<R>(data: &mut R) -> Result<Self>
 	where
 		R: Read + Seek,
 	{
@@ -88,10 +92,16 @@ impl PageHeader {
 			abgp,
 			stream_serial,
 			sequence_number,
+			segments: segment_table,
 			checksum,
 		};
 
-		Ok((header, segment_table))
+		Ok(header)
+	}
+
+	/// Returns the size of the page content, excluding the header
+	pub fn content_size(&self) -> usize {
+		self.segments.iter().map(|&b| usize::from(b)).sum::<usize>()
 	}
 
 	/// Returns the page's header type flag

ogg_pager/src/lib.rs

@@ -6,18 +6,23 @@ mod header;
 mod packets;
 mod paginate;
 
-use std::io::{Read, Seek, SeekFrom};
+use std::io::{Read, Seek};
 
 pub use crc::crc32;
 pub use error::{PageError, Result};
-pub use header::PageHeader;
+pub use header::{PageHeader, PAGE_HEADER_SIZE};
 pub use packets::{Packets, PacketsIter};
 pub use paginate::paginate;
 
 const CONTINUED_PACKET: u8 = 0x01;
+pub(crate) const MAX_WRITTEN_SEGMENT_COUNT: usize = 32;
+pub(crate) const MAX_WRITTEN_CONTENT_SIZE: usize = MAX_WRITTEN_SEGMENT_COUNT * 255;
 
 /// The maximum page content size
-pub const MAX_CONTENT_SIZE: usize = 65025;
+// NOTE: An OGG page can have up to 255 segments, or ~64KB. We cap it at 32 segments, or ~8KB when writing.
+pub const MAX_CONTENT_SIZE: usize = MAX_WRITTEN_CONTENT_SIZE * 4;
+/// The maximum number of segments a page can contain
+pub const MAX_SEGMENT_COUNT: usize = 255;
 /// The packet contains the first page of the logical bitstream
 pub const CONTAINS_FIRST_PAGE_OF_BITSTREAM: u8 = 0x02;
 /// The packet contains the last page of the logical bitstream
@@ -27,7 +32,6 @@ pub const CONTAINS_LAST_PAGE_OF_BITSTREAM: u8 = 0x04;
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct Page {
 	content: Vec<u8>,
-	segments: Vec<u8>,
 	header: PageHeader,
 	/// The position in the stream the page ended
 	pub end: u64,
@@ -48,12 +52,11 @@ impl Page {
 	///
 	/// NOTE: This will write the checksum as is. It is likely [`Page::gen_crc`] will have
 	/// to be used prior.
-	///
-	/// # Errors
-	///
-	/// See [`segment_table`]
-	pub fn as_bytes(&self) -> Result<Vec<u8>> {
-		let mut bytes = Vec::with_capacity(27 + self.segments.len() + self.content.len());
+	pub fn as_bytes(&self) -> Vec<u8> {
+		let segment_table = &self.header.segments;
+		let num_segments = segment_table.len();
+		let mut bytes =
+			Vec::with_capacity(PAGE_HEADER_SIZE + num_segments + self.header.content_size());
 
 		bytes.extend(b"OggS");
 		bytes.push(0); // Version
@@ -62,11 +65,11 @@ impl Page {
 		bytes.extend(self.header.stream_serial.to_le_bytes());
 		bytes.extend(self.header.sequence_number.to_le_bytes());
 		bytes.extend(self.header.checksum.to_le_bytes());
-		bytes.push(self.segments.len() as u8);
-		bytes.extend(&self.segments);
+		bytes.push(num_segments as u8);
+		bytes.extend(segment_table);
 		bytes.extend(self.content.iter());
 
-		Ok(bytes)
+		bytes
 	}
 
 	/// Attempts to get a Page from a reader
@@ -77,64 +80,44 @@ impl Page {
 	///
 	/// * [`std::io::Error`]
 	/// * [`PageError`]
-	pub fn read<V>(data: &mut V, skip_content: bool) -> Result<Self>
+	pub fn read<V>(data: &mut V) -> Result<Self>
 	where
 		V: Read + Seek,
 	{
-		let (header, segments) = PageHeader::read(data)?;
+		let header = PageHeader::read(data)?;
 
-		let mut content: Vec<u8> = Vec::new();
-		let content_len: u16 = segments.iter().map(|&b| u16::from(b)).sum();
-
-		if skip_content {
-			data.seek(SeekFrom::Current(i64::from(content_len)))?;
-		} else {
-			content = vec![0; content_len as usize];
-			data.read_exact(&mut content)?;
-		}
+		let mut content = vec![0; header.content_size()];
+		data.read_exact(&mut content)?;
 
 		let end = data.stream_position()?;
 
 		Ok(Page {
 			content,
-			segments,
 			header,
 			end,
 		})
 	}
 
 	/// Generates the CRC checksum of the page
-	///
-	/// # Errors
-	///
-	/// See [`Page::as_bytes`]
-	pub fn gen_crc(&mut self) -> Result<()> {
+	pub fn gen_crc(&mut self) {
 		// The value is computed over the entire header (with the CRC field in the header set to zero) and then continued over the page
 		self.header.checksum = 0;
-		self.header.checksum = crc::crc32(&self.as_bytes()?);
-
-		Ok(())
+		self.header.checksum = crc::crc32(&self.as_bytes());
 	}
 
 	/// Extends the Page's content, returning another Page if too much data was provided
 	///
 	/// This will do nothing if `content` is greater than the max page size. In this case,
 	/// [`paginate()`] should be used.
-	///
-	/// # Errors
-	///
-	/// *Only applicable if a new page is created*:
-	///
-	/// See [`Page::gen_crc`]
-	pub fn extend(&mut self, content: &[u8]) -> Result<Option<Page>> {
+	pub fn extend(&mut self, content: &[u8]) -> Option<Page> {
 		let self_len = self.content.len();
 		let content_len = content.len();
 
 		if self_len + content_len <= MAX_CONTENT_SIZE {
 			self.content.extend(content.iter());
 			self.end += content_len as u64;
 
-			return Ok(None);
+			return None;
 		}
 
 		if content_len <= MAX_CONTENT_SIZE {
@@ -147,24 +130,24 @@ impl Page {
 
 			let mut p = Page {
 				content: content[remaining..].to_vec(),
-				segments: segment_table(remaining)?,
 				header: PageHeader {
 					start: self.end,
 					header_type_flag: 1,
 					abgp: 0,
 					stream_serial: self.header.stream_serial,
 					sequence_number: self.header.sequence_number + 1,
+					segments: segment_table(remaining),
 					checksum: 0,
 				},
 				end: self.header().start + content.len() as u64,
 			};
 
-			p.gen_crc()?;
+			p.gen_crc();
 
-			return Ok(Some(p));
+			return Some(p);
 		}
 
-		Ok(None)
+		None
 	}
 
 	/// Returns the page's content
@@ -178,34 +161,19 @@ impl Page {
 	}
 
 	/// Returns the page's segment table
-	///
-	/// # Errors
-	///
-	/// See [`segment_table`]
-	pub fn segment_table(&self) -> Result<Vec<u8>> {
+	pub fn segment_table(&self) -> Vec<u8> {
 		segment_table(self.content.len())
 	}
 }
 
 /// Creates a segment table based on the length
-///
-/// # Errors
-///
-/// `length` > [`MAX_CONTENT_SIZE`]
-pub fn segment_table(length: usize) -> Result<Vec<u8>> {
-	match length {
-		0 => return Ok(vec![1, 0]),
-		l if l > MAX_CONTENT_SIZE => return Err(PageError::TooMuchData),
-		_ => {},
-	};
-
-	let mut last_len = (length % 255) as u8;
-	if last_len == 0 {
-		last_len = 255;
+pub fn segment_table(length: usize) -> Vec<u8> {
+	if length == 0 {
+		return vec![1, 0];
 	}
 
-	let mut needed = (length / 255) + 1;
-	needed = std::cmp::min(needed, 255);
+	let last_len = (length % 255) as u8;
+	let needed = (length / 255) + 1;
 
 	let mut segments = Vec::with_capacity(needed);
 
@@ -217,7 +185,7 @@ pub fn segment_table(length: usize) -> Result<Vec<u8>> {
 		}
 	}
 
-	Ok(segments)
+	segments
 }
 
 #[cfg(test)]
@@ -232,21 +200,21 @@ mod tests {
 				0x4F, 0x70, 0x75, 0x73, 0x48, 0x65, 0x61, 0x64, 0x01, 0x02, 0x38, 0x01, 0x80, 0xBB,
 				0, 0, 0, 0, 0,
 			],
-			segments: vec![19],
 			header: PageHeader {
 				start: 0,
 				header_type_flag: 2,
 				abgp: 0,
 				stream_serial: 1759377061,
 				sequence_number: 0,
+				segments: vec![19],
 				checksum: 3579522525,
 			},
 			end: 47,
 		};
 
 		let content = std::fs::read("test_assets/opus_ident_header.page").unwrap();
 
-		let page = Page::read(&mut Cursor::new(content), false).unwrap();
+		let page = Page::read(&mut Cursor::new(content)).unwrap();
 
 		assert_eq!(expected, page);
 	}
@@ -264,10 +232,7 @@ mod tests {
 
 		assert_eq!(
 			last_page_content.len() % 255,
-			*segment_table(last_page_content.len())
-				.unwrap()
-				.last()
-				.unwrap() as usize
+			*segment_table(last_page_content.len()).last().unwrap() as usize
 		);
 
 		for (i, page) in pages.into_iter().enumerate() {