From b3e8a5d117e7569c991e97b3d09e988fc0612f18 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Wed, 9 Sep 2026 14:16:57 -0400 Subject: [PATCH 01/15] feat(exif): read EXIF from any ReadAt byte source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gamut-ifd` shipped a streaming reader in P9 (#252) — `ReadAt`, `IfdReader`, `Rebased` — that fetches only the directory bodies and the values they reference. `gamut-exif` exposed none of it: its only entry point took a `&[u8]`, so pulling the few kilobytes of EXIF out of a 300 MB raw file meant loading the raw file. Move the parse onto `IfdReader` and add `ExifReader::parse_from` alongside `parse`, which is now the `&[u8]` case of it — a slice is a `ReadAt` source, so there is exactly one parse engine and the two entry points cannot drift. The marker is detected through the source and the TIFF stream is reached with `Rebased`, so every offset the crate reads or hands back stays in EXIF's own frame of reference. `parse` keeps its signature and its behaviour; the crate's existing tests for lenient/strict sub-IFD and thumbnail handling are unchanged and pass as they stood. The thumbnail range check moves from `usize` slicing to a 64-bit bound against the source length, which drops an overflow branch that only 32-bit targets could reach and stops a hostile `JPEGInterchangeFormatLength` being allocated before it is bounded. Deliberately synchronous: an async caller drives a `ReadAt` source itself, which keeps a runtime dependency out of a crate that has none. Refs #419 --- crates/gamut-exif/src/lib.rs | 1 + crates/gamut-exif/src/reader.rs | 156 +++----------- crates/gamut-exif/src/stream.rs | 291 +++++++++++++++++++++++++++ crates/gamut-exif/tests/streaming.rs | 128 ++++++++++++ 4 files changed, 445 insertions(+), 131 deletions(-) create mode 100644 crates/gamut-exif/src/stream.rs create mode 100644 crates/gamut-exif/tests/streaming.rs diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index cd9d9c5f..52d1dc6c 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -35,6 +35,7 @@ pub mod exif; pub mod gps; pub mod maker_note; pub mod reader; +pub mod stream; pub mod tag; pub mod thumbnail; pub mod value; diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index ce84211f..67abb90b 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -1,31 +1,17 @@ //! Reading an EXIF blob into the typed [`Exif`] model. //! //! An EXIF blob is an optional `Exif\0\0` marker followed by a TIFF stream. The 0th IFD and (when -//! present) the 1st IFD are the top-level chain [`gamut_ifd::read`] returns; the Exif, GPS, and -//! Interoperability directories hang off pointer *tags* that the generic reader cannot follow (it -//! cannot know which `LONG`s are offsets), so this reader chases those pointers explicitly and -//! removes them, representing each sub-IFD structurally on [`Exif`] instead. - -use gamut_ifd::{ByteOrder, Ifd, IfdReader, Variant, tags as ifd_tags}; +//! present) the 1st IFD are the top-level chain; the Exif, GPS, and Interoperability directories +//! hang off pointer *tags* that the generic TIFF reader cannot follow (it cannot know which +//! `LONG`s are offsets), so this reader chases those pointers explicitly and removes them, +//! representing each sub-IFD structurally on [`Exif`] instead. +//! +//! This module holds the reader's options and its `&[u8]` entry points. The parse itself is +//! generic over [`gamut_ifd::ReadAt`] and lives in [`crate::stream`]; a slice is simply one such +//! source, so there is exactly **one** parse engine and the two entry points cannot drift. -use crate::error::{ExifError, Result}; -use crate::exif::{EXIF_IFD_POINTER, Exif, GPS_IFD_POINTER, INTEROP_IFD_POINTER, MARKER}; -use crate::tag::ExifTag; -use crate::thumbnail::Thumbnail; - -/// The absolute offset of the Exif sub-IFD's out-of-line `MakerNote` value in `tiff`, or `None` -/// if the note is absent or inline. -fn maker_note_offset( - tiff: &[u8], - exif_ifd_at: u64, - order: ByteOrder, - variant: Variant, -) -> Option { - let mut reader = IfdReader::with_layout(tiff, order, variant); - let raw = reader.read_ifd(exif_ifd_at).ok()?; - let entry = raw.entry(ifd_tags::MAKER_NOTE)?; - reader.value_offset(entry) -} +use crate::error::Result; +use crate::exif::Exif; /// Reads an EXIF blob into an [`Exif`], with options for how the parse is bounded. /// @@ -33,8 +19,8 @@ fn maker_note_offset( /// lenient: a malformed Exif/GPS/Interop sub-IFD is dropped rather than failing the whole parse. #[derive(Debug, Clone, Default)] pub struct ExifReader { - require_marker: bool, - strict: bool, + pub(crate) require_marker: bool, + pub(crate) strict: bool, } impl ExifReader { @@ -65,121 +51,29 @@ impl ExifReader { /// Parses an EXIF blob into an [`Exif`]. /// + /// The `&[u8]` case of [`parse_from`](Self::parse_from) — a slice is a + /// [`ReadAt`](gamut_ifd::ReadAt) source — so the two share one parse engine. + /// /// # Errors /// - /// Returns [`ExifError::MissingMarker`] when the marker is required but absent, an - /// [`ExifError::Ifd`] when the TIFF stream is malformed, or (in [`strict`](Self::strict) mode) - /// [`ExifError::InvalidIfd`] when a sub-IFD pointer addresses a malformed directory. + /// Returns [`ExifError::MissingMarker`](crate::ExifError::MissingMarker) when the marker is + /// required but absent, an [`ExifError::Ifd`](crate::ExifError::Ifd) when the TIFF stream is + /// malformed, or (in [`strict`](Self::strict) mode) + /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) when a sub-IFD pointer addresses a + /// malformed directory. pub fn parse(&self, bytes: &[u8]) -> Result { - let tiff = match bytes.strip_prefix(MARKER) { - Some(rest) => rest, - None if self.require_marker => return Err(ExifError::MissingMarker), - None => bytes, - }; - - let file = gamut_ifd::read(tiff)?; - let order = file.order; - let variant = file.variant; - let mut ifds = file.ifds.into_iter(); - let mut image = ifds.next().ok_or(ExifError::Truncated)?; - // The next-IFD chain's second entry is the thumbnail directory (1st IFD), if any. - let thumbnail = match ifds.next() { - Some(ifd) => Some(self.read_thumbnail(ifd, tiff)?), - None => None, - }; - - // The Exif sub-IFD's own offset, captured before `follow` strips the pointer: the - // maker-note pin needs the note value's absolute source position. - let exif_ifd_at = image.get_u32(EXIF_IFD_POINTER).map(u64::from); - let exif = self.follow(&mut image, tiff, order, variant, EXIF_IFD_POINTER, "Exif")?; - let gps = self.follow(&mut image, tiff, order, variant, GPS_IFD_POINTER, "GPS")?; - let maker_note_at = match (&exif, exif_ifd_at) { - (Some(_), Some(at)) => maker_note_offset(tiff, at, order, variant), - _ => None, - }; - - // The Interoperability directory is reached from *inside* the Exif sub-IFD, not the 0th IFD. - let (exif, interop) = match exif { - Some(mut e) => { - let interop = - self.follow(&mut e, tiff, order, variant, INTEROP_IFD_POINTER, "Interop")?; - (Some(e), interop) - } - None => (None, None), - }; - - Ok(Exif::from_parts( - order, - image, - exif, - gps, - interop, - thumbnail, - maker_note_at, - )) - } - - /// Reads pointer tag `ptr` from `parent`, removes it (the pointer is represented structurally, - /// not as a data field), and parses the sub-IFD it addresses. - /// - /// Returns `Ok(None)` when the pointer is absent, or — in lenient mode — when the pointed-at - /// directory is malformed. - fn follow( - &self, - parent: &mut Ifd, - tiff: &[u8], - order: ByteOrder, - variant: Variant, - ptr: u16, - name: &'static str, - ) -> Result> { - let Some(offset) = parent.get_u32(ptr) else { - return Ok(None); - }; - parent.remove(ptr); - match gamut_ifd::read_ifd_at(tiff, u64::from(offset), order, variant) { - Ok(ifd) => Ok(Some(ifd)), - Err(_) if !self.strict => Ok(None), - Err(_) => Err(ExifError::InvalidIfd(name)), - } - } - - /// Builds a [`Thumbnail`] from the 1st IFD, slicing out its JPEG bytes (from the - /// `JPEGInterchangeFormat` offset / length) when present. In lenient mode an out-of-bounds - /// JPEG range yields a thumbnail without bytes; in strict mode it errors. - fn read_thumbnail(&self, ifd: Ifd, tiff: &[u8]) -> Result { - let offset = ifd.get_u32(ExifTag::JpegInterchangeFormat.tag_id()); - let length = ifd.get_u32(ExifTag::JpegInterchangeFormatLength.tag_id()); - let jpeg = match (offset, length) { - (Some(offset), Some(length)) => { - let range = (offset as usize).checked_add(length as usize); - match range.and_then(|end| tiff.get(offset as usize..end)) { - Some(bytes) => Some(bytes.to_vec()), - None if self.strict => { - return Err(ExifError::BadThumbnail("JPEG offset out of bounds")); - } - None => None, - } - } - _ => None, - }; - // The JPEGInterchangeFormat offset is structural — the bytes are captured above and the - // writer re-synthesises the offset — so drop it from the stored directory (mirroring how the - // sub-IFD pointer tags are stripped), leaving a value the model can't carry stale. - let mut ifd = ifd; - if jpeg.is_some() { - ifd.remove(ExifTag::JpegInterchangeFormat.tag_id()); - } - Ok(Thumbnail::from_parts(ifd, jpeg)) + self.parse_from(bytes) } } #[cfg(test)] mod tests { - use gamut_ifd::{TiffFile, Value, write}; + use gamut_ifd::{ByteOrder, Ifd, TiffFile, Value, Variant, write}; use super::*; - use crate::IfdKind; + use crate::error::ExifError; + use crate::exif::{EXIF_IFD_POINTER, GPS_IFD_POINTER, INTEROP_IFD_POINTER, MARKER}; + use crate::tag::{ExifTag, IfdKind}; /// Builds a small but structurally complete EXIF TIFF stream (0th IFD with Make/Orientation, /// an Exif sub-IFD with FNumber + a nested Interop sub-IFD, a GPS sub-IFD, and a thumbnail diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs new file mode 100644 index 00000000..a9780fe7 --- /dev/null +++ b/crates/gamut-exif/src/stream.rs @@ -0,0 +1,291 @@ +//! Reading EXIF from a positioned byte source rather than a slice. +//! +//! EXIF is usually small, but the file it is embedded in need not be: a raw `.NEF`/`.CR3` can be +//! hundreds of megabytes whose EXIF is a few kilobytes near the front. [`gamut_ifd`]'s +//! [`IfdReader`] already reads a TIFF stream through the positioned [`ReadAt`] trait, fetching only +//! the directory bodies and the values they reference, so this module lifts +//! [`ExifReader`](crate::ExifReader) onto the same source type: open a file, hand the reader a +//! [`gamut_ifd::StreamSource`], and pull the EXIF out without loading the image. +//! +//! This is the crate's **one** parse engine — [`ExifReader::parse`](crate::ExifReader::parse) is +//! the `&[u8]` case of it (`&[u8]` implements [`ReadAt`]), so the slice and streaming paths cannot +//! drift. It is deliberately synchronous: an async caller drives a [`ReadAt`] source itself, which +//! keeps a runtime dependency out of a crate that has none. + +use gamut_ifd::{Ifd, IfdReader, RawIfd, ReadAt, tags as ifd_tags}; + +use crate::error::{ExifError, Result}; +use crate::exif::{EXIF_IFD_POINTER, Exif, GPS_IFD_POINTER, INTEROP_IFD_POINTER, MARKER}; +use crate::reader::ExifReader; +use crate::tag::ExifTag; +use crate::thumbnail::Thumbnail; + +impl ExifReader { + /// Parses EXIF from a positioned byte source, reading only the parts it needs. + /// + /// The streaming twin of [`parse`](Self::parse), which is this method over a `&[u8]`. `source` + /// is taken by value; `&mut S` and `&mut dyn ReadAt` both implement [`ReadAt`], so a caller + /// that must keep its source can pass a borrow, and a caller that needs dynamic dispatch can + /// erase the type. + /// + /// ```no_run + /// use std::fs::File; + /// + /// use gamut_exif::ExifReader; + /// use gamut_ifd::StreamSource; + /// + /// // The EXIF of a large raw file, without reading the image. + /// let mut file = File::open("capture.dng")?; + /// let exif = ExifReader::new().parse_from(StreamSource::new(&mut file))?; + /// println!("{:?}", exif.make()); + /// # Ok::<(), Box>(()) + /// ``` + /// + /// # Errors + /// + /// Returns [`ExifError::MissingMarker`] when the marker is required but absent, an + /// [`ExifError::Ifd`] when the TIFF stream is malformed or the source fails, or (in + /// [`strict`](Self::strict) mode) [`ExifError::InvalidIfd`] / + /// [`ExifError::BadThumbnail`] when a sub-IFD pointer or thumbnail range is unusable. + pub fn parse_from(&self, mut source: S) -> Result { + let base = self.tiff_base(&mut source)?; + // Everything below addresses the TIFF stream, so offsets read out of it — and the offsets + // this crate hands back — stay in EXIF's own frame of reference. + let mut reader = IfdReader::open(source.rebased(base))?; + let order = reader.order(); + + let file = reader.read_file()?; + let mut ifds = file.ifds.into_iter(); + let mut image = ifds.next().ok_or(ExifError::Truncated)?; + // The next-IFD chain's second entry is the thumbnail directory (1st IFD), if any. + let thumbnail = match ifds.next() { + Some(ifd) => Some(self.read_thumbnail(ifd, &mut reader)?), + None => None, + }; + + // The Exif sub-IFD's own offset, captured before `follow` strips the pointer: the + // maker-note pin needs the note value's absolute source position. + let exif_ifd_at = image.get_u32(EXIF_IFD_POINTER).map(u64::from); + let exif = self.follow(&mut image, &mut reader, EXIF_IFD_POINTER, "Exif")?; + let gps = self.follow(&mut image, &mut reader, GPS_IFD_POINTER, "GPS")?; + let maker_note_at = match (&exif, exif_ifd_at) { + (Some(_), Some(at)) => maker_note_offset(&mut reader, at), + _ => None, + }; + + // The Interoperability directory is reached from *inside* the Exif sub-IFD, not the 0th IFD. + let (exif, interop) = match exif { + Some(mut e) => { + let interop = self.follow(&mut e, &mut reader, INTEROP_IFD_POINTER, "Interop")?; + (Some(e), interop) + } + None => (None, None), + }; + + Ok(Exif::from_parts( + order, + image, + exif, + gps, + interop, + thumbnail, + maker_note_at, + )) + } + + /// The offset at which the TIFF stream starts in `source`: past the `Exif\0\0` marker when it + /// is there, 0 when it is not. + /// + /// A source too short to hold the marker is treated as unmarked — the TIFF header that follows + /// is longer than the marker, so such a source cannot parse either way. + fn tiff_base(&self, source: &mut S) -> Result { + let mut head = [0u8; MARKER.len()]; + let marked = source.read_exact_at(0, &mut head).is_ok() && head.as_slice() == MARKER; + if marked { + Ok(MARKER.len() as u64) + } else if self.require_marker { + Err(ExifError::MissingMarker) + } else { + Ok(0) + } + } + + /// Reads pointer tag `ptr` from `parent`, removes it (the pointer is represented structurally, + /// not as a data field), and parses the sub-IFD it addresses. + /// + /// Returns `Ok(None)` when the pointer is absent, or — in lenient mode — when the pointed-at + /// directory is malformed. + fn follow( + &self, + parent: &mut Ifd, + reader: &mut IfdReader, + ptr: u16, + name: &'static str, + ) -> Result> { + let Some(offset) = parent.get_u32(ptr) else { + return Ok(None); + }; + parent.remove(ptr); + let followed = match reader.read_ifd(u64::from(offset)) { + Ok(raw) => reader.decode_ifd(&raw), + Err(e) => Err(e), + }; + match followed { + Ok(ifd) => Ok(Some(ifd)), + Err(_) if !self.strict => Ok(None), + Err(_) => Err(ExifError::InvalidIfd(name)), + } + } + + /// Builds a [`Thumbnail`] from the 1st IFD, fetching its JPEG bytes (from the + /// `JPEGInterchangeFormat` offset / length) when the range is wholly inside the stream. In + /// lenient mode an out-of-bounds range yields a thumbnail without bytes; in strict mode it + /// errors. + fn read_thumbnail(&self, ifd: Ifd, reader: &mut IfdReader) -> Result { + let ptr = ExifTag::JpegInterchangeFormat.tag_id(); + let offset = ifd.get_u32(ptr); + let length = ifd.get_u32(ExifTag::JpegInterchangeFormatLength.tag_id()); + let jpeg = match (offset, length) { + (Some(offset), Some(length)) => match read_range(reader, offset, length)? { + Some(bytes) => Some(bytes), + None if self.strict => { + return Err(ExifError::BadThumbnail("JPEG offset out of bounds")); + } + None => None, + }, + _ => None, + }; + // The JPEGInterchangeFormat offset is structural — the bytes are captured above and the + // writer re-synthesises the offset — so drop it from the stored directory (mirroring how the + // sub-IFD pointer tags are stripped), leaving a value the model can't carry stale. + let mut ifd = ifd; + if jpeg.is_some() { + ifd.remove(ptr); + } + Ok(Thumbnail::from_parts(ifd, jpeg)) + } +} + +/// Fetches `length` bytes at `offset`, or `None` when that range is not wholly inside the stream. +/// +/// The bound is checked against the source's length *before* anything is allocated, so a hostile +/// `JPEGInterchangeFormatLength` cannot make the reader reserve more than the stream can hold. +fn read_range( + reader: &mut IfdReader, + offset: u32, + length: u32, +) -> Result>> { + // Widened to 64 bits first: the sum of two `u32`s cannot overflow a `u64`. + let end = u64::from(offset) + u64::from(length); + if end > reader.source_mut().len()? { + return Ok(None); + } + let mut buf = vec![0u8; length as usize]; + reader + .source_mut() + .read_exact_at(u64::from(offset), &mut buf)?; + Ok(Some(buf)) +} + +/// The absolute offset of the Exif sub-IFD's out-of-line `MakerNote` value in the TIFF stream, or +/// `None` if the note is absent or inline. +fn maker_note_offset(reader: &mut IfdReader, exif_ifd_at: u64) -> Option { + let raw: RawIfd = reader.read_ifd(exif_ifd_at).ok()?; + let entry = raw.entry(ifd_tags::MAKER_NOTE)?; + reader.value_offset(entry) +} + +#[cfg(test)] +mod tests { + use std::io::Cursor; + + use gamut_ifd::{ByteOrder, StreamSource, TiffFile, Value, Variant, write}; + + use super::*; + + /// A minimal marked EXIF blob whose 0th IFD carries `Make`. + fn blob() -> Vec { + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); + let tiff = write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds: vec![image], + }) + .expect("write"); + let mut out = MARKER.to_vec(); + out.extend(tiff); + out + } + + /// The streaming entry point reads a `Read + Seek` source that is not a slice at all — the + /// capability the slice-only API could not offer. + #[test] + fn parse_from_reads_a_seekable_stream() { + let source = StreamSource::new(Cursor::new(blob())); + let exif = ExifReader::new().parse_from(source).expect("parse_from"); + assert_eq!(exif.make(), Some("Canon")); + } + + /// The marker is detected through the source, not by slicing: the same `require_marker` + /// contract `parse` has must hold for a stream, or the two entry points disagree. + #[test] + fn the_marker_is_detected_through_the_source() { + let marked = blob(); + let bare = marked[MARKER.len()..].to_vec(); + + assert!( + ExifReader::new() + .require_marker(true) + .parse_from(StreamSource::new(Cursor::new(marked))) + .is_ok() + ); + let err = ExifReader::new() + .require_marker(true) + .parse_from(StreamSource::new(Cursor::new(bare.clone()))) + .expect_err("a bare TIFF stream must be rejected"); + assert!(matches!(err, ExifError::MissingMarker), "{err:?}"); + // ...and without the requirement the same bare stream parses. + assert!( + ExifReader::new() + .parse_from(StreamSource::new(Cursor::new(bare))) + .is_ok() + ); + } + + /// A source shorter than the 6-byte marker cannot be read for one; it is unmarked, and + /// `require_marker` says so rather than reporting a torn read. + #[test] + fn a_source_too_short_for_the_marker_is_unmarked() { + let err = ExifReader::new() + .require_marker(true) + .parse_from(&b"Exi"[..]) + .expect_err("three bytes cannot carry the marker"); + assert!(matches!(err, ExifError::MissingMarker), "{err:?}"); + } + + /// A range is fetched only when it ends inside the stream — the bound that stops a hostile + /// length from being allocated. The boundary case (a range ending exactly at the end) is the + /// one an off-by-one would get wrong. + #[test] + fn a_range_is_fetched_only_when_it_ends_inside_the_stream() { + let data = [1u8, 2, 3, 4, 5, 6, 7, 8]; + let mut reader = + IfdReader::with_layout(&data[..], ByteOrder::LittleEndian, Variant::Classic); + assert_eq!( + read_range(&mut reader, 4, 4).expect("read"), + Some(vec![5, 6, 7, 8]), + "a range ending exactly at the end is inside" + ); + assert_eq!( + read_range(&mut reader, 4, 5).expect("read"), + None, + "one byte past the end is not" + ); + assert_eq!( + read_range(&mut reader, 0, u32::MAX).expect("read"), + None, + "a hostile length is refused before it is allocated" + ); + } +} diff --git a/crates/gamut-exif/tests/streaming.rs b/crates/gamut-exif/tests/streaming.rs new file mode 100644 index 00000000..88a19234 --- /dev/null +++ b/crates/gamut-exif/tests/streaming.rs @@ -0,0 +1,128 @@ +//! The laziness contract for [`ExifReader::parse_from`]: pulling EXIF out of a large file reads +//! only the metadata, never the image payload. +//! +//! This is the whole point of the streaming entry point — a raw `.NEF`/`.CR3` is hundreds of +//! megabytes whose EXIF is a few kilobytes near the front — and it is not visible from a test that +//! only checks the parsed values, because a reader that slurped the file whole would return exactly +//! the same [`Exif`](gamut_exif::Exif). It is pinned the way `gamut-ifd`'s own `tests/streaming.rs` +//! pins its "≤64 read bytes" contract: a counting [`ReadAt`] wrapper and a bound that a +//! payload-touching read would blow past by three orders of magnitude. + +use gamut_core::Result; +use gamut_exif::ExifReader; +use gamut_ifd::{ByteOrder, Ifd, ReadAt, TiffFile, Value, Variant, write}; + +/// The `Exif\0\0` marker that precedes the TIFF stream in a JPEG `APP1` payload. +const MARKER: &[u8] = b"Exif\x00\x00"; +/// `ExifIFD` pointer (Exif 3.0 §4.6.3). +const EXIF_IFD: u16 = 0x8769; +/// `GPSInfo` pointer. +const GPS_INFO: u16 = 0x8825; +/// `Interoperability` pointer. +const INTEROP_IFD: u16 = 0xA005; + +/// Four megabytes: the file the metadata is embedded in. +const FILE_LEN: usize = 4 * 1024 * 1024; +/// Where the (nonexistent) strip data claims to start — well inside the padding. +const STRIP_AT: u32 = 1024 * 1024; + +/// A [`ReadAt`] wrapper that counts the bytes fetched, pinning the laziness contract. +struct Counting { + inner: S, + bytes_read: u64, +} + +impl ReadAt for Counting { + fn read_exact_at(&mut self, offset: u64, buf: &mut [u8]) -> Result<()> { + self.bytes_read += buf.len() as u64; + self.inner.read_exact_at(offset, buf) + } + + fn len(&mut self) -> Result { + self.inner.len() + } +} + +/// A marked EXIF blob — 0th IFD, Exif and GPS sub-IFDs, a nested Interop sub-IFD and a thumbnail +/// directory — followed by four megabytes of image payload the strip tags point into. +fn large_file_with_exif() -> Vec { + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); // Make + image.set(0x0110, Value::Ascii("EOS R5".into())); // Model + image.set(0x0111, Value::Long(vec![STRIP_AT])); // StripOffsets, into the payload + image.set(0x0117, Value::Long(vec![STRIP_AT])); // StripByteCounts + + let mut interop = Ifd::new(); + interop.set(0x0001, Value::Ascii("R98".into())); // InteroperabilityIndex + + let mut exif = Ifd::new(); + exif.set(0x829D, Value::Rational(vec![(28, 10)])); // FNumber + exif.set(0x8827, Value::Short(vec![400])); // PhotographicSensitivity + exif.set_sub_ifd(INTEROP_IFD, vec![interop]); + + let mut gps = Ifd::new(); + gps.set(0x0000, Value::Byte(vec![2, 3, 0, 0])); // GPSVersionID + + image.set_sub_ifd(EXIF_IFD, vec![exif]); + image.set_sub_ifd(GPS_INFO, vec![gps]); + + let mut thumb = Ifd::new(); + thumb.set(0x0103, Value::Short(vec![6])); // Compression = JPEG + + let tiff = write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds: vec![image, thumb], + }) + .expect("write"); + + let mut data = MARKER.to_vec(); + data.extend(tiff); + assert!( + data.len() < STRIP_AT as usize, + "the metadata must precede the payload" + ); + data.resize(FILE_LEN, 0xAB); + data +} + +/// Extracting the EXIF of a four-megabyte file reads only the marker, the header, the directory +/// bodies and the values they reference — a bounded number of bytes that does not grow with the +/// file. +/// +/// The source is passed as `&mut Counting<_>` rather than by value, which is also the contract that +/// `&mut S` is itself a [`ReadAt`] source: without it the counter would be moved into the reader +/// and unreadable afterwards. +#[test] +fn extracting_exif_from_a_large_file_never_reads_the_payload() { + let data = large_file_with_exif(); + let mut counting = Counting { + inner: &data[..], + bytes_read: 0, + }; + + let exif = ExifReader::new() + .parse_from(&mut counting) + .expect("parse_from"); + + // The whole metadata tree really was reached — a reader that gave up early would also be + // "lazy", and this is what separates the two. + assert_eq!(exif.make(), Some("Canon")); + assert_eq!(exif.model(), Some("EOS R5")); + assert_eq!(exif.iso(), Some(400)); + assert!(exif.gps_ifd().is_some(), "the GPS sub-IFD was followed"); + assert!( + exif.interop_ifd().is_some(), + "the Interop sub-IFD was followed" + ); + assert!(exif.thumbnail().is_some(), "the 1st IFD was read"); + + // 251 bytes today: the marker, the header, five directory bodies and their out-of-line + // values. 512 leaves room for a tag or two without letting a megabyte through — four + // megabytes is the failure mode a slurping reader would show. + assert!( + counting.bytes_read <= 512, + "streaming parse read {} bytes of a {FILE_LEN}-byte file", + counting.bytes_read + ); +} From c49f83bcd84db33ff8dc09e37c2352ab5218147f Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Wed, 9 Sep 2026 14:19:56 -0400 Subject: [PATCH 02/15] feat(exif): report what a lenient parse discarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default reader drops a malformed Exif/GPS/Interop sub-IFD or an out-of-bounds thumbnail range so the rest of a real-world blob still parses, but it was silent about it: a blob that never carried GPS and one whose GPS pointer was dangling produced the same `Exif`. Worse, `follow` removed the pointer tag *before* attempting the parse, so the evidence of what had been there was gone by the time it failed. Add `ExifReader::parse_with_report` and `parse_from_with_report`, returning a `ReadReport` alongside the `Exif`. Each discarded region is named by a `DroppedRegion`, the tag that addressed it, the offset that tag carried, and a `DropReason` separating an address outside the blob from bytes inside it that were not a directory. Both enums are fieldless with an explicit `repr` and append-only discriminants, and `Dropped` is `Copy` plain data behind accessors, so the report crosses an FFI boundary unchanged. The pointer is now removed after the read is attempted rather than before. The removal itself is unchanged, so `parse` and `parse_from` return exactly what they did; they simply discard the report. In strict mode the first malformed region still fails the parse, so a strict report is always empty. Closes the reader-validation item `STATUS.md` had deferred. Two finer capabilities stay deferred and are recorded there: per-tag recovery inside one directory (a `gamut-ifd` concern — one bad entry fails its whole IFD) and a byte-completeness verdict over the blob. Refs #419 --- crates/gamut-exif/README.md | 29 +++- crates/gamut-exif/STATUS.md | 15 +- crates/gamut-exif/src/lib.rs | 9 ++ crates/gamut-exif/src/reader.rs | 27 ++++ crates/gamut-exif/src/report.rs | 252 ++++++++++++++++++++++++++++++ crates/gamut-exif/src/stream.rs | 114 +++++++++++--- crates/gamut-exif/tests/report.rs | 224 ++++++++++++++++++++++++++ 7 files changed, 648 insertions(+), 22 deletions(-) create mode 100644 crates/gamut-exif/src/report.rs create mode 100644 crates/gamut-exif/tests/report.rs diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index 7680d163..0f828ce1 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -44,7 +44,29 @@ let out = edited.to_bytes(); // Exif\0\0 + TIFF, ready to re-embe ``` For a bare TIFF stream (PNG `eXIf` / WebP `EXIF`) or a byte-order override, use [`ExifWriter`]; -[`ExifReader`] carries the read-side options (`require_marker`, `strict`). +[`ExifReader`] carries the read-side options (`require_marker`, `strict`) and two further entry +points: + +- **`parse_from`** reads through [`gamut_ifd::ReadAt`] instead of a slice, so the EXIF of a + 300 MB raw file costs a few hundred bytes of I/O rather than the whole file. `parse` is the + `&[u8]` case of it — one parse engine, two entry points. It is deliberately synchronous: an + async caller drives the source itself, which keeps a runtime dependency out of the crate. +- **`parse_with_report`** (and its `parse_from_with_report` twin) returns a `ReadReport` alongside + the `Exif`, naming every sub-IFD and thumbnail range the lenient reader discarded — the tag that + addressed it, the offset it carried, and whether the address was out of bounds or the bytes + there were corrupt. `parse` stays silent, as before. + +```rust +# use gamut_exif::ExifReader; +# fn demo(bytes: &[u8]) -> Result<(), gamut_exif::ExifError> { +let (exif, report) = ExifReader::new().parse_with_report(bytes)?; +for dropped in report.dropped() { + eprintln!("{dropped}"); // e.g. "dropped GPS at tag 0x8825, offset 65535: ..." +} +# let _ = exif; +# Ok(()) +# } +``` Enable the optional `geocoordinates` feature (also included by `full`) to convert a complete [`GpsInfo`] with `TryFrom` into `geocoordinates::Wgs84` or `geocoordinates::Coordinate`. The latter @@ -67,6 +89,11 @@ designed to be added without breaking the 1.0 API — the catalogue and vendor e - **exiftool-parity tag breadth** beyond the standard dictionary (unknown tags still round-trip losslessly via the raw `Ifd`). - **Uncompressed strip-based thumbnails** are read but not re-embedded (JPEG thumbnails are). +- **Per-tag error recovery inside one directory.** A single unparseable entry fails its whole + directory in `gamut-ifd`, so the report's granularity is the sub-IFD, not the individual tag. +- **A byte-completeness verdict** over the whole blob (which source bytes no parsed structure + claims). `gamut-ifd`'s audit engine has the machinery; `ReadReport` today reports only what was + dropped, not what was never reached. ## Status diff --git a/crates/gamut-exif/STATUS.md b/crates/gamut-exif/STATUS.md index 3a355f5f..b7e352b5 100644 --- a/crates/gamut-exif/STATUS.md +++ b/crates/gamut-exif/STATUS.md @@ -24,6 +24,7 @@ fixtures** (`tests/fixtures/`, regenerate with `GAMUT_REGEN_GOLDEN=1`). | P6 | §4.6 | **Keystone** — writer round-trip (endianness/pointers/thumbnail preserved) | ✅ | | P7 | §4.6 | MakerNote: opaque passthrough + vendor detection (no per-vendor decode) | ✅ | | P8 | — | exiv2 differential gate + golden fixtures | ✅ | +| P9 | §4.6 | `ReadAt` streaming entry point + lenient-drop report (`ReadReport`) | ✅ | ## Intentionally deferred (additive under the `#[non_exhaustive]` surface) @@ -35,5 +36,15 @@ fixtures** (`tests/fixtures/`, regenerate with `GAMUT_REGEN_GOLDEN=1`). round-trip losslessly because the raw `gamut_ifd::Ifd` is retained. - **Uncompressed strip-based thumbnails** are read (as their directory) but not re-embedded; JPEG thumbnails round-trip fully. -- **A reader coverage/validation report** — `gamut-ifd` has the machinery; exposing a toggle on - `ExifReader` is a non-breaking future addition. +- **Per-tag error recovery inside a directory.** `ExifReader::parse_with_report` names every + sub-IFD and thumbnail range the lenient reader discards (issue #419), but the granularity is the + directory: a single unparseable entry fails its whole IFD in `gamut-ifd`, which is the layer that + would have to recover per entry. nom-exif's `entry.into_result()` is finer-grained here. +- **A byte-completeness verdict.** `ReadReport` says what was *dropped*, not which source bytes no + parsed structure claims. `gamut-ifd`'s audit engine (`Tracked`, `SegmentMap`, `read_audited`) is + the machinery for it and is already used by `gamut-dng` and `gamut-tiff`; wiring it behind a + toggle on `ExifReader` stays a non-breaking future addition. +- **An async entry point.** Declined rather than deferred: `parse_from` is synchronous over + `gamut_ifd::ReadAt`, and an async caller drives that source itself. A `tokio` feature would put a + runtime dependency in a crate that has none and constrain the public shape against the + C-portability convention, for a capability the caller can supply. diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index 52d1dc6c..9f64f833 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -15,6 +15,13 @@ //! [`Exif::parse`] reads a blob and [`Exif::to_bytes`] re-serialises it (preserving the byte order); //! read tags with the typed accessors or the [`ExifTag`] catalogue. //! +//! Two further read entry points sit on [`ExifReader`]: +//! [`parse_from`](ExifReader::parse_from) reads through [`gamut_ifd::ReadAt`] rather than a slice, +//! so EXIF can be pulled out of a large raw file without loading it, and +//! [`parse_with_report`](ExifReader::parse_with_report) returns a [`ReadReport`] naming every +//! sub-IFD and thumbnail range the lenient reader discarded. `parse` is the `&[u8]` case of +//! `parse_from` and stays silent, so neither is a change for existing callers. +//! //! ``` //! use gamut_exif::{ByteOrder, Exif, ExifTag, Value}; //! @@ -35,6 +42,7 @@ pub mod exif; pub mod gps; pub mod maker_note; pub mod reader; +pub mod report; pub mod stream; pub mod tag; pub mod thumbnail; @@ -51,6 +59,7 @@ pub use gps::GpsConversionError; pub use gps::{GpsAltitude, GpsCoordinate, GpsInfo, GpsReference}; pub use maker_note::{MakerNote, MakerNoteVendor}; pub use reader::ExifReader; +pub use report::{DropReason, Dropped, DroppedRegion, ReadReport}; pub use tag::{ExifTag, IfdKind}; pub use thumbnail::Thumbnail; pub use value::{Rational, SRational, as_text}; diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 67abb90b..8f15964a 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -12,6 +12,7 @@ use crate::error::Result; use crate::exif::Exif; +use crate::report::ReadReport; /// Reads an EXIF blob into an [`Exif`], with options for how the parse is bounded. /// @@ -64,6 +65,32 @@ impl ExifReader { pub fn parse(&self, bytes: &[u8]) -> Result { self.parse_from(bytes) } + + /// Parses an EXIF blob and reports what a lenient parse discarded. + /// + /// [`parse`](Self::parse) is silent about the sub-IFDs and thumbnail bytes leniency drops; this + /// returns the same [`Exif`] alongside a [`ReadReport`] naming each one, so a caller can tell a + /// blob that never carried GPS from one whose GPS pointer was dangling. + /// + /// ``` + /// # use gamut_exif::{ByteOrder, Exif, ExifReader}; + /// # let bytes = Exif::new(ByteOrder::LittleEndian).to_bytes()?; + /// let (exif, report) = ExifReader::new().parse_with_report(&bytes)?; + /// assert!(report.is_empty()); // nothing was lost + /// for dropped in report.dropped() { + /// eprintln!("{dropped}"); + /// } + /// # let _ = exif; + /// # Ok::<(), gamut_exif::ExifError>(()) + /// ``` + /// + /// # Errors + /// + /// As [`parse`](Self::parse). In [`strict`](Self::strict) mode the first malformed region fails + /// the parse instead of being reported, so a strict report is always empty. + pub fn parse_with_report(&self, bytes: &[u8]) -> Result<(Exif, ReadReport)> { + self.parse_from_with_report(bytes) + } } #[cfg(test)] diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs new file mode 100644 index 00000000..351697f3 --- /dev/null +++ b/crates/gamut-exif/src/report.rs @@ -0,0 +1,252 @@ +//! What a lenient parse discarded. +//! +//! [`ExifReader`](crate::ExifReader) is lenient by default: a malformed Exif/GPS/Interop sub-IFD or +//! an out-of-bounds thumbnail range is dropped so the rest of the blob still parses. That is the +//! right default for real-world files, but on its own it is *silent* — a caller cannot tell a blob +//! that never carried GPS from one whose GPS pointer was dangling. +//! +//! A [`ReadReport`], returned by +//! [`ExifReader::parse_with_report`](crate::ExifReader::parse_with_report), names each discarded +//! region: **where** it was (a [`DroppedRegion`] and the tag that addressed it), **what offset** +//! addressed it, and **why** it went ([`DropReason`]). A well-formed blob reports nothing, so +//! `report.is_empty()` is the "this parse lost nothing" verdict. +//! +//! The granularity is the directory, not the individual tag: a single unparseable entry fails its +//! whole directory in the underlying TIFF/IFD reader, so the sub-IFD it sat in is what gets named. + +use core::fmt; + +use crate::exif::{EXIF_IFD_POINTER, GPS_IFD_POINTER, INTEROP_IFD_POINTER}; +use crate::tag::ExifTag; + +/// A region of an EXIF blob that a lenient parse can discard. +/// +/// Fieldless with an explicit `repr` and append-only discriminants, so the value crosses an FFI +/// boundary as a plain integer. `#[non_exhaustive]`: regions can be added post-1.0 without a +/// breaking change. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[repr(u8)] +#[non_exhaustive] +pub enum DroppedRegion { + /// The Exif sub-IFD, addressed by the `ExifIFD` pointer (`0x8769`) in the 0th IFD. + ExifIfd = 0, + /// The GPS sub-IFD, addressed by the `GPSInfo` pointer (`0x8825`) in the 0th IFD. + GpsIfd = 1, + /// The Interoperability sub-IFD, addressed by the `Interoperability` pointer (`0xA005`) + /// *inside* the Exif sub-IFD. + InteropIfd = 2, + /// The 1st IFD's embedded JPEG thumbnail bytes, addressed by `JPEGInterchangeFormat` + /// (`0x0201`) and sized by `JPEGInterchangeFormatLength` (`0x0202`). The thumbnail's own + /// directory survives; only its bytes are lost. + ThumbnailJpeg = 3, +} + +impl DroppedRegion { + /// The region's short name — the same spelling + /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) uses in strict mode. + #[must_use] + pub const fn name(self) -> &'static str { + match self { + Self::ExifIfd => "Exif", + Self::GpsIfd => "GPS", + Self::InteropIfd => "Interop", + Self::ThumbnailJpeg => "Thumbnail", + } + } + + /// The tag whose value addressed this region. + pub(crate) const fn tag(self) -> u16 { + match self { + Self::ExifIfd => EXIF_IFD_POINTER, + Self::GpsIfd => GPS_IFD_POINTER, + Self::InteropIfd => INTEROP_IFD_POINTER, + Self::ThumbnailJpeg => ExifTag::JpegInterchangeFormat.tag_id(), + } + } +} + +/// Why a region was discarded. +/// +/// Fieldless with an explicit `repr` and append-only discriminants; `#[non_exhaustive]` so reasons +/// can be distinguished more finely post-1.0 without a breaking change. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[repr(u8)] +#[non_exhaustive] +pub enum DropReason { + /// The address itself lay outside the blob — a pointer at or past the end of the TIFF stream, + /// or a byte range that is not wholly inside it. Nothing could have been read there. + OutOfBounds = 0, + /// The address was inside the blob, but the structure at it did not parse: a bad entry count, + /// an unreadable entry, or a value offset the directory could not resolve. + Malformed = 1, +} + +impl DropReason { + /// The clause [`Dropped`]'s `Display` uses for this reason. + const fn clause(self) -> &'static str { + match self { + Self::OutOfBounds => "addresses bytes outside the EXIF blob", + Self::Malformed => "is not a well-formed directory", + } + } +} + +/// One region a lenient parse discarded, named by where it was and why it went. +/// +/// `Copy` plain data reachable through accessors, so it is representable across an FFI boundary. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct Dropped { + region: DroppedRegion, + tag: u16, + offset: u64, + reason: DropReason, +} + +impl Dropped { + /// Records a drop of `region`, addressed by its tag at `offset`, for `reason`. + pub(crate) const fn new(region: DroppedRegion, offset: u64, reason: DropReason) -> Self { + Self { + region, + tag: region.tag(), + offset, + reason, + } + } + + /// Which region was discarded. + #[must_use] + pub const fn region(self) -> DroppedRegion { + self.region + } + + /// The tag whose value addressed the discarded region — the pointer tag for a sub-IFD, + /// `JPEGInterchangeFormat` for the thumbnail bytes. + #[must_use] + pub const fn tag(self) -> u16 { + self.tag + } + + /// The offset that tag carried, relative to the start of the TIFF stream (i.e. *after* any + /// `Exif\0\0` marker) — the same frame of reference EXIF's own offsets use. + #[must_use] + pub const fn offset(self) -> u64 { + self.offset + } + + /// Why the region was discarded. + #[must_use] + pub const fn reason(self) -> DropReason { + self.reason + } +} + +impl fmt::Display for Dropped { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "dropped {} at tag {:#06x}, offset {}: {}", + self.region.name(), + self.tag, + self.offset, + self.reason.clause() + ) + } +} + +/// What a lenient parse discarded — empty when the blob was carried across in full. +/// +/// Obtained from [`ExifReader::parse_with_report`](crate::ExifReader::parse_with_report) or +/// [`ExifReader::parse_from_with_report`](crate::ExifReader::parse_from_with_report). In +/// [`strict`](crate::ExifReader::strict) mode the first malformed region fails the parse instead, +/// so a report from a strict reader is always empty. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct ReadReport { + dropped: Vec, +} + +impl ReadReport { + /// An empty report — nothing discarded. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Every discarded region, in the order the reader met it. + #[must_use] + pub fn dropped(&self) -> &[Dropped] { + &self.dropped + } + + /// Whether the parse lost nothing. + #[must_use] + pub fn is_empty(&self) -> bool { + self.dropped.is_empty() + } + + /// Appends a discarded region. + pub(crate) fn record(&mut self, dropped: Dropped) { + self.dropped.push(dropped); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Each region reports the tag that actually addresses it — the value a caller uses to find + /// the pointer back in the source directory. + #[test] + fn each_region_carries_the_tag_that_addresses_it() { + for (region, tag) in [ + (DroppedRegion::ExifIfd, 0x8769), + (DroppedRegion::GpsIfd, 0x8825), + (DroppedRegion::InteropIfd, 0xA005), + (DroppedRegion::ThumbnailJpeg, 0x0201), + ] { + assert_eq!( + Dropped::new(region, 0, DropReason::OutOfBounds).tag(), + tag, + "wrong addressing tag for {region:?}" + ); + } + } + + /// The rendered form names the region, the tag, the offset and the reason — the four facts a + /// caller reporting a lossy parse needs, and the strings are part of the contract. + #[test] + fn the_rendered_drop_names_region_tag_offset_and_reason() { + assert_eq!( + Dropped::new(DroppedRegion::GpsIfd, 65_535, DropReason::OutOfBounds).to_string(), + "dropped GPS at tag 0x8825, offset 65535: addresses bytes outside the EXIF blob" + ); + assert_eq!( + Dropped::new(DroppedRegion::ExifIfd, 26, DropReason::Malformed).to_string(), + "dropped Exif at tag 0x8769, offset 26: is not a well-formed directory" + ); + assert_eq!( + Dropped::new(DroppedRegion::InteropIfd, 8, DropReason::Malformed).to_string(), + "dropped Interop at tag 0xa005, offset 8: is not a well-formed directory" + ); + assert_eq!( + Dropped::new(DroppedRegion::ThumbnailJpeg, 1, DropReason::OutOfBounds).to_string(), + "dropped Thumbnail at tag 0x0201, offset 1: addresses bytes outside the EXIF blob" + ); + } + + /// A fresh report is empty and stays consistent with what has been recorded — `is_empty` is + /// the "this parse lost nothing" verdict, so it must not be independent of the contents. + #[test] + fn a_report_is_empty_until_something_is_recorded() { + let mut report = ReadReport::new(); + assert!(report.is_empty()); + assert_eq!(report.dropped(), &[]); + + let drop = Dropped::new(DroppedRegion::ExifIfd, 7, DropReason::Malformed); + report.record(drop); + assert!(!report.is_empty()); + assert_eq!(report.dropped(), &[drop]); + assert_eq!(report.dropped()[0].region(), DroppedRegion::ExifIfd); + assert_eq!(report.dropped()[0].offset(), 7); + assert_eq!(report.dropped()[0].reason(), DropReason::Malformed); + } +} diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index a9780fe7..60dac822 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -15,8 +15,9 @@ use gamut_ifd::{Ifd, IfdReader, RawIfd, ReadAt, tags as ifd_tags}; use crate::error::{ExifError, Result}; -use crate::exif::{EXIF_IFD_POINTER, Exif, GPS_IFD_POINTER, INTEROP_IFD_POINTER, MARKER}; +use crate::exif::{EXIF_IFD_POINTER, Exif, MARKER}; use crate::reader::ExifReader; +use crate::report::{DropReason, Dropped, DroppedRegion, ReadReport}; use crate::tag::ExifTag; use crate::thumbnail::Thumbnail; @@ -47,7 +48,30 @@ impl ExifReader { /// [`ExifError::Ifd`] when the TIFF stream is malformed or the source fails, or (in /// [`strict`](Self::strict) mode) [`ExifError::InvalidIfd`] / /// [`ExifError::BadThumbnail`] when a sub-IFD pointer or thumbnail range is unusable. - pub fn parse_from(&self, mut source: S) -> Result { + pub fn parse_from(&self, source: S) -> Result { + self.parse_source(source, &mut ReadReport::new()) + } + + /// Parses EXIF from a positioned byte source and reports what a lenient parse discarded. + /// + /// The streaming twin of [`parse_with_report`](Self::parse_with_report). See [`ReadReport`]. + /// + /// # Errors + /// + /// As [`parse_from`](Self::parse_from). + pub fn parse_from_with_report(&self, source: S) -> Result<(Exif, ReadReport)> { + let mut report = ReadReport::new(); + let exif = self.parse_source(source, &mut report)?; + Ok((exif, report)) + } + + /// The crate's one parse engine: marker handling, the top-level chain, the thumbnail, and the + /// three pointer-addressed sub-IFDs, recording into `report` whatever leniency discards. + pub(crate) fn parse_source( + &self, + mut source: S, + report: &mut ReadReport, + ) -> Result { let base = self.tiff_base(&mut source)?; // Everything below addresses the TIFF stream, so offsets read out of it — and the offsets // this crate hands back — stay in EXIF's own frame of reference. @@ -59,15 +83,15 @@ impl ExifReader { let mut image = ifds.next().ok_or(ExifError::Truncated)?; // The next-IFD chain's second entry is the thumbnail directory (1st IFD), if any. let thumbnail = match ifds.next() { - Some(ifd) => Some(self.read_thumbnail(ifd, &mut reader)?), + Some(ifd) => Some(self.read_thumbnail(ifd, &mut reader, report)?), None => None, }; // The Exif sub-IFD's own offset, captured before `follow` strips the pointer: the // maker-note pin needs the note value's absolute source position. let exif_ifd_at = image.get_u32(EXIF_IFD_POINTER).map(u64::from); - let exif = self.follow(&mut image, &mut reader, EXIF_IFD_POINTER, "Exif")?; - let gps = self.follow(&mut image, &mut reader, GPS_IFD_POINTER, "GPS")?; + let exif = self.follow(&mut image, &mut reader, DroppedRegion::ExifIfd, report)?; + let gps = self.follow(&mut image, &mut reader, DroppedRegion::GpsIfd, report)?; let maker_note_at = match (&exif, exif_ifd_at) { (Some(_), Some(at)) => maker_note_offset(&mut reader, at), _ => None, @@ -76,7 +100,8 @@ impl ExifReader { // The Interoperability directory is reached from *inside* the Exif sub-IFD, not the 0th IFD. let (exif, interop) = match exif { Some(mut e) => { - let interop = self.follow(&mut e, &mut reader, INTEROP_IFD_POINTER, "Interop")?; + let interop = + self.follow(&mut e, &mut reader, DroppedRegion::InteropIfd, report)?; (Some(e), interop) } None => (None, None), @@ -110,38 +135,51 @@ impl ExifReader { } } - /// Reads pointer tag `ptr` from `parent`, removes it (the pointer is represented structurally, - /// not as a data field), and parses the sub-IFD it addresses. + /// Reads `region`'s pointer tag from `parent` and parses the sub-IFD it addresses, then removes + /// the pointer (it is represented structurally, not as a data field). /// /// Returns `Ok(None)` when the pointer is absent, or — in lenient mode — when the pointed-at - /// directory is malformed. + /// directory is unusable, in which case the drop is recorded in `report`. The removal happens + /// **after** the read is attempted: the pointer's tag and offset are what the report names, so + /// stripping it first would lose the identity of what was dropped. fn follow( &self, parent: &mut Ifd, reader: &mut IfdReader, - ptr: u16, - name: &'static str, + region: DroppedRegion, + report: &mut ReadReport, ) -> Result> { + let ptr = region.tag(); let Some(offset) = parent.get_u32(ptr) else { return Ok(None); }; - parent.remove(ptr); - let followed = match reader.read_ifd(u64::from(offset)) { + let offset = u64::from(offset); + let followed = match reader.read_ifd(offset) { Ok(raw) => reader.decode_ifd(&raw), Err(e) => Err(e), }; + parent.remove(ptr); match followed { Ok(ifd) => Ok(Some(ifd)), - Err(_) if !self.strict => Ok(None), - Err(_) => Err(ExifError::InvalidIfd(name)), + Err(_) if self.strict => Err(ExifError::InvalidIfd(region.name())), + Err(_) => { + let reason = address_reason(reader, offset)?; + report.record(Dropped::new(region, offset, reason)); + Ok(None) + } } } /// Builds a [`Thumbnail`] from the 1st IFD, fetching its JPEG bytes (from the /// `JPEGInterchangeFormat` offset / length) when the range is wholly inside the stream. In - /// lenient mode an out-of-bounds range yields a thumbnail without bytes; in strict mode it - /// errors. - fn read_thumbnail(&self, ifd: Ifd, reader: &mut IfdReader) -> Result { + /// lenient mode an out-of-bounds range yields a thumbnail without bytes and a recorded drop; + /// in strict mode it errors. + fn read_thumbnail( + &self, + ifd: Ifd, + reader: &mut IfdReader, + report: &mut ReadReport, + ) -> Result { let ptr = ExifTag::JpegInterchangeFormat.tag_id(); let offset = ifd.get_u32(ptr); let length = ifd.get_u32(ExifTag::JpegInterchangeFormatLength.tag_id()); @@ -151,7 +189,14 @@ impl ExifReader { None if self.strict => { return Err(ExifError::BadThumbnail("JPEG offset out of bounds")); } - None => None, + None => { + report.record(Dropped::new( + DroppedRegion::ThumbnailJpeg, + u64::from(offset), + DropReason::OutOfBounds, + )); + None + } }, _ => None, }; @@ -166,6 +211,18 @@ impl ExifReader { } } +/// Why an address that failed to parse failed: past the end of the stream, or inside it but +/// structurally bad. Separating the two is what makes a report actionable — a dangling pointer is +/// a different defect from a corrupt directory. +fn address_reason(reader: &mut IfdReader, offset: u64) -> Result { + let len = reader.source_mut().len()?; + if offset < len { + Ok(DropReason::Malformed) + } else { + Ok(DropReason::OutOfBounds) + } +} + /// Fetches `length` bytes at `offset`, or `None` when that range is not wholly inside the stream. /// /// The bound is checked against the source's length *before* anything is allocated, so a hostile @@ -264,6 +321,25 @@ mod tests { assert!(matches!(err, ExifError::MissingMarker), "{err:?}"); } + /// `address_reason` splits the two defects the report distinguishes, and the boundary is the + /// stream's length itself: the last byte is inside, the length is not. + #[test] + fn an_address_is_out_of_bounds_from_the_streams_length_onwards() { + let data = [0u8; 8]; + let mut reader = + IfdReader::with_layout(&data[..], ByteOrder::LittleEndian, Variant::Classic); + assert_eq!( + address_reason(&mut reader, 7).expect("reason"), + DropReason::Malformed, + "the last byte of the stream is inside it" + ); + assert_eq!( + address_reason(&mut reader, 8).expect("reason"), + DropReason::OutOfBounds, + "one past the last byte is outside it" + ); + } + /// A range is fetched only when it ends inside the stream — the bound that stops a hostile /// length from being allocated. The boundary case (a range ending exactly at the end) is the /// one an off-by-one would get wrong. diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs new file mode 100644 index 00000000..1c49cf73 --- /dev/null +++ b/crates/gamut-exif/tests/report.rs @@ -0,0 +1,224 @@ +//! The honesty contract for the lenient reader: nothing is discarded without being named. +//! +//! [`ExifReader`] drops a malformed sub-IFD or an out-of-bounds thumbnail range so the rest of a +//! real-world blob still parses. That leniency is only defensible if a caller can *ask* what it +//! cost — otherwise a blob that never carried GPS and one whose GPS pointer was dangling are +//! indistinguishable. Each test below feeds one deliberately broken blob to +//! [`ExifReader::parse_with_report`] and pins that the discarded region is named with the tag that +//! addressed it, the offset it carried, and a reason that separates "nothing could be there" from +//! "something was there and it was corrupt". The last test generalises it over a truncation sweep. + +use gamut_exif::{DropReason, DroppedRegion, ExifReader}; +use gamut_ifd::{ByteOrder, Ifd, IfdReader, TiffFile, Value, Variant, write}; + +/// `ExifIFD` pointer (Exif 3.0 §4.6.3). +const EXIF_IFD: u16 = 0x8769; +/// `GPSInfo` pointer. +const GPS_INFO: u16 = 0x8825; +/// `Interoperability` pointer, which lives inside the Exif sub-IFD. +const INTEROP_IFD: u16 = 0xA005; +/// `JPEGInterchangeFormat` — the 1st IFD's thumbnail offset. +const THUMB_OFFSET: u16 = 0x0201; +/// `JPEGInterchangeFormatLength`. +const THUMB_LENGTH: u16 = 0x0202; +/// An offset far past the end of any fixture here. +const DANGLING: u32 = 0xFFFF; + +/// Serialises `ifds` as a bare little-endian TIFF stream. +fn tiff(ifds: Vec) -> Vec { + write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds, + }) + .expect("write") +} + +/// A 0th IFD carrying `Make`, so every fixture has one tag that must survive the drop. +fn image_ifd() -> Ifd { + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); + image +} + +/// A dangling Exif, GPS or Interop pointer is reported with the tag that addressed it, the offset +/// it carried, and `OutOfBounds` — while the rest of the blob still parses. +#[test] +fn a_dangling_sub_ifd_pointer_is_named_with_its_tag_offset_and_reason() { + for (tag, region) in [ + (EXIF_IFD, DroppedRegion::ExifIfd), + (GPS_INFO, DroppedRegion::GpsIfd), + (INTEROP_IFD, DroppedRegion::InteropIfd), + ] { + // The Interop pointer is reached from inside the Exif sub-IFD, not the 0th IFD. + let mut image = image_ifd(); + if tag == INTEROP_IFD { + let mut exif = Ifd::new(); + exif.set(INTEROP_IFD, Value::Long(vec![DANGLING])); + image.set_sub_ifd(EXIF_IFD, vec![exif]); + } else { + image.set(tag, Value::Long(vec![DANGLING])); + } + + let (exif, report) = ExifReader::new() + .parse_with_report(&tiff(vec![image])) + .expect("a dangling pointer must not fail a lenient parse"); + + assert_eq!(exif.make(), Some("Canon"), "the rest of the blob survives"); + assert_eq!( + report.dropped().len(), + 1, + "exactly one region was dropped for {region:?}: {:?}", + report.dropped() + ); + let dropped = report.dropped()[0]; + assert_eq!(dropped.region(), region); + assert_eq!(dropped.tag(), tag, "named by the tag that addressed it"); + assert_eq!(dropped.offset(), u64::from(DANGLING)); + assert_eq!(dropped.reason(), DropReason::OutOfBounds); + } +} + +/// A pointer that lands *inside* the blob but on bytes that are not a directory is `Malformed`, +/// not `OutOfBounds` — the two are different defects, and a report that conflated them would send +/// a caller looking in the wrong place. +#[test] +fn an_in_bounds_pointer_to_corrupt_bytes_is_reported_as_malformed() { + let mut image = image_ifd(); + // Offset 1 is inside the stream but straddles the byte-order mark and the magic, so the entry + // count read there is nonsense (0x2A49 entries) and the directory cannot be read. + image.set(EXIF_IFD, Value::Long(vec![1])); + let bytes = tiff(vec![image]); + assert!(bytes.len() > 1, "offset 1 must really be inside the stream"); + + let (exif, report) = ExifReader::new() + .parse_with_report(&bytes) + .expect("lenient parse"); + + assert!(exif.exif_ifd().is_none(), "the sub-IFD was dropped"); + assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); + assert_eq!(report.dropped()[0].reason(), DropReason::Malformed); + assert_eq!(report.dropped()[0].offset(), 1); +} + +/// A thumbnail whose JPEG range runs past the end of the blob loses its bytes, and the loss is +/// named — the thumbnail's own directory survives, so without the report the missing bytes look +/// like a thumbnail that never had any. +#[test] +fn an_out_of_bounds_thumbnail_range_is_named() { + let mut thumb = Ifd::new(); + thumb.set(THUMB_OFFSET, Value::Long(vec![DANGLING])); + thumb.set(THUMB_LENGTH, Value::Long(vec![16])); + + let (exif, report) = ExifReader::new() + .parse_with_report(&tiff(vec![image_ifd(), thumb])) + .expect("lenient parse"); + + let thumbnail = exif.thumbnail().expect("the 1st IFD is still a thumbnail"); + assert_eq!( + thumbnail.jpeg(), + None, + "no bytes, rather than bytes from nowhere" + ); + assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); + let dropped = report.dropped()[0]; + assert_eq!(dropped.region(), DroppedRegion::ThumbnailJpeg); + assert_eq!(dropped.tag(), THUMB_OFFSET); + assert_eq!(dropped.offset(), u64::from(DANGLING)); + assert_eq!(dropped.reason(), DropReason::OutOfBounds); +} + +/// A blob that parses in full reports nothing: `is_empty` is the "this parse lost nothing" verdict, +/// so a report that named a region on a healthy file would make it useless. +#[test] +fn a_well_formed_blob_reports_no_drops() { + let mut interop = Ifd::new(); + interop.set(0x0001, Value::Ascii("R98".into())); + let mut exif = Ifd::new(); + exif.set(0x829D, Value::Rational(vec![(28, 10)])); + exif.set_sub_ifd(INTEROP_IFD, vec![interop]); + let mut gps = Ifd::new(); + gps.set(0x0000, Value::Byte(vec![2, 3, 0, 0])); + + let mut image = image_ifd(); + image.set_sub_ifd(EXIF_IFD, vec![exif]); + image.set_sub_ifd(GPS_INFO, vec![gps]); + + let (parsed, report) = ExifReader::new() + .parse_with_report(&tiff(vec![image])) + .expect("parse"); + + assert!(parsed.exif_ifd().is_some()); + assert!(parsed.gps_ifd().is_some()); + assert!(parsed.interop_ifd().is_some()); + assert!( + report.is_empty(), + "healthy blob reported {:?}", + report.dropped() + ); +} + +/// The law, over a truncation sweep: whenever a lenient parse succeeds, a sub-IFD pointer that was +/// present in the source has either been followed into the model or been named in the report — +/// never silently missing. +/// +/// Truncation is the cheapest generator of *varied* corruption: each prefix breaks a different +/// structure (a value, a directory body, a pointer target), so the sweep reaches drop paths no +/// hand-written fixture enumerates. +#[test] +fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { + let mut interop = Ifd::new(); + interop.set(0x0001, Value::Ascii("R98".into())); + let mut exif = Ifd::new(); + exif.set(0x829D, Value::Rational(vec![(28, 10)])); + exif.set_sub_ifd(INTEROP_IFD, vec![interop]); + let mut gps = Ifd::new(); + gps.set(0x0000, Value::Byte(vec![2, 3, 0, 0])); + let mut image = image_ifd(); + image.set_sub_ifd(EXIF_IFD, vec![exif]); + image.set_sub_ifd(GPS_INFO, vec![gps]); + let full = tiff(vec![image]); + + let mut parses = 0_usize; + let mut drops = 0_usize; + for end in 0..full.len() { + let data = &full[..end]; + let Ok((parsed, report)) = ExifReader::new().parse_with_report(data) else { + continue; + }; + parses += 1; + + // What the source actually carried, read back independently of the model. + let Ok(mut raw_reader) = IfdReader::open(data) else { + continue; + }; + let first = raw_reader.first_ifd_offset(); + let Ok(raw) = raw_reader.read_ifd(first) else { + continue; + }; + + for (tag, followed) in [ + (EXIF_IFD, parsed.exif_ifd().is_some()), + (GPS_INFO, parsed.gps_ifd().is_some()), + ] { + if raw.entry(tag).is_none() { + continue; + } + let named = report.dropped().iter().any(|d| d.tag() == tag); + assert_ne!( + followed, named, + "at truncation {end}, tag {tag:#06x} was followed={followed} and named={named}" + ); + drops += usize::from(named); + } + } + + assert!( + parses > 0, + "no truncation parsed — the sweep proved nothing" + ); + assert!( + drops > 0, + "no truncation dropped a sub-IFD — the sweep proved nothing" + ); +} From 5a766f7f2b80cdaf3a6c95787587e514f2537d59 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Wed, 9 Sep 2026 18:41:41 -0400 Subject: [PATCH 03/15] fix(exif): name trailing directories and propagate a failing source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #522 found the report's completeness claim false and its leniency too broad. Both are fixed here; neither existed before that PR. The report silently lost a top-level directory past the 1st IFD. EXIF defines exactly two, so `parse_source` took `ifds.next()` twice and dropped the iterator: a three-directory chain lost the third with an empty report, and `to_bytes` round-tripped 86 source bytes to 56. Add `DroppedRegion::TrailingIfd` and `DropReason::Unrepresentable` so such a directory is named at its own offset. It is the first region no tag addresses — the chain is followed through the structural next-IFD pointer — so `Dropped::tag` is 0 there and `Display` omits the tag clause rather than claiming tag 0x0000, which is a real tag number. The offsets come from a second walk of the chain that runs only when there is something to report, so the lazy read bound is untouched. A failing `ReadAt` source was swallowed and blamed on the file. `follow` matched `Err(_)` without inspecting the error and `address_reason` decided purely from `offset < len`, so a source whose transport failed mid-parse returned `Ok` with the sub-IFDs missing and a report calling structurally perfect directories malformed — worst for exactly the network-backed sources this entry point exists to enable, and asymmetric, since the same failure during the thumbnail read already propagated. Key both `follow` and the marker probe on `Error::kind`: `InvalidInput` is what leniency is for, everything else is propagated unchanged. Keying the probe on the kind rather than on a length also stops `require_marker(true)` reporting `MissingMarker` for a blob whose marker is unknown rather than absent, while a genuinely short slice still reads as unmarked, so `parse`'s behaviour on slices is unchanged. Two known losses remain outside the report, both below this crate: a shadowed duplicate tag (#528) and a single unparseable entry failing its whole directory (#521). The module docs, README and STATUS now say so, and `is_empty` is documented as a verdict over the regions the report covers rather than as "this parse lost nothing". Also: `stream` is a private module, as the record said it should be — it exports no items; `follow`'s comment no longer claims a reorder was load-bearing when the offset was already bound before the removal; and `DroppedRegion::name` no longer claims every variant matches `InvalidIfd`, which was never true of the thumbnail. Refs #419, #521, #528 --- crates/gamut-exif/README.md | 15 ++- crates/gamut-exif/STATUS.md | 16 ++- crates/gamut-exif/src/lib.rs | 9 +- crates/gamut-exif/src/reader.rs | 13 ++- crates/gamut-exif/src/report.rs | 114 +++++++++++++++---- crates/gamut-exif/src/stream.rs | 176 +++++++++++++++++++++++++++++- crates/gamut-exif/tests/report.rs | 54 +++++++++ 7 files changed, 352 insertions(+), 45 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index 0f828ce1..daeff75a 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -52,9 +52,11 @@ points: `&[u8]` case of it — one parse engine, two entry points. It is deliberately synchronous: an async caller drives the source itself, which keeps a runtime dependency out of the crate. - **`parse_with_report`** (and its `parse_from_with_report` twin) returns a `ReadReport` alongside - the `Exif`, naming every sub-IFD and thumbnail range the lenient reader discarded — the tag that - addressed it, the offset it carried, and whether the address was out of bounds or the bytes - there were corrupt. `parse` stays silent, as before. + the `Exif`, naming each region the lenient reader discarded — a malformed Exif/GPS/Interop + sub-IFD, an out-of-bounds thumbnail range, or a top-level directory past the 1st IFD — with the + tag that addressed it, the offset it carried, and a typed reason. `parse` stays silent, as + before. The report is complete over those regions but is **not** a byte-completeness verdict: an + empty report does not mean the parse lost nothing (see the deferred items below). ```rust # use gamut_exif::ExifReader; @@ -90,10 +92,13 @@ designed to be added without breaking the 1.0 API — the catalogue and vendor e losslessly via the raw `Ifd`). - **Uncompressed strip-based thumbnails** are read but not re-embedded (JPEG thumbnails are). - **Per-tag error recovery inside one directory.** A single unparseable entry fails its whole - directory in `gamut-ifd`, so the report's granularity is the sub-IFD, not the individual tag. + directory in `gamut-ifd`, so the report's granularity is the sub-IFD, not the individual tag + (issue #521). +- **A signal for a shadowed duplicate tag.** Two entries for one tag decode to the last, and the + earlier one is discarded a layer below this crate, where `ReadReport` cannot see it (issue #528). - **A byte-completeness verdict** over the whole blob (which source bytes no parsed structure claims). `gamut-ifd`'s audit engine has the machinery; `ReadReport` today reports only what was - dropped, not what was never reached. + dropped, not what was never reached (issue #521). ## Status diff --git a/crates/gamut-exif/STATUS.md b/crates/gamut-exif/STATUS.md index b7e352b5..9a2b27e3 100644 --- a/crates/gamut-exif/STATUS.md +++ b/crates/gamut-exif/STATUS.md @@ -24,7 +24,7 @@ fixtures** (`tests/fixtures/`, regenerate with `GAMUT_REGEN_GOLDEN=1`). | P6 | §4.6 | **Keystone** — writer round-trip (endianness/pointers/thumbnail preserved) | ✅ | | P7 | §4.6 | MakerNote: opaque passthrough + vendor detection (no per-vendor decode) | ✅ | | P8 | — | exiv2 differential gate + golden fixtures | ✅ | -| P9 | §4.6 | `ReadAt` streaming entry point + lenient-drop report (`ReadReport`) | ✅ | +| P9 | §4.6 | `ReadAt` streaming entry point + lenient-drop report (`ReadReport`, scoped to the regions `DroppedRegion` names) | ✅ | ## Intentionally deferred (additive under the `#[non_exhaustive]` surface) @@ -36,10 +36,16 @@ fixtures** (`tests/fixtures/`, regenerate with `GAMUT_REGEN_GOLDEN=1`). round-trip losslessly because the raw `gamut_ifd::Ifd` is retained. - **Uncompressed strip-based thumbnails** are read (as their directory) but not re-embedded; JPEG thumbnails round-trip fully. -- **Per-tag error recovery inside a directory.** `ExifReader::parse_with_report` names every - sub-IFD and thumbnail range the lenient reader discards (issue #419), but the granularity is the - directory: a single unparseable entry fails its whole IFD in `gamut-ifd`, which is the layer that - would have to recover per entry. nom-exif's `entry.into_result()` is finer-grained here. +- **Per-tag error recovery inside a directory.** `ExifReader::parse_with_report` names the + sub-IFDs, thumbnail ranges and trailing top-level directories the lenient reader discards (issue + #419), but the granularity is the directory: a single unparseable entry fails its whole IFD in + `gamut-ifd`, which is the layer that would have to recover per entry. nom-exif's + `entry.into_result()` is finer-grained here. +- **A signal for a shadowed duplicate tag.** `gamut_ifd::IfdReader::decode_ifd` builds a directory + with `Ifd::set`, which is last-wins, so two entries for one tag decode to the second and the + first is discarded with no signal this crate can observe. `ReadReport::is_empty()` is therefore a + verdict over the regions it covers, **not** "this parse lost nothing"; both the report's module + docs and the README say so. Fixing it needs a reporting decode path in `gamut-ifd` (issue #528). - **A byte-completeness verdict.** `ReadReport` says what was *dropped*, not which source bytes no parsed structure claims. `gamut-ifd`'s audit engine (`Tracked`, `SegmentMap`, `read_audited`) is the machinery for it and is already used by `gamut-dng` and `gamut-tiff`; wiring it behind a diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index 9f64f833..836ffe3a 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -18,9 +18,10 @@ //! Two further read entry points sit on [`ExifReader`]: //! [`parse_from`](ExifReader::parse_from) reads through [`gamut_ifd::ReadAt`] rather than a slice, //! so EXIF can be pulled out of a large raw file without loading it, and -//! [`parse_with_report`](ExifReader::parse_with_report) returns a [`ReadReport`] naming every -//! sub-IFD and thumbnail range the lenient reader discarded. `parse` is the `&[u8]` case of -//! `parse_from` and stays silent, so neither is a change for existing callers. +//! [`parse_with_report`](ExifReader::parse_with_report) returns a [`ReadReport`] naming the +//! sub-IFDs, thumbnail ranges and trailing directories the lenient reader discarded — see +//! [`report`] for what that covers and what it deliberately does not. `parse` is the `&[u8]` case +//! of `parse_from` and stays silent, so neither is a change for existing callers. //! //! ``` //! use gamut_exif::{ByteOrder, Exif, ExifTag, Value}; @@ -43,7 +44,7 @@ pub mod gps; pub mod maker_note; pub mod reader; pub mod report; -pub mod stream; +mod stream; pub mod tag; pub mod thumbnail; pub mod value; diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 8f15964a..338b8faa 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -68,15 +68,20 @@ impl ExifReader { /// Parses an EXIF blob and reports what a lenient parse discarded. /// - /// [`parse`](Self::parse) is silent about the sub-IFDs and thumbnail bytes leniency drops; this - /// returns the same [`Exif`] alongside a [`ReadReport`] naming each one, so a caller can tell a - /// blob that never carried GPS from one whose GPS pointer was dangling. + /// [`parse`](Self::parse) is silent about what leniency drops; this returns the same [`Exif`] + /// alongside a [`ReadReport`] naming each discarded region, so a caller can tell a blob that + /// never carried GPS from one whose GPS pointer was dangling. + /// + /// The report covers the regions [`DroppedRegion`](crate::DroppedRegion) enumerates and is + /// complete over them. It is **not** a byte-completeness verdict, and an empty report does not + /// mean the parse lost nothing — see the [`report`](crate::report) module for the two known + /// losses below this crate. /// /// ``` /// # use gamut_exif::{ByteOrder, Exif, ExifReader}; /// # let bytes = Exif::new(ByteOrder::LittleEndian).to_bytes()?; /// let (exif, report) = ExifReader::new().parse_with_report(&bytes)?; - /// assert!(report.is_empty()); // nothing was lost + /// assert!(report.is_empty()); // no covered region was discarded /// for dropped in report.dropped() { /// eprintln!("{dropped}"); /// } diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index 351697f3..9ca1c0a2 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -8,17 +8,37 @@ //! A [`ReadReport`], returned by //! [`ExifReader::parse_with_report`](crate::ExifReader::parse_with_report), names each discarded //! region: **where** it was (a [`DroppedRegion`] and the tag that addressed it), **what offset** -//! addressed it, and **why** it went ([`DropReason`]). A well-formed blob reports nothing, so -//! `report.is_empty()` is the "this parse lost nothing" verdict. +//! addressed it, and **why** it went ([`DropReason`]). //! -//! The granularity is the directory, not the individual tag: a single unparseable entry fails its -//! whole directory in the underlying TIFF/IFD reader, so the sub-IFD it sat in is what gets named. +//! # What this report does and does not claim +//! +//! It covers exactly the regions [`DroppedRegion`] enumerates — the three pointer-addressed +//! sub-IFDs, the thumbnail's JPEG bytes, and any top-level directory past the 1st IFD. Within that +//! set it is complete: nothing in it is discarded without an entry. +//! +//! It is **not** a byte-completeness verdict over the blob, and +//! [`is_empty`](ReadReport::is_empty) does not mean "this parse lost nothing". Two known losses sit +//! outside it, both below this crate in [`gamut_ifd`]: +//! +//! * a **duplicate tag** within one directory keeps the last occurrence and discards the earlier +//! one, with no signal this crate can observe (issue #528); +//! * a single unparseable **entry** fails its whole directory rather than being skipped, so the +//! report's granularity is the directory, never the individual tag (issue #521). +//! +//! Bytes that no parsed structure ever claimed are likewise not reported; that verdict would need +//! `gamut-ifd`'s audit engine and is tracked in #521. use core::fmt; use crate::exif::{EXIF_IFD_POINTER, GPS_IFD_POINTER, INTEROP_IFD_POINTER}; use crate::tag::ExifTag; +/// The [`Dropped::tag`] value for a region that no tag addresses. +/// +/// Zero is a real tag number in a GPS directory (`GPSVersionID`), but never a *pointer* tag, and +/// [`Dropped::tag`] only ever carries a pointer or offset tag — so it is unambiguous here. +const NO_TAG: u16 = 0; + /// A region of an EXIF blob that a lenient parse can discard. /// /// Fieldless with an explicit `repr` and append-only discriminants, so the value crosses an FFI @@ -39,11 +59,23 @@ pub enum DroppedRegion { /// (`0x0201`) and sized by `JPEGInterchangeFormatLength` (`0x0202`). The thumbnail's own /// directory survives; only its bytes are lost. ThumbnailJpeg = 3, + /// A top-level directory past the 1st IFD. + /// + /// EXIF defines exactly two: the 0th IFD (primary image) and the 1st (thumbnail). A stream + /// whose next-IFD chain runs on has more, and the [`Exif`](crate::Exif) model has nowhere to + /// put them — so they parse cleanly and are then discarded. No tag addresses one (the chain is + /// followed through the structural next-IFD pointer), so [`Dropped::tag`] is `0`. + TrailingIfd = 4, } impl DroppedRegion { - /// The region's short name — the same spelling - /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) uses in strict mode. + /// The region's short name, as it appears in [`Dropped`]'s `Display`. + /// + /// For the three sub-IFDs this is also the spelling + /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) uses in strict mode. The other two + /// have no such error: a strict out-of-bounds thumbnail is + /// [`ExifError::BadThumbnail`](crate::ExifError::BadThumbnail), and a trailing directory is + /// discarded in strict mode exactly as in lenient mode, since nothing about it is malformed. #[must_use] pub const fn name(self) -> &'static str { match self { @@ -51,16 +83,18 @@ impl DroppedRegion { Self::GpsIfd => "GPS", Self::InteropIfd => "Interop", Self::ThumbnailJpeg => "Thumbnail", + Self::TrailingIfd => "TrailingIFD", } } - /// The tag whose value addressed this region. + /// The tag whose value addressed this region, or [`NO_TAG`] when none does. pub(crate) const fn tag(self) -> u16 { match self { Self::ExifIfd => EXIF_IFD_POINTER, Self::GpsIfd => GPS_IFD_POINTER, Self::InteropIfd => INTEROP_IFD_POINTER, Self::ThumbnailJpeg => ExifTag::JpegInterchangeFormat.tag_id(), + Self::TrailingIfd => NO_TAG, } } } @@ -79,6 +113,9 @@ pub enum DropReason { /// The address was inside the blob, but the structure at it did not parse: a bad entry count, /// an unreadable entry, or a value offset the directory could not resolve. Malformed = 1, + /// Nothing was wrong with the region — it parsed cleanly — but the EXIF model has no place to + /// put it, so it could not be carried across. + Unrepresentable = 2, } impl DropReason { @@ -87,6 +124,7 @@ impl DropReason { match self { Self::OutOfBounds => "addresses bytes outside the EXIF blob", Self::Malformed => "is not a well-formed directory", + Self::Unrepresentable => "parsed cleanly but has no place in the EXIF model", } } } @@ -121,13 +159,21 @@ impl Dropped { /// The tag whose value addressed the discarded region — the pointer tag for a sub-IFD, /// `JPEGInterchangeFormat` for the thumbnail bytes. + /// + /// `0` when no tag addresses the region, which today means only + /// [`DroppedRegion::TrailingIfd`]: a top-level directory is reached through the structural + /// next-IFD pointer, not through a tag. #[must_use] pub const fn tag(self) -> u16 { self.tag } - /// The offset that tag carried, relative to the start of the TIFF stream (i.e. *after* any - /// `Exif\0\0` marker) — the same frame of reference EXIF's own offsets use. + /// The offset of the discarded region, relative to the start of the TIFF stream (i.e. *after* + /// any `Exif\0\0` marker) — the same frame of reference EXIF's own offsets use. + /// + /// For a sub-IFD or the thumbnail bytes this is the value the addressing tag carried; for a + /// [`TrailingIfd`](DroppedRegion::TrailingIfd) it is the directory's own position in the + /// stream. #[must_use] pub const fn offset(self) -> u64 { self.offset @@ -142,23 +188,30 @@ impl Dropped { impl fmt::Display for Dropped { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "dropped {} at tag {:#06x}, offset {}: {}", - self.region.name(), - self.tag, - self.offset, - self.reason.clause() - ) + let name = self.region.name(); + let clause = self.reason.clause(); + if self.tag == NO_TAG { + write!(f, "dropped {name} at offset {}: {clause}", self.offset) + } else { + write!( + f, + "dropped {name} at tag {:#06x}, offset {}: {clause}", + self.tag, self.offset + ) + } } } -/// What a lenient parse discarded — empty when the blob was carried across in full. +/// What a lenient parse discarded, over the regions [`DroppedRegion`] enumerates. /// /// Obtained from [`ExifReader::parse_with_report`](crate::ExifReader::parse_with_report) or /// [`ExifReader::parse_from_with_report`](crate::ExifReader::parse_from_with_report). In -/// [`strict`](crate::ExifReader::strict) mode the first malformed region fails the parse instead, -/// so a report from a strict reader is always empty. +/// [`strict`](crate::ExifReader::strict) mode the first *malformed* region fails the parse instead, +/// so a strict report can still be non-empty only for regions strictness does not reject (a +/// trailing directory is discarded either way). +/// +/// Read the module documentation for what this report deliberately does **not** cover: it is not a +/// byte-completeness verdict, and losses inside a single directory belong to [`gamut_ifd`]. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ReadReport { dropped: Vec, @@ -177,7 +230,11 @@ impl ReadReport { &self.dropped } - /// Whether the parse lost nothing. + /// Whether any of the regions this report covers was discarded. + /// + /// **Not** a "this parse lost nothing" verdict — see the module documentation. An empty report + /// means no sub-IFD, thumbnail range or trailing directory was dropped; it says nothing about + /// a shadowed duplicate tag (#528) or about source bytes no structure claimed (#521). #[must_use] pub fn is_empty(&self) -> bool { self.dropped.is_empty() @@ -194,7 +251,7 @@ mod tests { use super::*; /// Each region reports the tag that actually addresses it — the value a caller uses to find - /// the pointer back in the source directory. + /// the pointer back in the source directory — and a region no tag addresses reports `0`. #[test] fn each_region_carries_the_tag_that_addresses_it() { for (region, tag) in [ @@ -202,6 +259,7 @@ mod tests { (DroppedRegion::GpsIfd, 0x8825), (DroppedRegion::InteropIfd, 0xA005), (DroppedRegion::ThumbnailJpeg, 0x0201), + (DroppedRegion::TrailingIfd, NO_TAG), ] { assert_eq!( Dropped::new(region, 0, DropReason::OutOfBounds).tag(), @@ -233,8 +291,18 @@ mod tests { ); } + /// A region no tag addresses renders without a tag clause, rather than claiming tag `0x0000` — + /// which is a real tag number (`GPSVersionID`) and would read as a fact about the source. + #[test] + fn a_drop_with_no_addressing_tag_renders_without_one() { + assert_eq!( + Dropped::new(DroppedRegion::TrailingIfd, 120, DropReason::Unrepresentable).to_string(), + "dropped TrailingIFD at offset 120: parsed cleanly but has no place in the EXIF model" + ); + } + /// A fresh report is empty and stays consistent with what has been recorded — `is_empty` is - /// the "this parse lost nothing" verdict, so it must not be independent of the contents. + /// the verdict over the covered regions, so it must not be independent of the contents. #[test] fn a_report_is_empty_until_something_is_recorded() { let mut report = ReadReport::new(); diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 60dac822..972fbe98 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -12,6 +12,7 @@ //! drift. It is deliberately synchronous: an async caller drives a [`ReadAt`] source itself, which //! keeps a runtime dependency out of a crate that has none. +use gamut_core::ErrorKind; use gamut_ifd::{Ifd, IfdReader, RawIfd, ReadAt, tags as ifd_tags}; use crate::error::{ExifError, Result}; @@ -79,6 +80,7 @@ impl ExifReader { let order = reader.order(); let file = reader.read_file()?; + let trailing = file.ifds.len().saturating_sub(2); let mut ifds = file.ifds.into_iter(); let mut image = ifds.next().ok_or(ExifError::Truncated)?; // The next-IFD chain's second entry is the thumbnail directory (1st IFD), if any. @@ -86,6 +88,9 @@ impl ExifReader { Some(ifd) => Some(self.read_thumbnail(ifd, &mut reader, report)?), None => None, }; + // EXIF defines exactly two top-level directories, so anything further down the chain has + // nowhere to go in the model. Name it rather than letting the iterator drop it silently. + record_trailing_ifds(&mut reader, trailing, report)?; // The Exif sub-IFD's own offset, captured before `follow` strips the pointer: the // maker-note pin needs the note value's absolute source position. @@ -125,7 +130,14 @@ impl ExifReader { /// is longer than the marker, so such a source cannot parse either way. fn tiff_base(&self, source: &mut S) -> Result { let mut head = [0u8; MARKER.len()]; - let marked = source.read_exact_at(0, &mut head).is_ok() && head.as_slice() == MARKER; + let marked = match source.read_exact_at(0, &mut head) { + Ok(()) => head.as_slice() == MARKER, + // Too few bytes to hold a marker: unmarked. Keyed on the error *kind*, never on the + // source's length, so a slice keeps its exact behaviour while a transport failure + // (a disk error, a dropped network mount) is not misread as "no marker". + Err(e) if e.kind() == ErrorKind::InvalidInput => false, + Err(e) => return Err(e.into()), + }; if marked { Ok(MARKER.len() as u64) } else if self.require_marker { @@ -139,9 +151,15 @@ impl ExifReader { /// the pointer (it is represented structurally, not as a data field). /// /// Returns `Ok(None)` when the pointer is absent, or — in lenient mode — when the pointed-at - /// directory is unusable, in which case the drop is recorded in `report`. The removal happens - /// **after** the read is attempted: the pointer's tag and offset are what the report names, so - /// stripping it first would lose the identity of what was dropped. + /// directory is malformed, in which case the drop is recorded in `report`. Either way the + /// pointer is removed: preserving it would change what `to_bytes` emits for a malformed blob, + /// which is beyond this crate's remit here — #419's "the pointer was lost too" is answered by + /// *naming* the tag and offset in the report, not by keeping the entry. + /// + /// A failure that is not the *bytes* being wrong — a source whose transport failed — is + /// propagated unchanged in both modes. Leniency exists to tolerate corrupt files, and + /// reporting a structurally perfect directory as malformed because a disk read failed would be + /// a lie about the file. fn follow( &self, parent: &mut Ifd, @@ -161,6 +179,8 @@ impl ExifReader { parent.remove(ptr); match followed { Ok(ifd) => Ok(Some(ifd)), + // Not the file's fault: hand the transport failure back untouched. + Err(e) if e.kind() != ErrorKind::InvalidInput => Err(e.into()), Err(_) if self.strict => Err(ExifError::InvalidIfd(region.name())), Err(_) => { let reason = address_reason(reader, offset)?; @@ -211,6 +231,34 @@ impl ExifReader { } } +/// Records the top-level directories past the 1st IFD, which parse cleanly but have nowhere to go +/// in the [`Exif`] model. +/// +/// The offsets come from a second walk of the next-IFD chain. That costs a re-read of the +/// directory bodies, so it runs **only** when there is something to report — a well-formed EXIF +/// blob has one or two directories and never reaches it, leaving the lazy read bound untouched. +fn record_trailing_ifds( + reader: &mut IfdReader, + trailing: usize, + report: &mut ReadReport, +) -> Result<()> { + if trailing == 0 { + return Ok(()); + } + let mut offsets = Vec::with_capacity(trailing); + for raw in reader.ifds().skip(2) { + offsets.push(raw?.offset); + } + for offset in offsets { + report.record(Dropped::new( + DroppedRegion::TrailingIfd, + offset, + DropReason::Unrepresentable, + )); + } + Ok(()) +} + /// Why an address that failed to parse failed: past the end of the stream, or inside it but /// structurally bad. Separating the two is what makes a report actionable — a dangling pointer is /// a different defect from a corrupt directory. @@ -259,6 +307,7 @@ mod tests { use gamut_ifd::{ByteOrder, StreamSource, TiffFile, Value, Variant, write}; use super::*; + use crate::exif::{GPS_IFD_POINTER, INTEROP_IFD_POINTER}; /// A minimal marked EXIF blob whose 0th IFD carries `Make`. fn blob() -> Vec { @@ -321,6 +370,125 @@ mod tests { assert!(matches!(err, ExifError::MissingMarker), "{err:?}"); } + /// A `ReadAt` whose *transport* fails after `budget` successful reads — a disk error, a + /// dropped network mount. Distinct from a source whose bytes are merely wrong: `gamut-core` + /// maps the former to `Error::Io` and the latter to `Error::InvalidInput`. + struct FailingAfter { + inner: S, + budget: usize, + } + + impl ReadAt for FailingAfter { + fn read_exact_at(&mut self, offset: u64, buf: &mut [u8]) -> gamut_core::Result<()> { + let Some(left) = self.budget.checked_sub(1) else { + return Err(gamut_core::Error::Io(std::io::Error::other( + "transport lost", + ))); + }; + self.budget = left; + self.inner.read_exact_at(offset, buf) + } + + fn len(&mut self) -> gamut_core::Result { + self.inner.len() + } + } + + /// A structurally perfect blob with both 0th-IFD sub-directories, so every drop path is + /// reachable and none of them *should* fire. + fn healthy_blob() -> Vec { + let mut interop = Ifd::new(); + interop.set(0x0001, Value::Ascii("R98".into())); + let mut exif = Ifd::new(); + exif.set(0x829D, Value::Rational(vec![(28, 10)])); + exif.set_sub_ifd(INTEROP_IFD_POINTER, vec![interop]); + let mut gps = Ifd::new(); + gps.set(0x0000, Value::Byte(vec![2, 3, 0, 0])); + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); + image.set_sub_ifd(EXIF_IFD_POINTER, vec![exif]); + image.set_sub_ifd(GPS_IFD_POINTER, vec![gps]); + let tiff = write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds: vec![image], + }) + .expect("write"); + let mut out = MARKER.to_vec(); + out.extend(tiff); + out + } + + /// A failing source is propagated, never reported as a malformed file. + /// + /// Leniency exists to tolerate corrupt *bytes*. If the transport fails instead, the data may be + /// perfect, so silently returning `Ok` with the sub-IFDs missing — and a report blaming the + /// file — would be a lie, and the worst case is the network-backed source this entry point + /// exists to enable. The whole parse is swept one read at a time, so every read site is + /// covered: the marker probe, the header, each directory body and each out-of-line value. + #[test] + fn a_failing_source_is_propagated_not_reported_as_a_malformed_file() { + let data = healthy_blob(); + let (mut failures, mut successes) = (0, 0); + for budget in 0..40 { + let source = FailingAfter { + inner: &data[..], + budget, + }; + match ExifReader::new().parse_from_with_report(source) { + Ok((_, report)) => { + successes += 1; + assert!( + report.is_empty(), + "budget {budget}: a transport failure was blamed on the file: {:?}", + report.dropped() + ); + } + Err(ExifError::Ifd(e)) => { + failures += 1; + assert_eq!( + e.kind(), + ErrorKind::Io, + "budget {budget}: a transport failure must keep its kind" + ); + } + // `MissingMarker` here would mean the marker probe swallowed the error and + // decided the blob was unmarked; anything else is equally a misdiagnosis. + Err(other) => panic!("budget {budget}: transport failure became {other:?}"), + } + } + assert!(failures > 0 && successes > 0, "the sweep proved nothing"); + } + + /// A transport failure while probing for the marker is not a *missing* marker. + /// + /// The marker probe is the one read that happens before any parsing, and its result is a + /// three-way question — marked, unmarked, or unknown — collapsed onto a boolean. Deciding + /// "unmarked" from a failed read makes `require_marker(true)` answer `MissingMarker` for a blob + /// that may well carry one, which sends a caller to the wrong conclusion entirely. The split is + /// keyed on the error kind, so a short slice still reads as genuinely unmarked — which + /// `a_source_too_short_for_the_marker_is_unmarked` pins. + #[test] + fn a_transport_failure_probing_the_marker_is_not_a_missing_marker() { + let data = healthy_blob(); + let source = FailingAfter { + inner: &data[..], + budget: 0, + }; + let err = ExifReader::new() + .require_marker(true) + .parse_from(source) + .expect_err("a source that cannot be read must not parse"); + match err { + ExifError::Ifd(e) => assert_eq!( + e.kind(), + ErrorKind::Io, + "the transport failure must keep its kind" + ), + other => panic!("transport failure became {other:?}"), + } + } + /// `address_reason` splits the two defects the report distinguishes, and the boundary is the /// stream's length itself: the last byte is inside, the length is not. #[test] diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index 1c49cf73..992c9ce1 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -158,6 +158,60 @@ fn a_well_formed_blob_reports_no_drops() { ); } +/// A top-level directory past the 1st IFD is named rather than silently discarded. +/// +/// EXIF defines exactly two — the 0th (primary image) and the 1st (thumbnail) — so a longer +/// next-IFD chain parses cleanly and then has nowhere to go in the model. That is a real loss (the +/// bytes do not survive `to_bytes`), and it is the one drop with no addressing tag: the chain is +/// followed through the structural next-IFD pointer, so the reported tag is `0` and the reported +/// offset is the directory's own position. +#[test] +fn a_top_level_directory_past_the_thumbnail_is_named() { + for extra in 1..=2 { + let mut thumb = Ifd::new(); + thumb.set(0x0103, Value::Short(vec![6])); // Compression = JPEG + + let mut ifds = vec![image_ifd(), thumb]; + for n in 0..extra { + let mut trailing = Ifd::new(); + trailing.set(0x0131, Value::Ascii(format!("trailing {n}"))); // Software + ifds.push(trailing); + } + let bytes = tiff(ifds); + + let (exif, report) = ExifReader::new() + .parse_with_report(&bytes) + .expect("a long chain must still parse"); + assert_eq!(exif.make(), Some("Canon"), "the 0th IFD survives"); + assert!(exif.thumbnail().is_some(), "the 1st IFD survives"); + + // Where those directories actually sit, read back independently of the model. + let mut raw = IfdReader::open(&bytes[..]).expect("open"); + let offsets: Vec = raw + .ifds() + .map(|ifd| ifd.expect("chain link").offset) + .collect(); + assert_eq!( + offsets.len(), + 2 + extra, + "the fixture really has a long chain" + ); + + assert_eq!( + report.dropped().len(), + extra, + "every trailing directory is named: {:?}", + report.dropped() + ); + for (dropped, expected) in report.dropped().iter().zip(&offsets[2..]) { + assert_eq!(dropped.region(), DroppedRegion::TrailingIfd); + assert_eq!(dropped.tag(), 0, "no tag addresses a top-level directory"); + assert_eq!(dropped.offset(), *expected, "named at its own position"); + assert_eq!(dropped.reason(), DropReason::Unrepresentable); + } + } +} + /// The law, over a truncation sweep: whenever a lenient parse succeeds, a sub-IFD pointer that was /// present in the source has either been followed into the model or been named in the report — /// never silently missing. From ff6b5711a7aa5ac493a36d352885b7a83c747547 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Wed, 9 Sep 2026 19:42:26 -0400 Subject: [PATCH 04/15] fix(exif): propagate a failing source through the maker-note pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review of #522 found the one read site the transport/malformed split missed, plus a sweep whose fixture could not reach it. `maker_note_offset` did `read_ifd(..).ok()?`, discarding every error including `Error::Io`. The falsifier "a later read resurfaces it" is false: `follow` returns before reading at all when the GPS and Interop pointers are absent, which is the common case. So a source that failed there returned `Ok` with `maker_note_offset() == None` and an empty report — and since the writer uses that offset to pin the note, a vendor MakerNote with absolute internal offsets was re-emitted unpinned: wrong bytes, no error, nothing reported. It now keys on `Error::kind` like the other three sites. The sweep asserted it covered "every read site" while using a fixture with no MakerNote and an Interop pointer, so every budget that failed inside the pin was rescued by the later Interop read. It now runs over two fixtures — the second has an out-of-line MakerNote and neither optional pointer — and additionally asserts that an `Ok` from a failing source is the *whole* answer, not a quietly diminished one. Reverting the fix makes it fail at budget 9, the reviewer's own reproduction. `Dropped::tag` becomes `Option`. The `0` sentinel rested on "0 is never a pointer tag", which held only while every region was pointer-addressed — `TrailingIfd` broke that, and `0` is a real tag number (`GPSVersionID`). `Dropped` is unreleased, so this costs nothing now and could not be done later. `Display` gains one grammar with an explicit `(tag none)` rather than two shapes a caller would have to parse. `record_trailing_ifds` takes a `bool` instead of a count it only zero-tested, so `saturating_sub(2)` can no longer be mutated to `saturating_sub(1)` with identical behaviour. The walk is the single source of truth for which directories are trailing. The laziness bound drops from 512 to 300 bytes to make that guard falsifiable: an unnecessary re-walk costs 347 bytes against 251 clean, so it is now a failure rather than an invisible inefficiency. Also corrected: the deferral in #528 was justified by "no signal this crate can observe", which is false — `RawIfd::entries` is public and in on-disk order, and `follow` already holds the `RawIfd`, so a shadowed tag is detectable here in three lines. The deferral stands, but on the real reason: this crate cannot say *what* was lost without re-decoding it, and three crates need the same signal. STATUS.md and the README now say that; #528's body still carries the weaker reason. The retracted "this parse lost nothing" phrasing is gone from the last place it survived, and `TrailingIfd` being reported in strict mode too is documented as intended rather than left to prose. Refs #419, #521, #528 --- crates/gamut-exif/README.md | 5 +- crates/gamut-exif/STATUS.md | 12 +- crates/gamut-exif/src/report.rs | 97 ++++++++------ crates/gamut-exif/src/stream.rs | 193 ++++++++++++++++++++------- crates/gamut-exif/tests/report.rs | 24 +++- crates/gamut-exif/tests/streaming.rs | 9 +- 6 files changed, 240 insertions(+), 100 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index daeff75a..d559a80b 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -95,7 +95,10 @@ designed to be added without breaking the 1.0 API — the catalogue and vendor e directory in `gamut-ifd`, so the report's granularity is the sub-IFD, not the individual tag (issue #521). - **A signal for a shadowed duplicate tag.** Two entries for one tag decode to the last, and the - earlier one is discarded a layer below this crate, where `ReadReport` cannot see it (issue #528). + earlier one is discarded a layer below this crate. `gamut-exif` could *detect* the loss (compare + `RawIfd::entries` against the decoded `Ifd::fields()`), but not describe it without re-decoding + the shadowed entry, so the signal belongs where the discarding happens — a layer three crates + share (issue #528). - **A byte-completeness verdict** over the whole blob (which source bytes no parsed structure claims). `gamut-ifd`'s audit engine has the machinery; `ReadReport` today reports only what was dropped, not what was never reached (issue #521). diff --git a/crates/gamut-exif/STATUS.md b/crates/gamut-exif/STATUS.md index 9a2b27e3..511b6480 100644 --- a/crates/gamut-exif/STATUS.md +++ b/crates/gamut-exif/STATUS.md @@ -43,9 +43,15 @@ fixtures** (`tests/fixtures/`, regenerate with `GAMUT_REGEN_GOLDEN=1`). `entry.into_result()` is finer-grained here. - **A signal for a shadowed duplicate tag.** `gamut_ifd::IfdReader::decode_ifd` builds a directory with `Ifd::set`, which is last-wins, so two entries for one tag decode to the second and the - first is discarded with no signal this crate can observe. `ReadReport::is_empty()` is therefore a - verdict over the regions it covers, **not** "this parse lost nothing"; both the report's module - docs and the README say so. Fixing it needs a reporting decode path in `gamut-ifd` (issue #528). + first is discarded. `ReadReport::is_empty()` is therefore a verdict over the regions it covers, + **not** "this parse lost nothing"; both the report's module docs and the README say so. + + The deferral is a layering decision, **not** an inability to observe: `RawIfd::entries` is public + and in on-disk order and `follow` already holds the `RawIfd`, so `raw.entries.len() != + ifd.fields().len()` would detect a shadowed tag here in three lines. What `gamut-exif` cannot do + is say *what* was lost without re-decoding the shadowed entry — and `gamut-tiff` and `gamut-dng` + need the same signal, so it belongs in the shared layer (issue #528). Issue #528's own body + states the weaker, incorrect reason; read it with this correction. - **A byte-completeness verdict.** `ReadReport` says what was *dropped*, not which source bytes no parsed structure claims. `gamut-ifd`'s audit engine (`Tracked`, `SegmentMap`, `read_audited`) is the machinery for it and is already used by `gamut-dng` and `gamut-tiff`; wiring it behind a diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index 9ca1c0a2..385c58e5 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -21,7 +21,11 @@ //! outside it, both below this crate in [`gamut_ifd`]: //! //! * a **duplicate tag** within one directory keeps the last occurrence and discards the earlier -//! one, with no signal this crate can observe (issue #528); +//! one. This crate *could* detect that a directory lost an entry — `RawIfd::entries` is public +//! and in on-disk order, so comparing its length against the decoded `Ifd::fields()` finds it in +//! three lines — but it could not say what was lost without re-decoding the shadowed entry +//! itself. The signal belongs at the layer that does the discarding, which three crates share +//! (issue #528); //! * a single unparseable **entry** fails its whole directory rather than being skipped, so the //! report's granularity is the directory, never the individual tag (issue #521). //! @@ -33,12 +37,6 @@ use core::fmt; use crate::exif::{EXIF_IFD_POINTER, GPS_IFD_POINTER, INTEROP_IFD_POINTER}; use crate::tag::ExifTag; -/// The [`Dropped::tag`] value for a region that no tag addresses. -/// -/// Zero is a real tag number in a GPS directory (`GPSVersionID`), but never a *pointer* tag, and -/// [`Dropped::tag`] only ever carries a pointer or offset tag — so it is unambiguous here. -const NO_TAG: u16 = 0; - /// A region of an EXIF blob that a lenient parse can discard. /// /// Fieldless with an explicit `repr` and append-only discriminants, so the value crosses an FFI @@ -64,7 +62,11 @@ pub enum DroppedRegion { /// EXIF defines exactly two: the 0th IFD (primary image) and the 1st (thumbnail). A stream /// whose next-IFD chain runs on has more, and the [`Exif`](crate::Exif) model has nowhere to /// put them — so they parse cleanly and are then discarded. No tag addresses one (the chain is - /// followed through the structural next-IFD pointer), so [`Dropped::tag`] is `0`. + /// followed through the structural next-IFD pointer), so [`Dropped::tag`] is `None`. + /// + /// Reported in [`strict`](crate::ExifReader::strict) mode too: strictness rejects *malformed* + /// regions, and nothing about a trailing directory is malformed — it is well-formed and + /// unrepresentable. A strict report is therefore empty of everything *but* this. TrailingIfd = 4, } @@ -87,14 +89,14 @@ impl DroppedRegion { } } - /// The tag whose value addressed this region, or [`NO_TAG`] when none does. - pub(crate) const fn tag(self) -> u16 { + /// The tag whose value addressed this region, or `None` when no tag does. + pub(crate) const fn tag(self) -> Option { match self { - Self::ExifIfd => EXIF_IFD_POINTER, - Self::GpsIfd => GPS_IFD_POINTER, - Self::InteropIfd => INTEROP_IFD_POINTER, - Self::ThumbnailJpeg => ExifTag::JpegInterchangeFormat.tag_id(), - Self::TrailingIfd => NO_TAG, + Self::ExifIfd => Some(EXIF_IFD_POINTER), + Self::GpsIfd => Some(GPS_IFD_POINTER), + Self::InteropIfd => Some(INTEROP_IFD_POINTER), + Self::ThumbnailJpeg => Some(ExifTag::JpegInterchangeFormat.tag_id()), + Self::TrailingIfd => None, } } } @@ -135,7 +137,7 @@ impl DropReason { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct Dropped { region: DroppedRegion, - tag: u16, + tag: Option, offset: u64, reason: DropReason, } @@ -160,11 +162,13 @@ impl Dropped { /// The tag whose value addressed the discarded region — the pointer tag for a sub-IFD, /// `JPEGInterchangeFormat` for the thumbnail bytes. /// - /// `0` when no tag addresses the region, which today means only + /// `None` when no tag addresses the region, which today means only /// [`DroppedRegion::TrailingIfd`]: a top-level directory is reached through the structural - /// next-IFD pointer, not through a tag. + /// next-IFD pointer, not through a tag. This is an `Option` rather than a `0` sentinel because + /// `0` is a real tag number (`GPSVersionID`), and the region set is `#[non_exhaustive]` — the + /// next region without an addressing tag might well be one inside a GPS directory. #[must_use] - pub const fn tag(self) -> u16 { + pub const fn tag(self) -> Option { self.tag } @@ -187,17 +191,23 @@ impl Dropped { } impl fmt::Display for Dropped { + /// One grammar for every drop — `dropped (tag ) at offset : ` — with + /// `none` as the explicit absent marker rather than a second shape. A caller that scrapes this + /// line should not have to recognise two forms, and an absent tag is a fact worth stating. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let name = self.region.name(); let clause = self.reason.clause(); - if self.tag == NO_TAG { - write!(f, "dropped {name} at offset {}: {clause}", self.offset) - } else { - write!( + match self.tag { + Some(tag) => write!( + f, + "dropped {name} (tag {tag:#06x}) at offset {}: {clause}", + self.offset + ), + None => write!( f, - "dropped {name} at tag {:#06x}, offset {}: {clause}", - self.tag, self.offset - ) + "dropped {name} (tag none) at offset {}: {clause}", + self.offset + ), } } } @@ -206,9 +216,10 @@ impl fmt::Display for Dropped { /// /// Obtained from [`ExifReader::parse_with_report`](crate::ExifReader::parse_with_report) or /// [`ExifReader::parse_from_with_report`](crate::ExifReader::parse_from_with_report). In -/// [`strict`](crate::ExifReader::strict) mode the first *malformed* region fails the parse instead, -/// so a strict report can still be non-empty only for regions strictness does not reject (a -/// trailing directory is discarded either way). +/// [`strict`](crate::ExifReader::strict) mode the first *malformed* region fails the parse instead +/// of being reported — but a strict report is **not** therefore always empty: +/// [`DroppedRegion::TrailingIfd`] is well-formed and merely unrepresentable, so strictness has no +/// grounds to reject it and it is reported in both modes. /// /// Read the module documentation for what this report deliberately does **not** cover: it is not a /// byte-completeness verdict, and losses inside a single directory belong to [`gamut_ifd`]. @@ -255,11 +266,11 @@ mod tests { #[test] fn each_region_carries_the_tag_that_addresses_it() { for (region, tag) in [ - (DroppedRegion::ExifIfd, 0x8769), - (DroppedRegion::GpsIfd, 0x8825), - (DroppedRegion::InteropIfd, 0xA005), - (DroppedRegion::ThumbnailJpeg, 0x0201), - (DroppedRegion::TrailingIfd, NO_TAG), + (DroppedRegion::ExifIfd, Some(0x8769)), + (DroppedRegion::GpsIfd, Some(0x8825)), + (DroppedRegion::InteropIfd, Some(0xA005)), + (DroppedRegion::ThumbnailJpeg, Some(0x0201)), + (DroppedRegion::TrailingIfd, None), ] { assert_eq!( Dropped::new(region, 0, DropReason::OutOfBounds).tag(), @@ -275,29 +286,31 @@ mod tests { fn the_rendered_drop_names_region_tag_offset_and_reason() { assert_eq!( Dropped::new(DroppedRegion::GpsIfd, 65_535, DropReason::OutOfBounds).to_string(), - "dropped GPS at tag 0x8825, offset 65535: addresses bytes outside the EXIF blob" + "dropped GPS (tag 0x8825) at offset 65535: addresses bytes outside the EXIF blob" ); assert_eq!( Dropped::new(DroppedRegion::ExifIfd, 26, DropReason::Malformed).to_string(), - "dropped Exif at tag 0x8769, offset 26: is not a well-formed directory" + "dropped Exif (tag 0x8769) at offset 26: is not a well-formed directory" ); assert_eq!( Dropped::new(DroppedRegion::InteropIfd, 8, DropReason::Malformed).to_string(), - "dropped Interop at tag 0xa005, offset 8: is not a well-formed directory" + "dropped Interop (tag 0xa005) at offset 8: is not a well-formed directory" ); assert_eq!( Dropped::new(DroppedRegion::ThumbnailJpeg, 1, DropReason::OutOfBounds).to_string(), - "dropped Thumbnail at tag 0x0201, offset 1: addresses bytes outside the EXIF blob" + "dropped Thumbnail (tag 0x0201) at offset 1: addresses bytes outside the EXIF blob" ); } - /// A region no tag addresses renders without a tag clause, rather than claiming tag `0x0000` — - /// which is a real tag number (`GPSVersionID`) and would read as a fact about the source. + /// A region no tag addresses says so explicitly, in the same grammar as every other drop — + /// rather than claiming tag `0x0000`, which is a real tag number (`GPSVersionID`) and would + /// read as a fact about the source. #[test] - fn a_drop_with_no_addressing_tag_renders_without_one() { + fn a_drop_with_no_addressing_tag_says_so_in_the_same_grammar() { assert_eq!( Dropped::new(DroppedRegion::TrailingIfd, 120, DropReason::Unrepresentable).to_string(), - "dropped TrailingIFD at offset 120: parsed cleanly but has no place in the EXIF model" + "dropped TrailingIFD (tag none) at offset 120: parsed cleanly but has no place in the \ + EXIF model" ); } diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 972fbe98..cbf44f27 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -80,7 +80,7 @@ impl ExifReader { let order = reader.order(); let file = reader.read_file()?; - let trailing = file.ifds.len().saturating_sub(2); + let has_trailing = file.ifds.len() > 2; let mut ifds = file.ifds.into_iter(); let mut image = ifds.next().ok_or(ExifError::Truncated)?; // The next-IFD chain's second entry is the thumbnail directory (1st IFD), if any. @@ -90,7 +90,7 @@ impl ExifReader { }; // EXIF defines exactly two top-level directories, so anything further down the chain has // nowhere to go in the model. Name it rather than letting the iterator drop it silently. - record_trailing_ifds(&mut reader, trailing, report)?; + record_trailing_ifds(&mut reader, has_trailing, report)?; // The Exif sub-IFD's own offset, captured before `follow` strips the pointer: the // maker-note pin needs the note value's absolute source position. @@ -98,7 +98,7 @@ impl ExifReader { let exif = self.follow(&mut image, &mut reader, DroppedRegion::ExifIfd, report)?; let gps = self.follow(&mut image, &mut reader, DroppedRegion::GpsIfd, report)?; let maker_note_at = match (&exif, exif_ifd_at) { - (Some(_), Some(at)) => maker_note_offset(&mut reader, at), + (Some(_), Some(at)) => maker_note_offset(&mut reader, at)?, _ => None, }; @@ -167,7 +167,12 @@ impl ExifReader { region: DroppedRegion, report: &mut ReadReport, ) -> Result> { - let ptr = region.tag(); + // A region no tag addresses has, by definition, no pointer to follow — so `Ok(None)` is + // the answer, not a special case. Today `follow` is only ever called for the three + // pointer-addressed sub-IFDs, all of which have one. + let Some(ptr) = region.tag() else { + return Ok(None); + }; let Some(offset) = parent.get_u32(ptr) else { return Ok(None); }; @@ -234,18 +239,24 @@ impl ExifReader { /// Records the top-level directories past the 1st IFD, which parse cleanly but have nowhere to go /// in the [`Exif`] model. /// -/// The offsets come from a second walk of the next-IFD chain. That costs a re-read of the -/// directory bodies, so it runs **only** when there is something to report — a well-formed EXIF -/// blob has one or two directories and never reaches it, leaving the lazy read bound untouched. +/// The offsets come from a second walk of the next-IFD chain, which is the single source of truth +/// for *which* directories are trailing — `has_trailing` only says whether the walk is worth +/// starting. That costs a re-read of the directory bodies, so it runs **only** when there is +/// something to report: a well-formed EXIF blob has one or two directories and never reaches it, +/// leaving the lazy read bound untouched. +/// +/// A source that dies between the two walks turns a would-be success into an error. That is the +/// same rule the rest of this module follows — a transport failure is propagated, never swallowed — +/// and swallowing it only here would be the inconsistency. fn record_trailing_ifds( reader: &mut IfdReader, - trailing: usize, + has_trailing: bool, report: &mut ReadReport, ) -> Result<()> { - if trailing == 0 { + if !has_trailing { return Ok(()); } - let mut offsets = Vec::with_capacity(trailing); + let mut offsets = Vec::new(); for raw in reader.ifds().skip(2) { offsets.push(raw?.offset); } @@ -294,10 +305,27 @@ fn read_range( /// The absolute offset of the Exif sub-IFD's out-of-line `MakerNote` value in the TIFF stream, or /// `None` if the note is absent or inline. -fn maker_note_offset(reader: &mut IfdReader, exif_ifd_at: u64) -> Option { - let raw: RawIfd = reader.read_ifd(exif_ifd_at).ok()?; - let entry = raw.entry(ifd_tags::MAKER_NOTE)?; - reader.value_offset(entry) +/// +/// A transport failure here is propagated rather than folded into `None`. The offset is what +/// [`ExifWriter`](crate::ExifWriter) uses to *pin* the note in place on a rewrite, so losing it +/// silently re-emits a vendor MakerNote unpinned — wrong bytes, no error, and nothing in the +/// report. No later read is guaranteed to resurface the failure either: `follow` returns before +/// reading at all when the GPS and Interop pointers are absent, which is the common case. +fn maker_note_offset( + reader: &mut IfdReader, + exif_ifd_at: u64, +) -> Result> { + let raw: RawIfd = match reader.read_ifd(exif_ifd_at) { + Ok(raw) => raw, + Err(e) if e.kind() != ErrorKind::InvalidInput => return Err(e.into()), + // The directory parsed a moment ago in `follow`; if the bytes will not re-read now, the + // pin is simply unavailable, and that is not worth failing an otherwise good parse over. + Err(_) => return Ok(None), + }; + let Some(entry) = raw.entry(ifd_tags::MAKER_NOTE) else { + return Ok(None); + }; + Ok(reader.value_offset(entry)) } #[cfg(test)] @@ -394,8 +422,8 @@ mod tests { } } - /// A structurally perfect blob with both 0th-IFD sub-directories, so every drop path is - /// reachable and none of them *should* fire. + /// A structurally perfect blob with both 0th-IFD sub-directories and a nested Interop, so + /// every sub-IFD drop path is reachable and none of them *should* fire. fn healthy_blob() -> Vec { let mut interop = Ifd::new(); interop.set(0x0001, Value::Ascii("R98".into())); @@ -419,45 +447,120 @@ mod tests { out } + /// A structurally perfect blob that reaches the **maker-note** read site. + /// + /// `healthy_blob` cannot: it carries no `MakerNote`, and its Interop pointer means a failure + /// inside `maker_note_offset` is always resurfaced by the later Interop read. Here the GPS and + /// Interop pointers are both absent — so `follow` returns without reading at all — and the + /// out-of-line `MakerNote` makes the pin's offset something a caller can lose. + fn maker_note_blob() -> Vec { + let mut exif = Ifd::new(); + exif.set(0x829A, Value::Rational(vec![(1, 250)])); // ExposureTime + // Nine bytes: too wide to sit inline in the entry, so it has a real source offset. + exif.set( + ifd_tags::MAKER_NOTE, + Value::Undefined(b"Canon\0\0\0\0".to_vec()), + ); + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); + image.set_sub_ifd(EXIF_IFD_POINTER, vec![exif]); + let tiff = write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds: vec![image], + }) + .expect("write"); + let mut out = MARKER.to_vec(); + out.extend(tiff); + out + } + + /// The maker-note pin is real in the fixture the sweep uses, so losing it is observable. + /// + /// Without this the sweep below could pass against a blob that never had a pin to lose. + #[test] + fn the_maker_note_fixture_has_a_pin_to_lose() { + let exif = ExifReader::new() + .parse_from(&maker_note_blob()[..]) + .expect("parse"); + assert!( + exif.maker_note_offset().is_some(), + "the fixture must pin an out-of-line MakerNote" + ); + assert!( + exif.gps_ifd().is_none(), + "no GPS pointer to rescue a failure" + ); + assert!( + exif.interop_ifd().is_none(), + "no Interop pointer to rescue a failure" + ); + } + /// A failing source is propagated, never reported as a malformed file. /// /// Leniency exists to tolerate corrupt *bytes*. If the transport fails instead, the data may be /// perfect, so silently returning `Ok` with the sub-IFDs missing — and a report blaming the /// file — would be a lie, and the worst case is the network-backed source this entry point - /// exists to enable. The whole parse is swept one read at a time, so every read site is - /// covered: the marker probe, the header, each directory body and each out-of-line value. + /// exists to enable. The whole parse is swept one read at a time over two fixtures, between + /// them reaching every read site: the marker probe, the header, each directory body, each + /// out-of-line value, and the maker-note pin — which only `maker_note_blob` reaches. + /// + /// A silent loss shows up here as `Ok` from a source that failed, and the assertions cover both + /// shapes it can take: a spurious report entry blaming the file, or — as the maker-note pin did + /// — an `Exif` quietly missing something with nothing in the report at all. #[test] fn a_failing_source_is_propagated_not_reported_as_a_malformed_file() { - let data = healthy_blob(); - let (mut failures, mut successes) = (0, 0); - for budget in 0..40 { - let source = FailingAfter { - inner: &data[..], - budget, - }; - match ExifReader::new().parse_from_with_report(source) { - Ok((_, report)) => { - successes += 1; - assert!( - report.is_empty(), - "budget {budget}: a transport failure was blamed on the file: {:?}", - report.dropped() - ); - } - Err(ExifError::Ifd(e)) => { - failures += 1; - assert_eq!( - e.kind(), - ErrorKind::Io, - "budget {budget}: a transport failure must keep its kind" - ); + for (name, data) in [ + ("healthy", healthy_blob()), + ("maker-note", maker_note_blob()), + ] { + let clean = ExifReader::new() + .parse_from(&data[..]) + .expect("clean parse"); + let (mut failures, mut successes) = (0, 0); + for budget in 0..40 { + let source = FailingAfter { + inner: &data[..], + budget, + }; + match ExifReader::new().parse_from_with_report(source) { + Ok((exif, report)) => { + successes += 1; + assert!( + report.is_empty(), + "{name} budget {budget}: a transport failure was blamed on the file: \ + {:?}", + report.dropped() + ); + // An `Ok` from a failing source must be the *whole* answer, not a quietly + // diminished one: the maker-note pin went missing exactly this way. + assert_eq!( + exif.maker_note_offset(), + clean.maker_note_offset(), + "{name} budget {budget}: the maker-note pin was silently lost" + ); + } + Err(ExifError::Ifd(e)) => { + failures += 1; + assert_eq!( + e.kind(), + ErrorKind::Io, + "{name} budget {budget}: a transport failure must keep its kind" + ); + } + // `MissingMarker` here would mean the marker probe swallowed the error and + // decided the blob was unmarked; anything else is equally a misdiagnosis. + Err(other) => { + panic!("{name} budget {budget}: transport failure became {other:?}") + } } - // `MissingMarker` here would mean the marker probe swallowed the error and - // decided the blob was unmarked; anything else is equally a misdiagnosis. - Err(other) => panic!("budget {budget}: transport failure became {other:?}"), } + assert!( + failures > 0 && successes > 0, + "{name}: the sweep proved nothing" + ); } - assert!(failures > 0 && successes > 0, "the sweep proved nothing"); } /// A transport failure while probing for the marker is not a *missing* marker. diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index 992c9ce1..6b1b9025 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -73,7 +73,11 @@ fn a_dangling_sub_ifd_pointer_is_named_with_its_tag_offset_and_reason() { ); let dropped = report.dropped()[0]; assert_eq!(dropped.region(), region); - assert_eq!(dropped.tag(), tag, "named by the tag that addressed it"); + assert_eq!( + dropped.tag(), + Some(tag), + "named by the tag that addressed it" + ); assert_eq!(dropped.offset(), u64::from(DANGLING)); assert_eq!(dropped.reason(), DropReason::OutOfBounds); } @@ -123,13 +127,17 @@ fn an_out_of_bounds_thumbnail_range_is_named() { assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); let dropped = report.dropped()[0]; assert_eq!(dropped.region(), DroppedRegion::ThumbnailJpeg); - assert_eq!(dropped.tag(), THUMB_OFFSET); + assert_eq!(dropped.tag(), Some(THUMB_OFFSET)); assert_eq!(dropped.offset(), u64::from(DANGLING)); assert_eq!(dropped.reason(), DropReason::OutOfBounds); } -/// A blob that parses in full reports nothing: `is_empty` is the "this parse lost nothing" verdict, -/// so a report that named a region on a healthy file would make it useless. +/// A blob that parses in full reports nothing. +/// +/// `is_empty` is the verdict over the regions the report *covers* — deliberately not "this parse +/// lost nothing", which is a stronger claim this crate cannot make (see the `report` module). What +/// it must still guarantee is the direction tested here: a healthy file names nothing, or the +/// signal would be noise. #[test] fn a_well_formed_blob_reports_no_drops() { let mut interop = Ifd::new(); @@ -205,7 +213,11 @@ fn a_top_level_directory_past_the_thumbnail_is_named() { ); for (dropped, expected) in report.dropped().iter().zip(&offsets[2..]) { assert_eq!(dropped.region(), DroppedRegion::TrailingIfd); - assert_eq!(dropped.tag(), 0, "no tag addresses a top-level directory"); + assert_eq!( + dropped.tag(), + None, + "no tag addresses a top-level directory" + ); assert_eq!(dropped.offset(), *expected, "named at its own position"); assert_eq!(dropped.reason(), DropReason::Unrepresentable); } @@ -258,7 +270,7 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { if raw.entry(tag).is_none() { continue; } - let named = report.dropped().iter().any(|d| d.tag() == tag); + let named = report.dropped().iter().any(|d| d.tag() == Some(tag)); assert_ne!( followed, named, "at truncation {end}, tag {tag:#06x} was followed={followed} and named={named}" diff --git a/crates/gamut-exif/tests/streaming.rs b/crates/gamut-exif/tests/streaming.rs index 88a19234..d42e3851 100644 --- a/crates/gamut-exif/tests/streaming.rs +++ b/crates/gamut-exif/tests/streaming.rs @@ -118,10 +118,13 @@ fn extracting_exif_from_a_large_file_never_reads_the_payload() { assert!(exif.thumbnail().is_some(), "the 1st IFD was read"); // 251 bytes today: the marker, the header, five directory bodies and their out-of-line - // values. 512 leaves room for a tag or two without letting a megabyte through — four - // megabytes is the failure mode a slurping reader would show. + // values. Four megabytes is the failure mode a slurping reader would show — but the bound is + // tighter than "not the payload" on purpose. A reader that re-walks the next-IFD chain when it + // has no trailing directory to report costs 347 bytes here, so 300 is the value that makes + // that a *failure* rather than an invisible inefficiency; it still leaves ~20% headroom for a + // tag or two. assert!( - counting.bytes_read <= 512, + counting.bytes_read <= 300, "streaming parse read {} bytes of a {FILE_LEN}-byte file", counting.bytes_read ); From 8c4d3d107e191673271e50c8209d7f49d2e50d4a Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Wed, 9 Sep 2026 19:52:45 -0400 Subject: [PATCH 05/15] refactor(exif): drop an unreachable arm from the maker-note pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mise run mutants-diff` left one survivor: replacing `e.kind() != ErrorKind::InvalidInput` with `true` in `maker_note_offset` changed nothing, because the lenient arm behind it is unreachable. That guard was copied from the three sites where it is load-bearing, but this one is different: it runs only after `follow` has already read *and* decoded this exact directory at this exact offset, so a deterministic source cannot fail here for a reason the bytes explain. The malformed-input arm could never be taken, which is why no test could kill the mutant. Propagate every error with `?` instead. The transport-failure behaviour NEW-1 asked for is unchanged — that is what the sweep pins — and an unfalsifiable branch is removed rather than papered over with an exclusion or a test for a source that contradicts itself. Refs #419 --- crates/gamut-exif/src/stream.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index cbf44f27..775351e1 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -306,7 +306,7 @@ fn read_range( /// The absolute offset of the Exif sub-IFD's out-of-line `MakerNote` value in the TIFF stream, or /// `None` if the note is absent or inline. /// -/// A transport failure here is propagated rather than folded into `None`. The offset is what +/// A failure here is propagated rather than folded into `None`. The offset is what /// [`ExifWriter`](crate::ExifWriter) uses to *pin* the note in place on a rewrite, so losing it /// silently re-emits a vendor MakerNote unpinned — wrong bytes, no error, and nothing in the /// report. No later read is guaranteed to resurface the failure either: `follow` returns before @@ -315,13 +315,11 @@ fn maker_note_offset( reader: &mut IfdReader, exif_ifd_at: u64, ) -> Result> { - let raw: RawIfd = match reader.read_ifd(exif_ifd_at) { - Ok(raw) => raw, - Err(e) if e.kind() != ErrorKind::InvalidInput => return Err(e.into()), - // The directory parsed a moment ago in `follow`; if the bytes will not re-read now, the - // pin is simply unavailable, and that is not worth failing an otherwise good parse over. - Err(_) => return Ok(None), - }; + // Every error propagates, with no lenient arm — deliberately. This is reached only when + // `follow` has already read and decoded this exact directory at this exact offset, so a + // deterministic source cannot fail here for a reason the *bytes* explain. A malformed-input + // arm would therefore be unreachable, and an unreachable arm is a branch no test can falsify. + let raw: RawIfd = reader.read_ifd(exif_ifd_at)?; let Some(entry) = raw.entry(ifd_tags::MAKER_NOTE) else { return Ok(None); }; From 3242ea1b091c44c7cf604192c8ffe23cb5bad867 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 00:32:34 -0400 Subject: [PATCH 06/15] fix(exif): name a thumbnail offset that carries no length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 1st IFD carrying `JPEGInterchangeFormat` without `JPEGInterchangeFormatLength` fell into `read_thumbnail`'s catch-all `None` arm: no bytes, no error and no report entry, in either mode. That is a silent loss inside the exact region `ReadReport` claims completeness over — an address with nothing to size the read by, so the JPEG behind it is gone with no trace that it was ever addressed. Exif 3.0 §4.6.9.2 Table 21 marks both tags mandatory for a compressed thumbnail, so half the pair is a malformed range rather than an absent thumbnail. Lenient mode now names it with the new `DropReason::Incomplete` at the offset the tag carried; strict mode rejects it, as it already did for an out-of-bounds range. A length with no offset addresses nothing at all, so it stays silent. `DropReason` is `#[non_exhaustive]` with append-only discriminants, so the added reason is not a breaking change. Over a 3144-case truncation-and-corruption sweep against the previous release the strict rejection changes 12 cases, every one of them `strict(true)` with a byte flip inside the `JPEGInterchangeFormatLength` entry header; lenient mode is unchanged. --- crates/gamut-exif/src/reader.rs | 36 ++++++++++++++++++ crates/gamut-exif/src/report.rs | 17 +++++++++ crates/gamut-exif/src/stream.rs | 24 ++++++++++-- crates/gamut-exif/tests/report.rs | 62 +++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 3 deletions(-) diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 338b8faa..7724a432 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -257,6 +257,42 @@ mod tests { ); } + /// A thumbnail offset with no length is a malformed pair, and strict mode says so. + /// + /// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` + /// both mandatory for a compressed thumbnail. Half the pair therefore fails strictness for the + /// same reason an out-of-bounds range does — the sibling case above — rather than passing as a + /// thumbnail that simply has no bytes. The lenient half of the contract is the report, pinned in + /// `tests/report.rs`. + #[test] + fn a_thumbnail_offset_without_a_length_is_rejected_strictly() { + let mut image = Ifd::new(); + image.set(0x010F, Value::Ascii("Canon".into())); + let mut thumb = Ifd::new(); + thumb.set(ExifTag::Compression.tag_id(), Value::Short(vec![6])); + thumb.set( + ExifTag::JpegInterchangeFormat.tag_id(), + Value::Long(vec![4]), + ); + // ...and deliberately no JpegInterchangeFormatLength. + let bytes = write(&TiffFile { + order: ByteOrder::LittleEndian, + variant: Variant::Classic, + ifds: vec![image, thumb], + }) + .expect("write"); + + let err = ExifReader::new() + .strict(true) + .parse(&bytes) + .expect_err("strict must reject half a thumbnail pair"); + assert_eq!( + err.to_string(), + "invalid thumbnail: JPEGInterchangeFormat without JPEGInterchangeFormatLength", + "the message must name which half is missing" + ); + } + #[test] fn lenient_drops_a_dangling_sub_ifd_pointer_that_strict_rejects() { // An ExifIFD pointer that addresses far past the end of the stream. diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index 385c58e5..e6fe9fa5 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -56,6 +56,11 @@ pub enum DroppedRegion { /// The 1st IFD's embedded JPEG thumbnail bytes, addressed by `JPEGInterchangeFormat` /// (`0x0201`) and sized by `JPEGInterchangeFormatLength` (`0x0202`). The thumbnail's own /// directory survives; only its bytes are lost. + /// + /// Reported when the range lies outside the blob ([`DropReason::OutOfBounds`]) and when the + /// offset has no length beside it ([`DropReason::Incomplete`]) — Exif 3.0 §4.6.9.2 Table 21 + /// marks both tags mandatory for a compressed thumbnail, so half the pair addresses bytes + /// nothing can size. ThumbnailJpeg = 3, /// A top-level directory past the 1st IFD. /// @@ -118,6 +123,13 @@ pub enum DropReason { /// Nothing was wrong with the region — it parsed cleanly — but the EXIF model has no place to /// put it, so it could not be carried across. Unrepresentable = 2, + /// The region was addressed but never fully described, so there was no range to read: today + /// only a `JPEGInterchangeFormat` offset with no `JPEGInterchangeFormatLength` beside it. + /// + /// Distinct from [`OutOfBounds`](Self::OutOfBounds) — the address may be perfectly valid — and + /// from [`Malformed`](Self::Malformed), which is about bytes that *were* read and did not + /// parse. The repair is different in each case, which is why they are different reasons. + Incomplete = 3, } impl DropReason { @@ -127,6 +139,7 @@ impl DropReason { Self::OutOfBounds => "addresses bytes outside the EXIF blob", Self::Malformed => "is not a well-formed directory", Self::Unrepresentable => "parsed cleanly but has no place in the EXIF model", + Self::Incomplete => "is addressed but never fully described", } } } @@ -300,6 +313,10 @@ mod tests { Dropped::new(DroppedRegion::ThumbnailJpeg, 1, DropReason::OutOfBounds).to_string(), "dropped Thumbnail (tag 0x0201) at offset 1: addresses bytes outside the EXIF blob" ); + assert_eq!( + Dropped::new(DroppedRegion::ThumbnailJpeg, 42, DropReason::Incomplete).to_string(), + "dropped Thumbnail (tag 0x0201) at offset 42: is addressed but never fully described" + ); } /// A region no tag addresses says so explicitly, in the same grammar as every other drop — diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 775351e1..83e1d28c 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -197,8 +197,13 @@ impl ExifReader { /// Builds a [`Thumbnail`] from the 1st IFD, fetching its JPEG bytes (from the /// `JPEGInterchangeFormat` offset / length) when the range is wholly inside the stream. In - /// lenient mode an out-of-bounds range yields a thumbnail without bytes and a recorded drop; - /// in strict mode it errors. + /// lenient mode an unusable range yields a thumbnail without bytes and a recorded drop; in + /// strict mode it errors. + /// + /// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` + /// *both* mandatory for a compressed thumbnail, so an offset without a length is a malformed + /// pair, not an absent thumbnail: it addresses bytes nothing can size. A length without an + /// offset addresses nothing at all, so nothing was dropped and nothing is reported. fn read_thumbnail( &self, ifd: Ifd, @@ -223,7 +228,20 @@ impl ExifReader { None } }, - _ => None, + (Some(_), None) if self.strict => { + return Err(ExifError::BadThumbnail( + "JPEGInterchangeFormat without JPEGInterchangeFormatLength", + )); + } + (Some(offset), None) => { + report.record(Dropped::new( + DroppedRegion::ThumbnailJpeg, + u64::from(offset), + DropReason::Incomplete, + )); + None + } + (None, _) => None, }; // The JPEGInterchangeFormat offset is structural — the bytes are captured above and the // writer re-synthesises the offset — so drop it from the stored directory (mirroring how the diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index 6b1b9025..3f1ef4ad 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -288,3 +288,65 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { "no truncation dropped a sub-IFD — the sweep proved nothing" ); } +/// A thumbnail offset with no length beside it is named rather than silently ignored. +/// +/// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` both +/// mandatory for a compressed thumbnail, so half the pair is not "no thumbnail" — it is an address +/// with nothing to size the read by, and the JPEG behind it is lost. Before this the pair fell into +/// the reader's catch-all `None` arm: no bytes, no error, no report entry, inside the very region +/// this report claims completeness over. +#[test] +fn a_thumbnail_offset_without_a_length_is_named() { + let mut thumb = Ifd::new(); + thumb.set(0x0103, Value::Short(vec![6])); // Compression = JPEG + thumb.set(THUMB_OFFSET, Value::Long(vec![4])); // ...in bounds, so not OutOfBounds + // ...and deliberately no THUMB_LENGTH. + + let (exif, report) = ExifReader::new() + .parse_with_report(&tiff(vec![image_ifd(), thumb])) + .expect("lenient parse"); + + assert_eq!( + exif.thumbnail().and_then(|t| t.jpeg()), + None, + "there is no length, so there are no bytes" + ); + assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); + let dropped = report.dropped()[0]; + assert_eq!(dropped.region(), DroppedRegion::ThumbnailJpeg); + assert_eq!(dropped.tag(), Some(THUMB_OFFSET)); + assert_eq!(dropped.offset(), 4, "named at the offset the tag carried"); + assert_eq!( + dropped.reason(), + DropReason::Incomplete, + "not OutOfBounds — the address is inside the blob; the length is what is missing" + ); +} + +/// A thumbnail with neither JPEG tag is an uncompressed thumbnail, not a loss, and reports nothing. +/// +/// The other direction of the pair: a `JPEGInterchangeFormatLength` on its own addresses no bytes +/// at all, so there is nothing to name. Without this, reporting the incomplete pair could be +/// "fixed" by reporting every thumbnail that has no JPEG, which would make the signal noise. +#[test] +fn a_thumbnail_with_no_jpeg_range_reports_nothing() { + for extra in [None, Some((THUMB_LENGTH, 16))] { + let mut thumb = Ifd::new(); + thumb.set(0x0103, Value::Short(vec![1])); // Compression = uncompressed + if let Some((tag, value)) = extra { + thumb.set(tag, Value::Long(vec![value])); + } + let (exif, report) = ExifReader::new() + .parse_with_report(&tiff(vec![image_ifd(), thumb])) + .expect("lenient parse"); + assert!( + exif.thumbnail().is_some(), + "the 1st IFD is still a thumbnail" + ); + assert!( + report.is_empty(), + "nothing was addressed, so nothing was dropped: {:?}", + report.dropped() + ); + } +} From 44594fa1e261bb1ef39941770bc6e5488a39cd70 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 00:32:51 -0400 Subject: [PATCH 07/15] test(exif): sweep the thumbnail and trailing-chain reads for a failing source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The failing-source sweep claimed to reach "every read site", but neither fixture had a thumbnail or a trailing directory, so `read_range`'s fetch and `record_trailing_ifds`' chain re-walk were never swept — the two read sites added most recently, and the ones a transport failure would reach last. `maker_note_blob` becomes `deep_blob`: it keeps the out-of-line `MakerNote` and the absent GPS/Interop pointers that make a lost pin observable, and adds an in-bounds thumbnail range and a third top-level directory. The payload's position is not knowable before `write` lays the directories out, so the fixture patches a sentinel `JPEGInterchangeFormat` value and asserts the sentinel names exactly one value field. A trailing directory is a legitimate drop that a clean parse reports too, so the law can no longer be "the report is empty". It is now the stronger claim that an `Ok` from a failing source equals the clean parse in report, maker-note pin and thumbnail bytes — which still catches a transport failure blamed on the file, and also catches a quietly diminished answer. Over 40 budgets the deep fixture splits 17 `Ok` / 23 `Err`, every error keeping `ErrorKind::Io`. The one `ReadAt` method left unswept is `len`, which answers a length rather than reading bytes; the doc comment now says so instead of claiming every site. --- crates/gamut-exif/src/stream.rs | 122 ++++++++++++++++++++++++-------- 1 file changed, 91 insertions(+), 31 deletions(-) diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 83e1d28c..1bfa0b49 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -463,13 +463,22 @@ mod tests { out } - /// A structurally perfect blob that reaches the **maker-note** read site. + /// The bytes `deep_blob`'s thumbnail range addresses, appended past the directories. + const THUMB_BYTES: &[u8] = b"\xFF\xD8__jpg\xFF\xD9"; + /// A `JPEGInterchangeFormat` value patched to the real offset once the layout is known — + /// `write` lays the directories out, so the payload's position is not knowable before it runs. + const THUMB_SENTINEL: u32 = 0xDEAD_BEEF; + + /// A structurally perfect blob reaching every read site `healthy_blob` cannot. /// - /// `healthy_blob` cannot: it carries no `MakerNote`, and its Interop pointer means a failure - /// inside `maker_note_offset` is always resurfaced by the later Interop read. Here the GPS and - /// Interop pointers are both absent — so `follow` returns without reading at all — and the - /// out-of-line `MakerNote` makes the pin's offset something a caller can lose. - fn maker_note_blob() -> Vec { + /// `healthy_blob` has no `MakerNote`, no thumbnail bytes and no trailing directory, and its + /// Interop pointer means a failure inside `maker_note_offset` is always resurfaced by the later + /// Interop read. Here the GPS and Interop pointers are both absent — so `follow` returns without + /// reading at all — while three additions each open one otherwise-unswept read: an out-of-line + /// `MakerNote` gives the pin an offset a caller can lose, an in-bounds thumbnail range makes + /// `read_range` fetch, and a third top-level directory makes `record_trailing_ifds` re-walk the + /// chain. + fn deep_blob() -> Vec { let mut exif = Ifd::new(); exif.set(0x829A, Value::Rational(vec![(1, 250)])); // ExposureTime // Nine bytes: too wide to sit inline in the entry, so it has a real source offset. @@ -480,29 +489,70 @@ mod tests { let mut image = Ifd::new(); image.set(0x010F, Value::Ascii("Canon".into())); image.set_sub_ifd(EXIF_IFD_POINTER, vec![exif]); - let tiff = write(&TiffFile { + + let mut thumb = Ifd::new(); + thumb.set(ExifTag::Compression.tag_id(), Value::Short(vec![6])); + thumb.set( + ExifTag::JpegInterchangeFormat.tag_id(), + Value::Long(vec![THUMB_SENTINEL]), + ); + thumb.set( + ExifTag::JpegInterchangeFormatLength.tag_id(), + Value::Long(vec![THUMB_BYTES.len() as u32]), + ); + + let mut trailing = Ifd::new(); + trailing.set(0x0131, Value::Ascii("trailing".into())); // Software + + let mut tiff = write(&TiffFile { order: ByteOrder::LittleEndian, variant: Variant::Classic, - ifds: vec![image], + ifds: vec![image, thumb, trailing], }) .expect("write"); + + // The payload goes after the directories, so the sentinel is patched to where it lands. + let at = u32::try_from(tiff.len()).expect("the fixture fits in 32 bits"); + let sentinel = THUMB_SENTINEL.to_le_bytes(); + let hits: Vec = tiff + .windows(4) + .enumerate() + .filter(|(_, w)| *w == sentinel) + .map(|(i, _)| i) + .collect(); + assert_eq!( + hits.len(), + 1, + "the sentinel must name exactly one value field" + ); + tiff[hits[0]..hits[0] + 4].copy_from_slice(&at.to_le_bytes()); + tiff.extend_from_slice(THUMB_BYTES); + let mut out = MARKER.to_vec(); out.extend(tiff); out } - /// The maker-note pin is real in the fixture the sweep uses, so losing it is observable. + /// The deep fixture really carries every loss the sweep below claims to watch for. /// - /// Without this the sweep below could pass against a blob that never had a pin to lose. + /// Without this the sweep could pass against a blob that never had a pin, thumbnail bytes or a + /// trailing directory to lose — and against a clean report that already blamed the file, which + /// would make the sweep's report-equality law vacuous. #[test] - fn the_maker_note_fixture_has_a_pin_to_lose() { - let exif = ExifReader::new() - .parse_from(&maker_note_blob()[..]) + fn the_deep_fixture_has_a_pin_a_thumbnail_and_a_trailing_directory_to_lose() { + let data = deep_blob(); + let (exif, report) = ExifReader::new() + .parse_from_with_report(&data[..]) .expect("parse"); assert!( exif.maker_note_offset().is_some(), "the fixture must pin an out-of-line MakerNote" ); + assert_eq!( + exif.thumbnail().and_then(Thumbnail::jpeg), + Some(THUMB_BYTES), + "the fixture must have thumbnail bytes that were really fetched" + ); assert!( exif.gps_ifd().is_none(), "no GPS pointer to rescue a failure" @@ -511,6 +561,13 @@ mod tests { exif.interop_ifd().is_none(), "no Interop pointer to rescue a failure" ); + assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); + assert_eq!(report.dropped()[0].region(), DroppedRegion::TrailingIfd); + assert_eq!( + report.dropped()[0].reason(), + DropReason::Unrepresentable, + "the clean report must blame nothing on the file" + ); } /// A failing source is propagated, never reported as a malformed file. @@ -519,20 +576,22 @@ mod tests { /// perfect, so silently returning `Ok` with the sub-IFDs missing — and a report blaming the /// file — would be a lie, and the worst case is the network-backed source this entry point /// exists to enable. The whole parse is swept one read at a time over two fixtures, between - /// them reaching every read site: the marker probe, the header, each directory body, each - /// out-of-line value, and the maker-note pin — which only `maker_note_blob` reaches. + /// them reaching every site that reads bytes: the marker probe, the header, each directory + /// body, each out-of-line value, the three sub-IFD reads (`healthy_blob`), and the thumbnail + /// fetch, the trailing-chain re-walk and the maker-note pin (`deep_blob`). The one `ReadAt` + /// method left unswept is `len`, which answers a length rather than reading bytes. /// - /// A silent loss shows up here as `Ok` from a source that failed, and the assertions cover both - /// shapes it can take: a spurious report entry blaming the file, or — as the maker-note pin did - /// — an `Exif` quietly missing something with nothing in the report at all. + /// The law is that an `Ok` from a failing source is the **whole** answer — equal to the clean + /// parse in report, pin and thumbnail bytes — which catches both shapes a silent loss takes: a + /// spurious report entry blaming the file, or, as the maker-note pin did, an `Exif` quietly + /// missing something with nothing in the report at all. Equality against the clean report + /// rather than emptiness is what lets `deep_blob` be swept at all: its trailing directory is a + /// legitimate drop that a clean parse reports too. #[test] fn a_failing_source_is_propagated_not_reported_as_a_malformed_file() { - for (name, data) in [ - ("healthy", healthy_blob()), - ("maker-note", maker_note_blob()), - ] { - let clean = ExifReader::new() - .parse_from(&data[..]) + for (name, data) in [("healthy", healthy_blob()), ("deep", deep_blob())] { + let (clean, clean_report) = ExifReader::new() + .parse_from_with_report(&data[..]) .expect("clean parse"); let (mut failures, mut successes) = (0, 0); for budget in 0..40 { @@ -543,19 +602,20 @@ mod tests { match ExifReader::new().parse_from_with_report(source) { Ok((exif, report)) => { successes += 1; - assert!( - report.is_empty(), - "{name} budget {budget}: a transport failure was blamed on the file: \ - {:?}", - report.dropped() + assert_eq!( + report, clean_report, + "{name} budget {budget}: a transport failure changed the report" ); - // An `Ok` from a failing source must be the *whole* answer, not a quietly - // diminished one: the maker-note pin went missing exactly this way. assert_eq!( exif.maker_note_offset(), clean.maker_note_offset(), "{name} budget {budget}: the maker-note pin was silently lost" ); + assert_eq!( + exif.thumbnail().and_then(Thumbnail::jpeg), + clean.thumbnail().and_then(Thumbnail::jpeg), + "{name} budget {budget}: the thumbnail bytes were silently lost" + ); } Err(ExifError::Ifd(e)) => { failures += 1; From ba0a139ffe9b58372dfd93849a38cf1d3728ac89 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 00:33:22 -0400 Subject: [PATCH 08/15] docs(exif): correct the strict report, the offset frames and two doc links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five documented claims did not match the code, and each is now stated once and pinned where it can be falsified. `ExifReader::parse_with_report` said "a strict report is always empty". It is not, and `ReadReport`'s own docs in the same crate say the opposite and are right: strictness rejects malformed regions, and a trailing directory is well-formed and merely unrepresentable, so it is reported in both modes. The stale sentence predates `TrailingIfd` and re-hid the very loss that variant exists to surface. A strict three-directory chain now pins one `TrailingIfd` entry. `parse`'s error offsets and `Dropped::offset` are in different frames, and the divergence was undocumented and unmeasured. Routing through `IfdReader::open(source.rebased(base))` attaches the physical offset, so a diagnostic that was TIFF-stream-relative became blob-relative. Both frames are kept and named: a diagnostic points into the buffer the caller handed in, while a report offset addresses the TIFF structure the report describes, so for a marked blob they differ by the six-byte marker. A 3144-case truncation-and-corruption sweep against the previous release finds 52 differing error strings, all marked blobs, all differing by exactly six, and none differing once normalised; every bare-blob offset and every re-serialised byte is identical. The claim of zero mismatches was therefore wrong, and so was "neither is a change for existing callers". Two comments still described `Dropped::tag` as a `0` sentinel, sitting directly above assertions of `None`; the README still showed the pre-`Display`-rewrite grammar in a block nothing compiles; and two rustdoc links broke when the `ExifError` import was narrowed and the private `stream` module stopped being published. `RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-exif --no-deps` now reports only the link that predates this branch. Finally, the claim that a malformed-input arm in `maker_note_offset` is unreachable holds only for a deterministic source. A `ReadAt` may answer differently on a second read — a file rewritten underneath the reader, which is what `parse_from` exists to enable — and the comment now says so, along with why a hard error is still the right answer there. --- crates/gamut-exif/README.md | 13 +++-- crates/gamut-exif/src/lib.rs | 11 +++- crates/gamut-exif/src/reader.rs | 20 +++++-- crates/gamut-exif/src/report.rs | 9 ++- crates/gamut-exif/src/stream.rs | 10 +++- crates/gamut-exif/tests/report.rs | 91 ++++++++++++++++++++++++++++++- 6 files changed, 138 insertions(+), 16 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index d559a80b..ab724db0 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -53,17 +53,20 @@ points: async caller drives the source itself, which keeps a runtime dependency out of the crate. - **`parse_with_report`** (and its `parse_from_with_report` twin) returns a `ReadReport` alongside the `Exif`, naming each region the lenient reader discarded — a malformed Exif/GPS/Interop - sub-IFD, an out-of-bounds thumbnail range, or a top-level directory past the 1st IFD — with the - tag that addressed it, the offset it carried, and a typed reason. `parse` stays silent, as - before. The report is complete over those regions but is **not** a byte-completeness verdict: an - empty report does not mean the parse lost nothing (see the deferred items below). + sub-IFD, an unusable thumbnail range, or a top-level directory past the 1st IFD — with the tag + that addressed it, the offset it carried, and a typed reason. `parse` stays silent, as before. + The report is complete over those regions but is **not** a byte-completeness verdict: an empty + report does not mean the parse lost nothing (see the deferred items below). A `strict` report is + not always empty either: strictness rejects *malformed* regions, and a trailing directory is + well-formed and merely unrepresentable, so it is reported in both modes. ```rust # use gamut_exif::ExifReader; # fn demo(bytes: &[u8]) -> Result<(), gamut_exif::ExifError> { let (exif, report) = ExifReader::new().parse_with_report(bytes)?; for dropped in report.dropped() { - eprintln!("{dropped}"); // e.g. "dropped GPS at tag 0x8825, offset 65535: ..." + // e.g. "dropped GPS (tag 0x8825) at offset 65535: addresses bytes outside the EXIF blob" + eprintln!("{dropped}"); } # let _ = exif; # Ok(()) diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index 836ffe3a..9cf56215 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -21,7 +21,16 @@ //! [`parse_with_report`](ExifReader::parse_with_report) returns a [`ReadReport`] naming the //! sub-IFDs, thumbnail ranges and trailing directories the lenient reader discarded — see //! [`report`] for what that covers and what it deliberately does not. `parse` is the `&[u8]` case -//! of `parse_from` and stays silent, so neither is a change for existing callers. +//! of `parse_from` and stays silent. +//! +//! `parse` keeps its signature, its accept/reject verdict and its re-serialised bytes, with two +//! narrow exceptions, both measured over a 3 144-case truncation-and-corruption sweep against the +//! previous release. An error message's offset is now a position in the buffer the caller handed +//! in, so for a marked blob it is six bytes larger than before — the `Exif\0\0` marker — while a +//! [`Dropped::offset`] stays relative to the TIFF stream; and a 1st IFD carrying +//! `JPEGInterchangeFormat` with no `JPEGInterchangeFormatLength`, which Exif 3.0 §4.6.9.2 requires +//! together, is now named in the report instead of vanishing, and rejected in +//! [`strict`](ExifReader::strict) mode as the malformed pair it is. //! //! ``` //! use gamut_exif::{ByteOrder, Exif, ExifTag, Value}; diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 7724a432..7ebe77a4 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -7,8 +7,9 @@ //! representing each sub-IFD structurally on [`Exif`] instead. //! //! This module holds the reader's options and its `&[u8]` entry points. The parse itself is -//! generic over [`gamut_ifd::ReadAt`] and lives in [`crate::stream`]; a slice is simply one such -//! source, so there is exactly **one** parse engine and the two entry points cannot drift. +//! generic over [`gamut_ifd::ReadAt`] and lives in the crate's private `stream` module; a slice is +//! simply one such source, so there is exactly **one** parse engine and the two entry points cannot +//! drift. use crate::error::Result; use crate::exif::Exif; @@ -32,7 +33,7 @@ impl ExifReader { } /// Requires the `Exif\0\0` marker; a bare TIFF stream is then rejected with - /// [`ExifError::MissingMarker`]. + /// [`ExifError::MissingMarker`](crate::ExifError::MissingMarker). /// /// Off by default: the JPEG `APP1` segment carries the marker, but the WebP `EXIF` and PNG /// `eXIf` chunks carry a bare TIFF stream. @@ -62,6 +63,12 @@ impl ExifReader { /// malformed, or (in [`strict`](Self::strict) mode) /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) when a sub-IFD pointer addresses a /// malformed directory. + /// + /// An offset inside an error message is a position in `bytes` — the buffer the caller handed + /// in — so for a marked blob it counts the six-byte `Exif\0\0` marker. That is deliberately a + /// different frame from [`Dropped::offset`](crate::Dropped::offset), which is relative to the + /// start of the TIFF stream and therefore six smaller for the same position: a diagnostic + /// points into the caller's own bytes, while a report offset addresses the TIFF structure. pub fn parse(&self, bytes: &[u8]) -> Result { self.parse_from(bytes) } @@ -91,8 +98,11 @@ impl ExifReader { /// /// # Errors /// - /// As [`parse`](Self::parse). In [`strict`](Self::strict) mode the first malformed region fails - /// the parse instead of being reported, so a strict report is always empty. + /// As [`parse`](Self::parse). In [`strict`](Self::strict) mode the first *malformed* region + /// fails the parse instead of being reported — but a strict report is **not** therefore always + /// empty: [`DroppedRegion::TrailingIfd`](crate::DroppedRegion::TrailingIfd) is well-formed and + /// merely unrepresentable, so strictness has no grounds to reject it and it is reported in both + /// modes. pub fn parse_with_report(&self, bytes: &[u8]) -> Result<(Exif, ReadReport)> { self.parse_from_with_report(bytes) } diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index e6fe9fa5..c4ce35b8 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -191,6 +191,13 @@ impl Dropped { /// For a sub-IFD or the thumbnail bytes this is the value the addressing tag carried; for a /// [`TrailingIfd`](DroppedRegion::TrailingIfd) it is the directory's own position in the /// stream. + /// + /// This is **not** the frame the crate's *error* messages use. An [`ExifError`](crate::ExifError) + /// carries the offset of the byte the reader could not read in the source the caller handed in, + /// so for a marked blob it is 6 bytes (`MARKER.len()`) larger than the same position expressed + /// here. The two frames are deliberately different: a diagnostic points into the caller's own + /// buffer, while a report offset addresses the TIFF structure the report describes and matches + /// every offset stored inside the file. #[must_use] pub const fn offset(self) -> u64 { self.offset @@ -275,7 +282,7 @@ mod tests { use super::*; /// Each region reports the tag that actually addresses it — the value a caller uses to find - /// the pointer back in the source directory — and a region no tag addresses reports `0`. + /// the pointer back in the source directory — and a region no tag addresses reports `None`. #[test] fn each_region_carries_the_tag_that_addresses_it() { for (region, tag) in [ diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 1bfa0b49..12b007dd 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -335,8 +335,14 @@ fn maker_note_offset( ) -> Result> { // Every error propagates, with no lenient arm — deliberately. This is reached only when // `follow` has already read and decoded this exact directory at this exact offset, so a - // deterministic source cannot fail here for a reason the *bytes* explain. A malformed-input - // arm would therefore be unreachable, and an unreachable arm is a branch no test can falsify. + // *deterministic* source cannot fail here for a reason the bytes explain: over such a source a + // malformed-input arm is unreachable, and an unreachable arm is a branch no test can falsify. + // + // It is not unreachable in general. A `ReadAt` may answer differently on a second read — a file + // rewritten underneath the reader is precisely the case `parse_from` exists to enable — and then + // the directory really can fail here. A hard error is still the right answer for it: the pin is + // what keeps a vendor MakerNote's TIFF-absolute internal offsets valid on a rewrite, so + // continuing would re-emit the note unpinned, with wrong bytes and nothing in the report. let raw: RawIfd = reader.read_ifd(exif_ifd_at)?; let Some(entry) = raw.entry(ifd_tags::MAKER_NOTE) else { return Ok(None); diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index 3f1ef4ad..bf560b50 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -6,7 +6,10 @@ //! indistinguishable. Each test below feeds one deliberately broken blob to //! [`ExifReader::parse_with_report`] and pins that the discarded region is named with the tag that //! addressed it, the offset it carried, and a reason that separates "nothing could be there" from -//! "something was there and it was corrupt". The last test generalises it over a truncation sweep. +//! "something was there and it was corrupt". Three further tests pin the contract's edges: that a +//! *strict* report is not empty of the one loss strictness has no grounds to reject, that the +//! report's offsets and the crate's error offsets are deliberately in different frames, and — over +//! a truncation sweep — that no sub-IFD is ever dropped without being named. use gamut_exif::{DropReason, DroppedRegion, ExifReader}; use gamut_ifd::{ByteOrder, Ifd, IfdReader, TiffFile, Value, Variant, write}; @@ -23,6 +26,8 @@ const THUMB_OFFSET: u16 = 0x0201; const THUMB_LENGTH: u16 = 0x0202; /// An offset far past the end of any fixture here. const DANGLING: u32 = 0xFFFF; +/// The `Exif\0\0` marker a JPEG `APP1` payload carries before the TIFF stream. +const MARKER: &[u8] = b"Exif\x00\x00"; /// Serialises `ifds` as a bare little-endian TIFF stream. fn tiff(ifds: Vec) -> Vec { @@ -171,7 +176,7 @@ fn a_well_formed_blob_reports_no_drops() { /// EXIF defines exactly two — the 0th (primary image) and the 1st (thumbnail) — so a longer /// next-IFD chain parses cleanly and then has nowhere to go in the model. That is a real loss (the /// bytes do not survive `to_bytes`), and it is the one drop with no addressing tag: the chain is -/// followed through the structural next-IFD pointer, so the reported tag is `0` and the reported +/// followed through the structural next-IFD pointer, so the reported tag is `None` and the reported /// offset is the directory's own position. #[test] fn a_top_level_directory_past_the_thumbnail_is_named() { @@ -288,6 +293,7 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { "no truncation dropped a sub-IFD — the sweep proved nothing" ); } + /// A thumbnail offset with no length beside it is named rather than silently ignored. /// /// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` both @@ -350,3 +356,84 @@ fn a_thumbnail_with_no_jpeg_range_reports_nothing() { ); } } + +/// A strict report is not always empty: it still carries the loss strictness cannot reject. +/// +/// Strictness rejects *malformed* regions. A top-level directory past the 1st IFD is not malformed +/// — it parses cleanly and the [`Exif`](gamut_exif::Exif) model simply has nowhere to put it — so +/// strict has no grounds to fail on it, and dropping it silently would re-hide exactly the loss +/// `DroppedRegion::TrailingIfd` exists to surface. +#[test] +fn a_strict_parse_still_reports_a_trailing_directory() { + let mut thumb = Ifd::new(); + thumb.set(0x0103, Value::Short(vec![6])); // Compression = JPEG + let mut trailing = Ifd::new(); + trailing.set(0x0131, Value::Ascii("trailing".into())); // Software + + let (exif, report) = ExifReader::new() + .strict(true) + .parse_with_report(&tiff(vec![image_ifd(), thumb, trailing])) + .expect("a well-formed long chain must not fail even in strict mode"); + + assert_eq!(exif.make(), Some("Canon"), "the 0th IFD survives"); + assert_eq!(report.dropped().len(), 1, "{:?}", report.dropped()); + assert_eq!(report.dropped()[0].region(), DroppedRegion::TrailingIfd); + assert_eq!(report.dropped()[0].reason(), DropReason::Unrepresentable); +} + +/// The report's offsets and the crate's error offsets are in different frames, by the marker. +/// +/// A `Dropped::offset` addresses the TIFF stream, so it matches every offset stored inside the file +/// and is unchanged by whether the caller's buffer carries the six-byte `Exif\0\0` marker. An error +/// message instead names a byte of the buffer that was handed in, so the marker shifts it. Pinning +/// the pair together is what stops either frame drifting onto the other: unifying them would aim a +/// diagnostic outside the caller's buffer or renumber every reported offset. +#[test] +fn report_offsets_ignore_the_marker_but_error_offsets_include_it() { + let mut image = image_ifd(); + image.set(GPS_INFO, Value::Long(vec![DANGLING])); + let bare = tiff(vec![image]); + let mut marked = MARKER.to_vec(); + marked.extend(&bare); + + // The report frame is marker-invariant. + let offsets = |blob: &[u8]| -> Vec { + let (_, report) = ExifReader::new().parse_with_report(blob).expect("parse"); + report.dropped().iter().map(|d| d.offset()).collect() + }; + assert_eq!(offsets(&bare), vec![u64::from(DANGLING)]); + assert_eq!( + offsets(&marked), + offsets(&bare), + "a report offset addresses the TIFF stream, not the caller's buffer" + ); + + // The diagnostic frame is marker-shifted. The same corruption is applied to the TIFF stream in + // both blobs, so any offset difference is the marker and nothing else. + let mut broken_bare = bare.clone(); + broken_bare[4] ^= 0xFF; // the first-IFD offset in the TIFF header + let mut broken_marked = MARKER.to_vec(); + broken_marked.extend(&broken_bare); + + let message = |blob: &[u8]| -> String { + ExifReader::new() + .parse(blob) + .expect_err("a corrupt first-IFD offset must fail") + .to_string() + }; + let (bare_msg, marked_msg) = (message(&broken_bare), message(&broken_marked)); + let at = |m: &str| -> u64 { + let tail = m + .rsplit_once("byte offset: ") + .expect("the diagnostic names a byte offset") + .1; + tail.trim_end_matches(']') + .parse() + .expect("the byte offset parses") + }; + assert_eq!( + at(&marked_msg) - at(&bare_msg), + MARKER.len() as u64, + "an error offset counts the marker: {marked_msg} vs {bare_msg}" + ); +} From d650f62ae6627ddf60becd6713c427127ee359a5 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:44:33 -0400 Subject: [PATCH 09/15] refactor(exif): name the thumbnail drop reason for its one site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DropReason::Incomplete` was reachable from exactly one place — a `JPEGInterchangeFormat` offset with no `JPEGInterchangeFormatLength` beside it — but its name described a shape ("addressed but never fully described") rather than that site. A generic name on a single-site variant attracts unrelated reuse, and the discriminant is append-only once released, so rename it to `ThumbnailLengthMissing` before it freezes. A future defect that is merely *similar* gets its own variant on the `#[non_exhaustive]` enum instead. The rendered clause moves with it: `has no JPEGInterchangeFormatLength to size the read` states what is missing, which the old wording left to the reader. The variant is new on this branch and has never been published, so no released API changes. --- crates/gamut-exif/src/report.rs | 24 +++++++++++++++++------- crates/gamut-exif/src/stream.rs | 2 +- crates/gamut-exif/tests/report.rs | 2 +- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index c4ce35b8..91d05c12 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -58,7 +58,7 @@ pub enum DroppedRegion { /// directory survives; only its bytes are lost. /// /// Reported when the range lies outside the blob ([`DropReason::OutOfBounds`]) and when the - /// offset has no length beside it ([`DropReason::Incomplete`]) — Exif 3.0 §4.6.9.2 Table 21 + /// offset has no length beside it ([`DropReason::ThumbnailLengthMissing`]) — Exif 3.0 §4.6.9.2 Table 21 /// marks both tags mandatory for a compressed thumbnail, so half the pair addresses bytes /// nothing can size. ThumbnailJpeg = 3, @@ -123,13 +123,17 @@ pub enum DropReason { /// Nothing was wrong with the region — it parsed cleanly — but the EXIF model has no place to /// put it, so it could not be carried across. Unrepresentable = 2, - /// The region was addressed but never fully described, so there was no range to read: today - /// only a `JPEGInterchangeFormat` offset with no `JPEGInterchangeFormatLength` beside it. + /// The 1st IFD carried a `JPEGInterchangeFormat` offset with no `JPEGInterchangeFormatLength` + /// beside it, so there was no range to read and the JPEG behind the offset is lost. + /// + /// Deliberately named for that one site rather than for the shape of the defect: it is the + /// only thing this reason ever means, and a generic name on a single-site variant invites + /// unrelated reuse that a `#[non_exhaustive]` enum can add a *new* variant for instead. /// /// Distinct from [`OutOfBounds`](Self::OutOfBounds) — the address may be perfectly valid — and /// from [`Malformed`](Self::Malformed), which is about bytes that *were* read and did not /// parse. The repair is different in each case, which is why they are different reasons. - Incomplete = 3, + ThumbnailLengthMissing = 3, } impl DropReason { @@ -139,7 +143,7 @@ impl DropReason { Self::OutOfBounds => "addresses bytes outside the EXIF blob", Self::Malformed => "is not a well-formed directory", Self::Unrepresentable => "parsed cleanly but has no place in the EXIF model", - Self::Incomplete => "is addressed but never fully described", + Self::ThumbnailLengthMissing => "has no JPEGInterchangeFormatLength to size the read", } } } @@ -321,8 +325,14 @@ mod tests { "dropped Thumbnail (tag 0x0201) at offset 1: addresses bytes outside the EXIF blob" ); assert_eq!( - Dropped::new(DroppedRegion::ThumbnailJpeg, 42, DropReason::Incomplete).to_string(), - "dropped Thumbnail (tag 0x0201) at offset 42: is addressed but never fully described" + Dropped::new( + DroppedRegion::ThumbnailJpeg, + 42, + DropReason::ThumbnailLengthMissing + ) + .to_string(), + "dropped Thumbnail (tag 0x0201) at offset 42: has no JPEGInterchangeFormatLength to \ + size the read" ); } diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 12b007dd..b5ed92c9 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -237,7 +237,7 @@ impl ExifReader { report.record(Dropped::new( DroppedRegion::ThumbnailJpeg, u64::from(offset), - DropReason::Incomplete, + DropReason::ThumbnailLengthMissing, )); None } diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index bf560b50..45e08594 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -324,7 +324,7 @@ fn a_thumbnail_offset_without_a_length_is_named() { assert_eq!(dropped.offset(), 4, "named at the offset the tag carried"); assert_eq!( dropped.reason(), - DropReason::Incomplete, + DropReason::ThumbnailLengthMissing, "not OutOfBounds — the address is inside the blob; the length is what is missing" ); } From d3e2207399b16bb891aba90fc529b52b0ebbaeb2 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:44:56 -0400 Subject: [PATCH 10/15] fix(exif): name the unreadable range instead of a mandatory sibling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strict mode rejected a thumbnail offset with no length as `JPEGInterchangeFormat without JPEGInterchangeFormatLength`, which reads as "you failed to record a required tag". Exif 3.0 §4.6.9.2 Table 21 gives that pair's support level per `Compression` column: mandatory under **Compressed**, and `N` — not allowed to record — under all three uncompressed columns. So for an uncompressed thumbnail the message named a sibling the cited table forbids recording there. The rejection itself is unchanged and is not derived from the support level: an offset with nothing to size it addresses bytes that cannot be fetched, whatever `Compression` says. The message now states exactly that, so it asserts nothing the spec does not. Whether the rule should instead be conditioned on `Compression`, and whether a length with no offset should be rejected for symmetry, is a behavioural change with its own equivalence sweep to run. Refs #574 --- crates/gamut-exif/src/reader.rs | 4 ++-- crates/gamut-exif/src/stream.rs | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 7ebe77a4..8d103048 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -298,8 +298,8 @@ mod tests { .expect_err("strict must reject half a thumbnail pair"); assert_eq!( err.to_string(), - "invalid thumbnail: JPEGInterchangeFormat without JPEGInterchangeFormatLength", - "the message must name which half is missing" + "invalid thumbnail: JPEGInterchangeFormat offset with no length to size it", + "the message must name the unreadable range, not a missing mandatory tag" ); } diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index b5ed92c9..e4c1967a 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -229,8 +229,11 @@ impl ExifReader { } }, (Some(_), None) if self.strict => { + // States the structural fact — a range with no size — rather than naming a missing + // mandatory tag: Table 21 makes the sibling mandatory only under `Compression = + // Compressed`, and forbids recording it at all under the uncompressed columns. return Err(ExifError::BadThumbnail( - "JPEGInterchangeFormat without JPEGInterchangeFormatLength", + "JPEGInterchangeFormat offset with no length to size it", )); } (Some(offset), None) => { From 68a57fb2c15f4f978a4f6db8ba6d72b007f5f319 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:45:28 -0400 Subject: [PATCH 11/15] docs(exif): scope the Table 21 grounding and the streaming offset frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four sites cited Exif 3.0 §4.6.9.2 Table 21 as making the thumbnail pointer pair mandatory full stop. It does not: the table gives each 1st IFD tag's support level per `Compression` column, and both `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` are `M` only under **Compressed** — under all three uncompressed columns they are `N`, not allowed to record. The reader does not read `Compression`, so its rule cannot rest on that mandate. Each site now grounds the rule in the structure (an offset with nothing to size it addresses bytes that cannot be read) and states the table's conditioning as the open question it is. Two further notes the code did not carry: - `parse_from` and `parse_from_with_report` had no offset-frame note, though a streaming caller is the one most likely to correlate an error offset against a file. Both frames — error offsets counted in the caller's source, report offsets from the start of the TIFF stream — are now stated where they are produced, as they already were on the slice entry points. - The thumbnail pointer's removal is conditioned on bytes having been read, which is #548. That issue names only the out-of-bounds case; the missing-length arm is a second instance, and a sharper one, because the re-emitted blob still has an offset and still has no length, so a strict parse rejects a blob this crate itself wrote. Recorded beside the code. Refs #548, #574 --- crates/gamut-exif/src/reader.rs | 13 +++++++----- crates/gamut-exif/src/report.rs | 16 ++++++++++++--- crates/gamut-exif/src/stream.rs | 34 +++++++++++++++++++++++++++---- crates/gamut-exif/tests/report.rs | 16 ++++++++++----- 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 8d103048..dc850580 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -267,12 +267,15 @@ mod tests { ); } - /// A thumbnail offset with no length is a malformed pair, and strict mode says so. + /// A thumbnail offset with no length is an unreadable range, and strict mode says so. /// - /// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` - /// both mandatory for a compressed thumbnail. Half the pair therefore fails strictness for the - /// same reason an out-of-bounds range does — the sibling case above — rather than passing as a - /// thumbnail that simply has no bytes. The lenient half of the contract is the report, pinned in + /// A `JPEGInterchangeFormat` with nothing to size the read by addresses bytes that cannot be + /// fetched, so it fails strictness for the same reason an out-of-bounds range does — the + /// sibling case above — rather than passing as a thumbnail that simply has no bytes. The + /// message is pinned because it must state that structural fact and *not* claim a missing + /// mandatory tag: Exif 3.0 §4.6.9.2 Table 21 makes the pair mandatory only under + /// `Compression = Compressed`, and forbids recording either tag under the uncompressed + /// columns (issue #574). The lenient half of the contract is the report, pinned in /// `tests/report.rs`. #[test] fn a_thumbnail_offset_without_a_length_is_rejected_strictly() { diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index 91d05c12..a78b2f7c 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -58,9 +58,12 @@ pub enum DroppedRegion { /// directory survives; only its bytes are lost. /// /// Reported when the range lies outside the blob ([`DropReason::OutOfBounds`]) and when the - /// offset has no length beside it ([`DropReason::ThumbnailLengthMissing`]) — Exif 3.0 §4.6.9.2 Table 21 - /// marks both tags mandatory for a compressed thumbnail, so half the pair addresses bytes - /// nothing can size. + /// offset has no length beside it ([`DropReason::ThumbnailLengthMissing`]): an offset with + /// nothing to size it addresses bytes that cannot be read, which is a loss rather than an + /// absent thumbnail. That rule is structural and unconditional here — see + /// [`ThumbnailLengthMissing`](DropReason::ThumbnailLengthMissing) — and is *not* derived from + /// the pair's support level, which Exif 3.0 §4.6.9.2 Table 21 states only per `Compression` + /// column, a tag this crate does not read. ThumbnailJpeg = 3, /// A top-level directory past the 1st IFD. /// @@ -133,6 +136,13 @@ pub enum DropReason { /// Distinct from [`OutOfBounds`](Self::OutOfBounds) — the address may be perfectly valid — and /// from [`Malformed`](Self::Malformed), which is about bytes that *were* read and did not /// parse. The repair is different in each case, which is why they are different reasons. + /// + /// Recorded whatever the thumbnail's `Compression` says, because the reason it is a loss is + /// that the read has no length — not that a tag is missing where the spec requires one. Exif + /// 3.0 §4.6.9.2 Table 21 gives the pair's support level *per `Compression` column*: mandatory + /// under **Compressed**, and `N` (not allowed to record) under all three uncompressed columns. + /// Whether this crate should read `Compression` and condition the rule on it — and whether a + /// length with no offset should be rejected for symmetry — is open, and filed as issue #574. ThumbnailLengthMissing = 3, } diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index e4c1967a..84f5ef43 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -49,6 +49,14 @@ impl ExifReader { /// [`ExifError::Ifd`] when the TIFF stream is malformed or the source fails, or (in /// [`strict`](Self::strict) mode) [`ExifError::InvalidIfd`] / /// [`ExifError::BadThumbnail`] when a sub-IFD pointer or thumbnail range is unusable. + /// + /// An offset inside an error message is a position in `source` — the byte source the caller + /// handed in — so for a marked source it counts the six-byte `Exif\0\0` marker. That is + /// deliberately a different frame from [`Dropped::offset`](crate::Dropped::offset), which is + /// relative to the start of the TIFF stream and therefore six smaller for the same position. + /// It matters most here: a caller streaming from a file is the one likeliest to correlate an + /// error offset against bytes on disk, and it can do so directly only for the error frame — + /// a report offset must have the source's own start (and the marker) added back first. pub fn parse_from(&self, source: S) -> Result { self.parse_source(source, &mut ReadReport::new()) } @@ -57,6 +65,12 @@ impl ExifReader { /// /// The streaming twin of [`parse_with_report`](Self::parse_with_report). See [`ReadReport`]. /// + /// The two frames of [`parse_from`](Self::parse_from) meet here: every + /// [`Dropped::offset`](crate::Dropped::offset) in the returned report is relative to the start + /// of the TIFF stream, while an offset in a returned [`ExifError`] is a position in `source` + /// and includes any `Exif\0\0` marker. A caller that renders both beside each other must + /// normalise one of them. + /// /// # Errors /// /// As [`parse_from`](Self::parse_from). @@ -200,10 +214,14 @@ impl ExifReader { /// lenient mode an unusable range yields a thumbnail without bytes and a recorded drop; in /// strict mode it errors. /// - /// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` - /// *both* mandatory for a compressed thumbnail, so an offset without a length is a malformed - /// pair, not an absent thumbnail: it addresses bytes nothing can size. A length without an - /// offset addresses nothing at all, so nothing was dropped and nothing is reported. + /// An offset with no `JPEGInterchangeFormatLength` beside it addresses bytes nothing can size, + /// so it is a loss rather than an absent thumbnail — the JPEG behind the offset is unreadable. + /// A length with no offset addresses nothing at all, so nothing was dropped and nothing is + /// reported. Both halves of that rule are structural and apply whatever the thumbnail's + /// `Compression` says: Exif 3.0 §4.6.9.2 Table 21 gives the pair's support level *per + /// `Compression` column* (mandatory under **Compressed**, `N` — not allowed to record — under + /// all three uncompressed ones), and this reader does not consult that tag. Whether it should, + /// and whether the length-only case should be rejected for symmetry, is issue #574. fn read_thumbnail( &self, ifd: Ifd, @@ -249,6 +267,14 @@ impl ExifReader { // The JPEGInterchangeFormat offset is structural — the bytes are captured above and the // writer re-synthesises the offset — so drop it from the stored directory (mirroring how the // sub-IFD pointer tags are stripped), leaving a value the model can't carry stale. + // + // The removal is conditioned on bytes having been read, which is #548: when `jpeg` is + // `None` the pointer survives into the model and `to_bytes` re-emits it, so the emitted + // blob claims a thumbnail the report says was dropped. #548 names only the OutOfBounds + // case; the ThumbnailLengthMissing arm above is a SECOND instance of it, and a sharper + // one — the re-emitted blob still has an offset and still has no length, so a strict parse + // of it fails with the BadThumbnail this crate itself produced. Fixing the condition is a + // writer behaviour change and belongs to #548, not here. let mut ifd = ifd; if jpeg.is_some() { ifd.remove(ptr); diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index 45e08594..bfd40bf4 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -296,11 +296,12 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { /// A thumbnail offset with no length beside it is named rather than silently ignored. /// -/// Exif 3.0 §4.6.9.2 Table 21 marks `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` both -/// mandatory for a compressed thumbnail, so half the pair is not "no thumbnail" — it is an address -/// with nothing to size the read by, and the JPEG behind it is lost. Before this the pair fell into -/// the reader's catch-all `None` arm: no bytes, no error, no report entry, inside the very region -/// this report claims completeness over. +/// An offset with no `JPEGInterchangeFormatLength` is not "no thumbnail" — it is an address with +/// nothing to size the read by, and the JPEG behind it is lost. Before this the pair fell into the +/// reader's catch-all `None` arm: no bytes, no error, no report entry, inside the very region this +/// report claims completeness over. The rule is structural, not a support level: Exif 3.0 §4.6.9.2 +/// Table 21 states the pair's level per `Compression` column and the reader does not read that tag +/// (issue #574), so the fixture's `Compression` value is scene-setting, not the trigger. #[test] fn a_thumbnail_offset_without_a_length_is_named() { let mut thumb = Ifd::new(); @@ -334,6 +335,11 @@ fn a_thumbnail_offset_without_a_length_is_named() { /// The other direction of the pair: a `JPEGInterchangeFormatLength` on its own addresses no bytes /// at all, so there is nothing to name. Without this, reporting the incomplete pair could be /// "fixed" by reporting every thumbnail that has no JPEG, which would make the signal noise. +/// +/// This pins the *reporting* contract only. Whether a length-only 1st IFD should nonetheless be +/// *rejected* in strict mode — under `Compression = Compressed` Exif 3.0 §4.6.9.2 Table 21 marks +/// both tags mandatory, so it is as malformed as the offset-only case — is open, and filed as +/// issue #574. Both fixtures here are uncompressed, where the table forbids either tag outright. #[test] fn a_thumbnail_with_no_jpeg_range_reports_nothing() { for extra in [None, Some((THUMB_LENGTH, 16))] { From c5157fa048346de4f0346f08964f54dadd528657 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 06:43:03 -0400 Subject: [PATCH 12/15] docs(exif): ground the strict thumbnail rule and ship the changed verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit narrowed four sites that cited Exif 3.0 §4.6.9.2 Table 21 as making the thumbnail pointer pair mandatory outright. It missed a fifth — the crate's own front page — because it was found by grepping for the phrase "Table 21", and that site cites the clause without naming the table. Grepping for the citation (§4.6.9.2) finds all of them; grep for the citation, not the prose around it. `lib.rs` therefore still said the pair is "which Exif 3.0 §4.6.9.2 requires together" and called it "the malformed pair it is". Both are wrong: the table gives each 1st IFD tag a level *per `Compression` column*, and the pair is `N` (not allowed to record) under all three uncompressed columns. It now grounds the rule structurally, as the other sites do. Reading the table settles the open question of whether the unconditional strict refusal is defensible. `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` carry an identical level in all four columns (N N N M) and `Compression` itself is mandatory in all four, so an offset with no length is non-conformant under every column: there is no conformant 1st IFD the strict arm wrongly rejects. The rule stays unconditional, and issue #574's conditioning option is recorded as cheap rather than costly, since `Thumbnail::compression` already reads the tag. Three further corrections: - "a tag this crate does not read" was false — `Thumbnail::compression` is a public accessor. Every site now says this *reader* does not consult it. - The note beside the #548 comment argued that the missing-length instance is "sharper" because a strict parse of the re-emitted blob fails. Both instances self-reject, so that is a shared property. What actually separates them is novelty: the out-of-bounds instance is already rejected strictly before this change, while the missing-length one is created by it. - `parse_with_report` hands a caller both offset frames in one call and had no note saying so; `parse`'s error list omitted `BadThumbnail`, the variant this change gives a new way to reach. The changed strict verdict now appears in a shipped document. It is a fix, not a redefinition — the input that now fails was non-conformant under every column — so no major version is forced, but a caller running strict can learn of it from the README rather than only from a commit body. Refs #548, #574 --- crates/gamut-exif/README.md | 10 ++++++++++ crates/gamut-exif/src/lib.rs | 12 +++++++++--- crates/gamut-exif/src/reader.rs | 10 +++++++++- crates/gamut-exif/src/report.rs | 15 ++++++++++++--- crates/gamut-exif/src/stream.rs | 8 +++++--- crates/gamut-exif/tests/report.rs | 13 ++++++++----- 6 files changed, 53 insertions(+), 15 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index ab724db0..2242ec29 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -73,6 +73,16 @@ for dropped in report.dropped() { # } ``` +One read verdict changed with the report. A 1st IFD carrying `JPEGInterchangeFormat` with no +`JPEGInterchangeFormatLength` used to parse as a thumbnail that simply had no bytes; it is now a +**loss** — named in the report as `DropReason::ThumbnailLengthMissing`, and **rejected by `strict`** +with `ExifError::BadThumbnail`, since an offset with nothing to size it addresses bytes that cannot +be read. This is a fix rather than a redefinition, so it is not a breaking release: Exif 3.0 +§4.6.9.2 Table 21 gives both tags the *same* support level in each of its four `Compression` +columns — mandatory under **Compressed**, "not allowed to record" under the three uncompressed ones +— so the input that now fails was non-conformant under every one of them. A caller running `strict` +over blobs it previously accepted should still know the verdict moved. + Enable the optional `geocoordinates` feature (also included by `full`) to convert a complete [`GpsInfo`] with `TryFrom` into `geocoordinates::Wgs84` or `geocoordinates::Coordinate`. The latter preserves EXIF sea-level altitude as an orthometric height; the 2D `Wgs84` newtype intentionally diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index 9cf56215..dfac9f01 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -28,9 +28,15 @@ //! previous release. An error message's offset is now a position in the buffer the caller handed //! in, so for a marked blob it is six bytes larger than before — the `Exif\0\0` marker — while a //! [`Dropped::offset`] stays relative to the TIFF stream; and a 1st IFD carrying -//! `JPEGInterchangeFormat` with no `JPEGInterchangeFormatLength`, which Exif 3.0 §4.6.9.2 requires -//! together, is now named in the report instead of vanishing, and rejected in -//! [`strict`](ExifReader::strict) mode as the malformed pair it is. +//! `JPEGInterchangeFormat` with no `JPEGInterchangeFormatLength` is now named in the report +//! instead of vanishing, and rejected in [`strict`](ExifReader::strict) mode as the unreadable +//! range it is — an offset with nothing to size it addresses bytes that cannot be read. That rule +//! is structural, not a support level: Exif 3.0 §4.6.9.2 Table 21 states the pair's level only +//! *per `Compression` column*, and this reader does not consult that tag — though +//! [`Thumbnail::compression`](thumbnail::Thumbnail::compression) exposes it to callers (issue +//! #574). It refuses no conformant input: the table gives both tags the same level in all four +//! columns — `M` under **Compressed**, `N` (not allowed to record) under the three uncompressed +//! ones — so an offset with no length is non-conformant under every one of them. //! //! ``` //! use gamut_exif::{ByteOrder, Exif, ExifTag, Value}; diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index dc850580..45d4aa89 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -62,7 +62,9 @@ impl ExifReader { /// required but absent, an [`ExifError::Ifd`](crate::ExifError::Ifd) when the TIFF stream is /// malformed, or (in [`strict`](Self::strict) mode) /// [`ExifError::InvalidIfd`](crate::ExifError::InvalidIfd) when a sub-IFD pointer addresses a - /// malformed directory. + /// malformed directory or [`ExifError::BadThumbnail`](crate::ExifError::BadThumbnail) when the + /// 1st IFD's JPEG range is unusable — outside the blob, or an offset with no + /// `JPEGInterchangeFormatLength` to size it. /// /// An offset inside an error message is a position in `bytes` — the buffer the caller handed /// in — so for a marked blob it counts the six-byte `Exif\0\0` marker. That is deliberately a @@ -103,6 +105,12 @@ impl ExifReader { /// empty: [`DroppedRegion::TrailingIfd`](crate::DroppedRegion::TrailingIfd) is well-formed and /// merely unrepresentable, so strictness has no grounds to reject it and it is reported in both /// modes. + /// + /// The two offset frames of [`parse`](Self::parse) both reach the caller here, in one call: + /// an offset in a returned [`ExifError`](crate::ExifError) is a position in `bytes` and counts + /// any `Exif\0\0` marker, while every [`Dropped::offset`](crate::Dropped::offset) in the + /// report is relative to the start of the TIFF stream — six smaller for the same position in a + /// marked blob. A caller that renders both beside each other must normalise one of them. pub fn parse_with_report(&self, bytes: &[u8]) -> Result<(Exif, ReadReport)> { self.parse_from_with_report(bytes) } diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index a78b2f7c..f44a1760 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -63,7 +63,9 @@ pub enum DroppedRegion { /// absent thumbnail. That rule is structural and unconditional here — see /// [`ThumbnailLengthMissing`](DropReason::ThumbnailLengthMissing) — and is *not* derived from /// the pair's support level, which Exif 3.0 §4.6.9.2 Table 21 states only per `Compression` - /// column, a tag this crate does not read. + /// column, a tag this *reader* does not consult (a parsed + /// [`Thumbnail`](crate::Thumbnail) does expose it, through + /// [`compression`](crate::Thumbnail::compression)). ThumbnailJpeg = 3, /// A top-level directory past the 1st IFD. /// @@ -141,8 +143,15 @@ pub enum DropReason { /// that the read has no length — not that a tag is missing where the spec requires one. Exif /// 3.0 §4.6.9.2 Table 21 gives the pair's support level *per `Compression` column*: mandatory /// under **Compressed**, and `N` (not allowed to record) under all three uncompressed columns. - /// Whether this crate should read `Compression` and condition the rule on it — and whether a - /// length with no offset should be rejected for symmetry — is open, and filed as issue #574. + /// + /// Reading unconditionally costs nothing in conformance, because the two tags carry the *same* + /// level in every column: an offset with no length is non-conformant under all four, so there + /// is no conformant 1st IFD this rule wrongly names. Whether to condition it on `Compression` + /// anyway is filed as issue #574, and it is a cheap option rather than a costly one — + /// [`Thumbnail::compression`](crate::Thumbnail::compression) already reads the tag, so nothing + /// needs plumbing. The mirror case in that issue — a length with no offset — is genuinely + /// asymmetric and not merely unreached: an offset with no length *addresses bytes*, so + /// something is lost, while a length with no offset addresses nothing, so nothing is. ThumbnailLengthMissing = 3, } diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index 84f5ef43..b3721600 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -271,9 +271,11 @@ impl ExifReader { // The removal is conditioned on bytes having been read, which is #548: when `jpeg` is // `None` the pointer survives into the model and `to_bytes` re-emits it, so the emitted // blob claims a thumbnail the report says was dropped. #548 names only the OutOfBounds - // case; the ThumbnailLengthMissing arm above is a SECOND instance of it, and a sharper - // one — the re-emitted blob still has an offset and still has no length, so a strict parse - // of it fails with the BadThumbnail this crate itself produced. Fixing the condition is a + // case; the ThumbnailLengthMissing arm above is a SECOND instance of it. What separates + // them is not that a strict parse of the re-emitted blob fails — it fails for BOTH, since + // an out-of-bounds offset survives the round trip just as an unsized one does — but that + // the OutOfBounds instance is pre-existing (the default branch already rejects it + // strictly) while this one is created by adding the strict arm. Fixing the condition is a // writer behaviour change and belongs to #548, not here. let mut ifd = ifd; if jpeg.is_some() { diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index bfd40bf4..b4e58e96 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -300,8 +300,9 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { /// nothing to size the read by, and the JPEG behind it is lost. Before this the pair fell into the /// reader's catch-all `None` arm: no bytes, no error, no report entry, inside the very region this /// report claims completeness over. The rule is structural, not a support level: Exif 3.0 §4.6.9.2 -/// Table 21 states the pair's level per `Compression` column and the reader does not read that tag -/// (issue #574), so the fixture's `Compression` value is scene-setting, not the trigger. +/// Table 21 states the pair's level per `Compression` column and this reader does not consult that +/// tag — `Thumbnail::compression` exposes it, but nothing in the parse branches on it (issue #574) +/// — so the fixture's `Compression` value is scene-setting, not the trigger. #[test] fn a_thumbnail_offset_without_a_length_is_named() { let mut thumb = Ifd::new(); @@ -337,9 +338,11 @@ fn a_thumbnail_offset_without_a_length_is_named() { /// "fixed" by reporting every thumbnail that has no JPEG, which would make the signal noise. /// /// This pins the *reporting* contract only. Whether a length-only 1st IFD should nonetheless be -/// *rejected* in strict mode — under `Compression = Compressed` Exif 3.0 §4.6.9.2 Table 21 marks -/// both tags mandatory, so it is as malformed as the offset-only case — is open, and filed as -/// issue #574. Both fixtures here are uncompressed, where the table forbids either tag outright. +/// *rejected* in strict mode is open, and filed as issue #574: it is equally non-conformant (Exif +/// 3.0 §4.6.9.2 Table 21 gives both tags one level per `Compression` column — `M` under +/// **Compressed**, `N` under the three uncompressed ones), but it is not equally a *loss*, which +/// is what this report names. An offset with no length addresses bytes; a length with no offset +/// addresses nothing. Both fixtures here are uncompressed, where the table forbids either tag. #[test] fn a_thumbnail_with_no_jpeg_range_reports_nothing() { for extra in [None, Some((THUMB_LENGTH, 16))] { From 54c550a288c0d97218bfedb340b4f0dc9435479b Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:38:05 -0400 Subject: [PATCH 13/15] docs(exif): correct the offset-frame note and name Table 21's axis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four documentation corrections, none of which changes an executable line. `parse_with_report` said the two offset frames "both reach the caller here, in one call". The return is a sum type, so that is false: a blob carrying a reportable trailing directory *and* a strict-fatal dangling sub-IFD pointer returns, under `strict`, the error and no report at all — `record_trailing_ifds` runs before `follow`, so a drop it already recorded is discarded with the `Ok`. The streaming twin's wording ("the two frames meet here") was the correct one; the slice entry point now uses it and says why they only meet. `DropReason::ThumbnailLengthMissing` justified #574's conditioning option as cheap because `Thumbnail::compression` already reads the tag. That accessor reads a *finished* thumbnail, and the arm #574 would condition returns before one is built, so the fact does not reach the site. The conclusion holds for the adjacent reason the text now gives: the 1st IFD is in scope there and `Compression` is the same one-line lookup that reads the offset and the length two lines above. The #548 comment said the out-of-bounds instance is "pre-existing (the default branch already rejects it strictly)". Sitting on a `match`, "the default branch" reads as the default arm, under which the sentence is false. It names `master` now. Table 21's four columns are not four values of `Compression` — that tag has two. They are three uncompressed columns (Chunky, Planar, YCC), an axis of photometric and planar layout, plus Compressed. The README said "each of its four `Compression` columns" and now names the axis. The remaining "per column" sites are shorthand and are left alone. The README's changed-verdict note also moves under a `## Compatibility` heading and names 1.0.0 as the version the verdict changed from, since a reader arriving from a registry cannot otherwise date it. It separates the two axes that were argued through one another: the rule is readability-driven — an offset with no length cannot be read — while conformance is only what makes the move a fix rather than a redefinition. The claim it ships is the grounding the vendored table gives; the before/after comparison behind it stays in the pull request rather than becoming a harness that would have to depend on a published version. Refs #548, #574 --- crates/gamut-exif/README.md | 33 +++++++++++++++++++++++---------- crates/gamut-exif/src/reader.rs | 12 +++++++----- crates/gamut-exif/src/report.rs | 23 ++++++++++++++--------- crates/gamut-exif/src/stream.rs | 4 ++-- 4 files changed, 46 insertions(+), 26 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index 2242ec29..3f54761b 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -73,22 +73,35 @@ for dropped in report.dropped() { # } ``` -One read verdict changed with the report. A 1st IFD carrying `JPEGInterchangeFormat` with no -`JPEGInterchangeFormatLength` used to parse as a thumbnail that simply had no bytes; it is now a -**loss** — named in the report as `DropReason::ThumbnailLengthMissing`, and **rejected by `strict`** -with `ExifError::BadThumbnail`, since an offset with nothing to size it addresses bytes that cannot -be read. This is a fix rather than a redefinition, so it is not a breaking release: Exif 3.0 -§4.6.9.2 Table 21 gives both tags the *same* support level in each of its four `Compression` -columns — mandatory under **Compressed**, "not allowed to record" under the three uncompressed ones -— so the input that now fails was non-conformant under every one of them. A caller running `strict` -over blobs it previously accepted should still know the verdict moved. - Enable the optional `geocoordinates` feature (also included by `full`) to convert a complete [`GpsInfo`] with `TryFrom` into `geocoordinates::Wgs84` or `geocoordinates::Coordinate`. The latter preserves EXIF sea-level altitude as an orthometric height; the 2D `Wgs84` newtype intentionally drops altitude. Malformed references, rationals, DMS components, and out-of-range positions return the typed [`GpsConversionError`]. +## Compatibility + +**One read verdict changed after 1.0.0.** A 1st IFD carrying `JPEGInterchangeFormat` with no +`JPEGInterchangeFormatLength` used to parse as a thumbnail that simply had no bytes; from the next +release it is a **loss** — named in the report as `DropReason::ThumbnailLengthMissing`, and +**rejected by `strict`** with `ExifError::BadThumbnail`. A caller running `strict` over blobs +1.0.0 accepted should know the verdict moved. + +The rule is about **readability**: an offset with nothing to size it addresses bytes that cannot be +read, which is what `strict` is for. It is not about a support level, and the reader does not +consult `Compression` at all. + +**Conformance** is the separate question of whether the move is a *fix* or a *redefinition*, and it +is a fix, so no major version is forced. Exif 3.0 §4.6.9.2 Table 21 states each 1st IFD tag's +support level per thumbnail-format column — three uncompressed ones (Chunky, Planar, YCC) plus +**Compressed**, an axis of photometric and planar layout rather than the two-valued `Compression` +tag — and gives `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` the *same* level in each: +"not allowed to record" under the three uncompressed columns, mandatory under Compressed. An offset +with no length is therefore non-conformant under every column, and no conformant 1st IFD changes +verdict. That grounding is what this repository ships — the table is vendored under +`references/exif/`; the before/after comparison measured behind it is recorded in the pull request +that introduced the report, not committed here as a harness. + ## Scope v1 covers the **standard CIPA DC-008 tag dictionary** ([`ExifTag`]), full read/write round-trips diff --git a/crates/gamut-exif/src/reader.rs b/crates/gamut-exif/src/reader.rs index 45d4aa89..1186b1ca 100644 --- a/crates/gamut-exif/src/reader.rs +++ b/crates/gamut-exif/src/reader.rs @@ -106,11 +106,13 @@ impl ExifReader { /// merely unrepresentable, so strictness has no grounds to reject it and it is reported in both /// modes. /// - /// The two offset frames of [`parse`](Self::parse) both reach the caller here, in one call: - /// an offset in a returned [`ExifError`](crate::ExifError) is a position in `bytes` and counts - /// any `Exif\0\0` marker, while every [`Dropped::offset`](crate::Dropped::offset) in the - /// report is relative to the start of the TIFF stream — six smaller for the same position in a - /// marked blob. A caller that renders both beside each other must normalise one of them. + /// The two offset frames of [`parse`](Self::parse) meet here: an offset in a returned + /// [`ExifError`](crate::ExifError) is a position in `bytes` and counts any `Exif\0\0` marker, + /// while every [`Dropped::offset`](crate::Dropped::offset) in the report is relative to the + /// start of the TIFF stream — six smaller for the same position in a marked blob. A caller + /// that renders both beside each other must normalise one of them. They *meet* rather than + /// always arrive together: the return is a sum type, so a blob whose strict-fatal defect is + /// reached after a reportable one yields the error alone and no report. pub fn parse_with_report(&self, bytes: &[u8]) -> Result<(Exif, ReadReport)> { self.parse_from_with_report(bytes) } diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index f44a1760..7bb70b04 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -140,16 +140,21 @@ pub enum DropReason { /// parse. The repair is different in each case, which is why they are different reasons. /// /// Recorded whatever the thumbnail's `Compression` says, because the reason it is a loss is - /// that the read has no length — not that a tag is missing where the spec requires one. Exif - /// 3.0 §4.6.9.2 Table 21 gives the pair's support level *per `Compression` column*: mandatory - /// under **Compressed**, and `N` (not allowed to record) under all three uncompressed columns. + /// **readability**: the read has no length, not a tag is missing where the spec requires one. + /// Exif 3.0 §4.6.9.2 Table 21 gives the pair's support level *per column*: mandatory under + /// **Compressed**, and `N` (not allowed to record) under all three uncompressed columns. /// - /// Reading unconditionally costs nothing in conformance, because the two tags carry the *same* - /// level in every column: an offset with no length is non-conformant under all four, so there - /// is no conformant 1st IFD this rule wrongly names. Whether to condition it on `Compression` - /// anyway is filed as issue #574, and it is a cheap option rather than a costly one — - /// [`Thumbnail::compression`](crate::Thumbnail::compression) already reads the tag, so nothing - /// needs plumbing. The mirror case in that issue — a length with no offset — is genuinely + /// **Conformance** is a separate axis, and it is only what shows the unconditional rule names + /// nothing it should not: the two tags carry the *same* level in every column, so an offset + /// with no length is non-conformant under all four and there is no conformant 1st IFD this + /// rule wrongly names. Whether to condition it on `Compression` anyway is filed as issue #574, + /// and it is a cheap option rather than a costly one — not because + /// [`Thumbnail::compression`](crate::Thumbnail::compression) exists (that accessor reads a + /// *finished* thumbnail, and the arm #574 would condition returns before one is built) but + /// because the 1st IFD is already in scope there and `Compression` is the same one-line + /// lookup that reads the offset and the length two lines above. Nothing needs plumbing. + /// + /// The mirror case in that issue — a length with no offset — is genuinely /// asymmetric and not merely unreached: an offset with no length *addresses bytes*, so /// something is lost, while a length with no offset addresses nothing, so nothing is. ThumbnailLengthMissing = 3, diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index b3721600..e7d9c6e5 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -274,8 +274,8 @@ impl ExifReader { // case; the ThumbnailLengthMissing arm above is a SECOND instance of it. What separates // them is not that a strict parse of the re-emitted blob fails — it fails for BOTH, since // an out-of-bounds offset survives the round trip just as an unsized one does — but that - // the OutOfBounds instance is pre-existing (the default branch already rejects it - // strictly) while this one is created by adding the strict arm. Fixing the condition is a + // the OutOfBounds instance is pre-existing (`master` already rejects it strictly) while + // this one is created by adding the strict arm. Fixing the condition is a // writer behaviour change and belongs to #548, not here. let mut ifd = ifd; if jpeg.is_some() { From a652962389fa6109ca48ef1ec1509a738d8903ed Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 08:31:15 -0400 Subject: [PATCH 14/15] docs(exif): name Table 21's axis at every site and drop a release tense MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four documentation corrections. The macro-expanded `--all-features` lib target is byte-identical across this commit once doc lines are stripped (3 098 lines, sha256 f72897dd8d4100b022173ba1ffc0b2f3a0a86491cdaf077b3fad162a0305f292 at both ends), so nothing executable moved. The `## Compatibility` note said the changed verdict applies "from the next release". A README is packaged per version and rendered on the registry page for that version, so the copy shipped *with* the release that carries the change would tell its reader the change is still upcoming. The sentence now states the verdict without a tense; the heading above it is already anchored to 1.0.0 and stays correct in every published copy. Three in-crate sites said Table 21 states the pair's level "per `Compression` column". At each the phrase names the axis the levels vary over, and two go on to enumerate all four columns — so they assert the table is keyed on a tag that has two values (§4.6.5.1.4: 1 = uncompressed, 6 = JPEG). The table's header spans three uncompressed columns (Chunky, Planar, YCC) plus Compressed. They now name the thumbnail-format axis and say it is not that tag. `src/reader.rs` and the `read_thumbnail` comment already spoke exactly — "mandatory only under `Compression = Compressed`, and forbidden under the uncompressed columns" is a statement about one column, not about the axis — and are unchanged. The two remaining sites are in `tests/report.rs`. The README's own axis description was an appositive attached to a list whose fourth member is a compression state, so it called Compressed a photometric and planar layout. The description now scopes to the three uncompressed columns it actually distinguishes. `parse_from_with_report` lacked the sum-type caveat its slice twin carries: the two offset frames meet there but do not always arrive together, because a strict-fatal defect reached after a reportable one returns the error and discards the report. Both twins now say so. Refs #574 --- crates/gamut-exif/README.md | 25 +++++++++++++------------ crates/gamut-exif/src/lib.rs | 13 +++++++------ crates/gamut-exif/src/report.rs | 6 +++--- crates/gamut-exif/src/stream.rs | 11 +++++++---- 4 files changed, 30 insertions(+), 25 deletions(-) diff --git a/crates/gamut-exif/README.md b/crates/gamut-exif/README.md index 3f54761b..d2c5eb4f 100644 --- a/crates/gamut-exif/README.md +++ b/crates/gamut-exif/README.md @@ -82,10 +82,10 @@ the typed [`GpsConversionError`]. ## Compatibility **One read verdict changed after 1.0.0.** A 1st IFD carrying `JPEGInterchangeFormat` with no -`JPEGInterchangeFormatLength` used to parse as a thumbnail that simply had no bytes; from the next -release it is a **loss** — named in the report as `DropReason::ThumbnailLengthMissing`, and -**rejected by `strict`** with `ExifError::BadThumbnail`. A caller running `strict` over blobs -1.0.0 accepted should know the verdict moved. +`JPEGInterchangeFormatLength` used to parse as a thumbnail that simply had no bytes; it is a +**loss** — named in the report as `DropReason::ThumbnailLengthMissing`, and **rejected by +`strict`** with `ExifError::BadThumbnail`. A caller running `strict` over blobs 1.0.0 accepted +should know the verdict moved. The rule is about **readability**: an offset with nothing to size it addresses bytes that cannot be read, which is what `strict` is for. It is not about a support level, and the reader does not @@ -93,14 +93,15 @@ consult `Compression` at all. **Conformance** is the separate question of whether the move is a *fix* or a *redefinition*, and it is a fix, so no major version is forced. Exif 3.0 §4.6.9.2 Table 21 states each 1st IFD tag's -support level per thumbnail-format column — three uncompressed ones (Chunky, Planar, YCC) plus -**Compressed**, an axis of photometric and planar layout rather than the two-valued `Compression` -tag — and gives `JPEGInterchangeFormat` and `JPEGInterchangeFormatLength` the *same* level in each: -"not allowed to record" under the three uncompressed columns, mandatory under Compressed. An offset -with no length is therefore non-conformant under every column, and no conformant 1st IFD changes -verdict. That grounding is what this repository ships — the table is vendored under -`references/exif/`; the before/after comparison measured behind it is recorded in the pull request -that introduced the report, not committed here as a harness. +support level per thumbnail-format column: three uncompressed ones distinguished by photometric +interpretation and planar configuration (Chunky, Planar, YCC), plus **Compressed**. That axis is not +the two-valued `Compression` tag. The table gives `JPEGInterchangeFormat` and +`JPEGInterchangeFormatLength` the *same* level in each column: "not allowed to record" under the +three uncompressed columns, mandatory under Compressed. An offset with no length is therefore +non-conformant under every column, and no conformant 1st IFD changes verdict. That grounding is what +this repository ships — the table is vendored under `references/exif/`; the before/after comparison +measured behind it is recorded in the pull request that introduced the report, not committed here as +a harness. ## Scope diff --git a/crates/gamut-exif/src/lib.rs b/crates/gamut-exif/src/lib.rs index dfac9f01..04e21c52 100644 --- a/crates/gamut-exif/src/lib.rs +++ b/crates/gamut-exif/src/lib.rs @@ -31,12 +31,13 @@ //! `JPEGInterchangeFormat` with no `JPEGInterchangeFormatLength` is now named in the report //! instead of vanishing, and rejected in [`strict`](ExifReader::strict) mode as the unreadable //! range it is — an offset with nothing to size it addresses bytes that cannot be read. That rule -//! is structural, not a support level: Exif 3.0 §4.6.9.2 Table 21 states the pair's level only -//! *per `Compression` column*, and this reader does not consult that tag — though -//! [`Thumbnail::compression`](thumbnail::Thumbnail::compression) exposes it to callers (issue -//! #574). It refuses no conformant input: the table gives both tags the same level in all four -//! columns — `M` under **Compressed**, `N` (not allowed to record) under the three uncompressed -//! ones — so an offset with no length is non-conformant under every one of them. +//! is structural, not a support level: Exif 3.0 §4.6.9.2 Table 21 states the pair's level only *per +//! thumbnail-format column*, an axis that is not the two-valued `Compression` tag — which this +//! reader does not consult, though [`Thumbnail::compression`](thumbnail::Thumbnail::compression) +//! exposes it to callers (issue #574). It refuses no conformant input: the table gives both tags +//! the same level in all four columns — `M` under **Compressed**, `N` (not allowed to record) under +//! the three uncompressed ones — so an offset with no length is non-conformant under every one of +//! them. //! //! ``` //! use gamut_exif::{ByteOrder, Exif, ExifTag, Value}; diff --git a/crates/gamut-exif/src/report.rs b/crates/gamut-exif/src/report.rs index 7bb70b04..594d6fae 100644 --- a/crates/gamut-exif/src/report.rs +++ b/crates/gamut-exif/src/report.rs @@ -62,9 +62,9 @@ pub enum DroppedRegion { /// nothing to size it addresses bytes that cannot be read, which is a loss rather than an /// absent thumbnail. That rule is structural and unconditional here — see /// [`ThumbnailLengthMissing`](DropReason::ThumbnailLengthMissing) — and is *not* derived from - /// the pair's support level, which Exif 3.0 §4.6.9.2 Table 21 states only per `Compression` - /// column, a tag this *reader* does not consult (a parsed - /// [`Thumbnail`](crate::Thumbnail) does expose it, through + /// the pair's support level, which Exif 3.0 §4.6.9.2 Table 21 states only per + /// thumbnail-format column. That axis is not the two-valued `Compression` tag, which this + /// *reader* does not consult (a parsed [`Thumbnail`](crate::Thumbnail) does expose it, through /// [`compression`](crate::Thumbnail::compression)). ThumbnailJpeg = 3, /// A top-level directory past the 1st IFD. diff --git a/crates/gamut-exif/src/stream.rs b/crates/gamut-exif/src/stream.rs index e7d9c6e5..559849b9 100644 --- a/crates/gamut-exif/src/stream.rs +++ b/crates/gamut-exif/src/stream.rs @@ -69,7 +69,9 @@ impl ExifReader { /// [`Dropped::offset`](crate::Dropped::offset) in the returned report is relative to the start /// of the TIFF stream, while an offset in a returned [`ExifError`] is a position in `source` /// and includes any `Exif\0\0` marker. A caller that renders both beside each other must - /// normalise one of them. + /// normalise one of them. They *meet* rather than always arrive together: the return is a sum + /// type, so a source whose strict-fatal defect is reached after a reportable one yields the + /// error alone and no report. /// /// # Errors /// @@ -219,9 +221,10 @@ impl ExifReader { /// A length with no offset addresses nothing at all, so nothing was dropped and nothing is /// reported. Both halves of that rule are structural and apply whatever the thumbnail's /// `Compression` says: Exif 3.0 §4.6.9.2 Table 21 gives the pair's support level *per - /// `Compression` column* (mandatory under **Compressed**, `N` — not allowed to record — under - /// all three uncompressed ones), and this reader does not consult that tag. Whether it should, - /// and whether the length-only case should be rejected for symmetry, is issue #574. + /// thumbnail-format column* (mandatory under **Compressed**, `N` — not allowed to record — + /// under all three uncompressed ones); that axis is not the two-valued `Compression` tag, which + /// this reader does not consult. Whether it should, and whether the length-only case should be + /// rejected for symmetry, is issue #574. fn read_thumbnail( &self, ifd: Ifd, From 0bfdb5e240d666b1e28f3269d8bea3ad2e022f8d Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 09:30:07 -0400 Subject: [PATCH 15/15] docs(exif): name Table 21's axis at the last two sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes the correction `a6529623` applied to the three sites inside the previous round's manifest. `tests/report.rs:303` and `:342` carried the same false phrase — Table 21 states the pair's level "per `Compression` column" — and after `a6529623` they contradicted the three corrected sites in `src/`, which is the shape of defect this crate's own documentation was being repaired for. Both now name the thumbnail-format axis and say it is not that tag. `Compression` (tag 259) has two values in Exif 3.0 §4.6.5.1.4: 1 = uncompressed and 6 = JPEG. Table 21's four columns are three uncompressed ones (Chunky, Planar, YCC) plus Compressed, so the table is not keyed on that tag. `src/reader.rs` and `read_thumbnail`'s inline comment stay unchanged: both say the pair is mandatory only under `Compression = Compressed` and forbidden under the uncompressed columns, which is a statement about one column's value and is exact. Doc comments only. Every changed line is a `///` line, and the macro-expanded `--all-features` `report` test target is unchanged across this commit once doc lines are stripped and the `#[test]` harness's `TestDesc` source positions are normalised: 1 039 lines, sha256 764490d5971c45e8c967c086aecd7ee4f708566527157cfb090c5589a4a1eed2 at both ends. Un-normalised, sixteen lines differ and all sixteen are `start_line`/`end_line` in `TestDesc`, because two doc comments each gained a line and four tests moved down the file. The lib target is byte-identical without any normalisation (3 098 lines, sha256 f72897dd8d4100b022173ba1ffc0b2f3a0a86491cdaf077b3fad162a0305f292). Refs #574 --- crates/gamut-exif/tests/report.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/crates/gamut-exif/tests/report.rs b/crates/gamut-exif/tests/report.rs index b4e58e96..2ab9f375 100644 --- a/crates/gamut-exif/tests/report.rs +++ b/crates/gamut-exif/tests/report.rs @@ -300,9 +300,10 @@ fn a_truncated_blob_never_drops_a_sub_ifd_without_naming_it() { /// nothing to size the read by, and the JPEG behind it is lost. Before this the pair fell into the /// reader's catch-all `None` arm: no bytes, no error, no report entry, inside the very region this /// report claims completeness over. The rule is structural, not a support level: Exif 3.0 §4.6.9.2 -/// Table 21 states the pair's level per `Compression` column and this reader does not consult that -/// tag — `Thumbnail::compression` exposes it, but nothing in the parse branches on it (issue #574) -/// — so the fixture's `Compression` value is scene-setting, not the trigger. +/// Table 21 states the pair's level per thumbnail-format column, an axis that is not the two-valued +/// `Compression` tag — which this reader does not consult at all: `Thumbnail::compression` exposes +/// it, but nothing in the parse branches on it (issue #574) — so the fixture's `Compression` value +/// is scene-setting, not the trigger. #[test] fn a_thumbnail_offset_without_a_length_is_named() { let mut thumb = Ifd::new(); @@ -339,10 +340,11 @@ fn a_thumbnail_offset_without_a_length_is_named() { /// /// This pins the *reporting* contract only. Whether a length-only 1st IFD should nonetheless be /// *rejected* in strict mode is open, and filed as issue #574: it is equally non-conformant (Exif -/// 3.0 §4.6.9.2 Table 21 gives both tags one level per `Compression` column — `M` under -/// **Compressed**, `N` under the three uncompressed ones), but it is not equally a *loss*, which -/// is what this report names. An offset with no length addresses bytes; a length with no offset -/// addresses nothing. Both fixtures here are uncompressed, where the table forbids either tag. +/// 3.0 §4.6.9.2 Table 21 gives both tags one level per thumbnail-format column — `M` under +/// **Compressed**, `N` under the three uncompressed ones — and that axis is not the two-valued +/// `Compression` tag), but it is not equally a *loss*, which is what this report names. An offset +/// with no length addresses bytes; a length with no offset addresses nothing. Both fixtures here +/// are uncompressed, where the table forbids either tag. #[test] fn a_thumbnail_with_no_jpeg_range_reports_nothing() { for extra in [None, Some((THUMB_LENGTH, 16))] {