id3v2: Fix skipping of frames

Author: uklotzde

src/id3/v2/frame/header.rs

@@ -9,7 +9,8 @@ use std::io::Read;
 
 pub(crate) fn parse_v2_header<R>(
 	reader: &mut R,
-) -> Result<Option<(FrameId<'static>, u32, FrameFlags)>>
+	size: &mut u32,
+) -> Result<Option<(FrameId<'static>, FrameFlags)>>
 where
 	R: Read,
 {
@@ -24,6 +25,8 @@ where
 		return Ok(None);
 	}
 
+	*size = u32::from_be_bytes([0, header[3], header[4], header[5]]);
+
 	let id_bytes = &header[..3];
 	let id_str = std::str::from_utf8(id_bytes)
 		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(id_bytes.to_vec())))
@@ -32,16 +35,15 @@ where
 		})?;
 	let id = FrameId::new_cow(id_str)?;
 
-	let size = u32::from_be_bytes([0, header[3], header[4], header[5]]);
-
 	// V2 doesn't store flags
-	Ok(Some((id, size, FrameFlags::default())))
+	Ok(Some((id, FrameFlags::default())))
 }
 
 pub(crate) fn parse_header<R>(
 	reader: &mut R,
+	size: &mut u32,
 	synchsafe: bool,
-) -> Result<Option<(FrameId<'static>, u32, FrameFlags)>>
+) -> Result<Option<(FrameId<'static>, FrameFlags)>>
 where
 	R: Read,
 {
@@ -56,6 +58,12 @@ where
 		return Ok(None);
 	}
 
+	*size = u32::from_be_bytes([header[4], header[5], header[6], header[7]]);
+	// unsynch the frame size if necessary
+	if synchsafe {
+		*size = size.unsynch();
+	}
+
 	// For some reason, some apps make v3 tags with v2 frame IDs.
 	// The actual frame header is v3 though
 	let mut id_end = 4;
@@ -69,8 +77,6 @@ where
 	let id_str = std::str::from_utf8(id_bytes)
 		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(id_bytes.to_vec())))?;
 
-	let mut size = u32::from_be_bytes([header[4], header[5], header[6], header[7]]);
-
 	// Now upgrade the FrameId
 	let id = if invalid_v2_frame {
 		if let Some(id) = upgrade_v2(id_str) {
@@ -85,15 +91,10 @@ where
 	};
 	let frame_id = FrameId::new_cow(id)?;
 
-	// unsynch the frame size if necessary
-	if synchsafe {
-		size = size.unsynch();
-	}
-
 	let flags = u16::from_be_bytes([header[8], header[9]]);
 	let flags = parse_flags(flags, synchsafe);
 
-	Ok(Some((frame_id, size, flags)))
+	Ok(Some((frame_id, flags)))
 }
 
 pub(crate) fn parse_flags(flags: u16, v4: bool) -> FrameFlags {

src/id3/v2/frame/read.rs

@@ -13,7 +13,7 @@ use byteorder::{BigEndian, ReadBytesExt};
 
 pub(crate) enum ParsedFrame<'a> {
 	Next(Frame<'a>),
-	Skipped,
+	Skip { size: u32 },
 	Eof,
 }
 
@@ -26,13 +26,15 @@ impl<'a> ParsedFrame<'a> {
 	where
 		R: Read,
 	{
+		let mut size = 0u32;
+
 		// The header will be upgraded to ID3v2.4 past this point, so they can all be treated the same
 		let parse_header_result = match version {
-			Id3v2Version::V2 => parse_v2_header(reader),
-			Id3v2Version::V3 => parse_header(reader, false),
-			Id3v2Version::V4 => parse_header(reader, true),
+			Id3v2Version::V2 => parse_v2_header(reader, &mut size),
+			Id3v2Version::V3 => parse_header(reader, &mut size, false),
+			Id3v2Version::V4 => parse_header(reader, &mut size, true),
 		};
-		let (id, mut size, mut flags) = match parse_header_result {
+		let (id, mut flags) = match parse_header_result {
 			Ok(None) => {
 				// Stop reading
 				return Ok(Self::Eof);
@@ -44,7 +46,7 @@ impl<'a> ParsedFrame<'a> {
 					ParsingMode::BestAttempt | ParsingMode::Relaxed => {
 						// Skip this frame and continue reading
 						// TODO: Log error?
-						return Ok(Self::Skipped);
+						return Ok(Self::Skip { size });
 					},
 				}
 			},
@@ -116,14 +118,28 @@ impl<'a> ParsedFrame<'a> {
 						return handle_encryption(&mut compression_reader, size, id, flags);
 					}
 
-					return parse_frame(&mut compression_reader, id, flags, version, parse_mode);
+					return parse_frame(
+						&mut compression_reader,
+						size,
+						id,
+						flags,
+						version,
+						parse_mode,
+					);
 				}
 
 				if flags.encryption.is_some() {
 					return handle_encryption(&mut unsynchronized_reader, size, id, flags);
 				}
 
-				return parse_frame(&mut unsynchronized_reader, id, flags, version, parse_mode);
+				return parse_frame(
+					&mut unsynchronized_reader,
+					size,
+					id,
+					flags,
+					version,
+					parse_mode,
+				);
 			},
 			// Possible combinations:
 			//
@@ -138,7 +154,14 @@ impl<'a> ParsedFrame<'a> {
 					return handle_encryption(&mut compression_reader, size, id, flags);
 				}
 
-				return parse_frame(&mut compression_reader, id, flags, version, parse_mode);
+				return parse_frame(
+					&mut compression_reader,
+					size,
+					id,
+					flags,
+					version,
+					parse_mode,
+				);
 			},
 			// Possible combinations:
 			//
@@ -151,7 +174,7 @@ impl<'a> ParsedFrame<'a> {
 			},
 			// Everything else that doesn't have special flags
 			_ => {
-				return parse_frame(&mut reader, id, flags, version, parse_mode);
+				return parse_frame(&mut reader, size, id, flags, version, parse_mode);
 			},
 		}
 	}
