fix: assorted correctness fixes and dead-code cleanup

- aacs/resolve: a media-keys-only provider missing the VID classifies as
  VidUnavailable, not NoMaterial (an MK derives the VUK once the VID
  arrives).
- disc/bluray: mark a clip seen only after its .clpi parses, so a
  transient parse failure on the first PlayItem cannot suppress the
  clip's extents for a later PlayItem referencing it that succeeds.
- disc/patch: log rather than swallow mapfile record/flush failures on a
  reverify downgrade, so a failed persist cannot silently mismark a bad
  unit good on resume.
- mux/ts: flag a discontinuity when a partial PES is dropped, matching
  the other partial-drop paths.
- mux/demux_thread: the no-demuxer branch forwards an empty batch for
  early consumer-disconnect detection instead of reading the whole disc.
- io/pipeline: correct the send-timing log (as_secs_f64, not as_micros
  printed as ms).
- aacs/derive, aacs/variant, disc/read_error, keysource: comment/doc
  accuracy. sector/prefetched, udf: remove dead fields/functions.
- mux/disc: assert unit-aligned read counts in the test.
This commit is contained in:
Matthew Jackson
2026-07-08 14:44:15 -07:00
parent 67aba17173
commit 0d587d1154
13 changed files with 95 additions and 114 deletions
+8 -7
View File
@@ -203,13 +203,14 @@ pub fn derive_media_key_and_pk_from_dk(
let p_uv = &uvs[1 + 5 * uvs_idx..]; let p_uv = &uvs[1 + 5 * uvs_idx..];
let u_mask_shift = uvs[5 * uvs_idx]; // byte before the UV value let u_mask_shift = uvs[5 * uvs_idx]; // byte before the UV value
if u_mask_shift & 0xC0 != 0 { // `num_uvs` was computed via `take_while(.. c[0] & 0xC0 == 0)`, so
break; // device revoked // every iterated slot already has its revoked-marker bits clear — no
} // inner `& 0xC0` re-check is needed (it would be unreachable).
// Shifts of 32..=63 (0x20..=0x3F pass the 0xC0 mask above) would //
// panic in debug / wrap to a wrong mask in release. The MKB byte // Shifts of 32..=63 (0x20..=0x3F) have those bits clear but would
// is disc-controlled, so a crafted/corrupt MKB must not crash the // panic in debug / wrap to a wrong mask in release. The MKB byte is
// ripper: skip an out-of-range slot rather than `<<` it. // disc-controlled, so a crafted/corrupt MKB must not crash the ripper:
// skip an out-of-range slot rather than `<<` it.
if u_mask_shift >= 32 { if u_mask_shift >= 32 {
continue; continue;
} }
+6 -2
View File
@@ -106,8 +106,12 @@ pub fn resolve_keys_with_reason(
pub(crate) fn classify_resolve_failure(ctx: &ResolveContext<'_>) -> ResolveFailure { pub(crate) fn classify_resolve_failure(ctx: &ResolveContext<'_>) -> ResolveFailure {
let has_vid = *ctx.volume_id != [0u8; 16]; let has_vid = *ctx.volume_id != [0u8; 16];
let providers = super::provider::Providers(ctx.providers); let providers = super::provider::Providers(ctx.providers);
let has_derivation_material = // Media keys are also derivation material: with an MK you can derive the VUK
!providers.device_keys().is_empty() || !providers.processing_keys().is_empty(); // once you have the VID, so a media-keys-only provider that is merely missing
// the VID is VidUnavailable, not NoMaterial.
let has_derivation_material = !providers.device_keys().is_empty()
|| !providers.processing_keys().is_empty()
|| !providers.media_keys().is_empty();
if !has_vid && has_derivation_material { if !has_vid && has_derivation_material {
ResolveFailure::VidUnavailable ResolveFailure::VidUnavailable
} else { } else {
+5 -8
View File
@@ -201,14 +201,11 @@ pub fn walk_processing_key(
// parse stops, no inner re-check needed. // parse stops, no inner re-check needed.
let u_mask_shift = uvs[5 * uvs_idx]; let u_mask_shift = uvs[5 * uvs_idx];
if u_mask_shift & 0xC0 != 0 { // 0x20..=0x3F (32..=63) have their revoked-marker bits clear (so they
break; // pass the take_while above) but are out of range for a u32 shift.
} // `wrapping_shl` would silently compute shift % 32 (e.g. 32 → no shift
// 0x20..=0x3F (32..=63) pass the 0xC0 revoked-marker check but are // → 0xFFFF_FFFF), matching a wrong uv slot and deriving a wrong key.
// out of range for a u32 shift. `wrapping_shl` would silently // Disc-controlled byte: skip the slot instead.
// compute shift % 32 (e.g. 32 → no shift → 0xFFFF_FFFF), matching a
// wrong uv slot and deriving a wrong key. Disc-controlled byte:
// skip the slot instead.
if u_mask_shift >= 32 { if u_mask_shift >= 32 {
continue; continue;
} }
+6 -1
View File
@@ -85,13 +85,18 @@ impl Disc {
for play_item in &parsed.play_items { for play_item in &parsed.play_items {
let clip_dur = play_item.out_time.saturating_sub(play_item.in_time) as f64 / 45000.0; let clip_dur = play_item.out_time.saturating_sub(play_item.in_time) as f64 / 45000.0;
let mut pkt_count: u32 = 0; let mut pkt_count: u32 = 0;
let first_ref = seen_clips.insert(play_item.clip_id.clone());
let clpi_path = format!("/BDMV/CLIPINF/{}.clpi", play_item.clip_id); let clpi_path = format!("/BDMV/CLIPINF/{}.clpi", play_item.clip_id);
if let Ok(clpi_data) = udf_fs.read_file(reader, &clpi_path) { if let Ok(clpi_data) = udf_fs.read_file(reader, &clpi_path) {
if let Ok(clip_info) = clpi::parse(&clpi_data) { if let Ok(clip_info) = clpi::parse(&clpi_data) {
pkt_count = clip_info.source_packet_count; pkt_count = clip_info.source_packet_count;
// Mark the clip seen ONLY after its .clpi parses — a transient
// read/parse failure on the first PlayItem referencing a clip
// must not permanently suppress its extents/size for a later
// PlayItem referencing the same clip that succeeds.
let first_ref = seen_clips.insert(play_item.clip_id.clone());
// Only fetch/push the physical extents and add to the // Only fetch/push the physical extents and add to the
// total size the first time this clip_id is seen. // total size the first time this clip_id is seen.
if first_ref { if first_ref {
+19 -13
View File
@@ -473,9 +473,7 @@ pub(super) fn recovery_read<R: SectorSource + ?Sized>(
/// gets recorded NonTrimmed. Pure data structure — no I/O — so each phase /// gets recorded NonTrimmed. Pure data structure — no I/O — so each phase
/// helper is unit-testable by asserting the residual `SubRanges`. /// helper is unit-testable by asserting the residual `SubRanges`.
/// ///
/// Foundation for the phased `recover_section` orchestrator; not yet wired /// The residue tracker used by the phased `recover_section` orchestrator.
/// into the live loop (see the deferral note in the #50 work).
#[cfg_attr(not(test), allow(dead_code))]
#[derive(Debug, Clone, PartialEq, Eq, Default)] #[derive(Debug, Clone, PartialEq, Eq, Default)]
pub(super) struct SubRanges { pub(super) struct SubRanges {
/// (pos, len) pairs, sorted by pos, non-overlapping, all non-zero len. /// (pos, len) pairs, sorted by pos, non-overlapping, all non-zero len.
@@ -1461,13 +1459,22 @@ impl Disc {
if !bad.is_empty() { if !bad.is_empty() {
let n: usize = bad.len(); let n: usize = bad.len();
for (lba, cnt) in bad { for (lba, cnt) in bad {
let _ = m.record( if let Err(e) = m.record(
lba as u64 * 2048, lba as u64 * 2048,
cnt as u64 * 2048, cnt as u64 * 2048,
mapfile::SectorStatus::NonTrimmed, mapfile::SectorStatus::NonTrimmed,
) {
tracing::warn!(
lba,
"reverify downgrade: mapfile record failed ({e}) — unit may stay mismarked as good"
);
}
}
if let Err(e) = m.flush() {
tracing::warn!(
"reverify downgrade: mapfile flush failed ({e}) — downgrade not persisted; a resume could mismark it good"
); );
} }
let _ = m.flush();
// The re-verify ran AFTER `pipe.finish()` snapshotted // The re-verify ran AFTER `pipe.finish()` snapshotted
// `summary.stats`, so those stats still count the just- // `summary.stats`, so those stats still count the just-
// downgraded units as good. Refresh from the mapfile so // downgraded units as good. Refresh from the mapfile so
@@ -1547,14 +1554,13 @@ mod tests {
assert!(on("true")); assert!(on("true"));
} }
/// Transport failure (status=0xFF, USB-bridge crash) must be recognised by /// Transport failure (status=0xFF, USB-bridge crash) must be recognised and
/// the gate `handle_read_failure` now checks FIRST, so it aborts the pass /// abort the pass, rather than being treated as an ordinary bad sector and
/// (wedged_exit + BreakOuter) instead of treating the bridge crash as an /// hammering the crashed device for up to the per-range watchdog budget. The
/// ordinary bad sector and hammering the crashed device for up to the /// transport-failure classification predicate is not unit-testable in
/// per-range watchdog budget. `handle_read_failure` is not unit-testable in /// isolation, so this guards the predicate the production early-return keys
/// isolation, so this guards the classification predicate the production /// off, and the contrast that an ordinary read error is NOT misclassified as
/// early-return keys off, and the contrast that an ordinary read error is /// a transport failure.
/// NOT misclassified as a transport failure.
#[test] #[test]
fn transport_failure_is_recognised_for_patch_abort() { fn transport_failure_is_recognised_for_patch_abort() {
use crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE; use crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE;
+6 -18
View File
@@ -196,22 +196,10 @@ impl ReadCtx {
/// threshold is loose so we don't bail too early on a range that /// threshold is loose so we don't bail too early on a range that
/// has scattered good sectors mixed in. /// has scattered good sectors mixed in.
/// ///
/// `damage_threshold_pct = 6` mirrors `disc/patch.rs`'s /// `damage_threshold_pct = 6` is looser than Pass 1 (12%): Pass N triggers
/// `PASSN_DAMAGE_THRESHOLD_PCT`. Pass N triggers the damage-skip /// the damage-skip at half Pass 1 density because the patch loop exists to chip
/// at half the density Pass 1 uses (Pass 1 = 12%) because the /// away at bad ranges, so being more eager to skip clustered bad sectors
/// patch loop's whole job is to chip away at bad ranges — being /// converges faster on the recoverable good sectors inside a range.
/// more eager to skip clustered bad sectors converges faster on
/// the recoverable good sectors inside a range. The patch-side
/// `compute_damage_skip` reads its threshold directly from
/// `PASSN_DAMAGE_THRESHOLD_PCT`, which is an alias for this crate's
/// `PATCH_DAMAGE_THRESHOLD_PCT`, so the two are always in sync.
/// The patch loop's damage-skip is not yet unified with `handle_read_error`'s
/// jump path. (v0.20.8 unification attempt found the unification
/// itself blocked on the size-aware `range_remaining/4` cap that
/// lives in `compute_damage_skip` but not in
/// `handle_read_error::JumpAhead` — see
/// `tests/passn_handler_ab.rs` for the A/B fixture that pins
/// the divergence point.)
pub fn for_patch(batch: u16) -> Self { pub fn for_patch(batch: u16) -> Self {
Self { Self {
batch, batch,
@@ -443,8 +431,8 @@ const WEDGE_ABORT_THRESHOLD: u64 = 16;
const WEDGE_PASS_N_SKIP_SECTORS: u64 = 64; const WEDGE_PASS_N_SKIP_SECTORS: u64 = 64;
/// Single source of truth for the Pass-N damage-window threshold. /// Single source of truth for the Pass-N damage-window threshold.
/// Both [`ReadCtx::for_patch`] and `disc::patch::compute_damage_skip` /// [`ReadCtx::for_patch`] reads this constant for the Pass-N damage-skip
/// reference this constant so the two damage-skip paths cannot drift. /// threshold.
/// ///
/// 6% means: with a 16-entry sliding window, the damage-skip fires /// 6% means: with a 16-entry sliding window, the damage-skip fires
/// once 1 out of 16 recent reads has failed. Pass 1 uses a 12% /// once 1 out of 16 recent reads has failed. Pass 1 uses a 12%
+8 -2
View File
@@ -449,7 +449,10 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
} else { } else {
// Benign per-item OK: trace-level (L4) only; the // Benign per-item OK: trace-level (L4) only; the
// apply-side rolling summary carries throughput. // apply-side rolling summary carries throughput.
tracing::trace!("Pipeline send: OK in {:.3}ms", elapsed.as_micros()); tracing::trace!(
"Pipeline send: OK in {:.3}ms",
elapsed.as_secs_f64() * 1000.0
);
} }
} }
Ok(()) Ok(())
@@ -464,7 +467,10 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
std::any::type_name::<I>() std::any::type_name::<I>()
); );
} else { } else {
tracing::debug!("Pipeline send: failed after {:.3}ms", elapsed.as_micros()); tracing::debug!(
"Pipeline send: failed after {:.3}ms",
elapsed.as_secs_f64() * 1000.0
);
} }
} }
Err(e.0) Err(e.0)
+4 -4
View File
@@ -77,7 +77,7 @@ pub trait ResolveCtx {
fn mkb(&self) -> Result<&[u8], Error>; fn mkb(&self) -> Result<&[u8], Error>;
/// The disc's encrypted title keys, parsed from `Unit_Key_RO.inf` the same /// The disc's encrypted title keys, parsed from `Unit_Key_RO.inf` the same
/// way the library's resolver parses them ([`crate::aacs::inf::parse_unit_key_ro`]), /// way the library's resolver parses them ([`crate::aacs::inf::parse_unit_key_ro`]),
/// in on-disc order. Feed straight into [`crate::aacs::boil::uk_from_vuk`]. /// in on-disc order. Feed straight into [`crate::aacs::derive::decrypt_unit_key`].
fn enc_title_keys(&self) -> Result<&[[u8; 16]], Error>; fn enc_title_keys(&self) -> Result<&[[u8; 16]], Error>;
/// Up to `n` encrypted on-disc content sample units, for a source that /// Up to `n` encrypted on-disc content sample units, for a source that
/// validates a candidate server-side against real ciphertext. /// validates a candidate server-side against real ciphertext.
@@ -167,8 +167,8 @@ impl ResolveCtx for DiscInputsCtx<'_> {
/// holds, orchestrates the derivation down to Unit Keys using the library's /// holds, orchestrates the derivation down to Unit Keys using the library's
/// boil-down crypto primitives — never re-implementing AES. A source that holds /// boil-down crypto primitives — never re-implementing AES. A source that holds
/// pre-decrypted Unit Keys returns them directly; one that holds a VUK calls /// pre-decrypted Unit Keys returns them directly; one that holds a VUK calls
/// [`crate::aacs::boil::uk_from_vuk`]; one that holds device keys calls /// [`crate::aacs::derive::decrypt_unit_key`]; one that holds device keys calls
/// [`crate::aacs::boil::mk_from_dk`] → [`crate::aacs::boil::vuk_from_mk`] → `uk_from_vuk`. /// [`crate::aacs::derive::derive_media_key_from_dk`] → [`crate::aacs::derive::derive_vuk`] → `decrypt_unit_key`.
/// ///
/// Returning an empty `Vec` means "no key for this disc from this source"; an /// Returning an empty `Vec` means "no key for this disc from this source"; an
/// `Err` means the source itself failed (I/O, parse, network). The caller /// `Err` means the source itself failed (I/O, parse, network). The caller
@@ -222,7 +222,7 @@ pub fn resolve_and_apply(
/// success — so a wrong/partial key set is rejected and the loop continues. /// success — so a wrong/partial key set is rejected and the loop continues.
/// ///
/// CPS-unit numbering: a source returns Unit Keys carrying the POSITIONAL index /// CPS-unit numbering: a source returns Unit Keys carrying the POSITIONAL index
/// from [`crate::aacs::boil::uk_from_vuk`]; the library's canonical CPS-unit number is /// from [`crate::aacs::derive::decrypt_unit_key`]; the library's canonical CPS-unit number is
/// `position + 1` (matching [`crate::aacs::inf::parse_unit_key_ro`]'s `(i + 1)`), so /// `position + 1` (matching [`crate::aacs::inf::parse_unit_key_ro`]'s `(i + 1)`), so
/// the committed `AacsState.unit_keys` is byte-identical to the library-resolved /// the committed `AacsState.unit_keys` is byte-identical to the library-resolved
/// path. The number is cosmetic for descramble (the decrypt path strips it and /// path. The number is cosmetic for descramble (the decrypt path strips it and
+19 -3
View File
@@ -204,6 +204,14 @@ impl DemuxThread {
} }
} else { } else {
let _ = recycle_tx.send(buf); let _ = recycle_tx.send(buf);
// No demuxer (a BdTs title with zero streams): still send
// an empty batch so an early consumer disconnect is
// detected here too, exactly like the ts/ps branches above.
// Without it this worker reads the whole disc even after
// the consumer has dropped.
if tx.send(DemuxBatch::Ts(Vec::new())).is_err() {
return;
}
} }
} }
// Flush tail packets at EOF. // Flush tail packets at EOF.
@@ -479,7 +487,7 @@ mod tests {
#[test] #[test]
fn no_demuxer_configured_still_recycles_and_eofs() { fn no_demuxer_configured_still_recycles_and_eofs() {
// With neither ts nor ps set, the worker must still recycle buffers // With neither ts nor ps set, the worker must still recycle buffers
// and terminate with Eof — never emit a spurious Ts/Ps batch. // and terminate with Eof — also forward an empty batch per buffer for disconnect detection.
let (pf_tx, pf_rx) = bounded::<std::io::Result<Vec<u8>>>(4); let (pf_tx, pf_rx) = bounded::<std::io::Result<Vec<u8>>>(4);
let (rc_tx, rc_rx) = bounded::<Vec<u8>>(4); let (rc_tx, rc_rx) = bounded::<Vec<u8>>(4);
let (_dt, rx) = DemuxThread::spawn_zero_copy(pf_rx, rc_tx, (), None, None, None).unwrap(); let (_dt, rx) = DemuxThread::spawn_zero_copy(pf_rx, rc_tx, (), None, None, None).unwrap();
@@ -492,8 +500,16 @@ mod tests {
drop(pf_tx); drop(pf_tx);
let batches = collect_batches(&rx, Duration::from_secs(5)); let batches = collect_batches(&rx, Duration::from_secs(5));
assert_eq!(batches.len(), 1, "only the Eof sentinel"); // The no-demuxer branch now forwards an empty Ts batch per buffer for
assert!(matches!(batches[0], DemuxBatch::Eof)); // early consumer-disconnect detection (same rationale as the ts/ps
// branches), then the Eof sentinel.
assert_eq!(
batches.len(),
2,
"empty Ts disconnect-probe batch, then Eof"
);
assert!(matches!(batches[0], DemuxBatch::Ts(ref v) if v.is_empty()));
assert!(matches!(batches[1], DemuxBatch::Eof));
} }
#[test] #[test]
+9 -5
View File
@@ -1520,11 +1520,15 @@ mod tests {
"read at lba {lba} is not unit-aligned (offset {} % {ALIGN} != 0)", "read at lba {lba} is not unit-aligned (offset {} % {ALIGN} != 0)",
lba - ext_start lba - ext_start
); );
// Non-tail reads must be a whole number of units; the only // Non-tail reads must be a whole number of units; the only permitted
// permitted short read is the final partial unit (here COUNT is a // short read is a final partial unit (below one unit). Assert it
// multiple of ALIGN, so every read should be unit-multiple unless // rather than documenting it — a mid-stream non-unit-multiple read
// it shrank below one unit — which is itself a single unit). // would straddle AACS unit boundaries and decrypt under the wrong
let _ = count; // alignment.
assert!(
count as u32 % ALIGN == 0 || (count as u32) < ALIGN,
"read count {count} is neither a whole number of units nor a sub-unit tail"
);
} }
// At least one error was skipped (the bad unit) and a SectorSkipped // At least one error was skipped (the bad unit) and a SectorSkipped
+4
View File
@@ -167,6 +167,10 @@ impl PesAssembler {
self.buffer.clear(); self.buffer.clear();
self.active = false; self.active = false;
self.header_remaining = 0; self.header_remaining = 0;
// A dropped partial PES is a gap in the elementary stream — flag
// it so the NEXT completed PES carries a discontinuity, matching
// every other partial-drop path in this file (lines 395/479/504).
self.pending_discontinuity = true;
return; return;
} }
self.buffer.extend_from_slice(data); self.buffer.extend_from_slice(data);
+1 -4
View File
@@ -358,7 +358,6 @@ impl PrefetchedSectorSource {
// and suppresses `self`'s own `Drop` (which would otherwise // and suppresses `self`'s own `Drop` (which would otherwise
// double-`join`), leaving NO extra live endpoint behind. This // double-`join`), leaving NO extra live endpoint behind. This
// is the panic-free equivalent of the `Option::take` approach. // is the panic-free equivalent of the `Option::take` approach.
let total = self.total_sectors;
let me = std::mem::ManuallyDrop::new(self); let me = std::mem::ManuallyDrop::new(self);
// SAFETY: `me` is `ManuallyDrop`, so none of these fields will // SAFETY: `me` is `ManuallyDrop`, so none of these fields will
// be dropped by `me`. Each `ptr::read` performs exactly one // be dropped by `me`. Each `ptr::read` performs exactly one
@@ -367,7 +366,7 @@ impl PrefetchedSectorSource {
let producer = unsafe { std::ptr::read(&me.producer) }; let producer = unsafe { std::ptr::read(&me.producer) };
let rx = unsafe { std::ptr::read(&me.rx) }; let rx = unsafe { std::ptr::read(&me.rx) };
let recycle = unsafe { std::ptr::read(&me.recycle_tx) }; let recycle = unsafe { std::ptr::read(&me.recycle_tx) };
(rx, recycle, PrefetchShell { producer, total }) (rx, recycle, PrefetchShell { producer })
} }
} }
@@ -376,8 +375,6 @@ impl PrefetchedSectorSource {
/// producer, even though the channels have been peeled off. /// producer, even though the channels have been peeled off.
pub struct PrefetchShell { pub struct PrefetchShell {
producer: Option<JoinHandle<()>>, producer: Option<JoinHandle<()>>,
#[allow(dead_code)]
total: u32,
} }
impl Drop for PrefetchShell { impl Drop for PrefetchShell {
-47
View File
@@ -325,53 +325,6 @@ impl UdfFs {
Ok(merged) Ok(merged)
} }
/// All sector ranges that contain data (metadata + all files including STREAM).
/// For full disc-to-ISO dumps — reads only allocated sectors, skips gaps.
pub fn all_sector_ranges(&self, reader: &mut dyn SectorSource) -> Result<Vec<(u32, u32)>> {
let mut ranges = Vec::new();
// UDF structure sectors
let meta_end = self.metadata_start.saturating_add(self.metadata_sectors);
ranges.push((0, meta_end));
// Walk entire tree including STREAM directories
self.collect_all_file_ranges(reader, &self.root, &mut ranges)?;
// Merge overlapping/adjacent ranges and sort
ranges.sort_by_key(|r| r.0);
let merged = merge_ranges(&ranges);
Ok(merged)
}
fn collect_all_file_ranges(
&self,
reader: &mut dyn SectorSource,
entry: &DirEntry,
ranges: &mut Vec<(u32, u32)>,
) -> Result<()> {
for child in &entry.entries {
if child.is_dir {
self.collect_all_file_ranges(reader, child, ranges)?;
} else {
// Include the ICB sector
ranges.push((self.meta_to_abs(child.meta_lba)?, 1));
// Include ALL file data extents (large m2ts files have many)
if let Ok(extents) = self.read_icb_extents(reader, child.meta_lba) {
for (data_lba, data_len) in extents {
let abs_start = match self.partition_start.checked_add(data_lba) {
Some(v) => v,
None => continue,
};
let sector_count = (data_len as u64).div_ceil(2048) as u32;
ranges.push((abs_start, sector_count));
}
}
}
}
Ok(())
}
fn collect_file_ranges( fn collect_file_ranges(
&self, &self,
reader: &mut dyn SectorSource, reader: &mut dyn SectorSource,