Tests: Add official ID3v1 test suite

lofty/Cargo.toml

@@ -42,6 +42,7 @@ optional = true
 env_logger = "0.11.8"
 # WAV properties validity tests
 hound     = { git = "https://github.com/ruuda/hound.git", rev = "02e66effb33683dd6acb92df792683ee46ad6a59" }
+regex = "1.12.2"
 rusty-fork = "0.3.0"
 # tag_writer example
 structopt = { version = "0.3.26", default-features = false }

lofty/src/aac/read.rs

@@ -104,7 +104,8 @@ where
 	}
 
 	#[allow(unused_variables)]
-	let ID3FindResults(header, id3v1) = find_id3v1(reader, parse_options.read_tags)?;
+	let ID3FindResults(header, id3v1) =
+		find_id3v1(reader, parse_options.read_tags, parse_options.parsing_mode)?;
 
 	if header.is_some() {
 		let Some(new_stream_len) = stream_len.checked_sub(128) else {

lofty/src/ape/read.rs

@@ -108,7 +108,8 @@ where
 	// Starts with ['T', 'A', 'G']
 	// Exactly 128 bytes long (including the identifier)
 	#[allow(unused_variables)]
-	let ID3FindResults(id3v1_header, id3v1) = find_id3v1(data, parse_options.read_tags)?;
+	let ID3FindResults(id3v1_header, id3v1) =
+		find_id3v1(data, parse_options.read_tags, parse_options.parsing_mode)?;
 
 	if id3v1_header.is_some() {
 		id3v1_tag = id3v1;

lofty/src/ape/tag/write.rs

@@ -47,9 +47,11 @@ where
 	// If one is found, it'll be removed and rewritten at the bottom, where it should be
 	let mut header_ape_tag = (false, (0, 0));
 
-	let start = file.stream_position()?;
 	// TODO: Forcing the use of ParseOptions::default()
-	match read::read_ape_tag(file, false, ParseOptions::new())? {
+	let parse_options = ParseOptions::new();
+
+	let start = file.stream_position()?;
+	match read::read_ape_tag(file, false, parse_options)? {
 		(Some(mut existing_tag), Some(header)) => {
 			if write_options.respect_read_only {
 				// Only keep metadata around that's marked read only
@@ -68,7 +70,7 @@ where
 	}
 
 	// Skip over ID3v1 and Lyrics3v2 tags
-	find_id3v1(file, false)?;
+	find_id3v1(file, false, parse_options.parsing_mode)?;
 	find_lyrics3v2(file)?;
 
 	// In case there's no ape tag already, this is the spot it belongs

lofty/src/id3/mod.rs

@@ -6,9 +6,11 @@
 pub mod v1;
 pub mod v2;
 
+use crate::config::ParsingMode;
 use crate::error::{ErrorKind, LoftyError, Result};
 use crate::macros::try_vec;
 use crate::util::text::utf8_decode_str;
+use v1::constants::ID3V1_TAG_MARKER;
 use v2::header::Id3v2Header;
 
 use std::io::{Read, Seek, SeekFrom};
@@ -54,6 +56,7 @@ where
 pub(crate) fn find_id3v1<R>(
 	data: &mut R,
 	read: bool,
+	parse_mode: ParsingMode,
 ) -> Result<ID3FindResults<(), Option<v1::tag::Id3v1Tag>>>
 where
 	R: Read + Seek,
@@ -75,7 +78,7 @@ where
 	data.seek(SeekFrom::Current(-3))?;
 
 	// No ID3v1 tag found
-	if &id3v1_header != b"TAG" {
+	if id3v1_header != ID3V1_TAG_MARKER {
 		data.seek(SeekFrom::End(0))?;
 		return Ok(ID3FindResults(header, id3v1));
 	}
@@ -90,7 +93,7 @@ where
 
 		data.seek(SeekFrom::End(-128))?;
 
-		id3v1 = Some(v1::read::parse_id3v1(id3v1_tag))
+		id3v1 = Some(v1::tag::Id3v1Tag::parse(id3v1_tag, parse_mode)?)
 	}
 
 	Ok(ID3FindResults(header, id3v1))

lofty/src/id3/v1/constants.rs

@@ -1,3 +1,5 @@
+pub(crate) const ID3V1_TAG_MARKER: [u8; 3] = *b"TAG";
+
 /// All possible genres for ID3v1
 pub const GENRES: [&str; 192] = [
 	"Blues",

lofty/src/id3/v1/read.rs

@@ -1,64 +1,93 @@
-use super::constants::GENRES;
+use super::constants::{GENRES, ID3V1_TAG_MARKER};
 use super::tag::Id3v1Tag;
+use crate::config::ParsingMode;
+use crate::error::LoftyError;
+use crate::macros::err;
+use crate::util::text::latin1_decode;
 
-pub fn parse_id3v1(reader: [u8; 128]) -> Id3v1Tag {
-	let mut tag = Id3v1Tag {
-		title: None,
-		artist: None,
-		album: None,
-		year: None,
-		comment: None,
-		track_number: None,
-		genre: None,
-	};
-
-	let reader = &reader[3..];
-
-	tag.title = decode_text(&reader[..30]);
-	tag.artist = decode_text(&reader[30..60]);
-	tag.album = decode_text(&reader[60..90]);
-
-	let year = try_parse_year(&reader[90..94]).unwrap_or(0);
-	if year != 0 {
-		tag.year = Some(year);
-	}
+impl Id3v1Tag {
+	/// This is **NOT** a public API
+	#[doc(hidden)]
+	pub fn parse(reader: [u8; 128], parse_mode: ParsingMode) -> Result<Self, LoftyError> {
+		let mut tag = Self {
+			title: None,
+			artist: None,
+			album: None,
+			year: None,
+			comment: None,
+			track_number: None,
+			genre: None,
+		};
 
-	// Determine the range of the comment (30 bytes for ID3v1 and 28 for ID3v1.1)
-	// We check for the null terminator 28 bytes in, and for a non-zero track number after it.
-	// A track number of 0 is invalid.
-	let range = if reader[122] == 0 && reader[123] != 0 {
-		tag.track_number = Some(reader[123]);
+		if reader[..3] != ID3V1_TAG_MARKER {
+			err!(FakeTag);
+		}
 
-		94_usize..123
-	} else {
-		94..124
-	};
+		let reader = &reader[3..];
 
-	tag.comment = decode_text(&reader[range]);
+		tag.title = decode_text(&reader[..30]);
+		tag.artist = decode_text(&reader[30..60]);
+		tag.album = decode_text(&reader[60..90]);
 
-	if reader[124] < GENRES.len() as u8 {
-		tag.genre = Some(reader[124]);
-	}
+		tag.year = try_parse_year(&reader[90..94], parse_mode)?;
+
+		// Determine the range of the comment (30 bytes for ID3v1 and 28 for ID3v1.1)
+		// We check for the null terminator 28 bytes in, and for a non-zero track number after it.
+		// A track number of 0 is invalid.
+		let range = if reader[122] == 0 && reader[123] != 0 {
+			tag.track_number = Some(reader[123]);
+
+			94_usize..123
+		} else {
+			94..124
+		};
 
-	tag
+		tag.comment = decode_text(&reader[range]);
+
+		if reader[124] < GENRES.len() as u8 {
+			tag.genre = Some(reader[124]);
+		}
+
+		Ok(tag)
+	}
 }
 
 fn decode_text(data: &[u8]) -> Option<String> {
-	let read = data
-		.iter()
-		.filter(|c| **c != 0)
-		.map(|c| *c as char)
-		.collect::<String>();
+	let mut first_null_pos = data.len();
+	if let Some(null_pos) = data.iter().position(|&b| b == 0) {
+		if null_pos == 0 {
+			return None;
+		}
+
+		if data[null_pos..].iter().any(|b| *b != b'\0') {
+			log::warn!("ID3v1 text field contains trailing junk, skipping");
+		}
+
+		first_null_pos = null_pos;
+	}
 
-	if read.is_empty() { None } else { Some(read) }
+	Some(latin1_decode(&data[..first_null_pos]))
 }
 
-fn try_parse_year(input: &[u8]) -> Option<u16> {
+fn try_parse_year(input: &[u8], parse_mode: ParsingMode) -> Result<Option<u16>, LoftyError> {
 	let (num_digits, year) = input
 		.iter()
 		.take_while(|c| (**c).is_ascii_digit())
 		.fold((0usize, 0u16), |(num_digits, year), c| {
 			(num_digits + 1, year * 10 + u16::from(*c - b'0'))
 		});
-	(num_digits == 4).then_some(year)
+	if num_digits != 4 {
+		// The official test suite says that any year that isn't 4 characters should be a decoding failure.
+		// However, it seems most popular libraries (including us) will write "\0\0\0\0" for empty
+		// years, rather than "0000" as the "spec" would suggest.
+		if parse_mode == ParsingMode::Strict {
+			err!(TextDecode(
+				"ID3v1 year field contains non-ASCII digit characters"
+			));
+		}
+
+		return Ok(None);
+	}
+
+	Ok(Some(year))
 }

lofty/src/id3/v1/tag.rs

@@ -469,7 +469,7 @@ impl Id3v1TagRef<'_> {
 
 #[cfg(test)]
 mod tests {
-	use crate::config::WriteOptions;
+	use crate::config::{ParsingMode, WriteOptions};
 	use crate::id3::v1::Id3v1Tag;
 	use crate::prelude::*;
 	use crate::tag::items::Timestamp;
@@ -488,30 +488,31 @@ mod tests {
 		};
 
 		let tag = crate::tag::utils::test_utils::read_path("tests/tags/assets/test.id3v1");
-		let parsed_tag = crate::id3::v1::read::parse_id3v1(tag.try_into().unwrap());
+		let parsed_tag = Id3v1Tag::parse(tag.try_into().unwrap(), ParsingMode::Strict).unwrap();
 
 		assert_eq!(expected_tag, parsed_tag);
 	}
 
 	#[test_log::test]
 	fn id3v1_re_read() {
 		let tag = crate::tag::utils::test_utils::read_path("tests/tags/assets/test.id3v1");
-		let parsed_tag = crate::id3::v1::read::parse_id3v1(tag.try_into().unwrap());
+		let parsed_tag = Id3v1Tag::parse(tag.try_into().unwrap(), ParsingMode::Strict).unwrap();
 
 		let mut writer = Vec::new();
 		parsed_tag
 			.dump_to(&mut writer, WriteOptions::default())
 			.unwrap();
 
-		let temp_parsed_tag = crate::id3::v1::read::parse_id3v1(writer.try_into().unwrap());
+		let temp_parsed_tag =
+			Id3v1Tag::parse(writer.try_into().unwrap(), ParsingMode::Strict).unwrap();
 
 		assert_eq!(parsed_tag, temp_parsed_tag);
 	}
 
 	#[test_log::test]
 	fn id3v1_to_tag() {
 		let tag_bytes = crate::tag::utils::test_utils::read_path("tests/tags/assets/test.id3v1");
-		let id3v1 = crate::id3::v1::read::parse_id3v1(tag_bytes.try_into().unwrap());
+		let id3v1 = Id3v1Tag::parse(tag_bytes.try_into().unwrap(), ParsingMode::Strict).unwrap();
 
 		let tag: Tag = id3v1.into();
 
@@ -586,7 +587,7 @@ mod tests {
 			.dump_to(&mut bytes, WriteOptions::new().lossy_text_encoding(true))
 			.unwrap();
 
-		let id3v1 = crate::id3::v1::read::parse_id3v1(bytes.try_into().unwrap());
+		let id3v1 = Id3v1Tag::parse(bytes.try_into().unwrap(), ParsingMode::BestAttempt).unwrap();
 		assert_eq!(id3v1.artist.as_deref(), Some("l?fty"));
 
 		// And should fail when disabled

lofty/src/id3/v1/write.rs

@@ -1,5 +1,6 @@
+use super::constants::ID3V1_TAG_MARKER;
 use super::tag::Id3v1TagRef;
-use crate::config::WriteOptions;
+use crate::config::{ParseOptions, WriteOptions};
 use crate::error::{LoftyError, Result};
 use crate::id3::{ID3FindResults, find_id3v1};
 use crate::macros::err;
@@ -32,7 +33,9 @@ where
 	let file = probe.into_inner();
 
 	// This will seek us to the writing position
-	let ID3FindResults(header, _) = find_id3v1(file, false)?;
+	// TODO: Forcing the use of ParseOptions::default()
+	let parse_options = ParseOptions::default();
+	let ID3FindResults(header, _) = find_id3v1(file, false, parse_options.parsing_mode)?;
 
 	if tag.is_empty() && header.is_some() {
 		// An ID3v1 tag occupies the last 128 bytes of the file, so we can just
@@ -70,7 +73,7 @@ pub(super) fn encode(tag: &Id3v1TagRef<'_>, write_options: WriteOptions) -> Resu
 
 	let mut writer = Vec::with_capacity(128);
 
-	writer.write_all(b"TAG")?;
+	writer.write_all(&ID3V1_TAG_MARKER)?;
 
 	let title = resize_string(tag.title, 30, write_options)?;
 	writer.write_all(&title)?;

lofty/src/mpeg/read.rs

@@ -147,7 +147,8 @@ where
 	}
 
 	#[allow(unused_variables)]
-	let ID3FindResults(header, id3v1) = find_id3v1(reader, parse_options.read_tags)?;
+	let ID3FindResults(header, id3v1) =
+		find_id3v1(reader, parse_options.read_tags, parse_options.parsing_mode)?;
 
 	if header.is_some() {
 		file.id3v1_tag = id3v1;

lofty/src/musepack/read.rs

@@ -49,7 +49,8 @@ where
 	let pos_past_id3v2 = reader.stream_position()?;
 
 	#[allow(unused_variables)]
-	let ID3FindResults(header, id3v1) = find_id3v1(reader, parse_options.read_tags)?;
+	let ID3FindResults(header, id3v1) =
+		find_id3v1(reader, parse_options.read_tags, parse_options.parsing_mode)?;
 
 	if header.is_some() {
 		file.id3v1_tag = id3v1;

lofty/src/wavpack/read.rs

@@ -18,7 +18,8 @@ where
 	let mut id3v1_tag = None;
 	let mut ape_tag = None;
 
-	let ID3FindResults(id3v1_header, id3v1) = find_id3v1(reader, parse_options.read_tags)?;
+	let ID3FindResults(id3v1_header, id3v1) =
+		find_id3v1(reader, parse_options.read_tags, parse_options.parsing_mode)?;
 
 	if id3v1_header.is_some() {
 		id3v1_tag = id3v1;

lofty/tests/tags/id3v1/assets/LICENSE

@@ -0,0 +1,6 @@
+ID3v1 and ID3v1.1 testsuite
+
+  This testsuite was written by Martin Nilsson and is Copyright (c)
+  2003 Martin Nilsson. It may be used freely for non-commercial
+  purposes. More information about ID3 is available on
+  http://www.id3.org/