4959b48386e0190dd4688695bc7ed346429c667e
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matthew Jackson
|
4959b48386 |
Bind borrowed stream labels by the stream they name, not by a slot
A vendor label's `stream_number` is a slot in the one stream table its
config blob describes. The labels merged in from the playlists — to
cover streams the vendor named nothing for — carried a different number
entirely: a dense counter over every distinct stream found while
scanning the whole disc in directory order, related to no playlist's
slot numbering at all. Two coordinate systems, one field name. The
merge matched them by equality and the binder then counted streams
against the result.
Measured over the 44-image corpus: 22 discs merge such labels; of the
566 places one lands on a stream, 443 (78%) are a stream it does not
describe — the label states the PID it read itself from and it is a
different one. 142 of those are stopped by the language check
|
||
|
Matthew Jackson
|
1df36c78b2 | Merge branch 'fix/paramount-forced-sub-semantics' into dev | ||
|
Matthew Jackson
|
6d2ff4d1fc |
Read the vendor forced-subtitle field as the enumeration it is
One vendor's playlists.xml carries a per-subtitle-slot cell that looks
like a boolean, and it was parsed as one: value 1 meant forced, anything
else meant not forced. Across every image in the corpus that uses this
format the cell takes four values, and 1 is not the forced one.
Decoding three of those discs and counting every PGS display set:
* 1 marks a FULL dialogue track that additionally contains some
forced-narrative signs. All nine cells bearing it on one disc are
full tracks of 949-1411 display sets; all seven on another are full
tracks of 1602-1651. Neither disc has a small track among them.
* 2 and 3 mark a DEDICATED forced-narrative track, in its own trailing
stream slot, duplicating a language that already holds a full track.
The two 2 slots measured are 15 and 10 display sets with every one
flagged forced; the four 3 slots are 7, 14, 23 and 59 against
1216-2655 on the tracks they duplicate.
So the old reading was wrong in both directions — it flagged full
dialogue tracks forced, which is how one language came to present as two
identical full subtitle tracks with one of them marked forced, and it
threw away the cells naming the real forced tracks.
Content could not have corrected this afterwards. Clearing a wrong
forced label needs a disc whose authoring sets forced_on_flag, and on
the measured disc carrying four genuine forced tracks not one display
set anywhere sets it — there, the vendor cell is the only evidence there
is. The classification is now explicit: only a dedicated forced slot
earns the flag, an unrecognised value never does, and the
contains-forced-signs value is dropped rather than weakened into a
forced label, since a wrong forced flag on a full dialogue track is the
user-visible defect while a missing hint costs nothing.
Four of the crate's own tests had been asserting the boolean reading;
their subject was positional alignment, so they keep it and now use a
real forced value. The other two parsers that emit a forced qualifier
from vendor metadata were audited and are structurally immune — in both,
the forced marker names a slot of its own rather than hanging off a full
track's entry, so the failure has no encoding there — and each is now
pinned by a test saying so.
|
||
|
Matthew Jackson
|
94377c75fd |
Bind vendor stream labels by PID, not by per-title ordinal
A vendor label list describes ONE playlist's stream table, but
apply_labels re-numbered it from 1 inside every title. Where sibling
playlists cover the identical feature clip and enumerate different
subtitle sets, identical ordinals resolve to different PIDs, and the
same physical stream came out flagged forced in one title and not in
the other. On the title such a disc offers as its rip target that put
`forced` on an 873 MB full-dialogue English subtitle track — the
reported "I see English and English (Forced), they are identical".
The blobs are not wrong; the binding was. The list carries no playlist
id, but it carries a language per slot, and that sequence is a
fingerprint: on the corpus exactly one title's per-type language
sequence reproduces the list position for position, and content
confirms that title's binding is the correct one. So binding is now
two-tier:
* The title whose whole per-type language sequence sits on the list
(>= 2 streams, longest wins) is the ANCHOR — the table the list is
describing. Each of its slots yields a `(clip, PID) -> label` fact,
and a PID is the same elementary stream in every playlist that
plays that clip, so sibling playlists bind through the map. A slot
the anchor never showed us is not bound at all.
* Streams no anchor fact reaches still bind by the STN ordinal, but a
label whose language contradicts the stream it would land on is
dropped. Subtitle labels carry nothing but the qualifier, so an
unverifiable one is all risk and no gain: off the authoritative
path they additionally require both sides to STATE a language and
state the same one. Unlabelled beats mislabelled — the muxer's
demotable() guard can only clear a wrong `forced` on discs whose
authoring uses forced_on_flag, and half the measured discs never
set it.
Measured over 44 disc images, 9 change and every cross-title label
conflict goes away: 171 forced flags that contradicted a sibling
playlist are cleared, 61 correct ones are recovered on playlists that
had been missing them, 5 subtitle qualifiers and 1 audio purpose bound
against a contradicting language are dropped. No title gains a label it
did not have.
Known residual: on one disc the featurette playlists keep two forced
flags (down from nine) where a shifted list happens to coincide on
language. Ruling those out needs the list's provenance — which slots
are the vendor's and which were merged in by the MPLS gap-fill — and a
whole-sequence gate without it costs correct flags on discs whose
vendor slots are interleaved with gap-filled ones.
|
||
|
Matthew Jackson
|
0d4aab99df |
Stop naming specific commercial discs in the AACS and codec comments
The same scrub as the previous commit, over the files it did not reach: the variant-MKB layout notes, the 2.1 segment index observations, the PPS-revert regressions and the playlist-twin tiebreak. Measurements keep their numbers — "a v70 `0x2d` body = 46_100*2 + 16" is the useful part, and the title it came from never was. |
||
|
Matthew Jackson
|
b93d10082d | Merge branch 'fix/pgs-forced-probe-sampling' into dev | ||
|
Matthew Jackson
|
17bb13b077 |
Do not call a track forced off one display set of a sample
A forced verdict is an absence claim, and a sampled run sees a fraction of a track. Measured on real discs: tracks exist that flag about a quarter of their display sets and leave the rest unflagged, so a sample that catches one flagged set and nothing else promotes a full dialogue track to forced -- which players then force on screen. A sampled run now needs two display sets; a run that read every extent end to end has no unread gap and may still promote off one. |
||
|
Matthew Jackson
|
b68765fe84 |
Stop naming specific commercial discs in comments and tests
Fifteen references across six files named the discs a defect was first seen on. The parser leak found earlier was not an isolated slip — the same habit runs through the mux comments, the changelog and the AACS content verdict, where a title name was standing in for the shape of the problem. Every one is replaced with the property that actually mattered: a multi-clip title, a UHD Dolby Vision profile 7 dual-layer stream, a disc carrying an authored-bad TS packet. The comments are more useful for it — the reader needs to recognise the shape on a disc they have, not the one we happened to have. `SEG_MainFeature` stays: the parser matches on that literal, so it is a format token rather than a title. |
||
|
Matthew Jackson
|
abffa4235a |
Halve the seeks: eight 32 MiB windows, not sixteen 16 MiB ones
Measured on a real title: the same 256 MiB budget cut into sixteen windows per extent took 41.8s against the head-first read's 6.6s, because every jump collapsed the source's read batching into three-sector calls. Expected observations depend on total bytes read, not on how finely they are cut, and measured subtitle density (a display set every ~30-50 MB of clip) makes a 32 MiB window about even money on its own. |
||
|
Matthew Jackson
|
c94e9f4fb7 |
End a label section where the next one's stream list begins
The pixelogic walk finds the feature playlist's section by name and ends
it at the next `SEG_`/`SF_`/`FPL_` marker. Those markers are section
NAMES, and a project's trailing sections — the per-language notice,
disclaimer and dub-credit cards — carry none. On 8 of the 11
affected-format discs in the corpus the feature playlist is the last
NAMED section in the blob, so the terminator never fires and the walk
consumes the whole tail of the file as more of the feature's stream
list.
The card names are `{lang3}_{card}`, which passes `is_stream_token`, so
each one advances an STN counter, and a card whose name collides with a
catalogued component emits a label outright. Measured on the worst disc:
95 entries past the end of a 9-audio/21-PG list, five phantom audio
labels at STN 10-14 from `*_AC` notice cards (`AC` reads as the AC-3
codec), and 94 uncatalogued-component occurrences — which also took the
parse from High to Medium confidence and fired the vocabulary-gap
warning on four components that are deliberately not catalogued. A
second disc fabricated one subtitle label from a token in a following
playlist section named `FP_SingAlong`, which `FPL_` does not match.
What every section has, named or not, is a stream list that opens with
its video slots. So a `Video Stream N` entry repeating one this section
already listed is the first entry of the NEXT section, and ends this
one. Distinct video entries are kept, since a section may legitimately
list a secondary video stream; the memo of them is bounded at the BD STN
table's ceiling so disc bytes cannot grow it.
Replaying all 11 blobs through `assign_labels` before and after: the two
discs above lose exactly their phantom labels (11→6 and 5→4), the other
nine are byte-identical.
One residue is pinned rather than papered over: a card's name precedes
its own section's video slot, so a forward-only walk can still count the
FIRST card after the last real slot. It sits at the tail of a list
nothing follows in, so it can renumber nothing — at worst it costs a
parse its High confidence.
No other parser in src/labels/ walks a flat entry sequence with a
terminator set; the rest scope each stream to a structural range or read
its number off the entry itself. paramount and criterion gain immunity
pins for the boundary property specifically: a stream list cannot run
into the next element's, and a missing element boundary shortens the
list rather than extending it.
|
||
|
Matthew Jackson
|
28d5897b86 |
Apply the shape test to a mixed track too
A track that flags some of its own display sets and not others proves the authoring house makes the distinction, so it needs no sibling to corroborate the flag being in use -- but it still has to look like a full dialogue track before a forced label is cleared. A small track with a couple of flagged signs is a forced track, and demoting it is the mistake the shape test exists to prevent. |
||
|
Matthew Jackson
|
3ed8630535 |
Take a lone sample window from the middle of its extent
A title cut into 50-odd clips gives each extent one window's worth of budget. At the extent's head, every clip is sampled at the same relative position and the first clip's window lands on the opening of the feature -- the one stretch with no subtitles in it. |
||
|
Matthew Jackson
|
d0d8e2c9bf | Record the label numbering and vocabulary fixes in the changelog | ||
|
Matthew Jackson
|
01d4a1ba00 |
Catalogue the DUB forced-narrative marker and report vocabulary gaps once
Two halves of the same subtitle-labelling bug class. The numbering half
landed already: an entry the parser could not parse still occupies an STN
slot, so skipping it shifted every later label onto the wrong stream. This
is the other half — an entry the parser counts correctly but cannot
INTERPRET.
`DUB` names the forced-narrative subtitle track authored to accompany a
language's dubbed audio presentation: the signs and on-screen-text pass a
viewer still needs once the dialogue itself is dubbed. It is the same
editorial class as `*_TXT_FOR_`, spelled differently by some authoring
runs. Uncatalogued, it matched no component arm, so the token signalled
neither audio nor subtitle and the whole stream record was dropped at the
domain guard — a genuine forced track left with no forced qualifier even
after the numbering was right.
Evidence, from two independent discs in the corpus: the token appears only
inside the PG list, embedded in an otherwise contiguous run of
`{lang}[_{region}]_TXT_FOR_` siblings — one forced-narrative slot per
localized language — and takes exactly the slot where that language's
forced entry belongs. Both discs also carry that language's FULL subtitle
track as a separate, separately-labelled slot, so the DUB entry is not it.
The stream it lands on is a sparse PG track, the signature of a forced
pass rather than full dialogue. Deliberately not added to vocab::qualifier:
that maps free-form English label text, where a bare "dub" means dubbed
AUDIO. The forced-subtitle reading is specific to this token grammar.
The corpus sweep that found it also produced four components that are
deliberately NOT catalogued. They are per-language notice and disclaimer
clip names that merely collide with the `{lang3}_{component}` token shape;
each occurrence sits in a one-video-stream section next to the disclaimer
entry it names. They carry no editorial meaning, and mapping them would
attach a qualifier to a stream on the strength of a filename.
Which is also why an unmapped component now reports once per parse rather
than once per occurrence. It was a debug line nobody reads, and that is how
this gap survived to a user complaint; but a per-occurrence warn would bury
the signal under dozens of routine collisions on an ordinary disc. One
bounded, deduplicated line names the distinct components and says plainly
that any forced/SDH/commentary meaning they carry went unapplied. The
backing set is capped and truncates by chars, not bytes — the components
come from untrusted disc bytes, and a byte-offset slice can split a
multi-byte sequence and panic.
Two existing tests encoded the wrong behaviour and are corrected: the STN
numbering test expected the forced run to skip the DUB slot, and the
unclassifiable-slot test used DUB as its example of a token with no
meaning. The latter now uses a genuinely uncatalogued component.
|
||
|
Matthew Jackson
|
5c8b4dc7c5 | Note the bounded display-count over-count on a stalled read retry | ||
|
Matthew Jackson
|
841aa1a1c6 |
Drain a sampled run's tail and merge repeated extent memos
A window's last PES stays open in the demuxer until the next PUSI, which under sampling is in a different window or nowhere — so the last display set of every window was discarded, worst where the sample is thinnest. Flush the demuxer at the end of each window. A playlist that lists the same clip twice now merges the second read into that extent's memo (facts OR'd, display count MAXed, coverage the larger of the two) instead of overwriting it. |
||
|
Matthew Jackson
|
14049bb477 | Document the forced-probe redesign in the changelog | ||
|
Matthew Jackson
|
8ffce6b621 |
Sample PGS across the title instead of reading its head
The content-based forced-subtitle probe spent its whole 256 MiB budget
on the first sectors of a title. A feature's subtitles begin minutes in,
so the probe read the opening logos, hit the budget, observed no display
set at all and contributed nothing to any verdict — the vendor label was
always the only input.
The forced predicate is asymmetric: one non-forced display set disproves
forced permanently, while proving forced needs the whole track, and
genuine forced tracks are tiny where full tracks are huge. So the same
budget is now SPREAD over each extent in ~16 MiB windows placed on the
AACS unit grid, sized in proportion to the extent, ending at the extent's
end. Cost is unchanged; placement is not.
Also:
* Per-track early exit. A track that is disproven (and whose label
needs no correcting) stops asking for budget; an extent that owes
evidence only for such tracks is skipped outright, and evidence
already in the cache is never demuxed a second time.
* Content may now DEMOTE a wrong vendor forced flag, in the probe and
in the muxer, behind one shared guard: absence of forced_on_flag
only means something if some other track demonstrably uses it, and
the track must have the shape of a full dialogue track rather than
of a forced-narrative one. On a disc where no track sets the flag,
nothing is demotable.
* A sampled or budget-cut extent's evidence is memoised with the
COVERAGE behind it. It used to be filed under the extent's full key
and replayed to playlists that would have read far more of the clip,
turning a prefix into an absence claim about the whole extent.
|
||
|
Matthew Jackson
|
9fda690d00 |
Number vendor label streams by STN slot, not by parsed entry
Sweep of every parser in src/labels/ for the numbering bug fixed in
pixelogic: a blob lists one entry per STN slot, but the parser advances
its per-kind counter only for entries it can use, so every entry it
skips shifts all later labels onto the wrong stream.
Three parsers were affected; the rest key each label off a number the
blob states outright and are immune.
* paramount — `aud` / `sub` are the STN-ordered stream lists, and
`forced_sub` / `*_com1_idx` index those same cells. A cell with an
empty language was skipped without consuming its slot, so every
label behind it bound one stream early while the vendor's own
positional indices still pointed at the raw cell. `stream_number`
is now the cell's 1-based position. The forced flag is the payload
here, so the shift lands `forced` on a full-dialogue track.
* mpls_universal — its counters must agree with the stream list
`disc::bluray` builds from the same STN entries, since that list is
what `apply_labels` counts against. They disagreed twice: a
`coding_type == 0` padding entry was counted here and dropped
there, and a PG coding_type in an audio STN slot (a layout
`mpls::parse_stream_entry` has a dedicated arm for) was counted as
audio here and built as a subtitle there. Both rules now live in
one `label_type_for`.
* deluxe — a binding construction whose Language `getstatic` did not
resolve was skipped outright. It is still an STN slot; it just has
nothing to label. It now advances the counter, with the list it
belongs to taken from its CodingType argument or, failing that,
from what its binding type's resolved siblings showed.
Two paramount tests asserted the renumbering as if it were the spec
(`empty_middle_slot_does_not_inflate_stream_number`,
`audio_stream_numbering_skips_empty_slots`) and are rewritten. Immunity
pins added for ctrm, dbp and criterion so the property cannot rot.
Also scrubs two commercial disc titles from the menu-graphic filename
examples in png_filenames and vocab.
|
||
|
Matthew Jackson
|
f53abfe0d4 |
Stop naming a specific commercial disc in the label parser
Two comments identified the disc the STN-numbering bug was found on by its vendor project name. The reproduction does not need it: what matters is the SHAPE of the token list — placeholder slots, region-only tokens, an uncatalogued component — not which release happened to exhibit it. Both now describe the shape. The remaining `SEG_MainFeature` references stay. That is a vendor section name the parser matches on at pixelogic.rs:88, not a disc identifier — it is the format's vocabulary, like `FPL_` or the `eng_MLP_` stream tokens beside it, and removing it would break the parser. Also drops the last prohibited citation from the changelog: an mp4:// bullet said "no ffmpeg". The website changelog page is REGENERATED from this file at release time, so a scrub of the site alone would have been reverted by the next release. |
||
|
Matthew Jackson
|
80be34bff5 |
Number pixelogic subtitle labels by STN slot, not by parsed token
A pixelogic feature section lists one entry per STN slot. Only the
entries that parse were advancing the counters, so every surviving
label was renumbered 1..N and applied to the wrong stream.
Three kinds of entry were being skipped:
* `PG Stream N` placeholders — the subtitle twin of `Audio Stream N`,
which was already counted. The old comment claimed the corpus showed
subtitle tokens align without counting them; on a disc that has both
placeholders and later editorial tokens they do not.
* region-only tokens (`fra_CF_`, `spa_LS_`) — REGIONS sets `variant`
but neither `is_audio` nor `is_subtitle`, so the token is dropped.
* tokens whose distinguishing component is uncatalogued (`jpn_DUB_`).
On UHD_Crime101_WW_150728 the PG list is 18 slots: five placeholders,
four region-only tokens, and `jpn_DUB_`, with the seven `*_TXT_FOR_`
forced-narrative tokens at STN 11-16 and 18. Counting only the ten
that parse put `eng_SDH_` on STN 1 and the seven forced markers on STN
2-8 — the disc's FULL subtitle tracks. `labels::apply_labels` turns a
Forced qualifier into `SubtitleStream::forced`, which the muxer writes
as Matroska `FlagForced`, so the output offered an "English (forced)"
track carrying 1731 display sets of complete English dialogue while the
real 19-set forced track went unflagged.
Unclassifiable entries carry no stream type, so they advance the list
currently being enumerated; sections run video -> audio -> PG and
`domain` follows the last entry whose type was known.
|
||
|
Matthew Jackson
|
4447bd60ce |
Record the 1.6.0 fixes in the changelog
The fvi provenance fix, the key-service outage codes, the settings BOM data-loss fix and the per-track-kind picker rows all landed on dev without a changelog line. A release that ships undocumented fixes is one nobody can audit later. |
||
|
Matthew Jackson
|
c054893540 | Merge branch 'fix/key-service-outage-not-missing-key' into dev | ||
|
Matthew Jackson
|
6758370f8d | Merge branch 'fix/fvi-source-provenance' into dev | ||
|
Matthew Jackson
|
b2a274782a |
Report a key source that could not answer as its own failure, not as "no key"
The online key service returned HTTP 502 for about seven hours. Every rip in
that window ended with
key: online > no entry > NO KEY
Error: E7022 No key source has a decryption key for this disc (id: 422EB...)
which reads as "this disc is not in the key database". Operators went hunting
for a VUK that was never missing; the correct action was to wait.
E7022 is a claim about the WORLD: every source answered, and none holds a key
for this disc. A source that could not be reached made no such claim -- nothing
at all was learned. `resolve_and_apply_traced` collapsed the two anyway, with
its own comment naming the incident and pointing at the fix.
Three codes for the three different operator actions, each a variant with a
number and no English (the library ships none):
E7028 KeyServiceUnavailable unreachable / DNS / timeout / 5xx -> wait
E7029 KeyServiceUnauthorized 401 or 403 -> fix token
E7030 KeyServiceRateLimited 429 -> back off
None carries a payload: the key-service URL and its resolved address are
operator-confidential and must not ride out in a Display an operator pastes
into a bug report.
`resolve_and_apply_traced` now keeps `Err` and `Ok(empty)` apart. A source that
answered and holds nothing still records `KeyNode::NoEntry` -- that is the one
true "I looked, it is not there". A source that FAILED records an empty path,
and its reason is stamped onto `Disc::aacs_error`, the channel
`ensure_decryptable_keys` already reads for the E7017-vs-E7022 split. The gate
gained three arms alongside `AacsVidUnavailable` and raises the source's own
code.
`KeyOutcome` deliberately gains no variant. It is matched exhaustively by every
front-end's trace renderer (freemkv's `pipe::render_resolution_trace`,
autorip's `keysource::render_resolution_trace`), and this fix must not become a
breaking change across four repos to say something the error code already says
precisely. Dropping the false `NoEntry` node is enough for the trace line.
The three codes join `is_disc_level_no_key`: a service that is down, refusing
the token or throttling is down for every title, so a rip loop must stop rather
than issue N doomed requests -- and on 429, dig the hole deeper.
`FetchOutcome::errored`, documented as unreachable-in-production, now fires for
real: the negative-result cache stops memoising a transient outage.
Red before green: `key_source_failure_is_not_reported_as_a_missing_disc_key`
drives KeySource -> resolve -> aacs_error -> ensure_decryptable twice over the
same disc and asserts the verdicts differ. With the old conflation restored it
fails on the trace node, and with that assertion removed it fails
`left: 7022, right: 7028` at the gate.
|
||
|
Matthew Jackson
|
cf7ee69fd5 |
Record the source, not the destination, in the FVI header
The `fvi://` arm of `output()` passed the destination `.fvi` path as `FviSink::create`'s `source_path`, so every index named itself as its own source. `SourceInfo::default()` supplied the rest, making `source.medium` always "file" and `source.title` always 0 — three header members wrong, where FVI_FORMAT.md §6.2 defines `source` as describing the input. Beyond the wrong data, it made the output unreproducible: two machines indexing identical bytes emitted different files purely from where they wrote them, and a local filesystem path leaked into a shareable file. `output()` cannot see the source, so thread the provenance down from the driver, which can: `mux_stream` derives a `SourceInfo` per `MuxInput` arm and passes it through `drive_mux` to `output()`. Per the one-method-per-action rule this is a signature change, not an `output_with_source()` variant; the parameter is `Option<&SourceInfo>` so a caller with no provenance declares none rather than back-filling the destination. `SourceInfo`/`Medium` become public API. What each arm can honestly reach: - Session: everything — device path, the caller's title index, the title's playlist, the scanned volume id. - Url: the source URL, its scheme's medium, `title_index`, and the playlist off the opened stream's scanned title. - Iso: the image path and playlist. The title index is not in `MuxInput::Iso` (it carries a scanned `DiscTitle`, which has no index), so it stays 0. - Live: medium and playlist. The reader is an opaque `Box<dyn SectorSource>` with no path, and again no title index. Unreachable members are left empty rather than guessed — the sink already omits the empty ones. |
||
|
Matthew Jackson
|
e008e71a17 |
Add the missing direct coverage for parse_stss
Every other sample-table parser (stco, stsc, stts, ctts) had a "count lie" test proving the declared count is bounded by what the box actually holds, and stco/stsc had their own arithmetic pinned. parse_stss had neither - no test in this file ever called it with real entries, only with a too-short buffer. Added the same two: two distinct entries read from their own offsets (catching the o = 8 + i*4 arithmetic and the per-entry bounds check), and a declared count of 3 backed by only 2 real entries (catching the same "trust the box, not the count" contract the other four parsers already had). |
||
|
Matthew Jackson
|
c59e1e3342 |
Pin the sample-table parsers' short-buffer safety and byte arithmetic
parse_stco, parse_stsc, parse_stts, parse_ctts and parse_stss all open with the same "if b.len() < 8" guard before reading count = be32(b, 4), but no fixture anywhere in this file ever called any of them with a buffer shorter than 8 bytes - every test builds a complete box. A <-to-== mutant of that guard only rejects a buffer of EXACTLY 8 bytes and lets everything shorter fall through to an out-of-bounds be32 read, and nothing was exercising that fall-through to notice. One test now drives all five through every length from 0 to 8. Also: co64's 8-byte offsets were never actually built by any co64 fixture in this file (only the 32-bit stco path was exercised), so its manual byte-by-byte u64 assembly was unconstrained the same way parse_elst's was. And sample_offsets's sidx only advanced correctly by coincidence in every existing fixture, because none of them placed three or more samples in a single chunk back to back - the only shape where reusing the wrong sample's size becomes observable. Documented the <=8 boundary as equivalent across all five parsers (and the matching start < end in sample_offsets): the per-entry guard immediately below always breaks on the first entry at that exact boundary, so both branches converge on the same empty result. Confirmed by re-running each mutation against the full suite. |
||
|
Matthew Jackson
|
39d9714ae7 |
Pin parse_esds_asc's two independent boundary checks
asc_len == 0 and end > b.len() reject for different reasons - a useless zero-length ASC, and a truncated one - and nothing distinguished the || from an && that would only reject when both are true simultaneously, or the > from a < that would reject the common case of an esds with bytes after the ASC (more child boxes, padding) instead of only a genuine truncation. Documented the | in read_descriptor_len's accumulator as the same shift-then-mask equivalent already recorded in audio.rs's BitReader::read: the shift always vacates exactly the bits the mask fills, so | and ^ can't disagree. |
||
|
Matthew Jackson
|
dd940583c7 |
Pin mdhd_language's length boundary and three unasserted stsd codec paths
mdhd_language's guard was written as "b.len() < off + 2" (reject too short) but nothing distinguished that from "reject anything not exactly off + 2" - a buffer one byte longer than the minimum has to keep working, and a buffer missing the field entirely has to return None rather than read past the end. parse_stsd's mp4a path had no test at all: the AAC codec_private extraction depends on both codec == Aac and body.len() >= 28 being true together, and with no mp4a fixture anywhere in this file neither half of that condition, nor the boundary itself, was constrained. Also added the same header-length boundary check parse_elst already had (< 8 vs <= 8 - proved equivalent this time, since the very next guard on the empty slice catches the <= 8 case too), and one test walking every recognised audio fourcc (ac-3/ec-3/mp4a/the four dtsX variants) so a deleted match arm for any of them fails loudly instead of silently dropping that track. |
||
|
Matthew Jackson
|
4b7e4ddbb3 |
Pin find_boxes_capped's cap boundary and its size-field byte offsets
Nothing asserted the scan actually STOPS at cap rather than one match past it, or that the declared box size is decoded from its own four bytes rather than an adjacent one - every existing fixture used sizes small enough that all but the last size byte are zero, so an index slip reading the wrong byte would read the same zero and go unnoticed. |
||
|
Matthew Jackson
|
5222458411 |
Pin Stream::read's MAX_ALLOC_BYTES boundary and that write always rejects
Stream::read has its own s.size > MAX_ALLOC_BYTES cap, a separate call site from read_moov's over the same policy - checked they agree (both reject strictly greater than the cap, exact cap allowed) and they do, so this is not one of tonight's one-policy-two-copies bugs. But the boundary itself and the one-byte-over case were unasserted, and Mp4Reader::write returning an error (mp4:// is read-only) had no test either. Building Mp4Reader directly in the test (its fields are private but visible within this module) over the existing FakeBigReader avoids a real 256 MiB backing file for the boundary case. |
||
|
Matthew Jackson
|
dd5118ee9c |
Pin per-track handler routing, PID arithmetic and the shared sample budget
Nothing asserted that a hdlr other than vide/soun gets dropped rather than folded into the audio branch, that the per-track PID formulas (0x1011/0x1100 + track_idx) use the right operator and the right operand, that a sample-less track still advances track_idx for the next one, or that the cross-track sample_budget is actually decremented (as opposed to grown or divided) by each track's real count. All four were reachable with a single track_idx == 0, which made every existing fixture blind to +/-/* confusion on these sites - track_idx never moved past 0 in any of them. Also let audio_trak_missing omit stsz, needed to build a sample-less track for the track_idx test. |
||
|
Matthew Jackson
|
a8db2435cd |
Pin elst byte-offset decoding and a zero-timescale boundary in elst_offset_ticks
parse_elst's existing tests used segment_duration/media_time values that were almost all zero or 0xFF bytes, so an index slip in the version-1 byte extraction (reading a neighbouring byte, or one outside the entry entirely) could return the same value by coincidence and the test wouldn't notice. Added a fixture with every byte distinct and nonzero so any wrong offset is caught. elst_offset_ticks's `empty_movie_ticks > 0` guard on the Some(mts) arm looked like a pure optimisation, but shifting its boundary lets an empty_movie_ticks == 0 call fall into the division instead of skipping it - and a zero movie timescale (unreachable through from_reader, which filters it, but not through this function's own contract) makes that division panic. Pinned the boundary directly so the function stays safe on its own terms. Documented nine further mutants as equivalent rather than chasing them: media_edits/odd_rate and the None-arm's empty_movie_ticks check only gate a tracing::warn!, never the returned offset, and parse_elst's b.len() < 8 vs <= 8 boundary computes the same empty Vec either way once available = 0 is worked through. Confirmed by re-running each mutation against the full test suite. |
||
|
Matthew Jackson
|
ff18d4c3c8 |
Close the MEDIUM mutation gaps across transport, labels and codecs
The remaining triage items after tonight's HIGH fixes: 1,290 lines, almost all tests. Covers disc/mod.rs's DVD scan path (with real minimal VMG/VTS IFO fixtures rather than mocks), drive/mod.rs, labels/class_reader.rs and labels/mod.rs — the two biggest untriaged survivor clusters in the crate — plus hevc.rs and ps.rs. One production change, and it is an extraction rather than a behaviour change: MacScsiTransport::open mapped the shim's negative failure sentinels to typed errors inline, where nothing could reach it without a real IOKit FFI call. It is now map_shim_open_error, so the mapping can be pinned. It matters because collapsing -5 into the DeviceNotFound catch-all turns "another process holds the drive" into "no such drive", and an operator chasing the wrong problem is worse than a blunt error. Gate green on the pinned toolchain including the secrets scanner. |
||
|
Matthew Jackson
|
f8ed0b99f4 |
Pin read_moov's box-size boundaries and MAX_ALLOC_BYTES exactly
read_moov's forward-progress guard (box_size < header_len, OR'd with the EOF check) and the MAX_ALLOC_BYTES cap were only exercised on inputs well away from their boundaries, so a mutation testing pass found the exact edges unasserted: a size-8 (header-only) moov, a box that overruns the file by exactly the amount the OR/AND distinction can see, and a payload of precisely MAX_ALLOC_BYTES. Added a shared FakeBigReader (lifted out of an existing test's local struct so a new test can reuse it) to exercise the MAX_ALLOC_BYTES boundary without a multi-hundred-MiB backing file. |
||
|
Matthew Jackson
|
8189da1b0c |
Document why audio.rs's bit-packing | mutants are equivalent
Every mutation-testing survivor in this file is a | with ^ flip inside a bitstream packer: BitReader::read's accumulate step, the push closures in dac3_box/dec3_box/ddts_box, and the multi-field extractions in parse_eac3 and parse_dts. All nine are the same shape: shift an accumulator left by exactly the width of the next field, then OR it in, so the two operands never share a set bit and | and ^ agree on every input. Confirmed by running cargo-mutants against just these nine mutations after the existing test suite (which already exercises each function's field values) - all nine still survive, as expected for a genuinely equivalent mutant. Recorded the reasoning once at BitReader::read so nobody spends time chasing it site by site. |
||
|
Matthew Jackson
|
698ba36ae4 |
Document the pack_language and detect_rate equivalent mutants
Three pack_language mutants and two detect_rate boundary mutants survive mutation testing with no test able to close them, and it's not for lack of trying: they're equivalent by construction. Recording the proofs next to the code so nobody re-chases them: - (b[0] - 0x60) as u16, shifted << 10 then truncated to u16, is congruent mod 65536 to (b[0] + 0x60) as u16 shifted the same way, because 0x60 * 2 * 1024 is an exact multiple of 65536. The same swap on the second letter (shifted only << 5) is NOT equivalent, which is why only the first letter's mutant survives. - The two | with ^ mutations that OR the three packed fields together are equivalent because the fields (a lowercase letter minus 0x60, so 1..=26) always fit in 5 bits and never share a set bit once shifted into their 0/5/10 positions. - detect_rate's tolerance and tie-break comparisons only diverge from their <= mutants on an exact 0.5 fps distance or an exact tie, and a brute-force search over every achievable integer-nanosecond median found no case that lands on either boundary bit-exactly. |
||
|
Matthew Jackson
|
1eb8bdc9c7 | Merge branch 'mux-mp4' into dev | ||
|
Matthew Jackson
|
90a7fe2ff1 |
Assert the MP4 timing arithmetic, and name the faststart slack rule
MP4 track timing was numerically unasserted. Every operator in the PTS-to-ticks, duration, tkhd_dur and ctts chain could be flipped and the whole suite stayed green, because no test decoded an output file and checked a concrete number — the existing tests assert box presence and gross container shape only. That is the crate's worst failure mode: a title muxes "successfully" with silently wrong A/V sync or total duration, and nothing above can tell. The new tests build tracks with known PTS deltas and compare the emitted stts, ctts and tkhd.duration against computed values. Also lifted the faststart slack rule out of the match guard into faststart_fits(). A leftover hole of 1-7 bytes cannot be expressed as any ISO-BMFF box, since a box header is 8 bytes, so finish() must fall back to moov-at-end rather than write a free box that lies about its own size. The condition now has a name and a test instead of being an unexplained `g == 0 || g >= 8` inside a pattern guard. |
||
|
Matthew Jackson
|
4e70d9a5c5 |
Assert the undersized-buffer guard on the chunked read path
Drive::read and read_fua split any request larger than the transport's transfer limit into chunks and slice the caller's buffer by count * 2048. The up-front length check is the only thing between an undersized buffer and a "range end index out of range" panic out of a public API, and the comment above it records that this was once a live panic. Every existing read test stays on the single-chunk path, where an undersized buffer is already tolerated and returns Err(DiscRead), so the guard itself had no coverage at all and a mutation run flipped its arithmetic freely. The test drives a mock transport with a small transfer limit and asserts the two paths agree: an undersized buffer is an error either way, and behaviour on a caller mistake does not depend on the drive's transfer limit. Confirmed by hand that both the reported * -> + mutation and a < -> > flip now fail. |
||
|
Matthew Jackson
|
1b95d346bb |
Cite the H.264 spec directly, not a reference implementation
The escape-stripper comments named a third-party decoder as the authority for the cumulative-zero rule. This repo is public and does not cite other implementations; the rule is specified in ITU-T H.264 §7.3.1, which is the citation that belongs here anyway. No behaviour change — comments only. The scan-secrets gate caught it. |
||
|
Matthew Jackson
|
48663c6a2f | Merge branch 'mux-codec' into dev | ||
|
Matthew Jackson
|
a05f1d4498 | Merge branch 'mux-mkv' into dev | ||
|
Matthew Jackson
|
3ecb2510e8 |
Assert what the MKV mux and demux actually produce
A mutation run over mux/mkv.rs and mux/mkvstream.rs left 148 survivors.
Reading them turned up no wrong code, but a lot of code whose output
nothing ever looked at. Most of that is on the read side: parse_track
had a dedicated arm for Language, TrackName, FlagForced, Video and
Channels and not one of them was checked, so a re-mux could have lost
the audio language, the subtitle forced flag, every track label, the
resolution and the channel layout with the suite still green. Ten of
the eleven CodecID comparisons were unasserted too — only HEVC was
pinned — so any of them could have been mis-wired and the stream would
have gone to the wrong parser. The round-trip test now writes a real
three-track title through the muxer and reads it back through the
reader, and a separate test walks every registered CodecID.
The BPS statistics tag was the worst of the write side. Its test
asserted `file_bytes.contains("800")`, which a wrong bitrate passes
trivially — 80000 contains "800". Both the tag and the back-patched
Segment duration are now decoded and compared to a computed number, on
a title that declares no duration so the whole max_block_ticks →
seconds → bits chain is exercised. Cue points get the same treatment:
their CueTrack and CueClusterPosition were never read back, on either
the keyframe path or the i16-forced-split path, which are two hand-
written copies of the same three fields.
The rest closes arithmetic that only a bad disc reaches: a zero frame
rate or zero display-aspect denominator (both divisions), a
TimestampScale that does not fit an i64, a cluster timestamp of exactly
i64::MAX, a TrackNumber of 65537 that truncates onto the valid track 1,
and the shortest legal Block at both VINT widths. Two tests separate a
clean end of stream from a device failure: swallowing the second one
truncates the output at a bad sector and reports the rip complete.
Also pinned: only the first video and first audio track may be default
(the de-duplication lives in MkvStream::create and had no test at all),
the activation trigger is the first VIDEO track rather than track 0,
the measured field order reaches the file rather than just the helper
that computes it, and a Blu-ray 3D base/dependent pair builds the merge
instead of shipping two unrelated H.264 tracks.
96 of the 148 mutants verified killed by hand. Of the remainder, most
are equivalent — disjoint-bit `|` that `^` cannot change, delete-arm
mutants whose fallback is the same constant, guards on tracing calls —
and the write_frame branch at 1452 is unreachable: a cluster is always
open by the time it can be entered.
|
||
|
Matthew Jackson
|
048f125879 |
Kill codec mutation survivors and unify H.264's duplicated escape stripper
The mux/codec parsers (startcode, h264, hevc, dts) had 300 surviving mutants between them, and it turned out to be for the reason you'd fear: the exp-Golomb readers and the AU-boundary bitstream scanners had essentially no direct unit coverage, only indirect exercise through full-frame parse() calls that never touched the actual edge cases. Direct fixes to test gaps: - The shared BitReader's read_ue truncation guard (`leading_zeros > 31`) and skip_start_code's 4-byte-vs-3-byte boundary check had no test at their exact boundary. Added tests that hit the boundary precisely; a `>=`/`==`/`<=` typo either rejects a legal 31-leading- zero code or reads one byte past the buffer. - H.264's private SpsReader duplicates the same read_bits/read_ue shapes with no tests of its own at all (only reached through multi-field SPS parsing, several fields deep). Added direct tests. - HEVC's per-AU trailing-zero strip after the last NAL (no start code following) walks `end` down to trim padding; a wrong-direction typo there walks off the end of the buffer instead of terminating - exactly the "loop must make positive progress on malformed input" class. Added a test with a zero-padded trailing NAL. - HEVC's SEI match guards (`sei_mastering.is_none()` / `sei_content_light.is_none()`) implement "first HDR10 value in the title wins" - untested, and a naive test using both-messages-per-AU can't even exercise the guards because the whole-scan early return above them already handles that case. Split into single-message- per-AU tests that actually reach the arms. - parse_mastering_display/parse_content_light_level's length guards were `< N` with no boundary test; one-byte-short input now confirmed to return None instead of indexing out of bounds. - DTS's drain_front collapses duplicate offset-0 PTS markers after rebasing; untested, and the visible effect (front_pts()) can't tell a working collapse from a broken one since it already returns the right marker either way - the actual defect is unbounded growth of pts_marks over a long recording, so the new test asserts the bound directly across repeated drains. - DTS's dts_core_samples/dts_core_sample_rate header-length guard and next_core_boundary's syncword-length guard got exact-boundary tests the same way; also caught a nblks `<<`/`>>` direction bug candidate in the mutant (confirmed the real code is correct, just untested). Real bug found and fixed, not just a test gap: H.264's parse_sps_high_profile_ext re-implemented emulation-prevention byte stripping inline (a window scan: match `00 00 03` at position i, advance 3, else advance 1) instead of calling the existing unescape_ebsp_prefix used by slice-header parsing. On a run of 3+ real zero bytes ahead of an 0x03 - non-conformant, but this is disc bytes, not a spec-clean encoder - the two disagreed: unescape_ebsp_prefix's cumulative zero counter (matching the H.264 reference decode process and libavcodec's RBSP extractor) treats it as an escape and drops the 0x03; the window scan treats it as real payload and keeps it, corrupting the SPS bits read after it. Extracted the shared rule into `unescape_ebsp` (parameterized on output length so both the 16-byte slice-header prefix and the unbounded SPS case can share it) and pointed both call sites at the one implementation. Added a regression test pinning the shared function's behaviour on the input that used to separate them. All new tests hand-verified against the actual mutation (operator flipped or guard replaced by hand, confirmed red, then restored) per the mutation-testing brief, not just written and trusted. |
||
|
Matthew Jackson
|
65dbcb1ca6 |
Close mutation-testing gaps in the TS/PS mux (ts.rs, ps.rs, tsmux.rs)
A 12,330-mutant run left 159 survivors across these three files, all from missing assertions rather than wrong code — every gap here is a test, no production logic changed. Two shapes accounted for most of them: - Buffer-cap constants (MAX_PES_BUFFER_TOTAL, MAX_PS_BUFFER, MAX_BD_PES_PAYLOAD, PES_BUFFER_INIT_CAP) were only ever read by tests through their own symbol, so a mutated `*`/`-` in the constant's definition changes what the symbol itself evaluates to and every self-referential assertion still passes. Pinned each against a literal computed independently in the test. - Several `>`/`==` boundary checks on framing lengths (MPEG-2 pack header, system header, BD-TS adaptation field) were only ever exercised with slack in the buffer, never at the exact byte the check exists for. Added exact-fit cases for the pack header, system header, and psi_payload_base's AF-consumes-everything boundary. Real, higher-value gaps closed along the way: - ts.rs's per-PID discontinuity_flag and the NULL-TS concealment marker both require adaptation_field_length > 0 before trusting the AF flags byte; neither branch had a test proving af_len == 0 (no flags byte at all, ordinary payload underneath) is left alone. - header_remaining (PES header spillover across TS packets) only had single-continuation-packet coverage, which can't distinguish `-=` from `+=`/`*=` because the corrupted value never gets read again. Added a case spanning two continuations. - ps.rs's parse_stream_id_extension (used for HD-DVD 0xFD routing) walks nine optional PES-header/extension fields with a `pos +=` each; only the PTS/DTS pair had ever been exercised. One test now arms every field and checks the walk lands on the right byte. - find_ps_boundary's `sc + 3 >= len` guard had no test at sc == 0 with a bare 3-byte start code, the case a `+` -> `-` mutation turns into a debug-mode subtract-overflow panic on ordinary tail-of-buffer input. - tsmux.rs: an oversized video access unit must go out as a single unbounded-length PES; the `is_video || small-enough` guard that enforces this had no test with a video frame actually over the bounded-PES threshold, so a `||` -> `&&` mutant survived (it would silently split a keyframe across several look-alike-independent PES units). Also pinned the PES-length and PTS big-endian encodes at values above 255 / with bit 29+ set, where a `>>`/`<<` swap first becomes observable. Every test above was verified by hand: applied the exact mutation, confirmed the test fails (or the specific panic fires), then reverted. Left unclosed, all confirmed equivalent by hand-tracing rather than just left alone: - Every `<<8 | byte` PID/length bit-combine (ts.rs pid/PAT/PMT parsing, ps.rs dvd_audio_pid/hddvd_extended_pid/parse_pts): the two halves never share a bit, so `|` and `^` produce identical output for every input - no test can tell them apart. - ts.rs's `af_len > 183` check in process_packet: fully subsumed by the `payload_start >= TS_PACKET_BYTES` check three lines later for every af_len that could trip it. - ts.rs's out-of-range `pid_index` sentinel (-1 vs 1): unreachable, since a TS PID is masked to 13 bits (max 8191) and the table is always sized to at least 8192. - A cluster of "push an empty slice on an exact boundary" mutants in tsmux.rs's write_pes_chain (offset < hdr_len, af_bytes stuffing guards): the guarded write becomes a length-0 write_all, a no-op either way. Not reached this pass, for lack of a clean seam within the time available - ps.rs's extract_packets bounded-PES-length exact-fit checks (lines 278/282/303, the `sc+6>len` / `sc+6+pes_len>len` / force-flush cap arithmetic). The first two need a scenario where "proceed vs. wait one more byte" is observable in the packet list, and the third only shows up at a start-code offset (sc) that survives to the moment the cap check runs - in this code path sc is always 0 once an unbounded PES buffer starts accumulating, since nothing before it ever drains. Didn't find a construction in the time available; flagged rather than papered over with a self-referential assert. |
||
|
Matthew Jackson
|
b002da4221 |
Fail the identity probe when INQUIRY returns a short data phase
DriveId::from_drive issues three data-in commands. The two GET CONFIGURATION calls both clamp on bytes_transferred, with a comment noting it is device-reported and untrusted. INQUIRY, three lines above them, discarded it and decoded bytes 8..43 unconditionally. The buffer is pre-zeroed, so a drive answering GOOD status with a short or empty data phase — a USB-SATA bridge mid-wedge does exactly this — produced blank vendor, product and revision strings and a byte 0 of 0x00. Every platform enumerator gates on raw_inquiry[0] & 0x1F == the optical peripheral type, and 0x00 is DIRECT ACCESS, so the drive silently disappeared from the device list instead of reporting that its identity probe had failed. The operator sees no drive at all rather than an error. Anything shorter than the SPC-4 standard 36-byte header is now E9058 DriveInquiryShort, and the buffer is truncated to what actually arrived so nothing decodes past it. Exactly 36 bytes is still accepted: the vendor-specific tail is optional. This is the same defect as the READ CAPACITY short-transfer bug fixed earlier today, in the same crate, found the same way. |
||
|
Matthew Jackson
|
51d2b14d03 |
Give extract and analyze one label-parser tie-break, not two
mod.rs had two independent implementations of "pick the winning label parser". select_result owns the rule — highest confidence wins, and on a tie the earlier entry in PARSERS wins — and carries a regression test for a past bug where analyze() picked the LAST equal-confidence parser instead of the first. extract(), the path that actually ships, re-derived the same rule with its own inline scan and had no test of its own. The tie-break is load-bearing rather than incidental: array order encodes a trust ordering, with the hand-vetted parsers registered ahead of the ones that detect on any BD-J disc. A later parser winning a tie means the disc gets labels from a less trusted source, silently. extract now collects its candidates and calls select_result. Confirmed by flipping the tie-break to prefer the later entry: the shared test fails, where before the fix that mutation was invisible to the whole suite. This is the seventh instance tonight of one policy implemented twice with only one copy hardened, and the second in this file after the chapter mark_type filter. |
||
|
Matthew Jackson
|
c4ad4184ec |
Make the FMTS phase gate reachable by tests
An FMTS forensic segment interleaves two variants at the unit level: the disc carries both halves and we hold the key for exactly one parity. Decrypting the alternate half with our key produces garbage; leaving it as ciphertext is correct, because the muxer drops untouched ciphertext cleanly. The decision lived inline in apply_aacs_map's per-unit closure, where no test could reach it. A mutation run flipped the subtraction to an addition and the parity comparison, and every test still passed — so the gate that decides which half of a forensic segment we decrypt was entirely unasserted. It is now unit_is_our_phase(). The index arithmetic also saturates and clamps: both inputs come from the key map, so a unit below its own range start or a zero unit size means a malformed map, and neither may panic on debug overflow or a divide by zero inside a library a long-running service depends on. Four of the five surviving mutants now fail. The fifth, dividing by the unit size versus multiplying, is equivalent rather than uncovered: aligned offsets are exact multiples of the unit size, so the two differ only by a factor of unit_sectors squared, and unit_sectors is the constant 3 — odd, so parity is preserved on every reachable input. The proof is recorded in the test instead of a test that pretends to cover it. |
||
|
Matthew Jackson
|
528a6b7345 |
Walk past empty extents iteratively instead of recursing
fill_extents skipped an exhausted or zero-sector extent by calling itself, which costs a stack frame per skipped extent. Nothing filters sector_count == 0 out of a UDF or MPLS extent list, so a malformed disc declaring a long run of empty extents recursed once per extent before reading a single sector. Rust does not guarantee tail-call elimination, so that overflows the stack — which aborts the process rather than returning an io::Error, taking a long-running service down with it. The skip is now a loop. The regression test runs on a 256 KiB stack, where the recursive version dies and the loop finishes immediately. |
||
|
Matthew Jackson
|
fb321f51eb |
Reject a short READ CAPACITY reply, and count only entry marks as chapters
Two cases of the same shape: one policy implemented twice, with only one copy hardened. Disc::read_capacity decoded buf[0..4] from READ CAPACITY (10) without checking that the transport actually delivered four bytes, even though its comment claims to mirror decode_read_capacity — which has exactly that check, and documents why. A drive answering GOOD with an empty data phase leaves the buffer zeroed, so last_lba decodes to 0 and the probe reports a one-sector disc instead of an error. It now calls the shared decoder rather than re-deriving it. collect_chapter_summary filtered chapters on mark_type <= 1, counting the reserved type 0. PlaylistMark's own doc says filters must test == 1, and disc/bluray.rs did; the labels path did not, inflating the public chapter_count and letting a playlist whose only marks are reserved pass the chapter_count == 0 skip. Both sites now share PlaylistMark::is_chapter_mark so the copies cannot drift again. Both fixes were confirmed red before green. |
||
|
Matthew Jackson
|
e0ff0cfeb4 |
ci: make libfreemkv prove its five dependents still compile
Every job above proves libfreemkv builds; none proved anything built on it does. That gap bit today one level up — an engine signature change broke autorip and went unnoticed, because consumer CI only fires on a push to that consumer, and nobody pushed one. libfreemkv sits underneath all five dependents, so a break here costs more than a break anywhere else in the project. It is now the place the question gets asked, since it is the place the change happened. cargo check --all-targets only: each dependent has its own suite for its own behaviour. This answers the narrower question that went unanswered. |
||
|
Matthew Jackson
|
71686f1407 |
Lint the test code, and fix the 74 findings it had been hiding
Every other repo's CI now runs clippy with --all-targets. libfreemkv, the crate the other seven build against and the one held up as the reference workflow, was the last one still linting the library only — so its ~3,000 tests, by far the largest body of test code in the project, had never been linted at all. Turning the flag on surfaced 74 findings. Most were mechanical and applied with clippy --fix. The rest, by hand: - Four discarded Results in decrypt.rs. css::descramble_region returns a Result and four CSS tests threw it away, so a descramble that FAILED would have surfaced as a confusing buffer-comparison mismatch instead of the actual error. They expect() now. - A dead `kp` field on the PlantedWalk fixture. The test deliberately asserts Kp as the explicit AES-G3(dk, 1) relation from [C] §3.2.4 rather than against a stored value — its doc comment says so — which makes the field not just unused but a trap: the obvious "fix" of asserting against it would quietly weaken the test to comparing the fixture with itself. Removed. - Two hand-rolled ICB counters in the HD-DVD fixtures, a needless mut, three vec!s that only ever needed arrays, a filter_map whose every arm was Some, and a Vec::new()+push chain. - Doc list indentation in mkv.rs and mp4/read.rs, which was mis-rendering in the generated docs. - A five-[u8; 16]-tuple return type named FourLevelParts. Three lints are allowed at the specific sites, with reasons, because they are wrong for this domain: the underscores in the bitstream-header literals mark BITFIELD boundaries, not digit groups, so regrouping them uniformly would satisfy the lint by destroying the only thing they encode; and in three table-validation loops the loop variable is the domain value under test (a DTS SFREQ code, an AMODE value, a palette entry number), which is what the assertion messages name. |
||
|
Matthew Jackson
|
d50a7173ad |
Expose error_code so consumers can read a code instead of parsing one
io_error_code was private, so the predicates built on it (is_halt, is_skippable_title_stub, is_disc_level_no_key) were the only way to ask anything about an io::Error's origin. A consumer that needs the code itself — to report WHY a title failed rather than to branch on one of three known cases — had no route to it: mux_stream returns an io::Error, the typed Error is gone by then, and only the E<code> string prefix survives. That left every front-end to re-implement the prefix parse by hand, which is precisely the string-matching 1.5.x spent its time removing. One parser, exported. No behaviour change: the function is unchanged and the three predicates still call it. |
||
|
Matthew Jackson
|
e9811a1e01 |
ci: make the branch tip actually buildable
CI checked out one repo, but libfreemkv path-deps ../freemkv-unlock on the branch tip — release.sh swaps that to a git tag only inside the TAGGED commit, then restores the path dep on the branch. So the branch tip has never been buildable in CI by construction, and every green run we have ever had was a tag build. Windows and Linux were first compiled at release time, which is the worst moment to discover a build error. Both repos now check out into subdirectories (actions/checkout refuses a `path:` outside $GITHUB_WORKSPACE, and ../freemkv-unlock is outside), so the path dep resolves exactly as it does on a developer's machine. Every cargo step runs with working-directory: libfreemkv, and rust-cache is pointed at the same workspace. The sibling is taken from `dev`. On main/tag builds the Cargo.toml in that commit carries the git-tag dep instead, so the extra checkout is simply unused there. This is what makes the new branch policy's "dev must be green" achievable rather than aspirational. |
||
|
Matthew Jackson
|
f3841c8aca |
ci: build dev as well as main
Work now lands on dev and CI must be green there; main only moves at release time, to the tagged commit, so a push to main is the release validation run rather than day-to-day feedback. leak-guard already runs on every branch (on: [push, pull_request]) and release.yml stays tag-triggered, so neither needed a change. |
||
|
Matthew Jackson
|
7d48d820e5 |
fix(css): revert the hard-fail — it made real DVDs unrippable
I broke DVD ripping earlier today and the real-media acceptance gate caught it on its first full run. Greenland.iso failed with E7013 "Decryption failed"; reverting only this change made it rip clean in 8 seconds. That is a regression I introduced, not a pre-existing defect. WHAT I GOT WRONG. Round 9's crypto lens reported that descramble_region "descrambles with a key it just proved wrong" when the crib check rejects the cached key and the re-crack also fails. I agreed, and made it Error::DecryptFailed to match the AACS path, on the reasoning that CSS has no external key source so a failed crack on a readable sector should never happen. The premise was wrong. `attack_crib` is a HEURISTIC, not a proof: it finds a periodic run in the unscrambled header and predicts the run continues past 0x80. When that prediction does not hold, the crib reports a mismatch even for a CORRECT key — and the re-crack then fails BECAUSE the crib was never valid. So crib mismatch plus crack failure is the signature of a crib false positive, not of a stale key. The cached key is not proven wrong; it remains the best available evidence, and on a real DVD it is very probably right. Real discs hit this constantly. The deeper error was treating "no key" as one thing across schemes. An AACS unit key either opens a unit or it does not — the Verify-Media-Key relation decides it, and a wrong key is provable. A CSS title key is recovered from the data itself by an attack whose success varies sector by sector, so "the crack failed here" says something about THIS SECTOR's plaintext, not about the key. Unifying the policy was right for the schemes that can prove a key wrong. CSS cannot, and I folded it in anyway. decrypt_span keeps its shape and the cross-scheme test keeps its two AACS arms, with CSS now explicitly excluded and the reason stated. Three tests asserted the wrong behaviour and are corrected, including one I rewrote earlier today to pin exactly this. Every one of them passed the whole time the code was broken — because none of them had ever seen a real disc. The lesson is the one I kept stating and then did not act on: 3,013 unit tests, ~400 mutants killed and nine audit rounds did not catch this, and one acceptance run did. Synthetic media cannot reproduce what a real disc does. |
||
|
Matthew Jackson
|
42591c77fc |
test: constrain the AC-3/E-AC-3/DTS header decode and the boxes it emits
All 59 measured survivors in mp4/audio.rs: 50 killed, 9 proven equivalent, none left unaddressed. No production change — every extraction reads correct against ETSI TS 102 366 (5.3.2, 5.4.2, Annex E.1.3, F.4, F.6.1) and TS 102 114 5.3.1. It was a fixture gap, not a code defect, and a specific one: the existing fixtures gave several fields the SAME value (fscod=0, bsmod=0, lfeon=1, acmod=7) and asserted only derived channel counts. Nothing asserted the emitted dac3/dec3/ddts payload BYTES at all, so the packer's shifts and masks were entirely unconstrained. A wrong mask there does not crash — it writes a box declaring the wrong channel configuration, and a player believes it. Several kills needed fixtures designed to discriminate rather than merely exercise: the reduced-rate branch needed fscod == 3, which no test in the file reached — and `== -> !=` survived on an fscod=0 fixture only because the reduced table happens to return 48000 there too the DTS LFF mask needed a value where XOR and AND differ in MEANING: on the 5.1 fixtures LFF flips 1 -> 2 and BOTH codes mean "LFE present" the acmod mix-level skips needed three different acmods, because `^` is a no-op unless acmod == 4 exactly the ddts bitrate needed 44100/512, which does not divide evenly — at 48000 the rounding is invisible and `/ -> %` on den/2 survives The 9 equivalents are one pattern: OR-ing a shifted high part with a masked low part on disjoint bit lanes, where the mask is on the same line as the OR. Since cargo-mutants applies one mutation at a time, no single mutant can break both. Each was applied and observed green. FILED, not fixed: reserved sample-rate codes are silently guessed as 48 kHz (audio.rs:133, :20/:153) while a reserved DTS AMODE is refused with a comment explaining why. Same silent-wrong-metadata class, opposite answer. Refusing would make such a stream unmuxable, which is a product call. |
||
|
Matthew Jackson
|
2efe1425d6 |
refactor(decrypt): one orchestrator owns the no-key decision
There were TWO top-level decrypt paths: decrypt_sectors_impl for CSS and clear media, whose AACS arm was a bare `return Err` stub, and a wholly separate decrypt_sectors_mapped for AACS. Each scheme therefore decided its own answer to "there is no key for these bytes", and nothing held them to the same one. They drifted, in opposite directions, within a single release: css::descramble_region descrambled with a key the sector's own crib had just proven stale — garbage behind an intact clear header, reported Ok. the mapped path returned early for any LBA outside every range, before ever asking whether those bytes were ciphertext, so an unkeyable encrypted unit passed through and extract counted it as good. Both were fixed individually earlier today. This removes the shape that allowed them. decrypt_span is now the single orchestrator: it owns the loop, the refusal, and the loss count, and each scheme supplies only what genuinely differs. apply_aacs_map is a scheme step that reports what it could not open; it no longer decides what that means. The public wrappers (decrypt_sectors, _in_content, _mapped) are unchanged in signature and all funnel through it. Adding a scheme now means adding an arm here, which means answering the refusal question. That is the point. The new test asserts ONE verdict across all three schemes — AACS with no map, AACS with an encrypted unit outside every range, and CSS whose re-crack failed — plus that clear media is NOT a refusal. A per-scheme test cannot hold this: each would keep passing while the two disagreed. Flipping the AACS arm back to pass-through reds it. Also removes the last of the tracing-capture scaffolding. Serialising those captures crate-wide did not fix the 1-in-10 flake, and asserting the predicates directly made the helper, both capture subscribers and an unrelated dead OrderSink unused. Deleted rather than left behind. |
||
|
Matthew Jackson
|
b86f7aef17 |
fix(session): delete two dead accessors, make into_drive fallible
drive() and drive_mut() had ZERO callers — not in libfreemkv, freemkv, autorip, bdemu, keysources or kdb. Deleted rather than converted: dead public API that panics is not an API worth preserving the shape of. into_drive() had two callers and now returns Result. The empty-slot state is reachable through ordinary public use — stage_drive_as_reader moves the drive into the reader slot, and calling into_drive twice moves it out — so the panic was not guarding a caller error. identify() was converted for exactly this reason in this same release; the fix went to one of four public sinks and the other three were left. I deferred this on the assumption the blast radius was large. It was three call sites. Checking beats assuming. Also fixes a REAL FLAKE in the gate, which is worth more than the above. resolve_vid_only_bus_key_gate_reports_true_has_volume_id... failed about one full-suite run in ten while passing every time in isolation. It installed a capturing tracing subscriber to read back the has_volume_id field of a warn. That cannot be made reliable: dispatcher::set_default is THREAD-LOCAL while tracing's callsite-interest cache is GLOBAL. The original author knew, and called rebuild_interest_cache() — necessary but not sufficient. I first serialised every capture in the crate behind one lock (harness::with_captured_tracing, which also removed the same hand-rolled dance from three other sites). Still 1-in-10, because the cache can be re-evaluated against the process-default dispatch rather than the thread-local one. So the predicate is now a named function, handshake_has_volume_id, and the test asserts the VALUE. A boolean does not need a subscriber to check. The gate's hard-error behaviour keeps its own test. Measured: 14 consecutive full-suite runs, 2994 passed, 0 failed. A flaky gate is worse than a missing one — every green after it means less, and this one had been eroding trust in the whole suite. |
||
|
Matthew Jackson
|
5559987325 |
test(mux): replace a false-green discontinuity test with the property it named
a_signalled_discontinuity_survives_a_backstop_discard asserted that a SOURCE-signalled discontinuity on discarded bytes still reaches the AU that follows. It did not test that. Deleting the disc_marks push, or the mark-retirement loop inside discard_gap_before, left it passing. The mechanism: the `discontinuity = true` rode the FIRST over-cap push, which still has the next AU's delimiter at buf[0] — so it force-flushes as an over-long AU rather than discarding, and THAT AU consumes the mark. The assertion's `.find(|x| x.data.contains(&0x22))` then filters it out, and the flag it reads comes entirely from `pending_gap`, set by the second push's backstop. Behaviourally identical to the test 40 lines above it, under a name promising something else. I wrote it this morning, in the same commit that fixed a different test for having a fixture that never reached the code it named, while cataloguing that exact shape. Third instance today of writing the bug I was hunting. The two mechanisms cannot be isolated in one fixture — a fragment that trips the backstop sets pending_gap regardless — so they now get one test each. The replacement drives disc_marks end to end with no backstop involved: a flagged fragment that carries a complete AU and is emitted, not discarded. Nothing else pinned that path. Removing the disc_marks push reds it. Found by the round-9 opus escalation over test quality, dispatched because the sonnet pass over the same 17,000 lines of new test code returned zero findings. |
||
|
Matthew Jackson
|
72bcc371fb |
fix(io): a halted fsync is recognisable as a halt, not a hard failure
The three bounded-fsync failures returned bare io::ErrorKind values — TimedOut, Interrupted, Other/EIO. `is_halt()` matches on `io_error_code(e) == Some(E_HALTED)`, i.e. the "E<code>" prefix that `From<Error> for io::Error` mints, and that is documented as the ONLY recognised shape. A bare ErrorKind carries no prefix. So cancelling a rip while sync_all / finish was inside the bounded fsync made `is_halt()` return false, and the CLI reported a clean user cancel as a hard I/O failure at the end of an otherwise complete mux. All three were also mutually unclassifiable, which is the same information-loss the numeric-code scheme exists to prevent: a caller could not tell "cancelled" from "NFS wedged" from "worker died", and should not retry the third the way it retries the first. And their Display text is std English — "timed out", "operation interrupted" — reaching a user from library code, which this crate does not do. Now Error::Halted, Error::SyncTimeout (E_SYNC_TIMEOUT 9056) and Error::SyncWorkerLost (E_SYNC_WORKER_LOST 9057), on both platforms. E_HALTED also now maps to ErrorKind::Interrupted rather than falling into the 6000..=6999 InvalidData bucket. A stop is an interruption, not invalid data. Nothing branched on the old kind — every consumer uses is_halt() — so this is safe as well as more accurate. Two things worth recording. I first placed the E_HALTED arm AFTER the 6000..=6999 range arm and wrote a comment claiming it preceded it; match arms are ordered, so the range won and the comment was simply false. The test caught it. And the macOS test asserted only that each arm was non-Ok with a particular ErrorKind — it passed throughout the period the three were indistinguishable. It now asserts they can be TOLD APART, which is the property that actually matters. Found by the round-9 opus escalation over the API contract. |
||
|
Matthew Jackson
|
54d038e478 |
fix(udf): file_start_lba must skip a leading unrecorded extent
Regression from the round-8 change that started RETAINING ECMA-167
4/14.14.1.1 type-1 (allocated, not recorded) descriptors. Retaining them
is correct — dropping one slides every later extent's data down by the
hole's length, corrupting the file silently. But read_icb_extent still
took extents.first(), so the value it returns can now be a hole.
A type-1 extent's lba is where SPACE is allocated, not where bytes live.
IcbExtent's own doc says exactly that. file_start_lba hands the value
out as "the absolute starting LBA of a file's first data extent", and
ifo.rs uses it as the base for every VTS VOB extent:
file_start_lba(IFO) + vtstt_vobs + cell.first_sector
So a DVD whose IFO's first descriptor is type-1 reads its entire video
title set from the wrong place on disc. No error anywhere — the reads
succeed, they just land on unrelated sectors. Verified: the test reports
2900 instead of 2040 with the old code.
Same shape as file_extents/extents_abs_at dropping the recorded flag,
and the same root cause: one change taught read_icb_extents about a new
extent type and did not visit the accessors that consume its output.
Three of them; two are still open (task filed).
Found by the round-9 opus escalation over the API contract, dispatched
because the sonnet pass over the same scope returned zero findings.
Seven of its eight items were absences rather than wrong lines — the
class a wrong-line scan structurally cannot see.
|
||
|
Matthew Jackson
|
6868b93b7e |
fix(udf): retry the customary VDS location when the recorded one holds nothing
The Main Volume Descriptor Sequence was selected from the anchor's
declared extent whenever that extent's SHAPE was usable — length >= 16
sectors (ECMA-167 3/10.2.1), non-zero location, no address wrap — and
the customary location was tried only when the shape failed.
Shape is a property of the field, not of what is there. An anchor can
pass all three checks and point at nothing: a mastering tool that wrote
the reserve location, a stale anchor on a rewritten volume, or
deliberate corruption on an untrusted disc. The sweep then finds no
Partition Descriptor, partition_start stays 0, and the volume is
rejected as UdfNotFilesystem — while the real sequence sits unread at
the location the old fixed sweep would have found.
So the branch a DAMAGED disc actually takes had no recovery path, which
is backwards: that is the branch recovery exists for.
Now both are candidates and the fallback is retried on OUTCOME. This is
the same principle the Metadata File Location chain in this same
function already uses — treat the recorded value as a candidate, fall
back when it does not pan out — applied one level up. The sibling was
added in
|
||
|
Matthew Jackson
|
8d39ccc613 |
fix(decrypt): an encrypted unit outside every key range fails, not passes
decrypt_sectors_mapped returned early for any LBA no map range covers, on the reasoning that unmapped means clear filesystem or nav. "The map has no key here" and "there is nothing to decrypt here" are different statements, and only the second makes passing the unit through correct. On a multi-CPS disc an orphan clip — referenced by no playlist, so in no title extent and therefore in no range — hits the first and was treated as the second. The early return fired BEFORE the aacs_unit_encrypted gate below it, so nothing ever asked whether those bytes were ciphertext. extract_tree then counted them as bytes_good, dropped the .partial suffix, set complete = true and exited 0: a scrambled file on disk with a clean bill of health. FileResult's own doc already stated the intended contract — "unreadable sectors AND undecryptable units both land here (extract fails a bad decrypt loud)". The code did not implement it on this path. The fix is one line of symmetry. The split-unit branch immediately above already makes exactly this distinction, checking aacs_unit_seed_encrypted before refusing. This branch did not, so the same question got two answers eight lines apart — the duplication shape this release keeps finding. Deliberately NOT changed: the decrypt decision on an orphan is still to refuse rather than guess. Blind trial-decrypt is what the keymap-only model exists to remove, and extract has no CPS/forensic fetch source. The defect was never that we declined to key it; it was that declining looked like success. The test asserts BOTH directions — a clear out-of-range unit must still pass through byte-identical, because that is the ordinary whole-disc read and breaking it would trade one silent defect for a loud one. |
||
|
Matthew Jackson
|
8c0de5711e |
fix(mux): resync-gate drops reach errors(), including after the gap resolves
ResyncGate::dropped is zeroed the moment a keyframe disarms the gate,
and the only EOF warning fires for gates STILL armed. So a mid-title gap
that resolves left no trace anywhere — and most gaps do resolve. A rip
with several concealed gaps reported 0 errors and 0 lost bytes while
whole GOPs had been discarded, which disc.rs's own test comment calls
the ONLY channel through which loss is reported.
This is the other half of
|
||
|
Matthew Jackson
|
9f25a4c454 |
fix(mp4): refuse a video track with no resolved dimensions
Resolution::pixels() returned (0, 0) for Unknown, and the MP4 sink wrote it verbatim into tkhd (ISO/IEC 14496-12 8.3.2) and VisualSampleEntry (12.1.3). Both fields are MANDATORY there, so unlike Matroska — which omits the optional PixelWidth/PixelHeight elements — MP4 has nothing to leave out. The result was a structurally complete file that passes every container check, declares a 0x0 video track, cannot be rendered, and is written with no error anywhere. WHY IT WAS POSSIBLE, which is the part worth keeping: pixels() previously fabricated 1920x1080 for Unknown. That was wrong but playable, so this sink never needed a guard and the absence of one was invisible. Changing the sentinel to (0, 0) moved the defect instead of removing it — a zero PAIR still reads as a usable value, so the sink stored it and serialised it. The accessor's doc comment then ENUMERATED the callers it believed were safe: "the Matroska sink omits the optional elements, the VobSub writer omits its size: line, and no caller divides by either dimension." Two of those three are true. MP4 was not on the list because MP4 has no guard at all, and a prose list cannot enforce itself. mkv.rs's own comment even states the principle — "the check belongs in the one accessor rather than in each caller that remembered to write it" — and labels/mod.rs still carried its own duplicate Unknown test long after the accessor took that job over. So: pixels() now returns Option. Not because Option is tidier, but because every caller genuinely needs a DIFFERENT answer and the compiler is the only thing that reliably makes them choose one. Matroska and the metadata sinks take unwrap_or((0, 0)) with the reason stated at each site; the VobSub path degrades to a palette-only .idx; MP4 fails with E_MP4_UNKNOWN_RESOLUTION (9055). Six call sites, not the five my first grep showed — I piped it through `head` and acted on a truncated list. The compiler caught the sixth. That is the same mistake as trusting a lens that reported silence. |
||
|
Matthew Jackson
|
30bea12392 |
fix(css): no provable key is a hard failure, matching AACS
descramble_region descrambled with the key a sector's own crib had just proven stale, whenever the re-crack from that sector also failed. The clear header is not scrambled, so it survives intact: the sector still opens with a valid pack start and passes every structural check the PS demuxer applies. Only the payload is corrupted — exactly where nothing looks. Ok(0) dropped, exit 0. CSS has no external key source. The title key comes only from cracking the data, so on a READABLE sector "no key" is not a missing input, it is recovery failing on bytes we can see. That should never happen, and when it does the answer is not to emit something. Now Error::DecryptFailed — the same verdict the AACS path already gives for a unit no held key opens. Both alternatives to failing are bad data reported as success: descrambled with a rejected key it is garbage behind a valid header, and passed through untouched it is ciphertext where plaintext is meant to be. WHY IT WAS POSSIBLE, which matters more than the fix: There is no single place that owns "what do we do when there is no key". decrypt_sectors_impl looks like the central dispatch, but its AACS arm is a `return Err` stub — AACS decrypts entirely through decrypt_sectors_mapped, a separate top-level path. So CSS decided its own policy inside css/, AACS decided in decrypt.rs and mux/resolve.rs, and nothing held them to the same answer. The asymmetry was not an oversight; it was structurally permitted. How a disc decrypts is one process — resolve a key for this data, apply it, refuse if it cannot be proven. Only the resolve-and-apply step is scheme-specific. Filed as a task: the policy belongs in one orchestrator with the schemes supplying only what genuinely differs. Two tests changed rather than added, both of which pinned the old behaviour: the unit test asserted the sector was descrambled, and the integration test asserted the scramble flag was cleared, which is what descrambling-with-any-key does. Neither established that the result was CORRECT — the fourth bad-test shape. |
||
|
Matthew Jackson
|
b2b611fa3b |
fix(udf): a read fault locating the Metadata File is not a non-UDF disc
Regression I introduced in
|
||
|
Matthew Jackson
|
4f4b1ed222 |
fix(labels): make deluxe master-enum selection deterministic
identify_master_enums picks, for each fingerprint, the best candidate class out of CandidatePool. Its tie-break only prefers an exact ldc count over an inexact one, so two candidates that are BOTH inexact but both within LDC_COUNT_TOLERANCE are decided purely by iteration order — and the pool was a HashMap. Rust seeds HashMap per instance, so this is not merely unstable across runs: the new test resolves the SAME jar to both Alpha and Beta within a single process, across 16 iterations. The same disc could emit different commentary/SDH/descriptive labels on consecutive rips of unchanged input, with nothing in the output saying the choice was arbitrary. BTreeMap fixes it by construction rather than by a sort someone can forget to keep. The pool is capped at MAX_CANDIDATE_CLASSES, so the ordering cost is irrelevant. The test runs the whole identification sixteen times and asserts one distinct winner. A single run cannot distinguish deterministic from lucky, and the seed does not change within a process — so repetition is what makes this a test rather than a hope. Found by the round-9 labels pass, which was dispatched specifically because every one of the ten lenses had reported leaving deluxe.rs unread. 1,159 new lines that nobody had opened. |
||
|
Matthew Jackson
|
944e6a8b09 |
fix: align the Linux fsync error with macOS, and clear three stale docs
Round 9 findings, triaged and verified against the pinned tree.
writeback_file: a bounded-fsync WorkerLost returned bare ErrorKind::Other
on Linux where macOS returns EIO. Round 8 fixed the Linux arm to return
Err at all — the right fix — but stopped short of matching the value, so
a consumer distinguishing timeout / halt / lost-worker had nothing to
branch on for the third case on one platform. Now EIO on both.
Three doc comments described the pre-fix behaviour, one of them for
longer than the bug existed:
linux.rs durable_sync still said "all three fallbacks return Ok(())"
mod.rs sync_all still said Linux silently swallows fsync failures and
callers must not treat Ok(()) as a durability barrier
mod.rs SequentialSink::finish repeated the same caveat
All three now say what the code does: a bounded-fsync failure is an Err
on every platform, so Ok(()) IS a durability barrier. A doc that
describes a fixed bug is worse than no doc — it tells a caller to write
a workaround for something that no longer exists.
au_assembly: discard_gap_before duplicated drop_marks_before's
mark-retirement body verbatim and added one statement. Mine, from
earlier today. It now calls it. Two copies of the same retirement loop
is exactly how the two call sites would drift back together.
clpi: ClpiStream's audio_format / audio_rate / video_format / video_rate
are decoded from untrusted on-disc bytes on every parse and read by
nothing. The identically-named fields consumed in disc/bluray.rs belong
to mpls::StreamEntry, not to this struct — checked, because an earlier
round wrongly called a live function dead. Deleted, along with the seven
test assertions that pinned them; the tests that pin pid, coding_type
and language remain. Also removed a section-header comment orphaned by
the get_extents deletion, describing a fixture that no longer exists.
|
||
|
Matthew Jackson
|
5360f8d309 |
test: salvage the orphaned labels/disc triage, and extract build_labels
Thirteen agents triaging src/labels and src/disc died on a saturated
machine, leaving 5,836 insertions across 28 files uncommitted in a
worktree. Recovered by 3-way apply onto twelve commits of drift; zero
conflicts. The diff was archived to freemkv-private first, because a
worktree is not a backup and this one had already nearly been lost.
One production change, and it is the right one: mpls_universal::parse
read every playlist off the disc AND converted the entries to labels in
a single function, so the conversion — stream-type mapping, dedup key,
the dense global counters — could only be reached through a synthetic
UDF image. Extracted to build_labels(&[Playlist]), which unit tests can
drive from already-parsed values. Behaviour-preserving: same iteration
order, same skip-on-error.
Two collisions resolved by hand:
A second mod pass_progress_tests, written independently against the
same survivors as the one committed in
|
||
|
Matthew Jackson
|
8b8bcff106 |
test: pin five untrusted-input guards in the AACS 2.1 and CSS paths
Second pass over src/aacs and src/css. No production change; the only
non-test edits are two fixture bytes and one test rename.
Five latent panics on untrusted data, every guard correct and none
tested — so each was free to be deleted:
variant.rs:224 a 0x04 record not a multiple of 5 indexes p_uv[0..4]
off a one-byte tail
variant.rs:269 a 0x0c record shorter than the 0x04 slot count
slices past the cvalue table
stevenson.rs:177 short sector read -> index 138 into a 129-byte slice
stevenson.rs:208 a crib longer than the 1920-byte encrypted region
-> index 2058 into 2048
stevenson.rs:272 a header periodic all the way to offset 0 ->
subtract with overflow
That last one is reachable from ORDINARY DVD data — constant or padding
bytes are periodic. Verified on HEAD: widening the guard to <= 0x80
passes all 64 css tests unmutated.
media_key_variant_from_kp had only a soft-correction test, so every
step past that early return was unexecuted. The new two-slot fixture
puts the covering slot at index 1, so the uvs[1 + 5*idx] and
cvalues[idx*16] strides stop multiplying by zero.
derive.rs:319 + -> - confirmed killable, as the first pass predicted:
p == 0 makes (p-1)..32 underflow. Every prior fixture used a uv whose
lowest set bit was 4, 10 or 11, so trailing_zeros() was never 0.
One fixture bug caught and fixed rather than papered over: a |= mutant
first SURVIVED because mk[14]'s 0x04 bit happened to be set, making OR
and XOR agree. The byte is now clear and an assert_eq! pins it, so the
fixture cannot drift back into agreeing with the mutation it exists to
catch.
walk_mkb_be24_high_byte_is_honored renamed to
walk_mkb_be24_middle_byte_is_honored. Its 0x00_0110 length exercises
the << 8 term only, which is why << 16 -> >> 16 survived it. The name
was the lie; both framings are worth having, and the comment now points
at the genuine high-byte test at 0x01_0004.
derive.rs 146:32 and 154:30 stay untested, now with a proof rather than
a judgement: bit_pos == -1 requires current_v_mask == 0xFFFF_FFFF, and
calc_v_mask can never return that — its loop condition holds at
!v_mask == 0, so it always shifts at least once. Both branches are
reachable only after the walk has gone non-convergent and is heading
for the bounded exit, where the return value is undefined. Termination
is already pinned.
Equivalents proven by observing green, including six more OR/XOR pairs
on provably disjoint bit fields, and the two KEY_CORRECTION_DATA sites
where the constant is the documented all-zero placeholder so x ^ 0 ==
x | 0. Those become killable only if a real per-licensee KCD is wired
in.
A partial confirmation sweep (138 of 415 mutants before the box
saturated) found 135 caught, one timeout that is itself a detection,
and exactly one survivor — the KEY_CORRECTION_DATA equivalent above.
|
||
|
Matthew Jackson
|
d5a9e70700 |
fix(udf): read the Metadata File Location from the partition map
read_filesystem hardcoded the Metadata File's File Entry at block 0 of the physical partition — 'the metadata file ICB is at physical partition lba 0'. UDF 2.50 2.2.10 records where it actually lives, as a partition-relative Uint32 at offset 40 of the Metadata Partition Map. That field is the only thing on the volume that says where the entry is; block 0 is merely where authoring tools usually put it. On a conformant volume that recorded it elsewhere, block 0 holds something that is not a File Entry, metadata_start falls back to partition_start, the File Set Descriptor read there carries the wrong tag, and the volume is rejected as UdfNotFilesystem. Worse, a volume with a decoy file set at block 0 — as a rewritten or dual-structure volume can have — does not error at all: it mounts a different filesystem and reports success. Verified on HEAD: reverting the lookup reds four tests, e.g. the metadata partition beginning at 2000 where the map records 33754069. The recorded location is trusted only when the map's partition type identifier reads '*UDF Metadata Partition'. A Virtual (2.2.8) or Sparable (2.2.9) map is ALSO ECMA-167 3/10.7.3 Type 2 and records unrelated fields at offset 40, so its bytes must never be read as a location. Deleting that guard reds its own test. Block 0 stays in the candidate chain, so a volume whose map is absent or wrong but whose Metadata File does sit there keeps mounting exactly as before. This is additive, not a behaviour swap. Also 30 tests and ~55 more mutants across read_icb_extents, read_file_limited, read_inline_data, the prefetch stubs, parse_dstring and parse_udf_name. The metadata-partition branch — the branch EVERY real BD-ROM takes — had no test at all; nothing in the crate built a two-partition-map volume. Closes the max_bytes gap flagged earlier: 259 > -> == and > -> < now die on both the declared-size and the inline-ICB paths. Equivalents proven by application, notably two guards that read as protective but are unreachable: pm1_len is a single byte so 440 + pm1_len < 2048 always holds, and ad_offset + l_ad <= 2048 is enforced upstream so off + ad_size never exceeds the block. Bit 0 (Existence) stays unread, deliberately. ECMA-167 4/14.4.4 makes it a display hint, not a statement that the file is absent, and UDF 2.50 2.3.4.2 carries it through as the DOS hidden attribute. For a ripper the consequences are asymmetric: honouring it can silently drop a real .m2ts from the title list, ignoring it costs an extra name in a listing. Known structural limit, not fixed: read_filesystem takes only the FIRST extent of the Metadata File, so a fragmented metadata partition would map every sector past that extent to the wrong place. metadata_start being a single base LBA is what forbids the fix. |
||
|
Matthew Jackson
|
0bc8d7af9c |
test: constrain the AACS key-map gap fill, the PSI walk, and MP4 field offsets
Third pass over src/mux/. 40 survivors killed, no production change. resolve.rs — the deleted-statement cluster is now fully constrained. All 14 deletable statements probed; 9 were already caught, 5 survived: c.sort_unstable() in fill_base_key_gaps. Every existing case handed it cuts already in LBA order, but IndividualSegment.tbl is a record list. Verified on HEAD: deleting the sort passes all 54 resolve tests. The mutant lays a base-key fill straight over a forensic segment. last_idx = idx (FMTS gap fill) and last_idx = hit (multi-CPS cache hit). An extent with nothing to sample must inherit its neighbour's CPS unit; the mutants fall back to the first unit's key. Exactly the shape this file's own comments name — wrong key, no error, lost_bytes == 0. Both check_halt()? polls in probe_fmts_index_keys. These cannot be killed by outcome, since a later poll returns Halted too. The tests count reads instead, which is what the don't-hammer-a-struggling- drive rule actually says: after a Stop the drive is asked for zero content sectors. The four unresolved += 1 arms each got a test, and deleting each fails exactly one — one-to-one, so no fixture passes for the wrong reason. A control test pins that the baseline table resolves, so an expect_err cannot succeed for an unrelated reason. ts.rs::scan_streams was never entered. Six killed, two of which return wrong answers that look right: reading the PAT/PMT CRC as a table entry invents a stream on PID 546 out of CRC bytes, and dropping the ES_info_length skip decodes a descriptor as an entry and loses the one after it. Every existing PMT fixture declares ES_info_length = 0; a real BD PMT carries a registration descriptor on essentially every entry. Also ISO/IEC 13818-1 2.4.4.3 program_number == 0 is the network PID, not a program. mp4/read.rs — 13. Height read as the width beside it; channelcount; the 4-byte base-128 descriptor varint (every existing esds fixture uses a single byte); all three optional ES_Descriptor fields, whose loss is silent (an AAC track just loses its CodecPrivate); first-vs-last media edit, which is A/V desync of the difference; and the version-1 mvhd timescale offset, emitted by any writer whose duration exceeds 32 bits. dts.rs — 7, from a real cargo-mutants run over the file rather than guesswork. Including a buffer that IS the syncword, which is the state a sync split across PES packets lands in the moment its last byte arrives. Equivalents proven by application, not argued: the sample_encrypted_units guard pair is mutually redundant by construction (total*p/9 < total for p <= 8), so either alone is equivalent and both together are not; the PMT section_len guard is dead code where its PAT twin panics; three of the seven EXSS_HEADER_MIN_BYTES arithmetic mutants still sum to 10. |
||
|
Matthew Jackson
|
e99b634635 |
fix(mux): a backstop discard is a discontinuity; a stream-start trim is not
drop_marks_before retired discontinuity marks alongside timing marks at both of its call sites. At stream start that is right. At the MAX_AU_BUFFER backstop it is not, and the two are now separate. The backstop fires when 8 MiB accumulate with no AU start code in them — corrupt or hostile input — and throws the run away. There IS a prior AU in that case, and whatever emits next definitively does not continue it: a decoder handed that picture resolves its references against frames separated from it by megabytes of discarded data. Retiring the flag meant the resync gate (resync.rs, driven from mux/disc.rs) never armed, so the broken picture went out looking sound. Silent corruption is the one class of loss this crate refuses to have. At stream start the opposite holds. Bytes ahead of the first access-unit delimiter are the tail of an AU that began before sync, and there is no prior AU to be discontinuous from. Marking it would arm the gate at the head of every title and drop its opening GOP. That risk is why this was a decision rather than a fix, and splitting the call sites is what avoids paying it. Recorded as a sticky flag, not an offset mark. A mark placed at the new base is retired moments later by the pre-sync trim that follows resync — the gap has to outlive the bytes that caused it. I found that by writing the test first and watching it fail with the mark approach. The discard is a discontinuity whether or not the source signalled one, and a signalled one on discarded bytes still reaches the AU that follows; both directions are tested. Note the first over-cap run is NOT a discard: the next AU's delimiter is still at buf[0], so it force-flushes as an over-long access unit and loses nothing. Only a run with no opener at all reaches the backstop. The tests push twice for that reason — the single-push version passes without the fix. Swapping either call site for the other fails: reverting the backstop reds the two gap tests, and arming the gate at stream start reds the third. |
||
|
Matthew Jackson
|
f9d081ed45 |
test: drive the AACS 2.1 variant chain to a Media Key, and pin AES-G3
163 of 322 surviving mutants across src/aacs and src/css. No production line changed — every function read correct; the finding was always an absent test. Two structural holes, both verified against HEAD before landing. variant.rs had no test that ever produced a Media Key. Every terminal assertion in the module was an Err classification — NotVariantMkb, SoftCorrectionRequired, OnlineChallengeRequired. So the entire 2.1 success path (VARIANTS lookup, VKD selection, Kpnew, the final unwrap, the verify gate) was pinned by nothing, and that path produces the Media Key that becomes the VUK that decrypts every byte of a 2.1 disc. Built the first complete planted variant MKB: the VARIANTS entry is chosen as Kvn ^ 1 so the real VKD sits behind a decoy at table index 1, making the lookup load-bearing rather than incidentally correct. That one fixture kills 23 operator mutants across three functions. aesg3 — the subset-difference tree node function — was in the survivor list as replaceable by [0; 16], meaning every device key in the crate would derive the same Processing Key. It is caught today only as a side effect of a negative test added after the mutation run; nothing asserted the relation itself. Pinned now via the spec relation ([C] 3.2.2) using the FORWARD primitive, with s0 transcribed independently rather than read back from AESG3_SEED, so the test cannot agree with a mutated constant. Same shape in derive.rs: plant_mkb was one slot with zero descent, so slot indexing was the identity permutation and the ancestor-descent branch never ran — which is why 39 of recover_dk_position's mutants survived. Added a 3-slot fixture keyed at index 2 and a four-level descent fixture whose expected Processing Key is written out as an explicit aesg3 chain rather than computed by calc_pk_from_dk; a fixture built by the function under test moves with its own mutations. Two latent panics on untrusted input now have tests: a 0x05 cvalue table shorter than the 0x04 slot index, and a drive declaring more payload than the 32772-byte response buffer holds. 23 equivalents claimed with reasoning, and confirmed empirically where possible — all eight css/lfsr mutants were run and exactly the seven disjoint-bit-lane ones survived. Explicitly NOT claimed equivalent: derive.rs 146:32 and 154:30 are reachable, but only on the non-convergent bounded-exit path where the function's sole contract is termination. A test there would pin defined-but-meaningless output. Noted for the next pass: the pre-existing walk_mkb_be24_high_byte_is_honored used total length 0x0110, whose high byte is zero — it exercised the middle byte only, which is why << 16 -> >> 16 survived it. Left in place; a real one was added at 0x01_0004. |
||
|
Matthew Jackson
|
3e13a155fa |
refactor(clpi): delete the unused EP-map to sector-extent path
get_extents had no caller anywhere in the ecosystem, and neither did
anything feeding it. Removed: get_extents, resolved_ep_map, full_pts,
full_spn, parse_cpi, EpCoarse, EpFine, the ep_coarse/ep_fine fields,
the unused version field, and the 35 tests that exercised them.
This reverses the fix in
|
||
|
Matthew Jackson
|
b2e1982051 |
fix(clpi): resolve out_time past the last EP entry to the end of the clip
ClipInfo::get_extents fell back to `last EP SPN + 1` whenever out_time lay past the last entry-point. EP entries mark I-frames (BD-ROM Part 3, CPI / EP map) and a clip's final GOP lies after the last one, so a PlayItem covering a whole clip — whose OUT_time is the presentation end — always lands in that arm. The extent then stopped one source packet after the last I-frame. Measured on a fixture with 200,000 source packets and the last EP at SPN 131,072: sector_count came back 12,289 where covering the clip needs 18,750. Everything from the last entry point to EOF is outside the returned extent. Scope, stated plainly: get_extents has NO callers anywhere in the ecosystem today — it is #[allow(dead_code)] and documented as reserved for the timestamp-range read path. Nothing ships this loss. It is fixed now because a latent truncation in extent arithmetic is far cheaper to correct before it has callers than after. The SPN at-or-after an out-of-range out_time is the end of the clip, source_packet_count, with .max(last + 1) so a disc that under-declares its own packet count against its own EP map still yields a sane bound. Also 174 mutants killed across clpi, mpls, ifo and ebml — the first time any of these four files has been examined. And ebml's 8-byte VINT back-patch was duplicated verbatim in end_master and end_master_buf with its top four payload octets unreachable through either (they need a 16 MiB..256 TiB buffer); extracted to fixed_width_vint8 and tested across the full 56-bit payload, no behaviour change. 38 of ebml's 46 survivors are one equivalence cluster: every | in write_size / read_id / read_size / read_uint_val ORs into disjoint bit lanes, where ^ is the identical operation. Applied all 38 at once — green — then spot-checked four individually. |
||
|
Matthew Jackson
|
84a77f6a0e |
test: pin FMTS read_plan unit indexing to an unaligned range start
Five surviving mutants in AacsKeyMap::read_plan, all in the arithmetic that decides which half of a forensic segment this disc's key opens. The existing coverage used a forensic range starting exactly on the extent's first unit. Under that shape several wrong formulas agree with the right one by arithmetic accident: (lba + range_start) / us and (lba - range_start) * us both produce the correct kept set. Real ranges are not shaped like that. A range start comes from a source packet number — start_spn * 192 through clip_byte_to_lba in mux/resolve.rs — and 192-byte packets bear no relation to the 3-sector aligned unit, so range_start % 3 is whatever the disc says. Getting the parity wrong does not crash. It reads and decrypts the ALTERNATE variant's half: the units this key does not open decrypt to garbage, the units it does open are skipped. AACS 2.1 forensic marking is precisely what makes the two halves differ, so the failure is silent — a full-length rip carrying the wrong variant. Two fixtures are needed because no single one kills both: an unaligned range start with the extent beginning on it inverts the halves under the + form, and an extent offset one unit-remainder from the range start inverts them under the * form. Also pinned the short-tail guard. is for a remnant SMALLER than a unit — bytes with no following unit to desync. Widened to <=, the last whole unit of every extent bypasses the phase gate, so a forensic segment ending at an extent boundary contributes one alternate-variant unit to the rip. |
||
|
Matthew Jackson
|
9de88969ca |
test: constrain the DiscStream loss surface and the empty-title guards
Second mutation pass over src/mux/. 26 survivors killed, no production change. Verified on HEAD before landing: each mutation below passes all 1,237 mux tests unmutated-suite. The priority item was the honest-loss-reporting surface. Both DiscStream::errors and DiscStream::lost_bytes could return a constant with nothing failing — a rip that lost sectors would report zero loss to the caller. This project has already shipped one defect of that shape (a total decryption failure reported as an empty title, exit 0). Driven now through two short-read fills so both land on values that are neither 0 nor 1 and differ from each other; no constant and no field swap survives. MkvStream::finish -> Ok(()) also survived. MkvMuxer::finish has the zero-frame MkvInvalid guard and two tests cover it, but the Stream wrapper above it could return Ok unconditionally and bypass the guard entirely — the empty-title defence was one layer thinner than it looked. au_assembly: pinned au_opener_from behaviourally to the normative byte values for all four modes, with negative cases for codes that are explicitly not openers (MPEG-2 slice 0x01..0xAF, user data 0xB2, extension 0xB5, sequence end 0xB7 per 13818-2 Table 6-1; VC-1 0x0A/0x0B/0x0C; H.264 SPS/PPS/IDR-slice). au_assembly and codec/ hold independent copies of these constants; they agree today, and comparing constants would not catch logic drifting apart, so both sides are now pinned to the spec instead of to each other. demux_sink::sanitize: every filename component demux:// writes comes from disc-controlled text, so the path-separator arm is a traversal guard. Deleting it now fails, including an end-to-end case where base = "../evil/Title" must produce exactly one file inside the chosen directory. stts_and_ctts_expand renamed to stts_expands_runs_to_per_sample_deltas_in_order and given runs with distinct deltas AND distinct lengths. Its old name claimed ctts coverage it never had, which is why the composition-time chain went unconstrained for eight rounds; the doc comment now points at the tests that do cover ctts. Correction to the previous pass: codec/truehd.rs flush -> vec![] IS equivalent. Applied it, full mux suite green. TrueHD buffers across PES but parse emits every complete unit immediately, so a residual buffer at EOF is a truncated access unit and is correctly discarded. The vec![Default::default()] variants are genuinely different and are killed. Deliberately not constrained: mkv::set_opening_capture (diagnostics behind a process-global tracing check, flaky under the parallel runner), and the three stdio.rs header paths (StdioStream holds concrete io::Stdin/Stdout and cannot be driven without a production refactor to injectable Read/Write). |
||
|
Matthew Jackson
|
170fd0c064 |
test: constrain MP4 composition timing, MLP substream directory, and codec-private absence
Mutation testing over src/mux/. No production change — 49 survivors killed, all proven red before green. The MP4 composition-time chain was entirely unconstrained: VideoTiming::ctts, build_ctts and parse_ctts could each return a constant and the suite stayed green. Confirmed on HEAD: build_ctts -> vec![] passes all 1,220 mux tests. A demuxed B-frame title presenting in decode order would have shipped. The cause is a test whose name asserts coverage its body does not deliver — stts_and_ctts_expand builds an stts box and never touches ctts, and write_then_read_round_trip asserts sample sizes and keyframe flags but not one PTS. Same shape as the set_speed forwarding finding, different disguise. mlp_num_substreams / mlp_substr_header_size: every TrueHD fixture in the crate uses one substream and no extraword, so both could return a constant and agree with all of them. These position mlp_parity_ok's window over the AU header, so a constant mis-windows the parity check on exactly the multi-substream AUs that carry 7.1 and Atmos. CodecPrivate absent vs empty: mkv.rs writes Some(bytes) verbatim and omits the element on None (RFC 9559 5.1.4.1.24), so a zero-length Some emits a track header asserting the config IS empty. Four parsers could return Some(vec![]) before any frame. Also: mandatory ISO/IEC 14496-12 boxes (tkhd, vmhd, smhd, dinf, mdhd) could each build empty; HEVC num_extra_slice_header_bits (H.265 7.3.2.3) was never non-zero in any fixture, so the slice-type offset skip was unexercised; chapter names from the disc go straight into <ChapterString> and the & escape must run first; a stray 0x47 in a payload must not latch a TS resync. Documented as equivalent rather than killed: CodecParser::flush and the three parser flush bodies that differ from the mutant only by a tracing call, and DropTally::log_summary. |
||
|
Matthew Jackson
|
55b97ac576 |
fix(udf): skip deleted File Identifier Descriptors
read_directory decoded file-characteristics bit 1 (Directory) and bit 3 (Parent) but never bit 2 (Deleted) — ECMA-167 4/14.4.4. It followed the ICB of a descriptor naming a file that no longer exists. 4/14.4.3 permits a deleted FID's ICB field to specify an extent of length zero, so it need not point at a File Entry at all. Following it reads whatever descriptor occupies that metadata LBA: - deleted DIRECTORY FID: recursion lands on the File Set Descriptor (tag 256), hits the not-a-File-Entry arm and returns Err. One stale descriptor in one directory fails enumeration of the entire volume. - deleted FILE FID: read_file_size returns Ok(0) for a non-File-Entry tag, so a deleted name is reported as a real zero-byte file. Both directions of the same shape at once: a recoverable condition becoming a hard failure, and a non-existent entry becoming a plausible success. Bit 0 (Existence) is still unread. Skipping hidden files could hide real content, so it is left alone deliberately rather than folded in. Found by mutation testing: 48 of the 51 surviving mutants in read_directory are killed by the 17 tests added here, covering the short_ad decode byte by byte (ECMA-167 4/14.14.1), the extent-type mask, the AD bounds guard at the exact sector end, the tag-261 File Entry directory arm that no test reached at all, the multi-sector read offset, the FID stride including L_IU, and the nesting cap. Three survivors are genuine equivalents and are documented as such: l_fi > 0 vs >= 0 (the emptiness guard below reaches the same state), and the << 32 / | in the visited-set key (meta_start is constant across a walk, and the two halves are disjoint). |
||
|
Matthew Jackson
|
c610285910 |
test: constrain SectorSource speed forwarding and PassProgress percentages
Mutation testing left both unconstrained. sector/mod.rs — set_speed on the Box<dyn> and &mut dyn forwarding impls could be replaced with an empty body and nothing failed. This one hides better than the read methods because the trait's own default body is already a no-op, so a forwarder that swallowed the call is indistinguishable from a source with no speed control. Consequence is a silently absent value, not a wrong one: the recovery path lowers read speed through a damaged region, and a swallowed call leaves the drive at full speed while the caller believes it slowed down. Routed through a generic S: SectorSource bound, since a direct call on a &mut dyn receiver auto-derefs to the vtable and never enters the forwarding body. progress.rs — 42 survivors. All four percentage accessors could return a constant, read the wrong byte counter, or have their divide-by-zero guard inverted. Added exact-value tests (25%, not 'some percentage'), both sides of each guard, the overshoot clamp, and one test setting all three disc counters to distinct values at once — without it, a swapped field still passes every single-counter test. The Progress blanket impl for closures could return a constant true. That return value is the cancellation signal, so a constant-true body makes every closure-based consumer uncancellable. Each mutation applied, observed red, reverted. |
||
|
Matthew Jackson
|
e4b1e5b19e |
docs: correct info invocation and read timeouts
TROUBLESHOOTING step 2 said `freemkv info`, which needs a source URL; the drive route is `freemkv info disc://`. architecture.md quoted 1.5 s / 30 s for the read timeouts. The constants are READ_TIMEOUT_MS = 10_000 and READ_RECOVERY_TIMEOUT_MS = 60_000 (src/scsi/mod.rs:72,94). |
||
|
Matthew Jackson
|
8d4a6d54a4 |
Constrain five behaviours that mutation testing showed nothing constrained
Fifteen surviving mutants killed, from the highest-risk class: functions a mutant could replace wholesale with a constant while all 2,555 tests passed. None of the code was wrong. In every case a test was absent, which is why eight rounds of reading never found any of them. The one that generalises is in sector/mod.rs. Its existing test READS as covering `read_sectors` on the `&mut dyn SectorSource` forwarding impl — it takes a `&mut dyn`, calls the method, checks the spy. But the receiver auto-derefs and dispatches through the vtable straight to the spy, so the forwarding body is never entered. An earlier round hit this exact trap on `set_unit_base` and fixed it with a generic helper; the read path kept the test that looked right. Verified by stubbing the forwarding impl to Ok(0): the new test fails, the old one passes. That makes a tenth distinct shape of bad test in this audit, and the mutation list is how to find the rest — any forwarding-impl method in it has the same problem. decrypt.rs's two existing gate tests assert only `dropped == 0`, which is precisely what the `Ok(0)` mutant returns; one asserts nothing else at all. A wrapper that decrypts nothing therefore looked correct while the caller muxed scrambled MPEG. Now pinned by descrambling a real CSS sector and comparing against the plaintext it was built from — not against a re-derived descramble, which would only assert the code agrees with itself. css/mod.rs's `is_scrambled_uncracked` turns out to have no production callers at all; the enum is matched directly. Its three tests all assert only the true direction, which is exactly why the `-> true` mutant survived. It is public API, so a consumer routing on it would, under that mutant, refuse to rip every clear DVD. aacs/inf.rs's MKB drive read had no test whatsoever. Now pinned byte-for-byte across multi-pack concatenation, the single-pack case, a genuinely empty response, and error propagation — an unreadable MKB must surface as an error, not as an empty one. aacs/derive.rs's nine mutants are killed with planted MKBs built by inverting the AACS relations, so no real key material is involved. The assertions land on the derived Media Key rather than the intermediate positions: a recovered position that does not actually walk to the planted key is no better than None. A fixture-guard test asserts the planted MKB parses, since an unparseable one would make every `-> None` body look right. 2570 lib tests, debug and release. |
||
|
Matthew Jackson
|
93e1436fc0 |
Test that the Media Key verifier actually rejects a wrong key
`km_verifies` is the gate deciding whether a candidate Media Key belongs to the disc. The MK-pool brute force in resolve.rs runs every candidate through it, so a version that said yes to everything would accept whichever candidate it tried first and the rip would continue with a wrong Media Key — wrong VUK, wrong title keys, garbage plaintext, and no error raised anywhere. Nothing tested it. Whole-crate mutation testing reported `replace km_verifies -> bool with true` as SURVIVING: the body could be replaced with `true` and all 2,556 tests still passed. A verification routine whose verification was itself unverified, in the most safety-critical function in the crate. The implementation is correct — it matches the AACS relation `AES-D(km, mk_dv)[0..8] == 01 23 45 67 89 AB CD EF`. Only the defence was missing. No real key material is required to test it. That relation means a valid record for any chosen km is just `AES-E(km, <the constant> || anything)`, so the fixture is self-contained. The test asserts three things: the key the record was built for verifies; a key differing by ONE BIT does not, which is the assertion that kills the mutant and is a near-miss rather than a random key; and an MKB carrying no verify record does not default to yes, because unverifiable and verified are different answers. Confirmed by reintroducing the exact reported mutation and watching this test fail. This is the first defect found by mutation testing rather than by reading. Eight audit rounds and a security lens that read this file in full all missed it, because it is not a wrong line — it is an absent test, and only an instrument that asks "would anything notice if this were broken?" can see that. |
||
|
Matthew Jackson
|
18f8b285c4 |
Bound the BD-J label parsers, and stop a crafted disc hanging the scan
Ten defects in code no previous round had ever scoped. `src/labels/` identifies a disc's studio by parsing jar archives and JVM class files off untrusted media, so every byte here is attacker-controllable — and 813 of its lines were executed by no test at all. The worst is a non-terminating loop. A fallback stream-number scan advanced with `saturating_add`, and the comment says why: a crafted XML "must not overflow (panic in debug, wrap-to-0 in release)". Once the counter pins at u16::MAX and that number is taken, the loop cannot exit. So a fix for an overflow panic produced an unbounded hang, which is strictly worse — a panic is observable and catchable, and catch_unwind cannot interrupt a live loop. Reachable from about 8 MB of XML. Where the same overflow appears in the deluxe decoder the fix is checked_add and stop, NOT saturation — twice wrong there, because saturating would peg every stream past the ceiling at one number and apply_labels binds on (type, number), silently mislabelling tracks. A correctness bug wearing the costume of success. Round 7 capped the ldc-string retention per class; nothing capped the aggregate, so a 64 MiB jar held that budget for every class at once. Same defect one level up, which is the shape that keeps recurring in this directory. Four other amplifications are bounded the same way, each with a stated headroom and a paired test proving real media passes untouched — the tightest is 5x on a label length, the loosest 2000x on the stream numbering space, against BD's 32-per-type STN_table limit. Two are not caps at all: a quadratic membership scan became a set, and an attacker-derived length added to a cursor without saturation now cannot wrap. Nothing is excluded by either. A `#[cfg(test)]` hand-copy of a shipping parser was the ninth bad test this audit has found, and the first proven by mutation rather than inspection: deleting the guard from the REAL function left all 26 tests green, including the one named for that guard. Pointed at the real function, the same mutation fails. Separately, all three failure arms of the bounded fsync returned Ok(()) on both macOS and Linux, so sync_all reported success for a durability barrier that never ran. Only macOS was in scope; the Linux twin is fixed here too, because a platform disagreeing with its sibling about whether a failed sync is an error is the class that already produced an over-length SCSI CDB macOS rejected and the other two truncated. Note the behaviour change: a mux whose final sync times out on a wedged mount now fails rather than exiting 0. Three of the caps are proven by wall-clock deadline rather than an operation count, with 18-80x margin on the passing side. On a heavily oversubscribed machine those could flake. |
||
|
Matthew Jackson
|
c63dafcf1a |
Key each FMTS extent from its own CPS unit, and stop a bad ICB tag reading
as an empty directory On an AACS 2.1 disc the non-forensic gap fill hardcoded pool slot 0 as "the" base Unit Key, so every content LBA outside a forensic segment was keyed with CPS unit 1's key even on a disc carrying several CPS units. It does not fail loudly — it produces garbage plaintext. The gap fill now resolves each extent's own base key from its ciphertext, sharing the sampling and slot-picking the multi-CPS path already had rather than adding a second copy, and memoised in the existing per-disc cache. A disc with one base key still short-circuits with zero extra reads, which the existing probe-cost test pins. Forcing the slot back to a constant fails four tests, so the choice is load-bearing rather than incidental. A directory ICB whose descriptor tag is neither File Entry nor Extended File Entry (ECMA-167 4/14.9, 4/14.17) was turned into a successfully-read EMPTY directory, indistinguishable from a genuinely empty one, while the same tag on a file ICB was already a hard error. Fifth instance in this audit of a failure converted into a plausible success value, and the second in this very function — round 5 fixed a read error becoming a file size of zero here. An unrecorded extent (ECMA-167 4/14.14.1.1: allocated but not recorded, logically zeros that still occupy file space) was dropped entirely rather than contributing its length, so every later extent landed at the wrong file offset. Silent corruption, not an error. Extents now carry a recorded flag and the hole emits zeros without touching the media. The Volume Descriptor Sequence was swept at hardcoded sectors 32..64 while the anchor's own Main VDS Extent pointer was parsed into a comment and ignored; ECMA-167 3/10.2.1 defines that extent by the field, not by position, so a conformant volume placing it elsewhere failed to mount. drive_status decoded byte 5 as Media Status without checking the event header's NEA bit or notification class (MMC-6 §6.7), so a reply carrying no media event descriptor decoded as "no disc". The drive is untrusted input here, and this is the works-on-my-drive class. Two more tests were found asserting the defects they sit next to — one requiring unrecorded extents to be dropped, one that four drive fixtures built non-conformant replies the corrected decoder rightly rejects. Both rewritten. Combined with the DTS one in the previous commit that makes three tests this round that locked a bug in as intended behaviour, which is a different and worse failure than the tautological tests found so far: a tautology fails to catch a regression, these actively defend the defect. Not fixed, adjacent: extract_one_file streams extents sequentially and will now READ an unrecorded extent's sectors rather than writing guaranteed zeros. Offsets are right, and pressed media reads as zeros there, but it is not zero-guaranteed the way read_file now is; that needs a recorded flag through PlannedFile. |
||
|
Matthew Jackson
|
46eb88c51f |
Feed the CSS crack the canonical extent order, and stop Resolution faking 1080p
Seven defects in the code the test suite executes least — 913 lines of disc/mod.rs alone are run by no test at all, which is why this round scoped from coverage rather than from what previous rounds said they had read. Disc::scan_image kept its own copy of the crack's extent ordering and fed crack_key_outcome largest-cell-first. That is the fifth instance in this audit of a local reimplementation drifting from the canonical one, and the cost here is a key that does not descramble the feature: picking by sector count bypasses the capacity gate and can select a different VTS entirely. The copy is gone — which title comes from the canonical order the scan already applied, and the extents are handed over in playback order, exactly as decrypt_keys_for_title does. Its doc records why the duplicate existed so it cannot grow back. Resolution::pixels returned 1920x1080 for Unknown. That is the FOURTH instance of one trap and the other three were in this same file, two of them fixed hours earlier — without sweeping for siblings, which is the whole reason this one survived. It now returns (0, 0), and the sweep was done properly this time: every remaining Unknown arm across the crate is honest, and the two ColorSpace sites that look like fabrication are emitting H.273 code point 2, which is the spec's own "unspecified". Two callers carried local Unknown-to-zero workarounds — precisely the cost of making callers responsible for a lie — and one is now redundant. BD-ROM Part 3 code 0xA2 is the lossy secondary DTS stream, not lossless Master Audio. A test asserted the wrong mapping as intended behaviour, so correcting the code failed it; the test is deleted with a note pointing at its replacement. That is a NEW failure mode for this audit: not a test that cannot fail, but one that locks the defect in. There is no DtsExpress variant to map to, so it takes the lossy DTS-HD member and the approximation is documented. Also: DiscSession::identify could panic through drive_mut once the public API allows an absent drive — two siblings were converted in an earlier round and this one was missed; an extent end that added without saturating where the rest of the crate saturates; a diag reason string restating the comparator's sort keys and drifting from them, now derived from them; and a short read that advanced the offset by the full request, silently skipping the gap. That last one existed twice, in two reads with the same shape, now merged so they cannot drift apart. The short-read policy is a judgement call I could not derive from a spec: no skip_errors is a hard error, with skip_errors zero-fills and charges the loss. It deliberately does not retry mid-unit, because resuming inside an AACS aligned unit would trade a silent gap for a silent decrypt desync — the worse of the two. |
||
|
Matthew Jackson
|
dea968f32b |
Stop AudioChannels and SampleRate fabricating a value for Unknown
Three copies of the same two mappings existed. The canonical accessors returned 6 channels and 48000 Hz for Unknown; a third copy in diag.rs returned 0. The honest one was the copy. A plausible wrong answer is worse than an obvious one. Six channels at 48 kHz is indistinguishable from a real 5.1 track, so every caller became responsible for remembering to check the variant first — and this crate walked into exactly that: the json:// sink reported a confident 5.1 for audio whose neighbouring fields said "unknown". That was fixed at the call site earlier in this audit; this fixes it at the source. The accessors now return 0, which is what both in-crate call sites already coerced Unknown to by hand, so their guards are gone and the behaviour is unchanged. Zero is also obviously wrong if it ever reaches output, where six is not. The diag.rs duplicates are deleted rather than corrected — a fourth copy would have drifted too. Their only caller was a trace line in the same file, now on the canonical accessors. Their tests moved across and gained the Unknown case, which is the point: restoring either fabricated value fails both. Found by the round-7 correctness agent while fixing the json:// sink; it flagged the third copy as out of its scope rather than touching it. |
||
|
Matthew Jackson
|
079c9b1327 |
Add a seeded robustness harness for the untrusted-input parsers
Five parsers that take bytes straight off a disc are now swept with generated input asserting one property: they return Ok or Err and never panic. That is this crate's own hard rule, and the class seven rounds of reading is worst at. Written in-crate rather than with cargo-fuzz, which needs a nightly toolchain this project does not use, and without proptest or arbitrary, because one dev-dependency is a deliberate posture and the parsers take plain byte slices. What is given up is coverage-guided mutation, which is the real loss. What is gained is determinism: the same seed replays the same cases anywhere, so a CI failure reproduces locally verbatim. Three generators, and the second is the one that matters. Pure random bytes die at the magic check and exercise the entry guards only; prefixing valid magic is what reaches the parser body; mutating a mostly-zero record is what reaches the offset and count arithmetic a hostile image would lie about. That claim is MEASURED, not asserted. A harness whose cases all bounce off the entry guards is the fuzzing equivalent of a test that cannot fail, so one test counts how many generated cases parse to completion: 15,606 of 60,000, about 26%. If a future change to a guard drops that to zero, the test fails rather than continuing to report a meaningless pass. Two further tests pin that the three generators produce different bytes and that a seed replays identically. 1.2M cases across all five targets found nothing. On this evidence that is a real negative rather than an empty one. The first version of this file was itself broken in the way this audit keeps finding: its two meta-tests set FREEMKV_HARNESS_CASES and raced, because the test harness runs them in parallel and env mutation is unsound there. The budget is a parameter now, and the environment is read once at the call site. Two crate-internal parsers widened from private to pub(crate) so the harness can reach them. No public API change. |
||
|
Matthew Jackson
|
327087c70e |
Make five tests capable of failing, and stop the presence probe unmounting the disc
The worst of the five was a regression suite that never touched the code it guarded: nine batch-count tests called `safe_batch_count` and `buggy_batch_count`, both defined in the test file itself. The u16 truncation they exist to prevent could be reintroduced in sector/prefetched.rs with every one of them green. They now drive the real producer through the public API, and reinstating the truncation fails five of the nine. Worth recording that the symptom has changed since the original fix: the unit-alignment clamp below floors a zero batch at three sectors, so the bug is now a twenty-fold throughput cliff rather than the stall it once was. The MP4 reserve test's only numeric case was dominated by the floor and the buffer, so BYTES_PER_SAMPLE could be zeroed without failing it. It now has a case where the per-sample term dominates. The zero-count guard in FileSectorSource was likewise unfalsifiable — seek-past-EOF and a zero-length read both succeed — so the test now observes the file cursor. The AACS media-key ambiguity guard had no test at all; the pool scan is extracted so the verifier can be injected, because a genuine two-key collision needs one ciphertext decrypting under two AES-128 keys to plaintexts sharing a 64-bit magic, which is a 2^64 search and not a fixture. macOS implemented the documented cheap, side-effect-free presence probe by building a full exclusive transport — which force-unmounts the disc. Linux and Windows issue one TEST UNIT READY with no unmount; macOS was the outlier. It now walks the IOKit registry for the media object instead. The C shim's registry reads assumed CoreFoundation types the registry does not guarantee, so a driver publishing a CFNumber where a CFString was expected aborted the process from inside public API. Types are checked and a wrong type treated as absent. The unbounded waitpid on the unmount child is now a polled deadline, and the last-resort match gained the NULL check its two siblings already had. The empty-CDB guard existed only on Linux while a shared helper's comment claimed all three backends had it. Moved into the helper, so the comment is now true and macOS and Windows are covered. One finding was REJECTED with evidence rather than fixed. The TrueHD buffer-cap test was indeed bogus, but MAX_TRUEHD_BUF turns out to be unreachable by any input: the parser only retains data when the buffer is shorter than the declared AU, and that declaration is twelve bits, so the worst case is 8189 bytes against a 256 KiB cap. An exhaustive sweep over all 65536 AU headers confirmed it. The fixture now sits at the reachable ceiling and asserts that instead. The cap itself is left in place as defence, unreachable by construction, matching how the AC-3 resync guard was handled earlier in this audit. Two behaviour changes worth naming: Linux's empty-CDB error becomes InvalidCdbLength rather than a transport failure, and an unknown device now reports absent media rather than a not-found error, because the registry cannot tell an empty drive from a missing one. The latter is a conflation of the kind this audit has fixed three times; it is recorded for the next round rather than left silent. |
||
|
Matthew Jackson
|
b8fa5e74dc |
Stop the live rip path muxing Blu-ray 3D differently from the ISO path
Five defects, four of them the same shape: a local reimplementation of logic the crate already had, which had drifted from it. Each is now fixed by calling the canonical version rather than by patching the copy. DiscStream::new — the live disc:// path — built every parser through the plain codec lookup and never asked whether a video stream was an MVC dependent view, though resolve::build_demux_state does. The same 3D disc therefore muxed correctly from an ISO and incorrectly ripped live. The open-coded loop is gone; both paths now call build_demux_state. collect_psi_section reimplemented the continuity-counter gap test and disagreed with process_packet in the same file: it tolerated neither a duplicate packet nor an adaptation-field-only packet, which per ISO/IEC 13818-1 §2.4.3.3 does not increment the counter. A spec-legal PMT continuation was read as desync and the title's stream list came back empty. Both callers now share one `cc_is_gap`, and a duplicate packet's payload is no longer appended twice — doing so would have corrupted the section the check exists to protect. The json:// sink called the channel-count and sample-rate accessors unconditionally, and both fabricate a concrete value for Unknown, so it reported a confident 5.1 at 48 kHz for audio whose format was unknown while its own neighbouring string fields said "unknown". The keys are now omitted, matching mkv.rs. This matters more than it did: a sample-rate ladder fixed earlier in this audit means Unknown now reaches consumers that used to receive a wrong-but-concrete value. For an audio:// or sub:// sink the reference video track's output is filtered out, so its first PTS was never recorded and every delay was computed against zero — baking a wrong DELAY into the filename. The reference is now recorded whenever a frame is on the reference track, independent of whether that track has an output, so a normal title gets a correct delay; where no reference is ever observed the tag is omitted rather than guessed. A third copy of the channel/sample-rate mapping exists in src/diag.rs and was left alone as outside the confirmed set. It is the same drift shape and is recorded for the next round. |
||
|
Matthew Jackson
|
3f7d7af472 |
Bound three allocations an untrusted disc can drive without limit
The Program Stream demuxer appended every fed byte and enforced its 4 MiB cap only inside a branch reached once a start code had been found. Input containing no start code anywhere therefore hit no cap at all, and since a whole title is fed through this demuxer, a zero-filled or ciphertext VOB extent buffered the entire title — up to ~90 GB. When the buffer holds no start code, only a two-byte `00 00` prefix can begin a PS unit on the next feed, so that is kept and the rest dropped. The bound is exact rather than a heuristic: a start code can straddle a feed boundary by at most its first two bytes, so no real byte is discarded, and a test feeding `FF FF 00 00` then `01 E0 ...` pins that. The existing test named for this case fed a real start code first, so the cap it exercised was the in-PES one. Renamed to say what it covers. The BD-J label path had a different shape to anything found so far: the cap is on the COMPRESSED size of a disc file while the allocation scales with the decompressed size. A `.class` gated only by a path prefix inflates to the 64 MiB ceiling, yielding ~33M retained strings from `ldc` operands or ~67M pushes onto a symbolic stack whose depth was unbounded despite the Code attribute's own `max_stack` being parsed and then ignored. Bounded both, the stack by `max_stack` itself (JVMS §4.7.3). The VMG TT_SRPT title count is an untrusted u16 with no de-duplication, so ~800 KB of crafted IFO re-parsed one PGC 65535 times. Capped at 99, the DVD-Video maximum, so no conformant disc is clipped. Every cap carries stated headroom against real media, and each has a test locking that real media still passes. I rewrote three of the new assertions before landing them. They compared the result against the very constant under test — `total <= MAX_TT_SRPT_TITLES` — which passes vacuously the moment someone raises the constant, the most likely future regression and the seventh instance of this tautology shape in this audit. They now assert literals derived from the spec. The TT_SRPT fixture also had to change: with 65535 identical entries the de-duplication collapsed them on its own and the cap was never what bounded the result, so the test passed with the cap removed entirely. Distinct entries defeat dedup and leave the cap as the only guard; de-duplication now has its own fixture. Verified by raising each of the three constants and confirming all three tests fail. |
||
|
Matthew Jackson
|
fdd473d7e9 |
Remove emulation-prevention bytes before reading the H.264 slice header
The bytes after a NAL header are EBSP, not RBSP: ISO/IEC 14496-10 §7.4.1 has the encoder insert 0x03 after any 0x00 0x00, and §7.3.1 removes it before parsing. The measured-picture-type parse read the raw NAL instead, on the stated reasoning that slice_type is too early for an escape to intervene. That holds only up to a point. first_mb_in_slice is ue(v), so a value of 65535 or more needs sixteen leading zero bits and opens the payload with 0x00 0x00, which an encoder must then escape. A UHD frame is ~32,400 macroblocks, so a conforming Blu-ray never reaches it — but 8K does, and the disc is untrusted input. Such a stream decoded slice_type against a byte the encoder had inserted and reported the wrong picture type: a wrong result rather than an error, which is the class this lens exists for. The prefix is un-escaped into a 16-octet buffer rather than the whole NAL: the two ue(v) fields are at most 32 bits each, so nothing longer can be needed, and it keeps a per-frame allocation proportional to the frame off the path. The test pins both directions. It asserts the un-escaped prefix decodes to first_mb_in_slice = 65535 and slice_type = 2, AND that the raw EBSP does NOT — without that second assertion the test would pass whether or not the fix were present, which is the failure mode this audit has now found four times. The bit string was derived independently rather than by hand: my first attempt at the fixture was wrong by one nibble and the test caught it. Also covers the cases that must NOT be unescaped: a 0x03 not preceded by 00 00 is ordinary payload, and 00 00 03 03 keeps its second 0x03 because the escape resets the zero run. |
||
|
Matthew Jackson
|
05fed1b0e0 |
Log the OS error when a SCSI command fails on Windows and macOS
Both backends discarded the platform's own error code on the execute hot path — the one every READ(10) of a rip goes through — and collapsed every cause to the same status-0xFF transport failure. On Windows, open() and reset() in the same file both capture the Win32 error; execute() did not. That left ERROR_INVALID_PARAMETER (a struct layout regression, the exact class this file's SDK-layout tests exist to catch), ERROR_ACCESS_DENIED and ERROR_GEN_FAILURE (a genuinely wedged drive) indistinguishable, with nothing in the log to tell a code bug from a hardware one. On macOS the same, and worse: the file had no tracing calls at all, where the Linux and Windows backends both log their execute failures. Its open() carefully decodes the shim's sentinel into typed variants instead of flattening them, but execute() threw the IOKit return away — so another process taking exclusive access mid-rip and a real hardware wedge produced identical, empty diagnostics. Logged rather than added to the error type: the typed variant is public API, and the recovery classification is deliberately the same for all of these. What was missing is the breadcrumb, not the distinction. Two further findings from the same sweep were rejected. Windows reset() always returning Ok(()) and macOS ignoring timeout_ms are both already documented in the code as deliberate, and the reporter flagged them for completeness rather than as defects. Neither fix has a test: reaching either branch needs a failing ioctl or a failing IOKit call, and both files are compiled only on their own platform. |
||
|
Matthew Jackson
|
d444afbdfc |
Turn a release-only slice panic into an error, and stop calling 32 kHz 48 kHz
Four round-6 findings. FileSectorSource::read_sectors guarded its output buffer with a debug_assert, which is compiled out in release — so an undersized buffer panicked with 'range end index out of range' instead of returning an error, out of a public SectorSource impl where the length is caller input. Drive::read_fua already carries this exact guard, with a comment recording the same panic being fixed there, and PrefetchedSectorSource has a regression test for the same case; this impl had been given neither. The new test is red in release for precisely the predicted reason: 'range end index 8192 out of range for slice of length 2049'. parse_track's sample-rate ladder ended in an unconditional S48, so any SamplingFrequency below 44100 was recorded as 48 kHz. A 32000 Hz AC-3 or DTS track is legal and common in broadcast-sourced content, and the wrong rate then propagated into the reconstructed AudioStream. Anything below the lowest mapped rate is now Unknown, which is what the crate's canonical SampleRate::from_hz already returned — the ladder disagreed with it. The ladder itself stays, because the MKV element is a float and wants tolerance rather than exact equality. shim_open_exclusive used the mach port from IOMainPort without checking the return; on failure the port is left untouched and every IOKit call below ran against an uninitialised value. shim_list_drives in the same file does check it. build.rs treated cc and ar as successful if the process merely SPAWNED, so a genuine compile error in the macOS C shim produced no object file and surfaced later as an unexplained link failure against a missing symbol. The shim is macOS-only and is neither linted nor compiled on the other two platforms, so a mistake in it has exactly one chance to be noticed. The last two have no test: one needs IOMainPort to fail, the other needs a deliberately broken C shim, and neither is reachable from the test harness. Both mirror a correct sibling in the same file, which is the evidence available. |
||
|
Matthew Jackson
|
921404d135 |
Fix five clippy errors that only appear on the target CI lints
CI's clippy job runs on ubuntu-latest, so cfg(target_os = "linux") code is what the gate actually compiles — and none of it is built by clippy on a Mac. Five `-D warnings` errors were sitting in drive/linux.rs, scsi/linux.rs, io/writeback/linux.rs and the Linux arm of drive/mod.rs: four collapsible let-chains and one manual `% n == 0`. CI was red on the lint job while the local gate reported all green. Collapsed into let-chains, which the declared toolchain supports, and folded drive/mod.rs's length precondition into its chain so the body no longer needs a nested block. Found because an agent working on the SCSI backends reported the lints in passing while checking that a Windows-only file compiled. Worth noting how it stayed hidden: every one of these files is cfg-gated to a platform this machine is not, so no amount of local gating would have surfaced them. The companion change to the precommit script closes that hole. |