id3v2: Report bad or unsupported frame IDs

Author: uklotzde

src/error.rs

@@ -4,6 +4,7 @@
 //! which can be extended at any time.
 
 use crate::file::FileType;
+use crate::ItemKey;
 
 use std::collections::TryReserveError;
 use std::fmt::{Debug, Display, Formatter};
@@ -83,7 +84,11 @@ pub enum Id3v2ErrorKind {
 
 	// Frame
 	/// Arises when a frame ID contains invalid characters (must be within `'A'..'Z'` or `'0'..'9'`)
-	BadFrameId,
+	/// or if the ID is too short/long.
+	BadFrameId(Vec<u8>),
+	/// Arises when no frame ID is available in the ID3v2 specification for an item key
+	/// and the associated value type.
+	UnsupportedFrameId(ItemKey),
 	/// Arises when a frame doesn't have enough data
 	BadFrameLength,
 	/// Arises when reading/writing a compressed or encrypted frame with no data length indicator
@@ -133,7 +138,10 @@ impl Display for Id3v2ErrorKind {
 			},
 
 			// Frame
-			Self::BadFrameId => write!(f, "Failed to parse a frame ID"),
+			Self::BadFrameId(frame_id) => write!(f, "Failed to parse a frame ID: 0x{frame_id:x?}"),
+			Self::UnsupportedFrameId(item_key) => {
+				write!(f, "Unsupported frame ID for item key {item_key:?}")
+			},
 			Self::BadFrameLength => write!(
 				f,
 				"Frame isn't long enough to extract the necessary information"

src/id3/v2/frame/header.rs

@@ -24,8 +24,9 @@ where
 		return Ok(None);
 	}
 
-	let id_str = std::str::from_utf8(&frame_header[..3])
-		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId))?;
+	let frame_id = &frame_header[..3];
+	let id_str = std::str::from_utf8(frame_id)
+		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(frame_id.to_vec())))?;
 	let id = upgrade_v2(id_str).map_or_else(|| Cow::Owned(id_str.to_owned()), Cow::Borrowed);
 
 	let frame_id = FrameId::new_cow(id)?;
@@ -63,8 +64,9 @@ where
 		frame_id_end = 3;
 	}
 
+	let frame_id = &frame_header[..frame_id_end];
 	let id_str = std::str::from_utf8(&frame_header[..frame_id_end])
-		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId))?;
+		.map_err(|_| Id3v2Error::new(Id3v2ErrorKind::BadFrameId(frame_id.to_vec())))?;
 
 	let mut size = u32::from_be_bytes([
 		frame_header[4],

src/id3/v2/frame/id.rs

@@ -38,7 +38,9 @@ impl<'a> FrameId<'a> {
 		match id.len() {
 			3 => Ok(FrameId::Outdated(id)),
 			4 => Ok(FrameId::Valid(id)),
-			_ => Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into()),
+			_ => Err(
+				Id3v2Error::new(Id3v2ErrorKind::BadFrameId(id.into_owned().into_bytes())).into(),
+			),
 		}
 	}
 
@@ -52,7 +54,10 @@ impl<'a> FrameId<'a> {
 	pub(super) fn verify_id(id_str: &str) -> Result<()> {
 		for c in id_str.chars() {
 			if !c.is_ascii_uppercase() && !c.is_ascii_digit() {
-				return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into());
+				return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId(
+					id_str.as_bytes().to_vec(),
+				))
+				.into());
 			}
 		}
 
@@ -93,7 +98,7 @@ impl<'a> TryFrom<&'a ItemKey> for FrameId<'a> {
 					}
 				}
 
-				Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into())
+				Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(k.clone())).into())
 			},
 		}
 	}

src/id3/v2/frame/mod.rs

@@ -112,7 +112,12 @@ impl<'a> Frame<'a> {
 				None => id,
 				Some(upgraded) => Cow::Borrowed(upgraded),
 			},
-			_ => return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into()),
+			_ => {
+				return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId(
+					id.into_owned().into_bytes(),
+				))
+				.into())
+			},
 		};
 
 		let id = FrameId::new_cow(id_upgraded)?;
@@ -504,8 +509,12 @@ impl<'a> TryFrom<&'a TagItem> for FrameRef<'a> {
 				frame_id = id;
 			},
 			Err(_) => {
-				let Some(desc) = tag_item.key().map_key(TagType::Id3v2, true) else {
-					return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into());
+				let item_key = tag_item.key();
+				let Some(desc) = item_key.map_key(TagType::Id3v2, true) else {
+					return Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(
+						item_key.clone(),
+					))
+					.into());
 				};
 
 				match tag_item.value() {
@@ -525,7 +534,12 @@ impl<'a> TryFrom<&'a TagItem> for FrameRef<'a> {
 							content: locator.clone(),
 						})
 					},
-					_ => return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameId).into()),
+					_ => {
+						return Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(
+							item_key.clone(),
+						))
+						.into())
+					},
 				}
 			},
 		}

src/id3/v2/tag.rs

@@ -533,7 +533,9 @@ impl Accessor for Id3v2Tag {
 	fn set_comment(&mut self, value: String) {
 		let mut value = Some(value);
 		self.frames.retain_mut(|frame| {
-			let Some(CommentFrame { content, .. }) = filter_comment_frame_by_description_mut(frame, &EMPTY_CONTENT_DESCRIPTOR) else {
+			let Some(CommentFrame { content, .. }) =
+				filter_comment_frame_by_description_mut(frame, &EMPTY_CONTENT_DESCRIPTOR)
+			else {
 				return true;
 			};
 			if let Some(value) = value.take() {
@@ -759,7 +761,9 @@ impl SplitTag for Id3v2Tag {
 				) => {
 					if owner == MUSICBRAINZ_UFID_OWNER {
 						let mut identifier = Cursor::new(identifier);
-						let Ok(recording_id) = decode_text(&mut identifier, TextEncoding::Latin1, false) else {
+						let Ok(recording_id) =
+							decode_text(&mut identifier, TextEncoding::Latin1, false)
+						else {
 							return true; // Keep frame
 						};
 						tag.items.push(TagItem::new(