feat(exif): streaming ReadAt entry point + a report of what a lenient parse dropped - #522
Open
justin13888 wants to merge 15 commits into
Open
justin13888 wants to merge 15 commits into
justin13888 wants to merge 15 commits into
Conversation
`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<S: ReadAt>` 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
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
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
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<u16>`. 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
`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
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.
…g source 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.
…links 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.
This was referenced Sep 10, 2026
Open
`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.
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
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
…dict 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
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
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #419 named two gaps in
gamut-exif's reader, and this delivers both. Branchfeat/419-exif-streaming-reader, based onorigin/master(6a75ec4).A
ReadAtstreaming entry point.gamut-ifdshipped a lazy positioned reader in P9 (#252) —ReadAt,IfdReader,Rebased— andgamut-exifexposed 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.ExifReader::parse_from<S: ReadAt>now sits alongsideparse, andparseis the&[u8]case of it(a slice is a
ReadAtsource), so there is exactly one parse engine and the two entry pointscannot drift. The marker is detected through the source and the TIFF stream reached with
Rebased,so every offset the crate reads or hands back stays in EXIF's own frame of reference.
A drop report. The lenient reader discards 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, and
followremoved the pointer tag before attempting the parse, so the evidence ofwhat had been there was gone by the time it failed.
parse_with_report/parse_from_with_reportreturn a
ReadReportalongside theExifnaming each discarded region: aDroppedRegion, the tagthat addressed it, the offset that tag carried, and a
DropReasonseparating an address outside theblob from bytes inside it that were not a directory. Both enums are fieldless with an explicit
reprand append-only discriminants;
DroppedisCopyplain data behind accessors.Nothing existing changes.
parsekeeps its exact signature and behaviour and stays silent; thepointer removal moves after the read attempt but the removal itself is unchanged. The crate's
pre-existing tests for lenient/strict sub-IFD and thumbnail handling are untouched and pass as they
stood. Nothing in
gamut-ifdwas modified, and no dependency was added.Two incidental improvements ride along in the thumbnail path: the range check moves from
usizeslicing to a 64-bit bound against the source length, which drops an overflow branch only 32-bit
targets could reach, and it now bounds the range before allocating, so a hostile
JPEGInterchangeFormatLengthcannot make the reader reserve 4 GiB.Repaired after review (
5a766f7). A read-only review ofc49f83bprovedparse's equivalenceunusually strongly — a truncation sweep of a marked and a bare fixture across
{strict}×{require_marker}, ~2 600 cases, found 0 mismatches in error string or re-serialised bytes —and raised two Mediums that the third commit fixes:
discarded silently (
ifds.next()twice, then the iterator dropped), so a three-directory chainlost the third with an empty report.
DroppedRegion::TrailingIfd+DropReason::Unrepresentablenow name it at its own offset. Separately,
gamut_ifd::decode_ifddoes silently drop ashadowed duplicate-tag entry, which falsifies the premise of decision 7 — that is one layer below
this crate, so it is filed as gamut-ifd: decode_ifd silently discards a shadowed duplicate-tag entry #528 and the documented promise is narrowed everywhere it
appears rather than overclaimed.
ReadAtsource was blamed on the file.followmatchedErr(_)withoutinspecting the error, so a source whose transport failed mid-parse returned
Okwith sub-IFDsmissing and a report calling structurally perfect directories malformed — worst for the
network-backed sources this entry point exists to enable, and asymmetric with the thumbnail path,
which already propagated. Both
followand the marker probe now key onError::kind.Repaired again after a second review (
3242ea1,44594fa,ba0a139). An independent reviewverified every round-2 item closed by construction — it re-ran the mutation gate on the two new
files, re-ran a full corruption-and-truncation sweep against
master, and twice failed to break thenew failure-propagation contract — and raised six items, all documentation or test reach, plus one
real silent loss:
read_thumbnail's catch-all_ => Nonearmswallowed a 1st IFD carrying
JPEGInterchangeFormatwithoutJPEGInterchangeFormatLength: nobytes, no error, no report entry, in either mode. That is a silent loss inside the exact region
this PR's report claims completeness over, so it is in scope. 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 a new
DropReason::Incomplete, strict moderejects it as it already did an out-of-bounds range, and a length with no offset — which addresses
nothing — stays silent.
parse_with_reportclaimed "a strict report is always empty", whichReadReport's own docscontradict in this same branch and which is false: a strict parse of a well-formed three-directory
chain returns exactly one
TrailingIfdentry. The sentence predatesTrailingIfdand re-hid thevery loss that variant was added to surface. Corrected, stated identically in both places, and
pinned.
parse's error strings did change for marked blobs, which the previous body denied. Routingthrough
IfdReader::open(source.rebased(base))attaches the physical offset, so a diagnostic thatwas TIFF-stream-relative is now blob-relative. Both frames are kept deliberately and are now
documented and pinned against each other; see Validation for the measurement.
thumbnail or a trailing directory, so
read_range's fetch andrecord_trailing_ifds' chainre-walk were never swept. The fixture was extended rather than the claim narrowed.
0-sentinel comments sitting above assertions ofNone, a README sample showing thepre-rewrite
Displaygrammar, and two rustdoc links broken by the round-2 import narrowing.Closing repairs after a third review (
d650f62,d3e2207,68a57fb). The third reviewrebuilt the equivalence differential on its own fixture at 4 504 cases and reproduced this
branch's result independently — the same two families, the same +6 offset delta with zero residual,
the same 12 strict-only verdict changes, the same quadrant split, the same explanation for why 12
and not 16, and every doubly-successful case byte-identical. It re-ran the mutation gate to the
same numbers and closed all seven round-3 items. Four Lows remained, every one of them about what
the code claims rather than what it does:
§4.6.9.2 Table 21 as making the thumbnail pointer pair mandatory. That table states each 1st IFD
tag's support level per
Compressioncolumn:JPEGInterchangeFormat(513) andJPEGInterchangeFormatLength(514) areMonly under Compressed, andN— not allowed torecord — under all three uncompressed columns. So an uncompressed thumbnail carrying only an
offset was rejected with a message naming a sibling the cited table forbids recording there
(over-reach), while a compressed thumbnail carrying only a length is accepted (under-reach). The
rejection does not actually rest on that mandate — an offset with nothing to size it addresses
bytes that cannot be read, whatever
Compressionsays — so every site is narrowed to say exactlythat, and the message becomes
JPEGInterchangeFormat offset with no length to size it. Whetherthe rule should instead be conditioned on
Compression, and whether the length-only case shouldbe rejected for symmetry, is a behavioural change with its own sweep to run: filed as gamut-exif: should the thumbnail JPEG pair rule be conditioned on Compression? #574,
not decided here, because neither direction regresses the base.
DropReason::Incompleteis renamedDropReason::ThumbnailLengthMissing, before a releasefreezes its discriminant. It has exactly one call site and a name that described a shape, which
invites unrelated reuse; a merely similar future defect gets its own variant on the
#[non_exhaustive]enum instead. The variant is new on this branch and has never been published,so no released API changes. The rendered clause moves with it, from
is addressed but never fully describedtohas no JPEGInterchangeFormatLength to size the read.#548interaction has a second instance, and it is sharper.#548names only theout-of-bounds case, but
jpeg.is_some()gates the pointer removal for both lenient arms, so thenew one leaks the pointer identically — and there the re-emitted blob still has an offset and
still has no length, so a strict parse rejects a blob this crate itself just wrote. Executed
and recorded beside the code and below; the writer fix stays in
#548.parse_fromandparse_from_with_reportcarried no offset-frame note, though a streamingcaller is the one most likely to correlate an error offset against bytes on disk. Both frames are
now documented where they are produced, as they already were on the slice entry points.
One item is filed rather than fixed (#548, a dangling thumbnail pointer the writer re-emits —
pre-existing on
masterand a writer change outside this manifest), and the structural gap that lettwo of these rot undetected is filed as #549.
No human approved this plan or any of the repairs. This is an unattended automated run; the
decision record below is what a reviewer reads in place of an approval.
Validation
Round-3 gates, at
ba0a139fEvery command in this table was run in this worktree
at head
ba0a139f, and each one completed in that run. It is kept as the record of that head; theround-4 table below is separate and states plainly which gates were re-run at
68a57fb2and whichwere not.
CARGO_BUILD_JOBS=2 cargo test -p gamut-exif --all-featuresCARGO_BUILD_JOBS=2 cargo clippy -p gamut-exif --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkmise run check-testsmise run check-commitsconvco check origin/master..HEADmise run lint(whole workspace, memory-capped scope)mise run test(whole workspace, memory-capped scope)mise run mutants-diff(memory-capped scope)RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-exif --no-depsunresolved link to exif_tagsatsrc/tag.rs:4__CARGO_TEST_ROOTis required forfmt-checkin a nested worktree:cargo otherwise walks past the worktree root to the primary checkout's
Cargo.tomlwhen loading thetooling/*manifests and exits 101 on an untouched tree. It is an environment artefact, not amanifest problem, and no manifest was changed to work around it.
The
cargo docrow is classifiedpre-existing, demonstrated rather than asserted:diff <(git show origin/master:crates/gamut-exif/src/tag.rs) crates/gamut-exif/src/tag.rsis empty,so the file carrying the broken link is byte-identical to
master. The two links this branch broke(
crate::stream, which stopped resolving when the module stopped being published, andExifError::MissingMarker, which stopped resolving when5a766f7narrowed the import) are fixed;exif_tagsis not this branch's to fix, and is filed with the missing gate as #549.Cargo.tomlwas not touched, socheck-release-deps/check-ffi-features/check-ffi-headerdonot apply.
mise run coveragewas not run: nothing here adds a low-reach module — the diff adds oneenum variant, one match arm and six tests.
Round-4 gates, at
68a57fb2The round-4 diff is confined to
crates/gamut-exif/: one enum variant renamed, one error string,and documentation. These gates completed in this run, at head
68a57fb2.CARGO_BUILD_JOBS=2 cargo test -p gamut-exif --all-featuresCARGO_BUILD_JOBS=2 cargo clippy -p gamut-exif --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkmise run check-testsmise run check-commitsconvco check origin/master..HEADmise run mutants-diff(memory-capped scope)RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-exif --no-depsunresolved link to exif_tagsatsrc/tag.rs:4The
cargo docrow is the same pre-existing failure as atba0a139f, demonstrated the same way:diff <(git show origin/master:crates/gamut-exif/src/tag.rs) crates/gamut-exif/src/tag.rsis empty,so the file carrying the broken link is byte-identical to
master. It is filed with the missinggate as #549.
Workspace-wide
mise run lintandmise run testwere NOT re-run at68a57fb2. The renametouches a
pubenum, so that needed deciding rather than assuming:DropReasonis reachable fromoutside the crate only through whole-module re-exports (
pub use gamut_exif as exifin bothgamutandgamut-metadata), which cannot break on a variant rename, and a workspace grep forDropReasonacrosscrates/,tooling/anddocs/returns zero hits outsidecrates/gamut-exif/. The variant itself is new on this branch —src/report.rsdoes not exist onmaster— so nothing outside the branch has ever been able to name it. Both gates passed atba0a139f, and noCargo.toml, feature, or cross-crate signature moved since.mise run mutants-diffdid run at this head and rebuildsgamut-exifagainst its dependency graph, so acompile break inside the crate would have surfaced there.
check-release-deps/check-ffi-features/check-ffi-headerdo not apply:no manifest and no C-surface type changed.
mise run coveragewas not run: the round-4 diff adds nomodule and no new branch.
Executed, not asserted: the two interactions recorded below
Both were measured in this worktree rather than reasoned about, with throwaway harnesses that were
removed before committing.
The
#548second instance. A 1st IFD withCompression = 6andJPEGInterchangeFormat = 4and no length, parsed leniently, re-serialised, re-parsed:
So strict mode rejects the blob the crate itself emitted, which is the sharper form of gamut-exif: to_bytes re-emits a thumbnail pointer whose bytes the report says were dropped #548.
The blast-radius reason change. A blob carrying both a dangling
ExifIFDpointer and anincomplete thumbnail pair, parsed with
strict(true), run against both heads:read_thumbnailruns beforefollowinparse_source, so the thumbnail's verdict now arrivesfirst. Both heads reject; only the error identity moved.
The equivalence sweep, measured at
ba0a139fThe previous body claimed 0 mismatches in error string or re-serialised bytes for
parse. Thatclaim was wrong, and the correction is measured rather than argued. A harness dumps
parse'sobservable answer — the re-serialised bytes for an
Ok, the rendered error for anErr— for astructurally rich fixture in marked and bare form, across
{strict} × {require_marker}, over everytruncation prefix and every single-byte flip: 3 144 cases. The same binary was run against this
head and against
origin/master'scrates/gamut-exif/src.64 of 3 144 cases differ, in exactly two families and no others:
larger by exactly 6 — the length of the
Exif\0\0marker. Normalising that one number backmakes 0 of them differ. All 52 marked cases that carry a byte offset are in this set; all 26
bare cases that carry one are unchanged, which is the mechanism: the base is 0 for a bare stream.
strict(true), and all of them a byte flip inside theJPEGInterchangeFormatLengthentry header — the corruption that orphans the thumbnail offset, andtherefore the new strict rejection above. There are 12 rather than 16 because the four
bare + require_markercases already failed withMissingMarker.Nothing else moved: 0 other verdict changes, and every one of the 732
Okcases is byte-identicalin re-serialised output. Lenient mode is unchanged in verdict and in bytes.
What the round-3 tests pin
report_offsets_ignore_the_marker_but_error_offsets_include_it(tests/report.rs) — the twoframes, together, so neither can drift onto the other: the same dangling pointer reports the same
offset from a marked and a bare blob, while the same corruption produces error offsets differing by
exactly
MARKER.len().a_strict_parse_still_reports_a_trailing_directory(tests/report.rs) — the corrected strictcontract, on the one loss strictness has no grounds to reject.
a_thumbnail_offset_without_a_length_is_namedanda_thumbnail_with_no_jpeg_range_reports_nothing(
tests/report.rs) — both directions of the incomplete pair, so the fix cannot degenerate intoreporting every thumbnail that has no JPEG.
a_thumbnail_offset_without_a_length_is_rejected_strictly(inline insrc/reader.rs, beside itsout-of-bounds sibling) — the strict half, pinned on the message so the reader is told which half of
the pair is missing.
the_deep_fixture_has_a_pin_a_thumbnail_and_a_trailing_directory_to_lose(inline insrc/stream.rs) — the fixture-integrity guard, without which the sweep below could pass against ablob with nothing to lose and a clean report that already blamed the file.
a_failing_source_is_propagated_not_reported_as_a_malformed_file— now overhealthy_bloband theextended
deep_blob, under the stronger law that anOkfrom a failing source equals the cleanparse in report, maker-note pin and thumbnail bytes. Over 40 budgets the deep fixture splits
17
Ok/ 23Err, every error keepingErrorKind::Io, zero violations.Round-5 gates, at
c5157fa0Round 5 is documentation plus one doc-comment line added to
parse's# Errorslist. Noexecutable statement changed, so the workspace-wide
lint/test/coveragegates were notre-run: they were run at
68a57fb2and nothing they observe moved. The capped set below was runin full, and the crate's own build, test, doctest and rustdoc passes were re-run because doc
comments are compiled (intra-doc links, doctests).
The single
cargo docwarning isunresolved link to exif_tagsatsrc/tag.rs:4. It ispre-existing, demonstrated by
git show origin/master:crates/gamut-exif/src/tag.rs | sed -n 4preturning the identical line, andsrc/tag.rsis not in this branch's diff. It is alreadyfiled and is not repaired here. Re-running
cargo docwithout-D warningsshows it is theonly diagnostic, so none of round 5's new intra-doc links is broken.
Round-6 gates, at
54c550a2Round 6 is documentation only: the diff touches
README.mdplus rustdoc and//comments inthree source files, and
git diff -U0 -- crates/gamut-exif/srcfiltered to non-comment lines isempty. So the gates that observe executable behaviour were justified as not needed rather than
skipped for cost:
mise run lint,mise run test,mise run coverageandmise run mutants-diffall observe compiled statements and mutable expressions, and this round produces nonew mutant and moves no statement — they were run at
68a57fb2/c5157fa0and nothing theyobserve changed.
check-release-deps/check-ffi-features/check-ffi-headerneed no runeither: no
Cargo.tomland no public C-surface type is touched.What is needed is everything that compiles doc comments (intra-doc links, doctests) or reads the
commit series. All of it was run in full:
The single
cargo docwarning remainsunresolved link to exif_tagsatsrc/tag.rs:4, stillpre-existing (
src/tag.rsis not in this branch's diff) and still the only diagnostic, sonone of round 6's edited intra-doc links is broken.
The union search, and every site it finds
Decision 18 and the
c5157fa0commit body both claim that grepping for the citation4.6.9.2finds every site stating the Table 21 rule. That claim is false, and correcting it is round
6's own lesson (decision 25). The correct method is the union of two greps — for the citation
and for the table name — because a site can carry either half alone:
Nine distinct sites, listed at
54c550a2:README.md:95(Compatibility)src/lib.rs:34src/reader.rs:286src/report.rs:65src/report.rs:144src/stream.rs:221src/stream.rs:251tests/report.rs:302-303tests/report.rs:342src/stream.rs:251is the site the citation grep does not reach — the exact mirror of round 5'smiss, which named the clause without the table.
tests/report.rs:302-303shows the second way aline-scoped grep can half-see a site: the citation and the table name are split across a wrap.
No defect follows:
src/stream.rs:251was written correct and has never stated the false rule,so this round changes no text there.
Round-7 gates, at
a6529623(the eighth review pass)This round is documentation by construction, and the claim is proved by a stronger method than the
comment-line filter round 6 used. The
--all-featureslib target is macro-expanded at both endsof the round and compared with doc lines stripped (
///,//!,#[doc =):Nothing executable moved, so the gates that observe executable behaviour are not needed rather
than skipped for cost:
mise run lint,mise run test,mise run coverageandmise run mutants-diffall observe compiled statements and mutable expressions, and this round produces nonew mutant.
check-release-deps/check-ffi-features/check-ffi-headerneed no run either: noCargo.tomland no public C-surface type is touched. Everything that compiles a doc comment orreads the series was run in full:
The single
cargo docwarning is stillunresolved link to exif_tagsatsrc/tag.rs:4,pre-existing (
src/tag.rsis not in this branch's diff) and still the only diagnostic, so noneof this round's edited intra-doc links is broken.
Table 21 re-read from the vendored PDF, and what the nine sites now say
references/exif/exif-3.0-dc-008-translation-2023.pdf, §4.6.9.2, read withpdftotext -layout.The table header spans Uncompressed → Chunky | Planar | YCC, plus Compressed — four
columns.
JPEGInterchangeFormat(513 / 0x201) readsN N N M;JPEGInterchangeFormatLength(514 / 0x202) reads
N N N M— identical in every column, which is the fact the "no conformant 1stIFD changes verdict" claim rests on. §4.6.5.1.4 defines
Compressionas1 = uncompressed,6 = JPEG compression (thumbnails only),Other = reserved: two values, so the four columnscannot be its values.
Re-running the union search at
a6529623still returns exactly nine sites ingamut-exif(plustwo
gamut-icchits against a different specification, §7.2.11 of the ICC spec, correctly notsites). Their state after this round:
README.md(Compatibility)src/lib.rsCompressioncolumn"src/report.rsDroppedRegion::ThumbnailJpegCompressioncolumn"src/report.rsDropReason::ThumbnailLengthMissingsrc/stream.rsread_thumbnaildocCompressioncolumn"src/stream.rsread_thumbnailinline commentCompression = Compressedsrc/reader.rsstrict-rejection test docCompression = Compressedtests/report.rs:303Compressioncolumn"0bfdb5e2(decision 38)tests/report.rs:342(now 343)Compressioncolumn"0bfdb5e2(decision 38)The two exact sites are statements about one column's value (
Compression = Compressed) andabout the uncompressed group — not claims that the axis is the
Compressiontag — and bothverify against the table as read above. The two
tests/report.rssites carried the same falsephrase and sat outside the round's original manifest; the revision was requested rather than taken
silently, granted, and applied in
0bfdb5e2. Re-running the union search at0bfdb5e2returns thesame nine sites, and a grep for the phrase
per `Compression` columnacrosscrates/now returnsnone.
Gates at
0bfdb5e2, the granted manifest revisioncrates/gamut-exif/tests/report.rswas added to the manifest so the last two sites could becorrected. The change is doc comments only — every changed line is a
///line, and a filter ofthe diff for non-
///lines returns nothing — and the claim is proved the same way the rest of theround proved it, on the target that actually contains the file:
The digest moved, and the reason is published rather than normalised away silently. All sixteen
differing lines are
start_line:/end_line:inside thetest::TestDescvalues the#[test]macro generates; four tests moved down the file because two doc comments each gained a line. That
is source-position metadata for a failure message, not behaviour: no assertion, no expression, no
test name, no
should_panicand noignoreflag differs. Filtering the diff for lines that arenot
start_line/end_linereturns 0. Normalising those positions and re-comparing:The lib target needs no normalisation and is byte-identical at
a6529623and0bfdb5e2(3 098 lines, sha256
f72897dd8d4100b022173ba1ffc0b2f3a0a86491cdaf077b3fad162a0305f292), which isthe independent check that no
src/file moved with the test file. Gates run at0bfdb5e2:mise run lint,mise run test,mise run coverage,mise run mutants-diff,check-release-deps,check-ffi-featuresandcheck-ffi-headerare not needed here for thereasons given for
a6529623above, which the normalised expansion re-establishes at this commit:no executable statement moved, no mutable expression was introduced, no
Cargo.tomland no publicC-surface type is touched.
Risks and rollout
strictmode only — an accept becomes a reject. Stated hereexplicitly rather than left to a changelog reader's inference: the interface is unchanged, but a
1st IFD carrying
JPEGInterchangeFormatwith noJPEGInterchangeFormatLengthis now rejectedby
ExifReader::strict(true)where it previously parsed as a thumbnail without bytes. A callerthat passes such blobs through strict mode today will start seeing
ExifError::BadThumbnail("JPEGInterchangeFormat offset with no length to size it"). Thejustification is structural, not a support level: an offset with nothing to size it addresses
bytes that cannot be read, which is what strictness is for, and it is the same answer strict
already gave an out-of-bounds range. Blast radius measured above: 12 of 3 144 sweep cases, all
strict(true). Lenient callers see a new report entry and no change in verdict or bytes.carrying both a dangling sub-IFD pointer and an incomplete thumbnail pair failed on
masterasInvalidIfd("Exif")("malformed Exif sub-IFD") and now fails asBadThumbnail("invalidthumbnail: …"), because
read_thumbnailruns beforefollowinparse_source. Both headsreject the blob, so no accept-to-reject is added; what changes is which defect is named first.
Neither the 3 144-case sweep nor the reviewer's 4 504-case rebuild produced that pair — both
corpora corrupt one fixture at a time — so it was found by construction and is executed above,
not inferred. A caller matching on the error variant to route a repair would route this blob
differently.
diagnostic now points into the buffer the caller handed in. A caller that parses offsets out of a
Displaystring would need to adjust; a caller that readsDropped::offsetsees no change. Bothframes are now documented on the items that produce them and pinned against each other.
ExifReader, one new publicmodule (
report), one newDropReasonvariant (ThumbnailLengthMissing) on a#[non_exhaustive]enum with append-only discriminants. No existing signature or return typechanged, so this stays a minor version bump. The round-4 rename of that variant is not a break
either:
src/report.rsdoes not exist onmaster, so the old spelling was never published, anda workspace grep finds no use of
DropReasonoutsidecrates/gamut-exif/.parsethat the sweep does not see, sincethe parse moved from
gamut_ifd::read/read_ifd_atontoIfdReaderdirectly. Mitigated bygamut-ifd's own guarantee that the slice functions are thin wrappers over the same streamingengine (asserted in its
tests/robustness.rs), by the 3 144-case sweep above, and by leaving everypre-existing
gamut-exifreader test unmodified.3242ea1reverts the thumbnail fix,c49f83breverts the report,
b3e8a5dreverts the streaming entry point. Reverting5a766f7alone isnot safe — it would restore the silent trailing-directory drop and the swallowed transport
failure; roll back to
6a75ec4instead. The three round-4 commits (d650f62,d3e2207,68a57fb) are independently revertible: the first is a rename, the second an error string, thethird documentation only.
c5157fa0and54c550a2are documentation only as well and revertindependently of everything above them.
Issue
Refs #419. Not
Closes— the issue's title names per-entry error recovery, and the report'sgranularity is the directory: one unparseable entry fails its whole IFD inside
gamut-ifd, which isthe layer that would have to recover per entry. That, a byte-completeness verdict over the blob, and
the real-camera-DNG half of the laziness assertion are filed as #521 and recorded in
crates/gamut-exif/STATUS.md. The shadowed duplicate-tag entry found in the round-2 review is filedagainst
gamut-ifdas #528.Filed by this round and linked here rather than fixed:
gamut-exif:to_bytesre-emits a thumbnail pointer whose bytes the report says weredropped. Pre-existing on
masterbyte for byte, and the repair changes writer behaviour welloutside this manifest. The interaction is noted where the report documents its completeness: the
report does name the drop; it is the emitted blob that is silent about it.
structural reason two of this round's findings could rot undetected. It needs
.github/workflows/edits, which this run is not permitted to make.
gamut-exif: should the thumbnail JPEG pair rule be conditioned onCompression?Filed by round 4. The documentation over-reach is fixed here; the behavioural half — whether
strict should condition the rule on the
Compressioncolumn Table 21 keys it on, and whethercompressed-with-only-a-length should be rejected for symmetry — is a change in both directions
that needs its own equivalence sweep, and neither direction regresses
master.Decisions taken
Read to the end: the record is never edited in place. Three entries below are superseded by later ones — decision 28's exemption of the five "per
Compressioncolumn" sites by decision 31, decision 30's "from the next release" tense by decision 32, and decision 37's reduction of the deliverable to three of the five sites by decision 38, after the manifest revision it requested was granted. Decisions 31-38 were appended in round 8.Appended during delivery, in the same shape:
Appended after review, decided by the orchestrator on review:
Appended during the round-3 repairs, in the same shape:
Appended during the round-4 closing repairs, in the same shape:
Appended in round 5 (each names the round-4 entry it corrects; no entry above is edited):
Unresolved review notes
Malformedfixture intests/report.rspoints theExifIFDpointer at offset 1, whichstraddles the byte-order mark and the magic so the entry count read there is nonsense. It is
in-bounds and unparseable, which is exactly what the case needs, but it is a construction chosen
for that property rather than one seen in the wild. A reviewer who prefers a corpus-derived
fixture should say so.
tiff_basetreats a source too short to hold the six-byte marker as unmarked rather thansurfacing the short read.
5a766f7(review finding L4): the probe now keys onthe error kind, so a transport failure propagates and only a genuinely short source reads as
unmarked.
record_trailing_ifdsre-walks the next-IFD chain to recover the offsets of the directories itreports, because
read_filereturns decodedIfds without them. It runs only when there issomething to report, so no well-formed blob pays for it — but it is a second read of the
directory bodies, and a reviewer who would rather change the walk to carry offsets through (at
the cost of no longer sharing
read_filewithgamut_ifd::read, which is what the ~2 600-caseequivalence sweep rests on) should say so.
Appended in round 3 — corrections to the record above, which is never edited in place:
TrailingIfdis reported "with tag 0because no tag addresses it".
Dropped::tagisOption<u16>and returnsNone, deliberately:0is a real tag number (GPSVersionID), so a0sentinel would read as a fact about thesource. Two comments still carrying the old wording —
src/report.rsandtests/report.rs,both sitting directly above assertions of
None— are corrected inba0a139.the reason that "the entry is gone before this crate sees the
Ifd; any signal here would beinvented, not observed". That is wrong as stated, and this branch's own
reportmodule docs sayso:
RawIfd::entriesis public and in on-disk order, so comparing its length against the decodedIfd::fields()observes that a directory lost an entry, in three lines. What this crate cannotdo is say what was lost without re-decoding the shadowed entry. The conclusion stands — the
signal belongs at the layer that discards, which three crates share, and is filed as gamut-ifd: decode_ifd silently discards a shadowed duplicate-tag entry #528 — but it
stands on detectability-without-description, not on undetectability.
is
src/lib.rs's "neither is a change for existing callers". Both are replaced by the measuredresult in Validation above: 64 of 3 144 cases differ, 52 by exactly the marker length in an error
offset and 12 by the new strict rejection.
Round-3 notes that remain open:
thumbnail's JPEG bytes and delivers it — a dropped range is always named. The blob
to_bytesemits is a separate matter: it still carries the dangling
JPEGInterchangeFormatvalue, so adownstream reader of the re-serialised bytes sees a thumbnail pointer with no report beside it.
Filed as gamut-exif: to_bytes re-emits a thumbnail pointer whose bytes the report says were dropped #548 with the executed reproduction; a reviewer who thinks the writer change belongs in
this PR rather than its own should say so.
DropReason::Incompleteis currently reachable from exactly one site. It is named for thedefect rather than the site, so an out-of-line value whose count is present but whose type is
unreadable could reuse it later; a reviewer who would rather it were named
ThumbnailLengthMissingand kept single-purpose should say so before 1.0 freezes the discriminant.deep_blobfixture patches a sentinelJPEGInterchangeFormatvalue afterwritelays thedirectories out, because the payload's position is not knowable beforehand. It asserts the sentinel
names exactly one value field, so a collision fails loudly rather than silently patching the wrong
bytes — but a reviewer who would rather the fixture were a checked-in golden blob should say so.
Appended in round 4 — corrections to the record above, which is never edited in place:
21, which marks both tags mandatory for a compressed thumbnail". That is true of the Compressed
column only, and the reader never reads
Compression, so the mandate is not what the rule restson. Decision 15 replaces the grounding with the structural one; the behaviour decision 12 took is
unchanged, and the substantive question the citation raised is filed as gamut-exif: should the thumbnail JPEG pair rule be conditioned on Compression? #574. Decision 12's
rejection clause "half a mandatory pair is malformed" inherits the same correction — it is
half an unreadable range, which is why the length-only direction (where nothing is addressed)
is not symmetric with it by that argument alone.
Measured:line understates the blast radius. It reads "12 of 3144 sweep caseschange verdict, every one of them
strict(true)", which remains exact for that corpus. It is notthe whole radius: a blob carrying both a dangling sub-IFD pointer and an incomplete thumbnail
pair changes the error it fails with —
InvalidIfd("Exif")onmaster,BadThumbnailhere —because
read_thumbnailruns beforefollow. Neither this branch's 3 144-case sweep nor thereviewer's independent 4 504-case rebuild contains that pair, since both corrupt one fixture at a
time, so no sweep count would ever have surfaced it. Executed against both heads and recorded in
Validation and in Risks.
DropReason::Incompleteis renamed, adopting the name the round-3 note itself proposed:ThumbnailLengthMissing. See decision 16 for why a shape-named variant with one call site is thewrong thing to freeze a discriminant on.
Round-3 notes resolved in round 4:
DropReason::Incompleteis currently reachable from exactly one site.d650f62: renamed toDropReason::ThumbnailLengthMissing, single-purpose and named for thatsite, before a release freezes the discriminant.
Round-3 notes that remain open, unchanged:
beside the code (see decision 17). The writer fix stays in gamut-exif: to_bytes re-emits a thumbnail pointer whose bytes the report says were dropped #548.
deep_blobfixture patches a sentinelJPEGInterchangeFormatvalue afterwritelaysthe directories out. Deliberately kept: the fixture is built from named parts and asserts the
sentinel names exactly one value field, so a collision fails loudly; that is more legible than a
checked-in golden blob, whose contents a reader would have to decode to review. A reviewer who
disagrees should say so.
Malformedfixture intests/report.rspoints theExifIFDpointer at offset 1.Deliberately kept, for the same reason: it is constructed for the property the case needs
(in-bounds and unparseable) and says so in its own comment.
New in round 4:
that has never been published, one error string, and documentation. The only behavioural change
in the PR remains the one decision 12 took and round 3 measured.
New in round 5:
a single line added to
parse's# Errorslist.mise run mutants-diffover the whole branchreports 0 missed, confirming no executable behaviour moved. The only behavioural change in the
PR remains the one decision 12 took, round 3 measured, decision 19 grounded in Table 21 and
decision 22 has now put in the README.
src/tag.rs:4has a broken intra-doc link (exif_tags!). Pre-existing onmaster, in afile this branch does not touch, and already filed. Not repaired here.
#574should now be closed as answered for the offset-only half. Decision 19 givesthe table reading that makes the unconditional rule defensible, so the "should the refusal be
conditioned on
Compression?" half of that issue has an answer a human can act on. This rundoes not edit or close filed issues, so it is left as a note here.
New in round 8:
macro-expanding the
--all-featureslib target at54c550a2and ata6529623, stripping doclines, and comparing: 3 098 lines and sha256
f72897dd8d4100b022173ba1ffc0b2f3a0a86491cdaf077b3fad162a0305f292at both ends,diff -qidentical. The only behavioural change in the PR remains the one decision 12 took, round 3
measured, decision 19 grounded in Table 21 and decision 22 put in the README.
Manifest revision request:Granted, and resolved incrates/gamut-exif/tests/report.rs. Two of the five sitesdecision 31 corrects live there (lines 303 and 342).
0bfdb5e2(decision 38): both sites now name the thumbnail-format axis, matching the three
src/sites.No site in the crate still says the table is keyed on
Compression.line count. The
#[test]macro embeds each test's source position in itsTestDesc, so a doccomment that gains a line moves
start_line/end_linefor every test below it. Recorded herebecause it is a property of the method, not of this change: a later round using the same proof on
a test file should expect the digest to move, check that only those fields moved, and publish
both digests as this one does. On a lib target the method stays byte-exact.
## Compatibilitya convention or a one-off? This cratenow carries a dated changed-verdict section because a registry reader cannot otherwise date the
change. Decision 36 keeps it and declines to propose a repo-wide README convention on one
instance. If it should be one, that is a repository decision, not this branch's.
src/tag.rs:4still has a broken intra-doc link (exif_tags!). Pre-existing onmaster, ina file this branch does not touch, and already filed. Not repaired here.
human reads afterwards.