libfreemkv: fix rc.5.2 audit code findings
1. HEVC CRA->BLA false-trigger on 33-bit PTS wraparound (src/mux/codec/hevc.rs): the clip-boundary auto-detect compared the RAW 33-bit PES PTS against the high-water mark, so a single-clip title crossing 2^33->0 (~26.5h) false-armed pending_clip_boundary and rewrote a legitimate in-clip CRA(21)->BLA_W_LP(16), dropping valid RASL pictures (visible corruption) and breaking the single-clip byte-identical guarantee. Now unwrap the PTS onto a monotonic 64-bit timeline first (a near-full-period backstep is a wrap: add 2^33, update the watermark, do not arm). Regression test cra_after_33bit_pts_wrap_not_rewritten; the genuine-clip-join test still passes. 2. Single-pass recovery read bypassed the transport-failure abort (src/mux/disc.rs): the line-442 short-circuit only inspected the 10s read res. A transport failure (status 0xFF, wedged USB bridge) on the 60s recovery read fell into the skip_errors branch and zero-filled/ advanced, marching the disc at one bridge-recovery per probe (run-forever, hard rule #2). Re-check the recovery error for is_scsi_transport_failure() before the skip block and abort with Error::DiscRead. Test transport_failure_on_recovery_read_aborts_even_with_skip_errors. 3. Recovery-read SUCCESS branch had no coverage (src/mux/disc.rs tests): added RecoverableReader (errors when recovery=false, succeeds when recovery=true) and test recovery_read_success_muxes_recovered_data_no_skip driving fill_extents to the size-1 bottom-out and asserting the recovered data is muxed (counters advance, no skip). 4. TrueHD channel-correction probe omitted set_unit_base (src/disc/mod.rs correct_truehd_channels): the probe read via a DecryptingSectorSource without anchoring the AACS unit-alignment gate, so it degraded to absolute start_lba % 3 and returned DecryptFailed on a non-3-aligned extent, silently understating Atmos/7.1 as 5.1. Now call set_unit_base(ext.start_lba) before the probe read (no-op for CSS/None). 5. is_unit_aligned lba<unit_base latent trap (src/aacs/decrypt.rs): wrapping_sub mis-gated when lba < unit_base (2^32 == 1 mod 3). Switched to saturating_sub (clamps offset to 0, a unit boundary) and pinned the contract with is_unit_aligned_lba_below_base_is_well_defined plus is_unit_aligned_relative_to_base. cargo +1.86 fmt --check / clippy -D warnings / test --tests all green.
This commit is contained in:
Matthew Jackson
+82
-13
@@ -78,11 +78,19 @@ pub struct HevcParser {
|
||||
// so the rewrite branch is never reached and output is byte-identical to a
|
||||
// parser without this field.
|
||||
pending_clip_boundary: bool,
|
||||
// Highest PES PTS (90 kHz ticks) seen on this video stream so far. Used to
|
||||
// AUTO-DETECT a non-seamless clip boundary from the bitstream when the
|
||||
// caller never plumbs one in (the common case — see `BACKSTEP_TICKS`).
|
||||
// `None` until the first AU with a PTS.
|
||||
// Highest PES PTS seen on this video stream so far, on a MONOTONIC 64-bit
|
||||
// timeline (raw 33-bit PTS unwrapped across 2^33 wraparounds — see
|
||||
// `pts_wrap_offset`). Used to AUTO-DETECT a non-seamless clip boundary from
|
||||
// the bitstream when the caller never plumbs one in (the common case — see
|
||||
// `BACKSTEP_TICKS`). `None` until the first AU with a PTS.
|
||||
high_pts: Option<i64>,
|
||||
// Accumulated 2^33-tick offset applied to raw PES PTS values to unwrap them
|
||||
// onto the monotonic timeline `high_pts` lives on. The 33-bit 90 kHz PTS
|
||||
// wraps every ~26.5 h; a BD clip can start at a high base and cross the wrap
|
||||
// mid-title. Without unwrapping, the 2^33→0 step looks like a backward clip
|
||||
// reset and false-arms the CRA→BLA rewrite (corrupting a legitimate in-clip
|
||||
// CRA and dropping valid RASL pictures). Each detected wrap adds 2^33 here.
|
||||
pts_wrap_offset: i64,
|
||||
}
|
||||
|
||||
// A backward PES-PTS step larger than this (90 kHz ticks) marks a non-seamless
|
||||
@@ -97,6 +105,18 @@ pub struct HevcParser {
|
||||
// concatenated multi-clip title otherwise produces.
|
||||
const BACKSTEP_TICKS: i64 = 270_000;
|
||||
|
||||
// The 33-bit 90 kHz PES PTS counter wraps at 2^33 ticks (~26.5 h). When the raw
|
||||
// PTS steps backward by approximately a full period — i.e. it landed just past
|
||||
// the wrap — it is a counter wraparound, NOT a clip reset: unwrap it (add 2^33)
|
||||
// instead of arming the CRA→BLA rewrite. A genuine non-seamless clip join resets
|
||||
// the PTS to a fresh small base, a backward step of arbitrary (sub-2^33) size; a
|
||||
// wrap is specifically a step of ~2^33. We accept any backward step within one
|
||||
// `PTS_WRAP_PERIOD`/2 of a full period as a wrap (the new value is below the old
|
||||
// high-water but within a reorder window of the wrap point), which cleanly
|
||||
// separates the two cases since a clip reset to a small base is nowhere near 2^33
|
||||
// below the high-water unless the title is itself ~26 h long (impossible on BD).
|
||||
const PTS_WRAP_PERIOD: i64 = 1 << 33;
|
||||
|
||||
impl Default for HevcParser {
|
||||
fn default() -> Self {
|
||||
Self::new()
|
||||
@@ -115,6 +135,7 @@ impl HevcParser {
|
||||
cur_pps: None,
|
||||
pending_clip_boundary: false,
|
||||
high_pts: None,
|
||||
pts_wrap_offset: 0,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -263,17 +284,31 @@ impl CodecParser for HevcParser {
|
||||
// clip then consumes. Without this, the splice CRA's RASL leading
|
||||
// pictures reference pre-join frames gone after concatenation and a
|
||||
// linear decoder floods "Could not find ref with POC N" (the Top Gun
|
||||
// UHD defect). Uses the RAW 90 kHz PES PTS (not the rebased mux
|
||||
// timeline) and tracks the high-water mark so a single in-clip B-frame
|
||||
// dip never arms it. DTS-only AUs (no PTS) leave the watermark untouched.
|
||||
// UHD defect). Uses the 90 kHz PES PTS (not the rebased mux timeline)
|
||||
// UNWRAPPED onto a monotonic 64-bit timeline first — the raw 33-bit PTS
|
||||
// wraps every ~26.5 h, and a single-clip title that crosses 2^33→0 would
|
||||
// otherwise false-arm the rewrite (corrupting a legitimate in-clip CRA).
|
||||
// Tracks the high-water mark so a single in-clip B-frame dip never arms
|
||||
// it. DTS-only AUs (no PTS) leave the watermark untouched.
|
||||
if let Some(raw_pts) = pes.pts {
|
||||
match self.high_pts {
|
||||
Some(high) if raw_pts < high - BACKSTEP_TICKS => {
|
||||
self.pending_clip_boundary = true;
|
||||
self.high_pts = Some(raw_pts);
|
||||
// Unwrap onto the monotonic timeline. If the offset-adjusted value
|
||||
// dropped to roughly a full period (2^33) below the high-water, the
|
||||
// 33-bit counter wrapped: add another period and re-check, rather
|
||||
// than treat the wrap as a backward clip reset.
|
||||
let mut unwrapped = raw_pts + self.pts_wrap_offset;
|
||||
if let Some(high) = self.high_pts {
|
||||
if high - unwrapped > PTS_WRAP_PERIOD / 2 {
|
||||
self.pts_wrap_offset += PTS_WRAP_PERIOD;
|
||||
unwrapped += PTS_WRAP_PERIOD;
|
||||
}
|
||||
Some(high) => self.high_pts = Some(high.max(raw_pts)),
|
||||
None => self.high_pts = Some(raw_pts),
|
||||
}
|
||||
match self.high_pts {
|
||||
Some(high) if unwrapped < high - BACKSTEP_TICKS => {
|
||||
self.pending_clip_boundary = true;
|
||||
self.high_pts = Some(unwrapped);
|
||||
}
|
||||
Some(high) => self.high_pts = Some(high.max(unwrapped)),
|
||||
None => self.high_pts = Some(unwrapped),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1261,6 +1296,40 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression for the 33-bit PTS wraparound false-trigger (rc.5.2 audit #1):
|
||||
/// a SINGLE clip whose raw 90 kHz PES PTS crosses the 2^33 counter wrap
|
||||
/// (~26.5 h) must NOT be mistaken for a non-seamless clip join. Before the
|
||||
/// fix the raw 2^33→0 backward step armed `pending_clip_boundary` and the
|
||||
/// next in-clip CRA was wrongly rewritten CRA→BLA_W_LP (dropping valid RASL
|
||||
/// pictures — visible corruption). After unwrapping onto a monotonic
|
||||
/// timeline the wrap is absorbed and the CRA stays CRA.
|
||||
#[test]
|
||||
fn cra_after_33bit_pts_wrap_not_rewritten() {
|
||||
let mut parser = HevcParser::new();
|
||||
let period = 1i64 << 33;
|
||||
// Single clip, PTS climbing toward the 33-bit wrap. Start just below 2^33.
|
||||
let near_wrap = period - 90_000; // ~1 s before the wrap point
|
||||
parser.parse(&make_pes(cra_au(&[0x01]), Some(near_wrap)));
|
||||
parser.parse(&make_pes(cra_au(&[0x02]), Some(near_wrap + 3750)));
|
||||
// The counter wraps: raw PTS resets to a small value, but this is the
|
||||
// SAME continuous clip, one frame later. A naive raw comparison sees a
|
||||
// ~2^33 backward step and false-arms the boundary.
|
||||
let wrapped = parser.parse(&make_pes(cra_au(&[0x03]), Some(7500)));
|
||||
assert_eq!(
|
||||
nal_type_of(&nals_of(&wrapped[0].data)[0]),
|
||||
NAL_CRA_NUT,
|
||||
"a CRA whose PTS merely wrapped 2^33->0 must stay CRA, not become BLA"
|
||||
);
|
||||
// Continue past the wrap: PTS keeps climbing from the new low base; still
|
||||
// one continuous clip, the CRA after must remain CRA.
|
||||
let after = parser.parse(&make_pes(cra_au(&[0x04]), Some(11250)));
|
||||
assert_eq!(
|
||||
nal_type_of(&nals_of(&after[0].data)[0]),
|
||||
NAL_CRA_NUT,
|
||||
"post-wrap in-clip CRA must stay CRA"
|
||||
);
|
||||
}
|
||||
|
||||
/// Test 3: non-CRA NALs are never rewritten even when a boundary IS marked.
|
||||
/// IDR (19), RASL (8/9), VPS/SPS/PPS, and a trailing slice all pass through
|
||||
/// unmodified; the IDR clears the pending boundary so no later CRA is wrongly
|
||||
|
||||
+237
@@ -501,6 +501,27 @@ impl DiscStream {
|
||||
break;
|
||||
}
|
||||
|
||||
// Recovery read also failed. A transport failure here (status
|
||||
// 0xFF: USB-bridge crash / disconnect) is NOT a skippable bad
|
||||
// unit — same as the original 10s read above. The line-442
|
||||
// short-circuit only inspected `res`; the 60s recovery read
|
||||
// (`rec`) can wedge the bridge on its own, and falling into the
|
||||
// `skip_errors` branch below would zero-fill + advance, treating
|
||||
// a dead bridge as a skippable unit and marching the whole disc
|
||||
// at one bridge-recovery per probe (hard rule #2, "runs forever,
|
||||
// no MKV"). Re-check `rec` and abort, mirroring line 442.
|
||||
if let Some(e) = rec.as_ref().err() {
|
||||
if e.is_scsi_transport_failure() {
|
||||
let (status, sense) = extract_scsi_context(e);
|
||||
return Err(crate::error::Error::DiscRead {
|
||||
sector: lba as u64,
|
||||
status: Some(status),
|
||||
sense,
|
||||
}
|
||||
.into());
|
||||
}
|
||||
}
|
||||
|
||||
// Recovery read also failed. Skip the WHOLE failed unit or bail.
|
||||
// Zero-filling and advancing by the full unit keeps
|
||||
// current_offset unit-aligned, so the next read still begins on a
|
||||
@@ -1084,6 +1105,222 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// `SectorSource` that mirrors a marginal sector recoverable only with the
|
||||
/// drive's full ECC budget: every read covering `bad_sector` FAILS while
|
||||
/// `recovery=false` (the fast 10s pass) and SUCCEEDS (zeroed bytes) once
|
||||
/// `recovery=true` (the 60s ECC pass). Drives the single-pass bottom-out
|
||||
/// "last-chance recovery read" success branch in `fill_extents`, which the
|
||||
/// other test sources (ignoring the flag) never exercise.
|
||||
struct RecoverableReader {
|
||||
capacity: u32,
|
||||
bad_sector: u32,
|
||||
/// `(lba, count, recovery)` for every issued read.
|
||||
log: std::sync::Arc<std::sync::Mutex<Vec<(u32, u16, bool)>>>,
|
||||
}
|
||||
|
||||
impl crate::sector::SectorSource for RecoverableReader {
|
||||
fn read_sectors(
|
||||
&mut self,
|
||||
lba: u32,
|
||||
count: u16,
|
||||
buf: &mut [u8],
|
||||
recovery: bool,
|
||||
) -> crate::error::Result<usize> {
|
||||
self.log.lock().unwrap().push((lba, count, recovery));
|
||||
let end = lba + count as u32;
|
||||
let covers_bad = self.bad_sector >= lba && self.bad_sector < end;
|
||||
// Fail on the fast (non-recovery) pass; the 60s ECC recovery read
|
||||
// succeeds. Distinct non-0x02 sense byte so a transport-failure
|
||||
// re-check (status 0xFF) is provably NOT triggered here.
|
||||
if covers_bad && !recovery {
|
||||
return Err(crate::error::Error::DiscRead {
|
||||
sector: self.bad_sector as u64,
|
||||
status: Some(0x02),
|
||||
sense: None,
|
||||
});
|
||||
}
|
||||
let bytes = count as usize * 2048;
|
||||
buf[..bytes].fill(0);
|
||||
Ok(bytes)
|
||||
}
|
||||
|
||||
fn capacity_sectors(&self) -> u32 {
|
||||
self.capacity
|
||||
}
|
||||
}
|
||||
|
||||
/// Coverage for the single-pass bottom-out RECOVERY-READ SUCCESS branch
|
||||
/// (rc.5.2 audit #3): a sector that fails the fast 10s read but reads clean
|
||||
/// on the 60s ECC recovery read must have its RECOVERED data muxed — the
|
||||
/// cursor advances over the whole unit, byte counters move, and NO skip is
|
||||
/// counted. Pre-fix the test sources ignored `recovery`, so this branch was
|
||||
/// untested. Uses `unit_align=1` (None) so the bottom-out unit is a single
|
||||
/// sector, exercising the `(sectors as u32) <= align` path precisely.
|
||||
#[test]
|
||||
fn recovery_read_success_muxes_recovered_data_no_skip() {
|
||||
const COUNT: u32 = 10;
|
||||
let bad = 4u32;
|
||||
let log = std::sync::Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let reader = RecoverableReader {
|
||||
capacity: COUNT,
|
||||
bad_sector: bad,
|
||||
log: log.clone(),
|
||||
};
|
||||
let mut stream = DiscStream::new(
|
||||
Box::new(reader),
|
||||
synthetic_title(COUNT),
|
||||
crate::decrypt::DecryptKeys::None,
|
||||
8,
|
||||
ContentFormat::BdTs,
|
||||
);
|
||||
// skip_errors=false: if the recovery read did NOT succeed, fill_extents
|
||||
// would return Err — so reaching EOF cleanly proves recovery worked.
|
||||
stream.skip_errors = false;
|
||||
|
||||
let mut guard = 0;
|
||||
loop {
|
||||
match stream.fill_extents() {
|
||||
Ok(true) => {}
|
||||
Ok(false) => break,
|
||||
Err(e) => panic!("recovery read should have succeeded, got: {e}"),
|
||||
}
|
||||
guard += 1;
|
||||
assert!(guard < 1000, "fill_extents did not reach EOF");
|
||||
}
|
||||
|
||||
// No skip counted: the recovered unit was muxed, not zero-filled.
|
||||
assert_eq!(
|
||||
stream.errors, 0,
|
||||
"a successful recovery read must not count as a skipped sector"
|
||||
);
|
||||
assert_eq!(
|
||||
stream.lost_bytes, 0,
|
||||
"a successful recovery read loses no bytes"
|
||||
);
|
||||
// All COUNT sectors' worth of bytes were read through to the cursor end.
|
||||
assert_eq!(
|
||||
stream.bytes_read_total,
|
||||
COUNT as u64 * 2048,
|
||||
"every sector (including the recovered one) must be counted as read"
|
||||
);
|
||||
|
||||
// The bad sector was retried with recovery=true and that read SUCCEEDED.
|
||||
let reads = log.lock().unwrap();
|
||||
assert!(
|
||||
reads
|
||||
.iter()
|
||||
.any(|&(lba, count, rec)| rec && lba == bad && count == 1),
|
||||
"expected a recovery=true single-sector read at the bad sector; got {reads:?}"
|
||||
);
|
||||
// And the fast pass at the bad sector did happen with recovery=false.
|
||||
assert!(
|
||||
reads.iter().any(|&(lba, _c, rec)| !rec && lba == bad),
|
||||
"expected a non-recovery read to have first failed at the bad sector"
|
||||
);
|
||||
}
|
||||
|
||||
/// `SectorSource` that fails the fast (non-recovery) read covering
|
||||
/// `bad_sector` with an ordinary bad-sector error (status 0x02), then fails
|
||||
/// the 60s ECC recovery read with a TRANSPORT failure (status 0xFF). Models
|
||||
/// a bridge that wedges precisely during the last-chance recovery read.
|
||||
struct RecoveryTransportFailReader {
|
||||
capacity: u32,
|
||||
bad_sector: u32,
|
||||
log: std::sync::Arc<std::sync::Mutex<Vec<(u32, u16, bool)>>>,
|
||||
}
|
||||
|
||||
impl crate::sector::SectorSource for RecoveryTransportFailReader {
|
||||
fn read_sectors(
|
||||
&mut self,
|
||||
lba: u32,
|
||||
count: u16,
|
||||
buf: &mut [u8],
|
||||
recovery: bool,
|
||||
) -> crate::error::Result<usize> {
|
||||
self.log.lock().unwrap().push((lba, count, recovery));
|
||||
let end = lba + count as u32;
|
||||
if self.bad_sector >= lba && self.bad_sector < end {
|
||||
let status = if recovery {
|
||||
crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE
|
||||
} else {
|
||||
0x02
|
||||
};
|
||||
return Err(crate::error::Error::DiscRead {
|
||||
sector: self.bad_sector as u64,
|
||||
status: Some(status),
|
||||
sense: None,
|
||||
});
|
||||
}
|
||||
let bytes = count as usize * 2048;
|
||||
buf[..bytes].fill(0);
|
||||
Ok(bytes)
|
||||
}
|
||||
|
||||
fn capacity_sectors(&self) -> u32 {
|
||||
self.capacity
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression (rc.5.2 audit #2): a transport failure on the 60s ECC
|
||||
/// RECOVERY read (not just the initial 10s read) must ABORT, even under
|
||||
/// `skip_errors=true`. The line-442 short-circuit only inspected the
|
||||
/// original `res`; without a re-check the wedged-bridge recovery failure
|
||||
/// fell into the skip branch — zero-fill + advance — marching the disc at
|
||||
/// one bridge-recovery per unit ("runs forever, no MKV", hard rule #2). The
|
||||
/// fix re-checks the recovery error for `is_scsi_transport_failure()` before
|
||||
/// the skip block and returns `Error::DiscRead`.
|
||||
#[test]
|
||||
fn transport_failure_on_recovery_read_aborts_even_with_skip_errors() {
|
||||
const COUNT: u32 = 10;
|
||||
let bad = 4u32;
|
||||
let log = std::sync::Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let reader = RecoveryTransportFailReader {
|
||||
capacity: COUNT,
|
||||
bad_sector: bad,
|
||||
log: log.clone(),
|
||||
};
|
||||
let mut stream = DiscStream::new(
|
||||
Box::new(reader),
|
||||
synthetic_title(COUNT),
|
||||
crate::decrypt::DecryptKeys::None,
|
||||
8,
|
||||
ContentFormat::BdTs,
|
||||
);
|
||||
stream.skip_errors = true;
|
||||
|
||||
// Drive fill_extents across batches: the good leading sectors mux fine,
|
||||
// and the batch covering the bad sector shrinks to size 1, fails the
|
||||
// fast read (0x02), then the bottom-out recovery read returns the
|
||||
// transport failure (0xFF) — which must abort.
|
||||
let mut res = Ok(true);
|
||||
for _ in 0..1000 {
|
||||
res = stream.fill_extents();
|
||||
if !matches!(res, Ok(true)) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
res.is_err(),
|
||||
"a transport failure on the recovery read must abort fill_extents, got {res:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
stream.errors, 0,
|
||||
"a recovery-read transport-failure abort must NOT count as a skip"
|
||||
);
|
||||
assert_eq!(
|
||||
stream.lost_bytes, 0,
|
||||
"a transport-failure abort zero-fills nothing"
|
||||
);
|
||||
// Prove the bottom-out recovery read was actually reached and aborted on.
|
||||
let reads = log.lock().unwrap();
|
||||
assert!(
|
||||
reads
|
||||
.iter()
|
||||
.any(|&(lba, count, rec)| rec && lba == bad && count == 1),
|
||||
"expected a recovery=true read at the bad sector to have been attempted; got {reads:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression: a USB-bridge transport crash (status=0xFF) during a direct
|
||||
/// single-pass `disc://→mkv://` rip must ABORT immediately, even under
|
||||
/// `skip_errors=true`. The pre-fix behavior treated it as a skippable bad
|
||||
|
||||
Reference in New Issue
Block a user