@@ -194,13 +217,14 @@ fn handle_encryption<R: Read>(
 
 fn parse_frame<R: Read>(
 	reader: &mut R,
+	size: u32,
 	id: FrameId<'static>,
 	flags: FrameFlags,
 	version: Id3v2Version,
 	parse_mode: ParsingMode,
 ) -> Result<ParsedFrame<'static>> {
 	match parse_content(reader, id.as_str(), version, parse_mode)? {
 		Some(value) => Ok(ParsedFrame::Next(Frame { id, value, flags })),
-		None => Ok(ParsedFrame::Skipped),
+		None => Ok(ParsedFrame::Skip { size }),
 	}
 }

src/id3/v2/read.rs

@@ -1,7 +1,7 @@
 use super::frame::read::ParsedFrame;
 use super::tag::Id3v2Tag;
 use super::Id3v2Header;
-use crate::error::Result;
+use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
 use crate::id3::v2::util::synchsafe::UnsynchronizedStream;
 use crate::probe::ParsingMode;
 
@@ -34,6 +34,17 @@ where
 	Ok(ret)
 }
 
+fn skip_frame(reader: &mut impl Read, size: u32) -> Result<()> {
+	let size = u64::from(size);
+	let mut reader = reader.take(size);
+	let skipped = std::io::copy(&mut reader, &mut std::io::sink())?;
+	debug_assert!(skipped <= size);
+	if skipped != size {
+		return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameLength).into());
+	}
+	Ok(())
+}
+
 fn read_all_frames_into_tag<R>(
 	reader: &mut R,
 	header: Id3v2Header,
@@ -50,7 +61,9 @@ where
 		match ParsedFrame::read(reader, header.version, parse_mode)? {
 			ParsedFrame::Next(frame) => drop(tag.insert(frame)),
 			// No frame content found or ignored due to errors, but we can expect more frames
-			ParsedFrame::Skipped => continue,
+			ParsedFrame::Skip { size } => {
+				skip_frame(reader, size)?;
+			},
 			// No frame content found, and we can expect there are no more frames
 			ParsedFrame::Eof => break,
 		}