id3v2: Don't stop reading after skipping a frame

Author: uklotzde

src/id3/v2/frame/header.rs

@@ -3,95 +3,73 @@ use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
 use crate::id3::v2::util::synchsafe::SynchsafeInteger;
 use crate::id3::v2::util::upgrade::{upgrade_v2, upgrade_v3};
 use crate::id3::v2::FrameId;
-use crate::ParsingMode;
 
 use std::borrow::Cow;
 use std::io::Read;
 
 pub(crate) fn parse_v2_header<R>(
 	reader: &mut R,
-	parse_mode: ParsingMode,
 ) -> Result<Option<(FrameId<'static>, u32, FrameFlags)>>
 where
 	R: Read,
 {
-	let mut frame_header = [0; 6];
-	match reader.read_exact(&mut frame_header) {
+	let mut header = [0; 6];
+	match reader.read_exact(&mut header) {
 		Ok(_) => {},
 		Err(_) => return Ok(None),
 	}
 
 	// Assume we just started reading padding
-	if frame_header[0] == 0 {
+	if header[0] == 0 {
 		return Ok(None);
 	}
 
-	let frame_id_bytes = &frame_header[..3];
-	let frame_id = match std::str::from_utf8(frame_id_bytes)
-		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(frame_id_bytes.to_vec())).into())
+	let id_bytes = &header[..3];
+	let id_str = std::str::from_utf8(id_bytes)
+		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(id_bytes.to_vec())))
 		.map(|id_str| {
 			upgrade_v2(id_str).map_or_else(|| Cow::Owned(id_str.to_owned()), Cow::Borrowed)
-		})
-		.and_then(FrameId::new_cow)
-	{
-		Ok(id) => id,
-		Err(err) => match parse_mode {
-			ParsingMode::Strict => return Err(err),
-			ParsingMode::BestAttempt | ParsingMode::Relaxed => return Ok(None),
-		},
-	};
+		})?;
+	let id = FrameId::new_cow(id_str)?;
 
-	let size = u32::from_be_bytes([0, frame_header[3], frame_header[4], frame_header[5]]);
+	let size = u32::from_be_bytes([0, header[3], header[4], header[5]]);
 
 	// V2 doesn't store flags
