felixc / rexiv2

Rust library for read/write access to media-file metadata (Exif, XMP, and IPTC)
GNU General Public License v3.0
79 stars 17 forks source link

Metadata::get_tag_rational(): avoid panic if denominator is 0 #61

Closed hfiguiere closed 1 year ago

hfiguiere commented 1 year ago

This happen if the tag doesn't exist.

stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:48:5
   3: num_rational::Ratio<T>::reduce
   4: rexiv2::Metadata::get_tag_rational

(the tag doesn't exist)

felixc commented 1 year ago

Hmm that's very strange... When I test with a file/buffer that doesn't have the tag set at all, I correctly get None, thanks to the boolean return value from gexiv2, which the code was already checking.

I'd like to land this patch anyway since it's a robustness improvement in the case of some malformed data.

Do you have a reproducible test case that triggers this? Is it possible the tag is in fact set, but somehow set with a 0 denominator?

Here's what I'm working with (using the raw-tag-access feature for lower-level comparison):


let minipng = [137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0,
              1, 0, 0, 0, 1, 8, 0, 0, 0, 0, 58, 126, 155, 85, 0, 0, 0, 10, 73, 68, 65,
              84, 8, 215, 99, 248, 15, 0, 1, 1, 1, 0, 27, 182, 238, 86, 0, 0, 0, 0, 73,
              69, 78, 68, 174, 66, 96, 130];
let meta = rexiv2::Metadata::new_from_buffer(&minipng).unwrap();

assert_eq!(meta.get_tag_raw("Exif.Photo.MaxApertureValue"), Err(rexiv2::Rexiv2Error::NoValue));
assert_eq!(meta.get_tag_rational("Exif.Photo.MaxApertureValue"), None);

let ratio = num_rational::Ratio::new_raw(16, 10);
meta.set_tag_rational("Exif.Photo.MaxApertureValue", &ratio);

assert_eq!(meta.get_tag_raw("Exif.Photo.MaxApertureValue"), Ok(vec![0, 0, 0, 16, 0, 0, 0, 10]));
assert_eq!(meta.get_tag_rational("Exif.Photo.MaxApertureValue"), Some(ratio));

meta.clear_tag("Exif.Photo.MaxApertureValue");
assert_eq!(meta.get_tag_raw("Exif.Photo.MaxApertureValue"), Err(rexiv2::Rexiv2Error::NoValue));
assert_eq!(meta.get_tag_rational("Exif.Photo.MaxApertureValue"), None);

EDIT: I think I'm unable to create a failing test case, since Exiv2 won't allow setting a rational field with a 0 denominator.

codecov[bot] commented 1 year ago

Codecov Report

Base: 68.67% // Head: 68.63% // Decreases project coverage by -0.04% :warning:

Coverage data is based on head (9e72f6e) compared to base (ff52e16). Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #61 +/- ## ========================================== - Coverage 68.67% 68.63% -0.04% ========================================== Files 1 1 Lines 964 966 +2 ========================================== + Hits 662 663 +1 - Misses 302 303 +1 ``` | [Impacted Files](https://codecov.io/gh/felixc/rexiv2/pull/61?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Felix+Crux) | Coverage Δ | | |---|---|---| | [src/lib.rs](https://codecov.io/gh/felixc/rexiv2/pull/61?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Felix+Crux#diff-c3JjL2xpYi5ycw==) | `68.63% <66.66%> (-0.04%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Felix+Crux). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Felix+Crux)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

hfiguiere commented 1 year ago

I think it's from a "generated" exif from gphoto2 querying the EXIF from a RAW file.

Now the 0 denominator WILL happen whatever. It's not a "if" but a "when". I'll see about getting a data sample.

felixc commented 1 year ago

Thanks for the context! I merged this and the fix is included in the new 0.10 release that I just pushed out.

If you have a data sample handy, that would still be useful for adding a test for this case. Cheers!