Audit round 7: account for every clip that cannot be resolved

Ten lenses over v1.6.4..HEAD, every claim read against the code before
it was believed. Seven confirmed; six are here, one is recorded for the
next round. All of these are the same family — a failure wearing the
shape of success — which is the family that once shipped 9 MB of
ciphertext inside a main-movie m2ts at rc=0.

A clip whose extents cannot be resolved is now accounted for, in both
disc readers. Only `UdfUnrecordedExtent` used to count: every other way
`file_extents` can fail — a scratched sector under the clip's ICB
(DiscRead), an allocation-descriptor chain that never terminated, a file
whose data is embedded rather than extent-mapped — fell through to the
ordinary "file absent" path. On Blu-ray that yielded a title advertising
its full runtime with a clip's bytes silently missing, because the size
and the play-item timing had already counted it. On HD-DVD it was worse:
the clip was never added to `unusable`, so a split feature still composed
from FEATURE_1 alone and offered half a movie as the whole thing. Neither
emitted a single log line. Absence is still benign — a 2D disc has no
.ssif and the extension fallback exists for exactly that.

`Halted` is excluded deliberately, and that exclusion is the whole reason
the first version of this fix was wrong. Cancellation makes EVERY drive
command return `Halted`; classifying it as a disc defect would have
dropped each remaining playlist in turn and handed back a truncated title
list at success — the same defect, wearing a cancel. `parse_playlist`
returns Option and has no channel to propagate a halt, so the existing
behaviour is preserved rather than made worse. Propagating it properly is
next round's work.

Both log sites now emit the error's OWN code instead of a hardcoded 6017.
Accounting a scratched disc (E6000) as an authoring hole would send
anyone triaging it looking for the wrong thing entirely.

AD type 3 is embedded data, not a descriptor list (ECMA-167 4/14.6.8).
`read_icb_extents` lumped it in with the reserved values and decoded the
file's own CONTENT as (length, LBA) pairs, manufacturing extents out of
arbitrary bytes and pointing the reader at unrelated sectors. This same
release already taught `read_directory` to honour type 3; this is the
file half of that decision. It is an error rather than an empty list,
because an empty list reaches the caller as a clip that contributed
nothing while its declared duration still counts it — the silent loss
pointed the other way. A legally zero-length embedded file still returns
an empty list. New code E6018: reusing DiscRead would have mislabelled a
deterministic structural property as transient I/O and fed the retry and
NonTrimmed machinery a byte that will never change.

`file_extents_addressing`, `extents_abs_at` and `AbsExtent` drop to
`pub(crate)`. The first hands back unrecorded extents UNFLAGGED, in a
shape identical to the safe call's return; its doc says callers must use
`file_extents` instead, but a doc comment is not a guard. No dependent
crate references any of the three.

Three tests close gaps the audit found, each proven red before green:
a held AC-3 access unit must not resume as a normal frame after its track
poisons; the PS resume cursor must survive a drain that rebases it (three
separate mutants caught); and AD type 3 must be refused rather than
decoded. The first attempt at the HD-DVD test passed with the fix
reverted, which made it worthless — it needed a VTI fixture before the
composition path ran at all.

