MP4: Skip over invalid ilst atoms by default

Signed-off-by: Serial <[email protected]>

Author: Serial-ATA

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+- **MP4**: Skip over invalid `ilst` atoms by default ([issue](https://github.com/Serial-ATA/lofty-rs/issues/291)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/292))
+
 ## [0.17.0] - 2023-11-14
 
 ### Added

src/error.rs

@@ -4,7 +4,7 @@
 //! which can be extended at any time.
 
 use crate::file::FileType;
-use crate::ItemKey;
+use crate::tag::item::ItemKey;
 
 use std::collections::TryReserveError;
 use std::fmt::{Debug, Display, Formatter};

src/mp4/ilst/mod.rs

@@ -755,19 +755,27 @@ mod tests {
 	use crate::tag::utils::test_utils;
 	use crate::tag::utils::test_utils::read_path;
 	use crate::{
-		Accessor as _, AudioFile, ItemKey, ItemValue, ParseOptions, SplitTag as _, Tag,
-		TagExt as _, TagItem, TagType,
+		Accessor as _, AudioFile, ItemKey, ItemValue, ParseOptions, ParsingMode, SplitTag as _,
+		Tag, TagExt as _, TagItem, TagType,
 	};
 	use std::io::{Cursor, Read as _, Seek as _, Write as _};
 
-	fn read_ilst(path: &str) -> Ilst {
-		let tag = crate::tag::utils::test_utils::read_path(path);
+	fn read_ilst(path: &str, parse_mode: ParsingMode) -> Ilst {
+		let tag = std::fs::read(path).unwrap();
 		let len = tag.len();
 
 		let cursor = Cursor::new(tag);
-		let mut reader = AtomReader::new(cursor, crate::ParsingMode::Strict).unwrap();
+		let mut reader = AtomReader::new(cursor, parse_mode).unwrap();
+
+		super::read::parse_ilst(&mut reader, parse_mode, len as u64).unwrap()
+	}
 
-		super::read::parse_ilst(&mut reader, crate::ParsingMode::Strict, len as u64).unwrap()
+	fn read_ilst_strict(path: &str) -> Ilst {
+		read_ilst(path, ParsingMode::Strict)
+	}
+
+	fn read_ilst_bestattempt(path: &str) -> Ilst {
+		read_ilst(path, ParsingMode::BestAttempt)
 	}
 
 	fn verify_atom(ilst: &Ilst, ident: [u8; 4], data: &AtomData) {
@@ -843,7 +851,7 @@ mod tests {
 
 	#[test]
 	fn ilst_re_read() {
-		let parsed_tag = read_ilst("tests/tags/assets/ilst/test.ilst");
+		let parsed_tag = read_ilst_strict("tests/tags/assets/ilst/test.ilst");
 
 		let mut writer = Vec::new();
 		parsed_tag.dump_to(&mut writer).unwrap();
@@ -935,7 +943,7 @@ mod tests {
 
 	#[test]
 	fn issue_34() {
-		let ilst = read_ilst("tests/tags/assets/ilst/issue_34.ilst");
+		let ilst = read_ilst_strict("tests/tags/assets/ilst/issue_34.ilst");
 
 		verify_atom(
 			&ilst,
@@ -954,7 +962,7 @@ mod tests {
 
 	#[test]
 	fn advisory_rating() {
-		let ilst = read_ilst("tests/tags/assets/ilst/advisory_rating.ilst");
+		let ilst = read_ilst_strict("tests/tags/assets/ilst/advisory_rating.ilst");
 
 		verify_atom(
 			&ilst,
@@ -1073,7 +1081,7 @@ mod tests {
 
 	#[test]
 	fn multi_value_atom() {
-		let ilst = read_ilst("tests/tags/assets/ilst/multi_value_atom.ilst");
+		let ilst = read_ilst_strict("tests/tags/assets/ilst/multi_value_atom.ilst");
 		let artist_atom = ilst.get(&AtomIdent::Fourcc(*b"\xa9ART")).unwrap();
 
 		assert_eq!(
@@ -1184,7 +1192,7 @@ mod tests {
 
 	#[test]
 	fn invalid_atom_type() {
-		let ilst = read_ilst("tests/tags/assets/ilst/invalid_atom_type.ilst");
+		let ilst = read_ilst_strict("tests/tags/assets/ilst/invalid_atom_type.ilst");
 
 		// The tag contains 3 items, however the last one has an invalid type. We will stop at that point, but retain the
 		// first two items.
@@ -1195,4 +1203,19 @@ mod tests {
 		assert_eq!(ilst.disk().unwrap(), 1);
 		assert_eq!(ilst.disk_total().unwrap(), 2);
 	}
+
+	#[test]
+	fn invalid_string_encoding() {
+		let ilst = read_ilst_bestattempt("tests/tags/assets/ilst/invalid_string_encoding.ilst");
+
+		// The tag has an album string with some unknown encoding, but the rest of the tag
+		// is valid. We should have all items present except the album.
+		assert_eq!(ilst.len(), 3);
+
+		assert_eq!(ilst.artist().unwrap(), "Foo artist");
+		assert_eq!(ilst.title().unwrap(), "Bar title");
+		assert_eq!(ilst.comment().unwrap(), "Baz comment");
+
+		assert!(ilst.album().is_none());
+	}
 }

src/mp4/ilst/read.rs

@@ -2,7 +2,7 @@ use super::constants::{
 	BE_SIGNED_INTEGER, BE_UNSIGNED_INTEGER, BMP, JPEG, PNG, RESERVED, UTF16, UTF8,
 };
 use super::{Atom, AtomData, AtomIdent, Ilst};
-use crate::error::Result;
+use crate::error::{Result, LoftyError};
 use crate::id3::v1::constants::GENRES;
 use crate::macros::{err, try_vec};
 use crate::mp4::atom_info::AtomInfo;
@@ -144,11 +144,25 @@ fn parse_data<R>(
 where
 	R: Read + Seek,
 {
+	let handle_error =
+		|err: LoftyError, parsing_mode: ParsingMode| -> Result<()> {
+			match parsing_mode {
+				ParsingMode::Strict => Err(err),
+				ParsingMode::BestAttempt | ParsingMode::Relaxed => {
+					log::warn!("Skipping atom with invalid content: {}", err);
+					Ok(())
+				},
+			}
+		};
+
 	if let Some(mut atom_data) = parse_data_inner(reader, parsing_mode, &atom_info)? {
 		// Most atoms we encounter are only going to have 1 value, so store them as such
 		if atom_data.len() == 1 {
 			let (flags, content) = atom_data.remove(0);
-			let data = interpret_atom_content(flags, content)?;
+			let data = match interpret_atom_content(flags, content) {
+				Ok(data) => data,
+				Err(err) => return handle_error(err, parsing_mode),
+			};
 
 			tag.atoms.push(Atom {
 				ident: atom_info.ident,
@@ -160,7 +174,11 @@ where
 
 		let mut data = Vec::new();
 		for (flags, content) in atom_data {
-			let value = interpret_atom_content(flags, content)?;
+			let value = match interpret_atom_content(flags, content) {
+				Ok(data) => data,
+				Err(err) => return handle_error(err, parsing_mode),
+			};
+			
 			data.push(value);
 		}