IFF: Support invalid chunk padding

Author: Serial-ATA

CHANGELOG.md

@@ -19,6 +19,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     when during generic conversions ([issue](https://github.com/Serial-ATA/lofty-rs/issues/621)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/623))
 - **Timestamp**: Only enforce valid year in strict mode ([issue](https://github.com/Serial-ATA/lofty-rs/issues/615)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/616))
 - **OGG Vorbis**: Fixed potential infinite loop while property reading ([issue](https://github.com/Serial-ATA/lofty-rs/issues/620)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/622))
+- **IFF**: Support chunks with invalid padding ([issue](https://github.com/Serial-ATA/lofty-rs/issues/619)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/627))
+  - When RIFF/AIFF chunks have an odd length, they should be padded, and the padding is not counted in the chunk size.
+    However, some encoders incorrectly include the padding in the size, which can cause the parser to go out of sync.
+    We no longer assume padding is valid and have additional checks to stay in sync when possible.
 
 ## [0.23.2] - 2026-02-14
 

lofty/src/id3/v2/write/chunk_file.rs

@@ -1,6 +1,6 @@
-use crate::config::WriteOptions;
+use crate::config::{ParseOptions, WriteOptions};
 use crate::error::{LoftyError, Result};
-use crate::iff::chunk::Chunks;
+use crate::iff::chunk::{Chunks, IFF_CHUNK_HEADER_SIZE};
 use crate::macros::err;
 use crate::util::io::{FileLike, Length, Truncate};
 
@@ -10,7 +10,6 @@ use byteorder::{ByteOrder, WriteBytesExt};
 
 const CHUNK_NAME_UPPER: [u8; 4] = [b'I', b'D', b'3', b' '];
 const CHUNK_NAME_LOWER: [u8; 4] = [b'i', b'd', b'3', b' '];
-const RIFF_CHUNK_HEADER_SIZE: usize = 8;
 
 pub(in crate::id3::v2) fn write_to_chunk_file<F, B>(
 	file: &mut F,
@@ -23,54 +22,70 @@ where
 	LoftyError: From<<F as Length>::Error>,
 	B: ByteOrder,
 {
-	// Only rely on the actual file for the first chunk read
+	const FIRST_CHUNK_LEN: u64 = (IFF_CHUNK_HEADER_SIZE as u64) + 4;
+
+	// We only want to rely on the file size for the first chunk read.
+	// Since a file can have trailing junk, but otherwise be valid, we actually want to use the
+	// first chunk size, which (should) encompass the entire stream.
 	let file_len = file.len()?;
 
-	let mut chunks = Chunks::<B>::new(file_len);
-	chunks.next(file)?;
+	let mut chunks = Chunks::<_, B>::new(file, file_len);
+
+	// TODO: Forcing the use of ParseOptions::default()
+	let parse_options = ParseOptions::default();
+	let Some(first_chunk) = chunks.next(parse_options.parsing_mode)? else {
+		err!(UnknownFormat);
+	};
 
-	let mut actual_stream_size = chunks.size;
+	let mut actual_stream_size = first_chunk.size();
 
+	let file = chunks.into_inner();
 	file.rewind()?;
 
 	let mut file_bytes = Cursor::new(Vec::with_capacity(actual_stream_size as usize));
 	file.read_to_end(file_bytes.get_mut())?;
 
-	if file_bytes.get_ref().len() < (actual_stream_size as usize + RIFF_CHUNK_HEADER_SIZE) {
+	if file_bytes.get_ref().len() < (actual_stream_size as usize + IFF_CHUNK_HEADER_SIZE as usize) {
 		err!(SizeMismatch);
 	}
 
 	// The first chunk format is RIFF....WAVE
-	file_bytes.seek(SeekFrom::Start(12))?;
+	file_bytes.seek(SeekFrom::Start(FIRST_CHUNK_LEN))?;
+
+	let mut existing_id3_tag = None;
+
+	let mut chunks = Chunks::<_, B>::new(file_bytes, u64::from(actual_stream_size));
+	while let Some(chunk) = chunks.next(parse_options.parsing_mode)? {
+		if chunk.fourcc == CHUNK_NAME_UPPER || chunk.fourcc == CHUNK_NAME_LOWER {
+			// Need to add FIRST_CHUNK_LEN since we started the chunk reader at that offset
+			let chunk_start = chunk.start() + FIRST_CHUNK_LEN;
 
-	let (mut exising_id3_start, mut existing_id3_size) = (None, None);
+			// Can't trust the written chunk size, since some encoders don't handle padding
+			// correctly, see Chunks::skip(). Since skip detects invalid padding, we just let it
+			// do the work and figure out where we are in the file afterwards.
+			chunks.skip()?;
 
-	let mut chunks = Chunks::<B>::new(u64::from(actual_stream_size));
-	while let Ok(true) = chunks.next(&mut file_bytes) {
-		if chunks.fourcc == CHUNK_NAME_UPPER || chunks.fourcc == CHUNK_NAME_LOWER {
-			exising_id3_start = Some(file_bytes.stream_position()? - 8);
-			existing_id3_size = Some(chunks.size);
+			let chunk_end = chunks.stream_position() + FIRST_CHUNK_LEN;
+
+			log::debug!(
+				"Found existing ID3v2 chunk, size: {} bytes",
+				chunk_end - chunk_start
+			);
+			existing_id3_tag = Some(chunk_start..chunk_end);
 			break;
 		}
-
-		chunks.skip(&mut file_bytes)?;
 	}
 
-	if let (Some(exising_id3_start), Some(mut existing_id3_size)) =
-		(exising_id3_start, existing_id3_size)
-	{
-		// We need to remove the padding byte if it exists
-		if existing_id3_size % 2 != 0 {
-			existing_id3_size += 1;
-		}
+	let mut file_bytes = chunks.into_inner();
+
+	if let Some(existing_id3_tag) = existing_id3_tag {
+		let tag_size = existing_id3_tag.end - existing_id3_tag.start;
 
-		let existing_tag_end =
-			exising_id3_start as usize + RIFF_CHUNK_HEADER_SIZE + existing_id3_size as usize;
 		let _ = file_bytes
 			.get_mut()
-			.drain(exising_id3_start as usize..existing_tag_end);
+			.drain(existing_id3_tag.start as usize..existing_id3_tag.end as usize);
 
-		actual_stream_size -= existing_id3_size + RIFF_CHUNK_HEADER_SIZE as u32;
+		actual_stream_size -= tag_size as u32;
 	}
 
 	if !tag.is_empty() {
@@ -94,7 +109,7 @@ where
 			err!(TooMuchData)
 		};
 
-		let tag_position = actual_stream_size as usize + RIFF_CHUNK_HEADER_SIZE;
+		let tag_position = actual_stream_size as usize + IFF_CHUNK_HEADER_SIZE as usize;
 
 		file_bytes.get_mut().splice(
 			tag_position..tag_position,

lofty/src/iff/aiff/read.rs

@@ -52,7 +52,9 @@ where
 	let compression_present = verify_aiff(data)?;
 
 	let current_pos = data.stream_position()?;
-	let file_len = data.seek(SeekFrom::End(0))?;
+
+	// - 12 for the FORM chunk content we already read
+	let file_len = data.seek(SeekFrom::End(0))?.saturating_sub(12);
 
 	data.seek(SeekFrom::Start(current_pos))?;
 
@@ -65,12 +67,11 @@ where
 
 	let mut id3v2_tag: Option<Id3v2Tag> = None;
 
-	let mut chunks = Chunks::<BigEndian>::new(file_len);
-
-	while let Ok(true) = chunks.next(data) {
-		match &chunks.fourcc {
+	let mut chunks = Chunks::<_, BigEndian>::new(data, file_len);
+	while let Some(mut chunk) = chunks.next(parse_options.parsing_mode)? {
+		match &chunk.fourcc {
 			b"ID3 " | b"id3 " if parse_options.read_tags => {
-				let tag = chunks.id3_chunk(data, parse_options)?;
+				let tag = chunk.id3_chunk(parse_options)?;
 				if let Some(existing_tag) = id3v2_tag.as_mut() {
 					log::warn!("Duplicate ID3v2 tag found, appending frames to previous tag");
 
@@ -84,58 +85,56 @@ where
 				id3v2_tag = Some(tag);
 			},
 			b"COMM" if parse_options.read_properties && comm.is_none() => {
-				if chunks.size < 18 {
+				if chunk.size() < 18 {
 					decode_err!(@BAIL Aiff, "File has an invalid \"COMM\" chunk size (< 18)");
 				}
 
-				comm = Some(chunks.content(data)?);
-				chunks.correct_position(data)?;
+				comm = Some(chunk.content()?);
 			},
 			b"SSND" if parse_options.read_properties => {
-				stream_len = chunks.size;
-				chunks.skip(data)?;
+				stream_len = chunk.size();
 			},
 			b"ANNO" if parse_options.read_tags => {
-				annotations.push(chunks.read_pstring(data, None)?);
+				annotations.push(chunk.read_string(None)?);
 			},
 			// These four chunks are expected to appear at most once per file,
 			// so there's no need to replace anything we already read
 			b"COMT" if comments.is_empty() && parse_options.read_tags => {
-				if chunks.size < 2 {
+				if chunk.size() < 2 {
 					continue;
 				}
 
-				let num_comments = data.read_u16::<BigEndian>()?;
+				let num_comments = chunk.read_u16::<BigEndian>()?;
 
 				for _ in 0..num_comments {
-					let timestamp = data.read_u32::<BigEndian>()?;
-					let marker_id = data.read_u16::<BigEndian>()?;
-					let size = data.read_u16::<BigEndian>()?;
+					let timestamp = chunk.read_u32::<BigEndian>()?;
+					let marker_id = chunk.read_u16::<BigEndian>()?;
+					let size = chunk.read_u16::<BigEndian>()?;
 
-					let text = chunks.read_pstring(data, Some(u32::from(size)))?;
+					let text = chunk.read_string(Some(u32::from(size)))?;
 
 					comments.push(Comment {
 						timestamp,
 						marker_id,
 						text,
 					})
 				}
-
-				chunks.correct_position(data)?;
 			},
 			b"NAME" if text_chunks.name.is_none() && parse_options.read_tags => {
-				text_chunks.name = Some(chunks.read_pstring(data, None)?);
+				text_chunks.name = Some(chunk.read_string(None)?);
 			},
 			b"AUTH" if text_chunks.author.is_none() && parse_options.read_tags => {
-				text_chunks.author = Some(chunks.read_pstring(data, None)?);
+				text_chunks.author = Some(chunk.read_string(None)?);
 			},
 			b"(c) " if text_chunks.copyright.is_none() && parse_options.read_tags => {
-				text_chunks.copyright = Some(chunks.read_pstring(data, None)?);
+				text_chunks.copyright = Some(chunk.read_string(None)?);
 			},
-			_ => chunks.skip(data)?,
+			_ => {},
 		}
 	}
 
+	let data = chunks.into_inner();
+
 	if !annotations.is_empty() {
 		text_chunks.annotations = Some(annotations);
 	}

lofty/src/iff/aiff/tag.rs

@@ -1,13 +1,13 @@
-use crate::config::WriteOptions;
+use crate::config::{ParseOptions, WriteOptions};
 use crate::error::{LoftyError, Result};
-use crate::iff::chunk::Chunks;
-use crate::macros::err;
+use crate::iff::chunk::{Chunks, IFF_CHUNK_HEADER_SIZE};
+use crate::macros::{encode_err, err};
 use crate::tag::{Accessor, ItemKey, ItemValue, MergeTag, SplitTag, Tag, TagExt, TagItem, TagType};
 use crate::util::io::{FileLike, Length, Truncate};
 
 use std::borrow::Cow;
 use std::convert::TryFrom;
-use std::io::{SeekFrom, Write};
+use std::io::Write;
 
 use byteorder::BigEndian;
 use lofty_attr::tag;
@@ -427,60 +427,69 @@ where
 		LoftyError: From<<F as Truncate>::Error>,
 		LoftyError: From<<F as Length>::Error>,
 	{
+		// FORM....AIFF
+		const FIRST_CHUNK_LEN: u64 = (IFF_CHUNK_HEADER_SIZE as u64) + 4;
+
 		super::read::verify_aiff(file)?;
-		let file_len = file.len()?.saturating_sub(12);
+		let file_len = file.len()?.saturating_sub(FIRST_CHUNK_LEN);
 
 		let text_chunks = Self::create_text_chunks(&mut tag)?;
 
-		let mut chunks_remove = Vec::new();
-
-		let mut chunks = Chunks::<BigEndian>::new(file_len);
+		let mut chunks_to_remove = Vec::new();
+		let mut comm_end = None;
 
-		while let Ok(true) = chunks.next(file) {
-			match &chunks.fourcc {
+		// TODO: Forcing the use of ParseOptions::default()
+		let parse_options = ParseOptions::default();
+		let mut chunks = Chunks::<_, BigEndian>::new(file, file_len);
+		while let Some(chunk) = chunks.next(parse_options.parsing_mode)? {
+			match &chunk.fourcc {
 				b"NAME" | b"AUTH" | b"(c) " | b"ANNO" | b"COMT" => {
-					let start = (file.stream_position()? - 8) as usize;
-					let mut end = start + 8 + chunks.size as usize;
+					// Need to add FIRST_CHUNK_LEN since we started the chunk reader at that offset
+					let start = chunk.start() + FIRST_CHUNK_LEN;
 
-					if chunks.size % 2 != 0 {
-						end += 1
-					}
+					// Can't trust the written chunk size, since some encoders don't handle padding
+					// correctly, see Chunks::skip(). Since skip detects invalid padding, we just let it
+					// do the work and figure out where we are in the file afterwards.
+					chunks.skip()?;
 
-					chunks_remove.push((start, end))
+					let end = chunks.stream_position() + FIRST_CHUNK_LEN;
+
+					chunks_to_remove.push((start as usize, end as usize))
+				},
+				b"COMM" => {
+					chunks.skip()?;
+					comm_end = Some(chunks.stream_position() + FIRST_CHUNK_LEN);
 				},
 				_ => {},
 			}
-
-			chunks.skip(file)?;
 		}
 
+		let Some(comm_end) = comm_end else {
+			encode_err!(@BAIL Aiff, "COMM chunk not found");
+		};
+
+		let file = chunks.into_inner();
 		file.rewind()?;
 
 		let mut file_bytes = Vec::new();
 		file.read_to_end(&mut file_bytes)?;
 
-		if chunks_remove.is_empty() {
-			file.seek(SeekFrom::Start(16))?;
-
-			let mut size = [0; 4];
-			file.read_exact(&mut size)?;
-
-			let comm_end = (20 + u32::from_le_bytes(size)) as usize;
-			file_bytes.splice(comm_end..comm_end, text_chunks);
+		if chunks_to_remove.is_empty() {
+			file_bytes.splice((comm_end as usize)..(comm_end as usize), text_chunks);
 		} else {
-			chunks_remove.sort_unstable();
-			chunks_remove.reverse();
+			chunks_to_remove.sort_unstable();
+			chunks_to_remove.reverse();
 
-			let first = chunks_remove.pop().unwrap(); // Infallible
+			let first = chunks_to_remove.pop().unwrap(); // Infallible
 
-			for (s, e) in &chunks_remove {
+			for (s, e) in &chunks_to_remove {
 				file_bytes.drain(*s..*e);
 			}
 
 			file_bytes.splice(first.0..first.1, text_chunks);
 		}
 
-		let total_size = ((file_bytes.len() - 8) as u32).to_be_bytes();
+		let total_size = ((file_bytes.len() - IFF_CHUNK_HEADER_SIZE as usize) as u32).to_be_bytes();
 		file_bytes.splice(4..8, total_size.to_vec());
 
 		file.rewind()?;