Also: four error codes were missing from the uniqueness test that claims
to cover every published code, so a new variant reusing 6014, 6016 or
6017 would have passed it.
This commit is contained in:
Matthew Jackson
2026-08-18 13:46:03 -07:00
parent 313460c97f
commit 0563b58f2e
6 changed files with 542 additions and 14 deletions
+109 -1
View File
@@ -957,7 +957,31 @@ impl Disc {
unusable.insert(name.to_ascii_lowercase());
}
Err(crate::error::Error::Halted) => return Err(crate::error::Error::Halted),
Err(_) => {}
// EVERY other failure means the same thing: no truthful read
// plan for this clip. A scratched sector under its ICB
// (DiscRead), an allocation-descriptor chain that never
// terminated (UdfAdChainTooLong), a file whose data is
// embedded rather than extent-mapped (UdfEmbeddedData) — all
// of them used to land in a bare `Err(_) => {}`: no log, and
// the clip NOT marked unusable, so a split feature still
// composed from FEATURE_1 alone and presented half a movie as
// a whole one. That is the very outcome the arm above was
// written to prevent, reachable through every error but one.
//
// The code logged is the error's own. Reusing 6017 here would
// account a scratched disc as an authoring hole.
//
// (`UdfNotFound` needs no special case: these names came from
// `ts_dir.entries`, so the lookup cannot miss.)
Err(e) => {
tracing::warn!(
target: "freemkv::disc",
clip = ?name,
code = e.code(),
"clip extents could not be resolved; dropping every title that names it"
);
unusable.insert(name.to_ascii_lowercase());
}
}
if !extents.is_empty() {
clip_extents.insert(name.to_ascii_lowercase(), (name.clone(), *size, extents));
@@ -2848,6 +2872,90 @@ mod tests {
);
}
/// A clip whose extents cannot be resolved must drop the SPLIT FEATURE.
///
/// Catches reverting the `Err(e) => { warn; unusable.insert(..) }` arm in
/// `Disc::scan_hddvd_titles` back to the bare `Err(_) => {}` it replaced.
///
/// RED BEFORE GREEN, and TWO earlier attempts at this test did NOT go red.
/// The first asserted "no title names the broken clip" — that passes either
/// way, because a clip that resolves to no extents is never inserted into
/// `clip_extents` and so yields no per-clip title regardless. The second
/// fixed that but shipped no `.vti`: `order` is built solely from
/// `parse_vti_clip_order` of the navigation file, so with no `.vti` it is
/// EMPTY, `feature` is empty, and no composed title is ever built — the
/// assertion held vacuously with the fix reverted. Hence the synthetic
/// `HVA00001.VTI` below: it is what makes the composer run at all.
///
/// The defect lives one level up from the per-clip titles, in the composed
/// feature: `unusable` is what tells the composer that a part is MISSING
/// rather than merely absent, and the old bare `Err(_) => {}` populated it
/// for nothing but an unrecorded extent. So a scratched sector under
/// FEATURE_2's ICB (`Error::DiscRead`) left FEATURE_1 composing a title
/// named "FEATURE" by itself — half a movie offered as the whole one.
#[test]
fn scan_hddvd_titles_drops_a_split_feature_whose_part_cannot_be_read() {
let mut disc = MemDisc::new();
// The VTI clip table is the ONLY source of authored order, and the
// composed feature title exists only for clips it names. Without it
// this test cannot distinguish the fix from its absence.
let vti_bytes = synthetic_vti(&["FEATURE_1.EVO", "FEATURE_2.EVO"]);
let files = vec![
file_with("HVA00001.VTI", 90, 20000, vti_bytes, true),
file("FEATURE_1.EVO", 100, 5000, 4 * 2048, true),
file("FEATURE_2.EVO", 101, 9000, 4 * 2048, true),
];
let root = DirSpec {
name: String::new(),
icb_lba: 10,
dir_data_lba: 11,
files: Vec::new(),
subdirs: vec![DirSpec {
name: "HVDVD_TS".to_string(),
icb_lba: 20,
dir_data_lba: 21,
files,
subdirs: vec![],
}],
};
build_udf_skeleton(&mut disc, 10);
lay_dir(&mut disc, &root);
// Blank FEATURE_2's ICB (laid at PART_START + 101). Its descriptor tag
// is then 0, neither 261 nor 266 — what the parser sees when the sector
// holding an ICB cannot be read back intact. Deliberately NOT an
// unrecorded extent: that class was already handled.
disc.put_bytes(PART_START + 101, &[0u8; 2048]);
let udf = crate::udf::read_filesystem(&mut disc).expect("fs");
assert!(
matches!(
udf.file_extents(&mut disc, "/HVDVD_TS/FEATURE_2.EVO"),
Err(crate::error::Error::DiscRead { .. })
),
"fixture must fail with DiscRead, not UdfUnrecordedExtent"
);
// Guard the guard: if the VTI ever stopped parsing, `order` would be
// empty and the assertion below would hold for the wrong reason.
assert_eq!(
parse_vti_clip_order(&synthetic_vti(&["FEATURE_1.EVO", "FEATURE_2.EVO"])),
vec!["FEATURE_1.EVO".to_string(), "FEATURE_2.EVO".to_string()],
"fixture VTI must yield both feature parts in authored order"
);
let titles = Disc::scan_hddvd_titles(&mut disc, &udf, None).expect("scan");
// FEATURE_1 alone must not be offered as the feature. It may still
// appear as its own standalone clip title — that stands alone and is
// honest — but nothing may present it as the composed whole.
let composed: Vec<_> = titles
.iter()
.filter(|t| t.clips.len() > 1 || t.playlist.eq_ignore_ascii_case("FEATURE"))
.map(|t| &t.playlist)
.collect();
assert!(
composed.is_empty(),
"a split feature missing one part must not compose; got {composed:?}"
);
}
/// A clip whose file has a zero-byte size (a degenerate/empty allocation:
/// its ICB's allocation descriptor has `data_len == 0`, the UDF AD-list
/// terminator, so `file_extents` yields no extent at all) must not