From 661ab138c62ac587f7c62740d3cba5989d7aa7cd Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Fri, 24 Jul 2026 08:32:37 -0700 Subject: [PATCH] Fix audit findings: DTS AMODE bound, key-fetch negative memoization, PGS probe coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - dts: accept all 16 legal AMODE channel-arrangement codes (0-15), not just 0-9. Per ETSI TS 102 114 the 6-bit AMODE field has 16 defined arrangements; only 16-63 are reserved. ffmpeg's ff_dca_channels[16] confirms 10-15 are decodable 6/7/8-channel layouts. The old bound of 10 dropped spec-legal multichannel core frames as undecodable, silencing recoverable audio. Add a regression test (literal 0..16 range) that fails if the bound reverts to 10. - keysource: only memoize a NEGATIVE (empty) key-fetch result when every source genuinely ran and none held the key — never when a source Err'd (network down, unreachable). A transient outage was being cached as a permanent "no key" for the fingerprint, permanently dropping a unit that could be recovered once the source came back. Thread an `errored` flag out of the drivers and gate the cache insert on it. Tests cover both the recover-after-outage case and that a genuine absence is still memoized. - pgs_forced_probe: add happy-path coverage feeding real synthetic BD-TS PGS display sets through the full demux -> parse -> observe -> apply path, both a forced verdict landing and a non-forced verdict clearing a vendor flag. - mp4: correct fit_report doc (audio carried is AC-3/E-AC-3 AND DTS/DTS-HD). - scan_iso test: add independent fixture expectations (volume id) so the parity test is no longer purely tautological against a re-run of the same composition. --- src/disc/pgs_forced_probe.rs | 130 ++++++++++++++++++++++++ src/keysource.rs | 187 +++++++++++++++++++++++++++++++---- src/mux/codec/dts.rs | 53 +++++++++- src/mux/mp4/mod.rs | 5 +- tests/scan_iso.rs | 12 ++- 5 files changed, 360 insertions(+), 27 deletions(-) diff --git a/src/disc/pgs_forced_probe.rs b/src/disc/pgs_forced_probe.rs index fb08f92..24184be 100644 --- a/src/disc/pgs_forced_probe.rs +++ b/src/disc/pgs_forced_probe.rs @@ -182,6 +182,136 @@ mod tests { assert!(s.forced, "no content observed → vendor forced preserved"); } + /// A reader that serves a fixed BD-TS byte stream once (across sequential + /// `read_sectors` calls), then EOF — so the probe's demux→parse→observe→apply + /// path runs on real synthetic PGS content. + struct TsReader { + data: Vec, + pos: usize, + } + impl SectorSource for TsReader { + fn read_sectors( + &mut self, + _lba: u32, + _count: u16, + buf: &mut [u8], + _recovery: bool, + ) -> crate::error::Result { + if self.pos >= self.data.len() { + return Ok(0); + } + let n = buf.len().min(self.data.len() - self.pos); + buf[..n].copy_from_slice(&self.data[self.pos..self.pos + n]); + self.pos += n; + Ok(n) + } + fn capacity_sectors(&self) -> u32 { + self.data.len().div_ceil(SECTOR_BYTES) as u32 + } + } + + // PGS PCS layout (matches the private constants in mux::codec::pgs): a + // display-set frame begins with a PCS (segment type 0x16); byte 13 is + // number_of_composition_objects; byte 17 is the first object's flags, whose + // 0x40 bit is forced_on_flag. + const PCS_SEG: u8 = 0x16; + const PCS_NUM_OBJECTS_OFF: usize = 13; + const PCS_FLAGS_OFF: usize = 17; + const PCS_FORCED_FLAG: u8 = 0x40; + + /// One PGS display-set elementary payload with a single composition object; + /// `forced` sets forced_on_flag. + fn pcs_display(forced: bool) -> Vec { + let mut d = vec![0u8; 18]; + d[0] = PCS_SEG; + d[PCS_NUM_OBJECTS_OFF] = 1; + d[PCS_FLAGS_OFF] = if forced { PCS_FORCED_FLAG } else { 0 }; + d + } + + /// Wrap an elementary payload in one 192-byte BD-TS PES packet (PUSI, PTS + /// present) on `pid`. `cc` is the 4-bit continuity counter. + fn bd_pes_packet(pid: u16, cc: u8, es: &[u8]) -> Vec { + let mut pkt = vec![0u8; 192]; + // pkt[0..4] = TP_extra_header (zeros). TS packet starts at pkt[4]. + pkt[4] = 0x47; // sync + pkt[5] = 0x40 | ((pid >> 8) & 0x1F) as u8; // PUSI + PID high 5 bits + pkt[6] = (pid & 0xFF) as u8; // PID low 8 bits + pkt[7] = 0x10 | (cc & 0x0F); // adaptation=payload-only + continuity counter + // PES header (at ts payload = pkt[8..]): 00 00 01 stream_id len flags. + let p = 8; + pkt[p] = 0x00; + pkt[p + 1] = 0x00; + pkt[p + 2] = 0x01; + pkt[p + 3] = 0xBD; // private_stream_1 (carries the standard PES extension) + pkt[p + 4] = 0x00; // PES packet length hi (0 = unbounded; ignored by demux) + pkt[p + 5] = 0x00; // PES packet length lo + pkt[p + 6] = 0x80; // flags1 ('10' marker) + pkt[p + 7] = 0x80; // flags2 → PTS present + pkt[p + 8] = 0x05; // PES_header_data_length = 5 (one PTS) + // 5-byte PTS with the mandatory marker bits (bytes 0,2,4 low bit = 1). + pkt[p + 9] = 0x21; + pkt[p + 10] = 0x00; + pkt[p + 11] = 0x01; + pkt[p + 12] = 0x00; + pkt[p + 13] = 0x01; + let es_off = p + 14; // ES data follows the 14-byte PES header + let n = es.len().min(192 - es_off); + pkt[es_off..es_off + n].copy_from_slice(&es[..n]); + pkt + } + + /// Two BD-TS PES on `pid`: the FIRST carries `es` (the observed display set); + /// the second (a fresh PUSI) exists only to flush the first PES out of the + /// demuxer — the probe never calls `TsDemuxer::flush`, so an open PES stays + /// buffered until the next PES start arrives. + fn ts_stream(pid: u16, es: &[u8]) -> Vec { + let mut s = bd_pes_packet(pid, 0, es); + s.extend_from_slice(&bd_pes_packet(pid, 1, &pcs_display(false))); + s + } + + #[test] + fn forced_display_sets_apply_forced_verdict() { + // Feed REAL synthetic PGS bytes through the full demux→parse→observe→apply + // path: a forced display set must flip a vendor-not-forced PGS track to + // forced. Mutation guard: inverting ForcedTracker::is_forced flips this. + let pid = 0x1200u16; + let mut reader = TsReader { + data: ts_stream(pid, &pcs_display(true)), + pos: 0, + }; + let mut title = pgs_title(pid, false); // vendor label says NOT forced + probe_and_set_forced(&mut reader, &mut title); + let Stream::Subtitle(s) = &title.streams[0] else { + panic!() + }; + assert!( + s.forced, + "an all-forced PGS track → forced verdict applied onto the stream" + ); + } + + #[test] + fn nonforced_display_sets_clear_forced_verdict() { + // A non-forced display set observed on the wire overrides a vendor-forced + // label → the track settles as not-forced. + let pid = 0x1200u16; + let mut reader = TsReader { + data: ts_stream(pid, &pcs_display(false)), + pos: 0, + }; + let mut title = pgs_title(pid, true); // vendor label says forced + probe_and_set_forced(&mut reader, &mut title); + let Stream::Subtitle(s) = &title.streams[0] else { + panic!() + }; + assert!( + !s.forced, + "a non-forced display set observed → forced verdict cleared" + ); + } + #[test] fn no_pgs_streams_is_noop() { // A title with no PGS subtitle streams is a no-op (the reader is never diff --git a/src/keysource.rs b/src/keysource.rs index 9acc909..a7c3c16 100644 --- a/src/keysource.rs +++ b/src/keysource.rs @@ -375,14 +375,42 @@ pub fn resolve_and_apply_traced( /// [`resolve_and_apply`] this does not validate/commit to a disc — the read's /// decorator re-decrypts with the returned keys, which is the validation. pub fn fetch_unit_keys(sources: &[Box], ctx: &dyn ResolveCtx) -> Vec { + drive_unit_keys(sources, ctx).keys +} + +/// Whether a driver run resolved keys, and — when it did NOT — whether the miss +/// was a genuine "no source holds this key" (`errored == false`) or at least one +/// source FAILED (`errored == true`, e.g. a network source was unreachable). The +/// distinction gates negative-result memoization: an empty-because-absent result +/// is safe to cache, an empty-because-a-source-was-down result is transient and +/// must NOT be cached (the key may resolve once the source recovers). +struct FetchOutcome { + keys: Vec, + errored: bool, +} + +/// [`fetch_unit_keys`] plus the error signal: drive `sources` in order, return the +/// first source's non-empty Unit Keys, and flag whether any source that failed to +/// answer did so with an `Err` (a source failure) rather than an empty `Ok` +/// (genuine absence — see [`KeySource::get_unit_keys`]). +fn drive_unit_keys(sources: &[Box], ctx: &dyn ResolveCtx) -> FetchOutcome { + let mut errored = false; for source in sources { - if let Ok(uks) = source.get_unit_keys(ctx) { - if !uks.is_empty() { - return uks; + match source.get_unit_keys(ctx) { + Ok(uks) if !uks.is_empty() => { + return FetchOutcome { + keys: uks, + errored: false, + }; } + Ok(_) => {} + Err(_) => errored = true, } } - Vec::new() + FetchOutcome { + keys: Vec::new(), + errored, + } } /// The forensic counterpart to [`fetch_unit_keys`]: drive `sources` in order and @@ -392,14 +420,29 @@ pub fn fetch_unit_keys(sources: &[Box], ctx: &dyn ResolveCtx) -> /// keying on `disc_hash`) ignores them. Whatever the winning source returns — /// ≥ 1 key — is trusted as the COMPLETE ordered set; no fixed count is assumed. pub fn fetch_fmts_indexes(sources: &[Box], ctx: &dyn ResolveCtx) -> Vec { + drive_fmts_indexes(sources, ctx).keys +} + +/// [`fetch_fmts_indexes`] plus the error signal (see [`drive_unit_keys`]): the +/// forensic counterpart that flags whether any source `Err`ed during the miss. +fn drive_fmts_indexes(sources: &[Box], ctx: &dyn ResolveCtx) -> FetchOutcome { + let mut errored = false; for source in sources { - if let Ok(uks) = source.get_fmts_indexes(ctx) { - if !uks.is_empty() { - return uks; + match source.get_fmts_indexes(ctx) { + Ok(uks) if !uks.is_empty() => { + return FetchOutcome { + keys: uks, + errored: false, + }; } + Ok(_) => {} + Err(_) => errored = true, } } - Vec::new() + FetchOutcome { + keys: Vec::new(), + errored, + } } /// Build the read-time [`crate::sector::KeyFetch`] from the disc's public AACS @@ -426,12 +469,17 @@ pub fn key_fetch( // batch: the resolved keys are disc-level (a clip's index / CPS keys are // identical for every title that references it), so the first batch resolves // over the network and every repeat is answered from the cache with no - // request. Empty replies are cached too — a key the service lacks for a batch - // won't appear on a re-ask, so re-hitting the network buys nothing. Each - // operation gets its OWN cache: a base batch and a forensic anchor never - // collide, and the same bytes could legitimately resolve differently per op. - // The per-kind driver: `fetch_unit_keys` or `fetch_fmts_indexes`. - type FetchDriver = fn(&[Box], &dyn ResolveCtx) -> Vec; + // request. A GENUINELY-empty reply (every source ran and none held the key) + // is cached too — the key the service lacks for a batch won't appear on a + // re-ask, so re-hitting the network buys nothing. But an empty reply caused + // by a source FAILURE (network down, source unreachable) is NOT cached: that + // is a transient miss, and caching it would permanently drop a unit that + // could be recovered once the source recovers — the `errored` flag on + // `FetchOutcome` draws exactly that line. Each operation gets its OWN cache: + // a base batch and a forensic anchor never collide, and the same bytes could + // legitimately resolve differently per op. + // The per-kind driver: `drive_unit_keys` or `drive_fmts_indexes`. + type FetchDriver = fn(&[Box], &dyn ResolveCtx) -> FetchOutcome; fn make_op( inputs: DiscInputs, make_sources: std::sync::Arc Vec> + Send + Sync>, @@ -460,16 +508,22 @@ pub fn key_fetch( // derives unit keys from `enc_title_keys`, which a V10 disc parses at // the 48-byte stride — hardcoding the V20 stride here corrupted them. let ctx = DiscInputsCtx::new(&di); - let keys: Vec<[u8; 16]> = drive(&sources, &ctx).into_iter().map(|u| u.key).collect(); - cache - .lock() - .unwrap_or_else(|e| e.into_inner()) - .insert(fp, keys.clone()); + let outcome = drive(&sources, &ctx); + let keys: Vec<[u8; 16]> = outcome.keys.into_iter().map(|u| u.key).collect(); + // Memoize a positive result always; memoize a NEGATIVE (empty) result + // only when it is a genuine absence, never when a source errored — a + // transient outage must not permanently poison this fingerprint. + if !keys.is_empty() || !outcome.errored { + cache + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(fp, keys.clone()); + } keys }) } - let unit = make_op(inputs.clone(), make_sources.clone(), fetch_unit_keys); - let fmts = make_op(inputs, make_sources, fetch_fmts_indexes); + let unit = make_op(inputs.clone(), make_sources.clone(), drive_unit_keys); + let fmts = make_op(inputs, make_sources, drive_fmts_indexes); crate::sector::KeyFetch::new(unit, fmts) } @@ -867,6 +921,97 @@ mod tests { ); } + /// A transient source outage must NOT be memoized as a permanent "no key": + /// a fingerprint whose first fetch failed because the source errored must be + /// re-asked, and once the source recovers the key resolves. Regression guard + /// for the negative-result memoization fix — caching the errored empty would + /// permanently drop a recoverable unit for the rest of the op. + #[test] + fn errored_empty_is_not_cached_and_retries_when_source_recovers() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let key = [0x77u8; 16]; + // Shared across every `make_sources()` rebuild: call 0 errors (source + // down), every later call succeeds (source recovered). + let calls = Arc::new(AtomicUsize::new(0)); + + struct Flaky { + calls: Arc, + key: [u8; 16], + } + impl KeySource for Flaky { + fn get_unit_keys(&self, _ctx: &dyn ResolveCtx) -> Result, Error> { + if self.calls.fetch_add(1, Ordering::SeqCst) == 0 { + Err(Error::AacsNoKeys) // first attempt: source unreachable + } else { + Ok(vec![UnitKey::new(0, self.key)]) + } + } + } + + let calls_c = Arc::clone(&calls); + let make: Arc Vec> + Send + Sync> = Arc::new(move || { + vec![Box::new(Flaky { + calls: Arc::clone(&calls_c), + key, + }) as Box] + }); + + let cb = key_fetch(empty_inputs(), make); + let samples = vec![vec![0xCDu8; 8]]; + + // First fetch: the source errors → empty, but the miss must NOT be cached. + assert!( + cb.unit_keys(&samples).is_empty(), + "source down → empty this time" + ); + // Second fetch, SAME samples: not blocked by a cached empty → the now- + // recovered source resolves the key. + assert_eq!( + cb.unit_keys(&samples), + vec![key], + "recovered source resolves — errored empty was not memoized" + ); + } + + /// A GENUINE absence (a source that runs and returns an empty `Ok`) is still + /// memoized — the benefit the fix preserves. A source counting its calls must + /// be asked exactly once for a fingerprint whose first (clean) reply was empty. + #[test] + fn genuine_empty_is_still_memoized() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let calls = Arc::new(AtomicUsize::new(0)); + + struct AlwaysEmpty { + calls: Arc, + } + impl KeySource for AlwaysEmpty { + fn get_unit_keys(&self, _ctx: &dyn ResolveCtx) -> Result, Error> { + self.calls.fetch_add(1, Ordering::SeqCst); + Ok(Vec::new()) // ran fine, genuinely holds no key + } + } + + let calls_c = Arc::clone(&calls); + let make: Arc Vec> + Send + Sync> = Arc::new(move || { + vec![Box::new(AlwaysEmpty { + calls: Arc::clone(&calls_c), + }) as Box] + }); + + let cb = key_fetch(empty_inputs(), make); + let samples = vec![vec![0xEFu8; 8]]; + + assert!(cb.unit_keys(&samples).is_empty()); + assert!(cb.unit_keys(&samples).is_empty()); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "a clean empty reply is cached — the source is asked only once" + ); + } + /// The two `KeyFetch` operations route to the two DISTINCT trait methods: /// `unit_keys` drives `get_unit_keys`, `fmts_indexes` drives /// `get_fmts_indexes`. A source that returns different keys per method proves diff --git a/src/mux/codec/dts.rs b/src/mux/codec/dts.rs index e0570a3..938e3df 100644 --- a/src/mux/codec/dts.rs +++ b/src/mux/codec/dts.rs @@ -688,7 +688,14 @@ fn dts_core_duration_ns(data: &[u8]) -> u64 { /// `DTS_AMODE_COUNT`; `lfe_present == DTS_LFE_FLAG_INVALID` is rejected. const DTS_PCMBLOCK_SAMPLES: u32 = 32; const DTS_SUBBAND_SAMPLES: u32 = 8; -const DTS_AMODE_COUNT: u32 = 10; +/// Number of LEGAL `AMODE` (channel-arrangement) codes. The 6-bit AMODE field +/// (ETSI TS 102 114 §5.3.1) has 16 defined channel arrangements, codes 0-15; +/// only 16-63 are reserved/user-defined and undecodable. ffmpeg's +/// `ff_dca_channels[16] = {1,2,2,2,2,3,3,4,4,5,6,6,6,7,8,8}` confirms all 16 are +/// decodable — codes 10-15 are the 6/7/8-channel layouts. A frame is dropped +/// only when `audio_mode >= DTS_AMODE_COUNT` (i.e. a truly reserved 16-63 code); +/// dropping a legal 10-15 multichannel core would silence recoverable audio. +const DTS_AMODE_COUNT: u32 = 16; const DTS_LFE_FLAG_INVALID: u32 = 3; /// Sample rate (Hz) per core `SFREQ` code (ETSI TS 102 114 Table 6-4); a `0` @@ -1946,8 +1953,11 @@ mod tests { d[5] = (d[5] & 0x03) | (14u8 << 2); assert_eq!(core_header_drop_reason(&d), Some(DropReason::PcmBlocks)); - // audio_mode >= 10: AMODE = byte7 bits3-0 (high 4) + byte8 bits7-6. Set - // AMODE high nibble to 0xF → audio_mode >= 60. + // audio_mode reserved (>= 16): AMODE = byte7 bits3-0 (high 4) + byte8 + // bits7-6. Set AMODE high nibble to 0xF → audio_mode = 60, a genuinely + // RESERVED code (16-63) a decoder rejects. (Codes 10-15 are LEGAL + // multichannel layouts and must NOT be dropped — see + // legal_multichannel_amode_is_not_dropped.) let mut d = good.clone(); d[7] |= 0x0F; assert_eq!(core_header_drop_reason(&d), Some(DropReason::Amode)); @@ -1975,6 +1985,43 @@ mod tests { assert_eq!(core_header_drop_reason(&d), Some(DropReason::PcmRes)); } + #[test] + fn legal_multichannel_amode_is_not_dropped() { + // ETSI TS 102 114 §5.3.1: AMODE is a 6-bit field with 16 LEGAL + // channel-arrangement codes (0-15); only 16-63 are reserved. ffmpeg's + // ff_dca_channels[16] = {1,2,2,2,2,3,3,4,4,5,6,6,6,7,8,8} confirms codes + // 10-15 are decodable 6/7/8-channel layouts. The decodability gate must + // KEEP them — dropping a spec-legal multichannel core silences audio the + // recover-100% goal must preserve. + fn set_amode(core: &mut [u8], amode: u32) { + // audio_mode = (byte7 & 0x0F) << 2 | (byte8 >> 6). + core[7] = (core[7] & 0xF0) | ((amode >> 2) & 0x0F) as u8; + core[8] = (core[8] & 0x3F) | (((amode & 0x03) << 6) as u8); + } + + // Every legal code 0-15 is kept — the range is a literal (NOT + // DTS_AMODE_COUNT) so reverting the bound to 10 makes 10-15 fail here. + for amode in 0u32..16 { + let mut core = make_dts_core(512); + set_amode(&mut core, amode); + assert_eq!( + core_header_drop_reason(&core), + None, + "legal AMODE {amode} must not be dropped" + ); + } + // The first reserved code (16) and above are still rejected. + for amode in [16u32, 40, 63] { + let mut core = make_dts_core(512); + set_amode(&mut core, amode); + assert_eq!( + core_header_drop_reason(&core), + Some(DropReason::Amode), + "reserved AMODE {amode} must be dropped" + ); + } + } + /// Real-data fixture (ignored). Re-parses a raw `.dts` elementary stream /// through `DtsParser` and writes the emitted access units back out, so the /// garbage-extension → core-only drop can be validated against an actual diff --git a/src/mux/mp4/mod.rs b/src/mux/mp4/mod.rs index ecb8a5a..1d7c52f 100644 --- a/src/mux/mp4/mod.rs +++ b/src/mux/mp4/mod.rs @@ -158,8 +158,9 @@ pub struct Mp4FitReport { } /// Compute the fit plan without opening a file. Video: the first primary -/// HEVC/H.264 track. Audio: every AC-3 / E-AC-3 track. Everything else is -/// skipped with a reason. +/// HEVC/H.264 track. Audio: every track `audio::audio_fits` carries — the Dolby +/// family (AC-3 / E-AC-3) and DTS (core / DTS-HD HRA / DTS-HD MA). Everything +/// else is skipped with a reason. pub fn fit_report(title: &DiscTitle) -> Mp4FitReport { let mut included = Vec::new(); let mut skipped = Vec::new(); diff --git a/tests/scan_iso.rs b/tests/scan_iso.rs index c4a8e98..2ccacff 100644 --- a/tests/scan_iso.rs +++ b/tests/scan_iso.rs @@ -141,10 +141,20 @@ fn scan_iso_matches_manual_scan_image_path() { .expect("manual scan_image succeeds"); assert_eq!(disc.capacity_sectors, manual.capacity_sectors, "capacity"); - assert_eq!(disc.capacity_sectors, expected_capacity, "capacity value"); assert_eq!(disc.titles.len(), manual.titles.len(), "title count"); assert_eq!(disc.encrypted, manual.encrypted, "encrypted flag"); assert_eq!(disc.format, manual.format, "disc format"); + + // Independent expectations (not parity against a re-run of the same + // composition): the scanned Disc must match KNOWN properties of the fixture + // itself — its capacity (max LBA + 1), its PVD volume id ("TEST_DISC"), and + // that a UDF with no /AACS directory is unencrypted. These would fail even if + // scan_iso and the manual path drifted together. + assert_eq!(disc.capacity_sectors, expected_capacity, "capacity value"); + assert_eq!( + disc.volume_id, "TEST_DISC", + "PVD volume id from the fixture" + ); assert!(!disc.encrypted, "minimal UDF (no /AACS) is not encrypted"); // The returned reader is usable: correct capacity and a real read of sector