-	Ok(Some((frame_id, size, FrameFlags::default())))
+	Ok(Some((id, size, FrameFlags::default())))
 }
 
 pub(crate) fn parse_header<R>(
 	reader: &mut R,
 	synchsafe: bool,
-	parse_mode: ParsingMode,
 ) -> Result<Option<(FrameId<'static>, u32, FrameFlags)>>
 where
 	R: Read,
 {
-	let mut frame_header = [0; 10];
-	match reader.read_exact(&mut frame_header) {
+	let mut header = [0; 10];
+	match reader.read_exact(&mut header) {
 		Ok(_) => {},
 		Err(_) => return Ok(None),
 	}
 
 	// Assume we just started reading padding
-	if frame_header[0] == 0 {
+	if header[0] == 0 {
 		return Ok(None);
 	}
 
 	// For some reason, some apps make v3 tags with v2 frame IDs.
 	// The actual frame header is v3 though
-	let mut frame_id_end = 4;
+	let mut id_end = 4;
 	let mut invalid_v2_frame = false;
-	if frame_header[3] == 0 && !synchsafe {
+	if header[3] == 0 && !synchsafe {
 		invalid_v2_frame = true;
-		frame_id_end = 3;
+		id_end = 3;
 	}
 
-	let frame_id_bytes = &frame_header[..frame_id_end];
-	let id_str = match std::str::from_utf8(frame_id_bytes)
-		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(frame_id_bytes.to_vec())).into())
-	{
-		Ok(id_str) => id_str,
-		Err(err) => match parse_mode {
-			ParsingMode::Strict => return Err(err),
-			ParsingMode::BestAttempt | ParsingMode::Relaxed => return Ok(None),
-		},
-	};
+	let id_bytes = &header[..id_end];
+	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([
-		frame_header[4],
-		frame_header[5],
-		frame_header[6],
-		frame_header[7],
-	]);
+	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 {
@@ -105,20 +83,14 @@ where
 	} else {
 		Cow::Owned(id_str.to_owned())
 	};
-	let frame_id = match FrameId::new_cow(id) {
-		Ok(frame_id) => frame_id,
-		Err(err) => match parse_mode {
-			ParsingMode::Strict => return Err(err),
-			ParsingMode::BestAttempt | ParsingMode::Relaxed => return Ok(None),
-		},
-	};
+	let frame_id = FrameId::new_cow(id)?;
 
 	// unsynch the frame size if necessary
 	if synchsafe {
 		size = size.unsynch();
 	}
 
-	let flags = u16::from_be_bytes([frame_header[8], frame_header[9]]);
+	let flags = u16::from_be_bytes([header[8], header[9]]);
 	let flags = parse_flags(flags, synchsafe);
 
 	Ok(Some((frame_id, size, flags)))

src/id3/v2/frame/read.rs

@@ -11,23 +11,43 @@ use std::io::Read;
 
 use byteorder::{BigEndian, ReadBytesExt};
 
-impl<'a> Frame<'a> {
+pub(crate) enum ParsedFrame<'a> {
+	Next(Frame<'a>),
+	Skipped,
+	Eof,
+}
+
+impl<'a> ParsedFrame<'a> {
 	pub(crate) fn read<R>(
 		reader: &mut R,
 		version: Id3v2Version,
 		parse_mode: ParsingMode,
-	) -> Result<(Option<Self>, bool)>
+	) -> Result<Self>
 	where
 		R: Read,
 	{
 		// The header will be upgraded to ID3v2.4 past this point, so they can all be treated the same
-		let (id, mut size, mut flags) = match match version {
-			Id3v2Version::V2 => parse_v2_header(reader, parse_mode)?,
-			Id3v2Version::V3 => parse_header(reader, false, parse_mode)?,
-			Id3v2Version::V4 => parse_header(reader, true, parse_mode)?,
-		} {
-			None => return Ok((None, true)),
-			Some(frame_header) => frame_header,
+		let parse_header_result = match version {
+			Id3v2Version::V2 => parse_v2_header(reader),
+			Id3v2Version::V3 => parse_header(reader, false),
+			Id3v2Version::V4 => parse_header(reader, true),
+		};
+		let (id, mut size, mut flags) = match parse_header_result {
+			Ok(None) => {
+				// Stop reading
+				return Ok(Self::Eof);
+			},
+			Ok(Some(some)) => some,
+			Err(err) => {
+				match parse_mode {
+					ParsingMode::Strict => return Err(err),
+					ParsingMode::BestAttempt | ParsingMode::Relaxed => {
+						// Skip this frame and continue reading
+						// TODO: Log error?
+						return Ok(Self::Skipped);
+					},
+				}
+			},
 		};
 
 		// Get the encryption method symbol
@@ -154,7 +174,7 @@ fn handle_encryption<R: Read>(
 	size: u32,
 	id: FrameId<'static>,
 	flags: FrameFlags,
-) -> Result<(Option<Frame<'static>>, bool)> {
+) -> Result<ParsedFrame<'static>> {
 	if flags.data_length_indicator.is_none() {
 		return Err(Id3v2Error::new(Id3v2ErrorKind::MissingDataLengthIndicator).into());
 	}
@@ -169,7 +189,7 @@ fn handle_encryption<R: Read>(
 	};
 
 	// Nothing further we can do with encrypted frames
-	Ok((Some(encrypted_frame), false))
+	Ok(ParsedFrame::Next(encrypted_frame))
 }
 
 fn parse_frame<R: Read>(
@@ -178,9 +198,9 @@ fn parse_frame<R: Read>(
 	flags: FrameFlags,
 	version: Id3v2Version,
 	parse_mode: ParsingMode,
-) -> Result<(Option<Frame<'static>>, bool)> {
+) -> Result<ParsedFrame<'static>> {
 	match parse_content(reader, id.as_str(), version, parse_mode)? {
-		Some(value) => Ok((Some(Frame { id, value, flags }), false)),
-		None => Ok((None, false)),
+		Some(value) => Ok(ParsedFrame::Next(Frame { id, value, flags })),
+		None => Ok(ParsedFrame::Skipped),
 	}
 }

src/id3/v2/read.rs

@@ -1,4 +1,4 @@
-use super::frame::Frame;
+use super::frame::read::ParsedFrame;
 use super::tag::Id3v2Tag;
 use super::Id3v2Header;
 use crate::error::Result;
@@ -47,12 +47,12 @@ where
 	tag.set_flags(header.flags);
 
 	loop {
-		match Frame::read(reader, header.version, parse_mode)? {
+		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,
 			// No frame content found, and we can expect there are no more frames
-			(None, true) => break,
-			(Some(f), false) => drop(tag.insert(f)),
-			// No frame content found, but we can expect more frames
-			_ => {},
+			ParsedFrame::Eof => break,
 		}
 	}