Skip to content

Move some errors to strict mode #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `AtomData::data_type()` to get the data type code of the atom content.

### Changed
- **Ilst**: Add new rules for `gnre` atom upgrades ([issue](https://github.com/Serial-ATA/lofty-rs/issues/409)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/485))
- In the case that a `©gen` and `gnre` atom are present in a file, there was no way to tell which `©gen` atoms were upgraded.
the new rules are:
- `gnre` present + no `©gen` present, `gnre` gets upgraded as normal
- `gnre` present + `©gen` present, `©gen` takes precedence and `gnre` is discarded
- With [ParsingOptions::implicit_conversions](https://docs.rs/lofty/latest/lofty/config/struct.ParseOptions.html#method.implicit_conversions)
set to `false`, `gnre` will be retained as an atom of type `Unknown`.
- **Ilst**:
- Add new rules for `gnre` atom upgrades ([issue](https://github.com/Serial-ATA/lofty-rs/issues/409)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/485))
- In the case that a `©gen` and `gnre` atom are present in a file, there was no way to tell which `©gen` atoms were upgraded.
the new rules are:
- `gnre` present + no `©gen` present, `gnre` gets upgraded as normal
- `gnre` present + `©gen` present, `©gen` takes precedence and `gnre` is discarded
- With [ParsingOptions::implicit_conversions](https://docs.rs/lofty/latest/lofty/config/struct.ParseOptions.html#method.implicit_conversions)
set to `false`, `gnre` will be retained as an atom of type `Unknown`.
- Ignore invalid `covr` data types when not using `ParsingMode::Strict` ([issue](https://github.com/Serial-ATA/lofty-rs/issues/482)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/486))
- **RIFF INFO**: Ignore text decoding errors when not using `ParsingMode::Strict` ([issue](https://github.com/Serial-ATA/lofty-rs/issues/373)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/486))
- RIFF INFO tags may be encoded with a non UTF-8 system encoding, that we have no way of knowing. It's no longer an error to read these files,
it's just unlikely that anything useful come out of the RIFF INFO tags.

### 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))
Expand Down
8 changes: 7 additions & 1 deletion lofty/src/iff/wav/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,13 @@ where
err!(SizeMismatch);
}

super::tag::read::parse_riff_info(data, &mut chunks, end, &mut riff_info)?;
super::tag::read::parse_riff_info(
data,
&mut chunks,
end,
&mut riff_info,
parse_options.parsing_mode,
)?;
},
_ => {
data.seek(SeekFrom::Current(-4))?;
Expand Down
6 changes: 5 additions & 1 deletion lofty/src/iff/wav/tag/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ pub(crate) fn tagitems_into_riff<'a>(

#[cfg(test)]
mod tests {
use crate::config::WriteOptions;
use crate::config::{ParsingMode, WriteOptions};
use crate::iff::chunk::Chunks;
use crate::iff::wav::RiffInfoList;
use crate::prelude::*;
Expand Down Expand Up @@ -383,6 +383,7 @@ mod tests {
&mut Chunks::<LittleEndian>::new(tag.len() as u64),
(tag.len() - 1) as u64,
&mut parsed_tag,
ParsingMode::Strict,
)
.unwrap();

Expand All @@ -399,6 +400,7 @@ mod tests {
&mut Chunks::<LittleEndian>::new(tag.len() as u64),
(tag.len() - 1) as u64,
&mut parsed_tag,
ParsingMode::Strict,
)
.unwrap();

Expand All @@ -415,6 +417,7 @@ mod tests {
&mut Chunks::<LittleEndian>::new(tag.len() as u64),
(tag.len() - 13) as u64,
&mut temp_parsed_tag,
ParsingMode::Strict,
)
.unwrap();

Expand All @@ -433,6 +436,7 @@ mod tests {
&mut Chunks::<LittleEndian>::new(tag_bytes.len() as u64),
(tag_bytes.len() - 1) as u64,
&mut riff_info,
ParsingMode::Strict,
)
.unwrap();

Expand Down
33 changes: 25 additions & 8 deletions lofty/src/iff/wav/tag/read.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::RiffInfoList;
use crate::error::Result;
use crate::config::ParsingMode;
use crate::error::{ErrorKind, Result};
use crate::iff::chunk::Chunks;
use crate::macros::decode_err;
use crate::util::text::utf8_decode_str;
Expand All @@ -13,24 +14,40 @@ pub(in crate::iff::wav) fn parse_riff_info<R>(
chunks: &mut Chunks<LittleEndian>,
end: u64,
tag: &mut RiffInfoList,
parse_mode: ParsingMode,
) -> Result<()>
where
R: Read + Seek,
{
while data.stream_position()? != end && chunks.next(data).is_ok() {
let key_str = utf8_decode_str(&chunks.fourcc)
.map_err(|_| decode_err!(Wav, "Non UTF-8 item key found in RIFF INFO"))?;
.map_err(|_| decode_err!(Wav, "Invalid item key found in RIFF INFO"))?;

if !verify_key(key_str) {
decode_err!(@BAIL Wav, "RIFF INFO item key contains invalid characters");
}

tag.items.push((
key_str.to_owned(),
chunks
.read_cstring(data)
.map_err(|_| decode_err!(Wav, "Failed to read RIFF INFO item value"))?,
));
let key = key_str.to_owned();
let value;
match chunks.read_cstring(data) {
Ok(cstr) => value = cstr,
Err(e) => {
if parse_mode == ParsingMode::Strict {
decode_err!(@BAIL Wav, "Failed to read RIFF INFO item value")
}

// RIFF INFO tags have no standard text encoding, so they will occasionally default
// to the system encoding, which isn't always UTF-8. In reality, if one item fails
// they likely all will, but we'll keep trying.
if matches!(e.kind(), ErrorKind::StringFromUtf8(_)) {
continue;
}

return Err(e);
},
}

tag.items.push((key, value));
}

Ok(())
Expand Down
12 changes: 11 additions & 1 deletion lofty/src/mp4/ilst/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,17 @@ where
DataType::Jpeg => Some(MimeType::Jpeg),
DataType::Png => Some(MimeType::Png),
DataType::Bmp => Some(MimeType::Bmp),
_ => err!(BadAtom("\"covr\" atom has an unknown type")),
_ => {
if parsing_mode == ParsingMode::Strict {
err!(BadAtom("\"covr\" atom has an unknown type"))
}

log::warn!(
"Encountered \"covr\" atom with an unknown type of `{}`, discarding",
Into::<u32>::into(data_type)
);
return Ok(());
},
};

let picture_data = AtomData::Picture(Picture {
Expand Down