diff --git a/CHANGELOG.md b/CHANGELOG.md index ca8b051ad..8e81a1455 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,15 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- **ItemKey**: `ItemKey::TrackArtists`, available for ID3v2, Vorbis Comments, APE, and MP4 Ilst ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454)) +- **ItemKey**: `ItemKey::TrackArtists`, available for ID3v2, Vorbis Comments, APE, and MP4 Ilst ([PR](https://github.com/Serial-ATA/lofty-rs/pull/454)) - This is a multi-value item that stores each artist for a track. It should be retrieved with `Tag::get_strings` or `Tag::take_strings`. - For example, a track has `ItemKey::TrackArtist` = "Foo & Bar", then `ItemKey::TrackArtists` = ["Foo", "Bar"]. - See +- **UnsynchronizedStream**: `UnsynchronizedStream::get_ref()` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/459)) ### Fixed - **MusePack**: Fix potential panic when the beginning silence makes up the entire sample count ([PR](https://github.com/Serial-ATA/lofty-rs/pull/449)) - **Timestamp**: Support timestamps without separators (ex. "20240906" vs "2024-09-06") ([issue](https://github.com/Serial-ATA/lofty-rs/issues/452)) ([PR](https://github.com/Serial-ATA/lofty-rs/issues/453)) -- **ID3v2**: `ItemKey::Director` will now be written correctly as a TXXX frame ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454)) +- **ID3v2**: + - `ItemKey::Director` will now be written correctly as a TXXX frame ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454)) + - When skipping invalid frames in `ParsingMode::{BestAttempt, Relaxed}`, the parser will no longer be able to go out of the bounds + of the frame content ([issue](https://github.com/Serial-ATA/lofty-rs/issues/458)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/459)) ## [0.21.1] - 2024-08-28 diff --git a/lofty/src/id3/v2/frame/content.rs b/lofty/src/id3/v2/frame/content.rs index 23deed254..84f4253ed 100644 --- a/lofty/src/id3/v2/frame/content.rs +++ b/lofty/src/id3/v2/frame/content.rs @@ -21,6 +21,8 @@ pub(super) fn parse_content( version: Id3v2Version, parse_mode: ParsingMode, ) -> Result>> { + log::trace!("Parsing frame content for ID: {}", id); + Ok(match id.as_str() { // The ID was previously upgraded, but the content remains unchanged, so version is necessary "APIC" => { diff --git a/lofty/src/id3/v2/frame/read.rs b/lofty/src/id3/v2/frame/read.rs index 4d348f729..b415496bb 100644 --- a/lofty/src/id3/v2/frame/read.rs +++ b/lofty/src/id3/v2/frame/read.rs @@ -4,18 +4,18 @@ use crate::config::{ParseOptions, ParsingMode}; use crate::error::{Id3v2Error, Id3v2ErrorKind, Result}; use crate::id3::v2::frame::content::parse_content; use crate::id3::v2::header::Id3v2Version; +use crate::id3::v2::tag::ATTACHED_PICTURE_ID; use crate::id3::v2::util::synchsafe::{SynchsafeInteger, UnsynchronizedStream}; use crate::id3::v2::{BinaryFrame, FrameFlags, FrameHeader, FrameId}; use crate::macros::try_vec; use std::io::Read; -use crate::id3::v2::tag::ATTACHED_PICTURE_ID; use byteorder::{BigEndian, ReadBytesExt}; pub(crate) enum ParsedFrame<'a> { Next(Frame<'a>), - Skip { size: u32 }, + Skip, Eof, } @@ -46,16 +46,19 @@ impl<'a> ParsedFrame<'a> { match parse_options.parsing_mode { ParsingMode::Strict => return Err(err), ParsingMode::BestAttempt | ParsingMode::Relaxed => { + log::warn!("Failed to read frame header, skipping: {}", err); + // Skip this frame and continue reading - // TODO: Log error? - return Ok(Self::Skip { size }); + skip_frame(reader, size)?; + return Ok(Self::Skip); }, } }, }; if !parse_options.read_cover_art && id == ATTACHED_PICTURE_ID { - return Ok(Self::Skip { size }); + skip_frame(reader, size)?; + return Ok(Self::Skip); } if size == 0 { @@ -64,7 +67,9 @@ impl<'a> ParsedFrame<'a> { } log::debug!("Encountered a zero length frame, skipping"); - return Ok(Self::Skip { size }); + + skip_frame(reader, size)?; + return Ok(Self::Skip); } // Get the encryption method symbol @@ -252,6 +257,26 @@ fn parse_frame( ) -> Result> { match parse_content(reader, id, flags, version, parse_mode)? { Some(frame) => Ok(ParsedFrame::Next(frame)), - None => Ok(ParsedFrame::Skip { size }), + None => { + skip_frame(reader, size)?; + Ok(ParsedFrame::Skip) + }, } } + +// Note that this is only ever given the full frame size. +// +// In the context of `ParsedFrame::read`, the reader is restricted to the frame content, so this +// is a safe operation, regardless of where we are in parsing the frame. +// +// This assumption *CANNOT* be made in other contexts. +fn skip_frame(reader: &mut impl Read, size: u32) -> Result<()> { + log::trace!("Skipping frame of size {}", size); + + let size = u64::from(size); + let mut reader = reader.take(size); + let skipped = std::io::copy(&mut reader, &mut std::io::sink())?; + debug_assert!(skipped <= size); + + Ok(()) +} diff --git a/lofty/src/id3/v2/read.rs b/lofty/src/id3/v2/read.rs index f99f2287a..b7fe1fbaa 100644 --- a/lofty/src/id3/v2/read.rs +++ b/lofty/src/id3/v2/read.rs @@ -2,7 +2,7 @@ use super::frame::read::ParsedFrame; use super::header::Id3v2Header; use super::tag::Id3v2Tag; use crate::config::ParseOptions; -use crate::error::{Id3v2Error, Id3v2ErrorKind, Result}; +use crate::error::Result; use crate::id3::v2::util::synchsafe::UnsynchronizedStream; use crate::id3::v2::{Frame, FrameId, Id3v2Version, TimestampFrame}; use crate::tag::items::Timestamp; @@ -130,19 +130,6 @@ fn construct_tdrc_from_v3(tag: &mut Id3v2Tag) { } } -fn skip_frame(reader: &mut impl Read, size: u32) -> Result<()> { - log::trace!("Skipping frame of size {}", size); - - let size = u64::from(size); - let mut reader = reader.take(size); - let skipped = std::io::copy(&mut reader, &mut std::io::sink())?; - debug_assert!(skipped <= size); - if skipped != size { - return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameLength).into()); - } - Ok(()) -} - fn read_all_frames_into_tag( reader: &mut R, header: Id3v2Header, @@ -181,9 +168,7 @@ where } }, // No frame content found or ignored due to errors, but we can expect more frames - ParsedFrame::Skip { size } => { - skip_frame(reader, size)?; - }, + ParsedFrame::Skip => {}, // No frame content found, and we can expect there are no more frames ParsedFrame::Eof => break, } diff --git a/lofty/src/id3/v2/tag.rs b/lofty/src/id3/v2/tag.rs index efd87629f..c442118d5 100644 --- a/lofty/src/id3/v2/tag.rs +++ b/lofty/src/id3/v2/tag.rs @@ -574,7 +574,7 @@ pub(crate) struct GenresIter<'a> { } impl<'a> GenresIter<'a> { - pub fn new(value: &'a str, preserve_indexes: bool) -> GenresIter<'_> { + pub fn new(value: &'a str, preserve_indexes: bool) -> GenresIter<'a> { GenresIter { value, pos: 0, diff --git a/lofty/src/id3/v2/tag/tests.rs b/lofty/src/id3/v2/tag/tests.rs index 10fa44233..94220c6d9 100644 --- a/lofty/src/id3/v2/tag/tests.rs +++ b/lofty/src/id3/v2/tag/tests.rs @@ -1554,3 +1554,32 @@ fn artists_tag_conversion() { assert_eq!(id3v2_artists, ARTISTS); } + +#[test_log::test] +fn ensure_frame_skipping_within_bounds() { + // This tag has an invalid `TDEN` frame, but it is skippable in BestAttempt/Relaxed parsing mode. + // We should be able to continue reading the tag as normal, reaching the other `TDTG` frame. + + let path = "tests/tags/assets/id3v2/skippable_frame_otherwise_valid.id3v24"; + let tag = read_tag_with_options( + &read_path(path), + ParseOptions::new().parsing_mode(ParsingMode::BestAttempt), + ); + + assert_eq!(tag.len(), 1); + assert_eq!( + tag.get(&FrameId::Valid(Cow::Borrowed("TDTG"))), + Some(&Frame::Timestamp(TimestampFrame::new( + FrameId::Valid(Cow::Borrowed("TDTG")), + TextEncoding::Latin1, + Timestamp { + year: 2014, + month: Some(6), + day: Some(10), + hour: Some(2), + minute: Some(16), + second: Some(10), + }, + ))) + ); +} diff --git a/lofty/src/id3/v2/util/synchsafe.rs b/lofty/src/id3/v2/util/synchsafe.rs index 516d877ee..019208a02 100644 --- a/lofty/src/id3/v2/util/synchsafe.rs +++ b/lofty/src/id3/v2/util/synchsafe.rs @@ -78,6 +78,25 @@ impl UnsynchronizedStream { pub fn into_inner(self) -> R { self.reader } + + /// Get a reference to the inner reader + /// + /// # Examples + /// + /// ```rust + /// use lofty::id3::v2::util::synchsafe::UnsynchronizedStream; + /// use std::io::Cursor; + /// + /// # fn main() -> lofty::error::Result<()> { + /// let reader = Cursor::new([0xFF, 0x00, 0x1A]); + /// let unsynchronized_reader = UnsynchronizedStream::new(reader); + /// + /// let reader = unsynchronized_reader.get_ref(); + /// assert_eq!(reader.position(), 0); + /// # Ok(()) } + pub fn get_ref(&self) -> &R { + &self.reader + } } impl Read for UnsynchronizedStream { diff --git a/lofty/src/ogg/tag.rs b/lofty/src/ogg/tag.rs index e5e4f4a87..cb36ed82b 100644 --- a/lofty/src/ogg/tag.rs +++ b/lofty/src/ogg/tag.rs @@ -210,7 +210,7 @@ impl VorbisComments { /// let all_artists = vorbis_comments.get_all("ARTIST").collect::>(); /// assert_eq!(all_artists, vec!["Foo artist", "Bar artist", "Baz artist"]); /// ``` - pub fn get_all<'a>(&'a self, key: &'a str) -> impl Iterator + Clone + '_ { + pub fn get_all<'a>(&'a self, key: &'a str) -> impl Iterator + Clone + 'a { self.items .iter() .filter_map(move |(k, v)| (k.eq_ignore_ascii_case(key)).then_some(v.as_str())) diff --git a/lofty/src/ogg/vorbis/properties.rs b/lofty/src/ogg/vorbis/properties.rs index 1bdc1c7e9..caa69abec 100644 --- a/lofty/src/ogg/vorbis/properties.rs +++ b/lofty/src/ogg/vorbis/properties.rs @@ -121,7 +121,7 @@ where let last_page_abgp = last_page.header().abgp; if properties.sample_rate > 0 { - let total_samples = last_page_abgp.saturating_sub(first_page_abgp) as u128; + let total_samples = u128::from(last_page_abgp.saturating_sub(first_page_abgp)); // Best case scenario if total_samples > 0 { diff --git a/lofty/tests/tags/assets/id3v2/skippable_frame_otherwise_valid.id3v24 b/lofty/tests/tags/assets/id3v2/skippable_frame_otherwise_valid.id3v24 new file mode 100644 index 000000000..d29308bb9 Binary files /dev/null and b/lofty/tests/tags/assets/id3v2/skippable_frame_otherwise_valid.id3v24 differ