Audit round 1: playback order, silent title drops, image durability
Four fixes from the first audit round. Every finding was verified against a pinned tree and read directly before being accepted. resolve_vts_key sorted a VTS title-VOB extents largest-first. That is the 1.5.1 garbage bug, and it grew back in a new code path: the comment claimed it "matched the scan heuristic", but that heuristic WAS the bug and had already been fixed in decrypt_keys_for_title, which documents the rule (PLAYBACK ORDER, never largest-cell-first) and pins it with a regression test. A CSS DVDs biggest cell opens with a long clear run and crack_key shares one sector budget across the extent list, so starting there can exhaust it without ever MEETING scrambled data — and CSS recovers the key from scrambled data itself. The crack then returns None, the caller falls back to the disc-wide key, and every VOB in that VTS is descrambled wrongly: corrupt PES behind an intact header, written out as a complete extract at exit 0. parse_pgcit dropped titles silently in THREE places — an unparseable PGC, an out-of-range PGC index, and a truncated entry table. The finder caught one; the other two turned up on reading the function. parse_vmg already counts and warns per skipped title SET for exactly this reason, and this was the last place a disc could quietly report fewer titles than it has. write_image called flush() and returned Ok. flush() only pushes bytes into the page cache and promises nothing about durability, so a 6-90 GB image could be reported complete while still unwritten — a crash or an unmounted volume then leaves a truncated file the caller was told was finished. Now into_inner (so a buffered-write error surfaces instead of being dropped by BufWriter::drop) followed by sync_all. timeline used abs() on a saturating_sub result. Every other comparison in that module is saturating because the timestamps come off a disc and are not trusted; abs() panics on i64::MIN, which saturating_sub can produce.
This commit is contained in:
@@ -190,7 +190,13 @@ pub(crate) fn codec_label(codec: Codec) -> &'static str {
|
||||
/// to `.sup`, VobSub records `.idx` entries and emits the sidecar at finish.
|
||||
trait EsWriter: Send {
|
||||
/// Write one frame's payload to `w`. Returns the number of bytes written to
|
||||
/// the main file (used by the VobSub writer for `.idx` filepos tracking).
|
||||
/// the main file.
|
||||
///
|
||||
/// Nothing in production reads this count: the sole caller discards it with
|
||||
/// `?`, and `VobSubWriter` tracks `.idx` fileposes from its own `pos` field.
|
||||
/// This previously claimed the VobSub writer depended on the return value,
|
||||
/// which would have a maintainer believe a wrong count shows up as broken
|
||||
/// `.idx` output. It does not.
|
||||
fn write_frame(&mut self, w: &mut dyn Write, f: &PesFrame, pts_ns: i64) -> io::Result<usize>;
|
||||
|
||||
/// Finalize. Default: no-op. The VobSub writer serializes its `.idx` here.
|
||||
|
||||
+13
-6
@@ -680,8 +680,9 @@ pub struct MkvMuxer<W: Write + Seek> {
|
||||
cues: Vec<CuePoint>,
|
||||
frame_count: u64,
|
||||
/// Frames handed to `write_frame` that were dropped because no cluster was
|
||||
/// open yet (a cluster only opens on a track-0 video keyframe). See
|
||||
/// `write_frame` for the track-0 invariant.
|
||||
/// open yet (a cluster only opens on a keyframe from the primary video
|
||||
/// track, whatever index that is — not necessarily track 0). See
|
||||
/// `write_frame` for the cluster-driver invariant.
|
||||
///
|
||||
/// The ALL-dropped case is surfaced as an error by `finish()`, but via
|
||||
/// `frame_count == 0`, not via this counter. A PARTIAL drop — leading audio /
|
||||
@@ -1683,11 +1684,17 @@ impl<W: Write + Seek> MkvMuxer<W> {
|
||||
|
||||
/// Finish the MKV file: write Cues element.
|
||||
///
|
||||
/// # Track-0 invariant
|
||||
/// # Cluster-driver invariant
|
||||
///
|
||||
/// A cluster only opens on a track-0 video keyframe, so the caller must
|
||||
/// supply track 0 as the video track and deliver a keyframe on it before
|
||||
/// (or alongside) other-track data. If no track-0 keyframe ever arrives,
|
||||
/// A cluster only opens on a keyframe from the PRIMARY VIDEO TRACK — the
|
||||
/// first track whose type is video, at whatever index it occupies (see
|
||||
/// `cluster_driver`, which is `primary_video_track.unwrap_or(0)`; index 0
|
||||
/// is only the fallback for a file with no video track at all). The caller
|
||||
/// must deliver a keyframe on that track before (or alongside) other-track
|
||||
/// data. This said "track-0" and required the caller to place video at
|
||||
/// index 0, which the code has never actually required — so a reader
|
||||
/// debugging dropped frames would suspect track ordering, which is not it.
|
||||
/// If no such keyframe ever arrives,
|
||||
/// every `write_frame` is silently dropped; rather than emit a structurally
|
||||
/// valid but empty MKV (zero clusters, zero frames), `finish` returns
|
||||
/// `Error::MkvInvalid` when frames were submitted but none were written.
|
||||
|
||||
+7
-1
@@ -264,7 +264,13 @@ impl SeamPlan {
|
||||
// the end of the list.
|
||||
let stepped_back = pos.last_raw_ns.is_some_and(|last| raw_ns < last)
|
||||
&& if has_reorder {
|
||||
(raw_ns.saturating_sub(next_in)).abs() <= CLIP_START_TOLERANCE_NS
|
||||
// saturating_abs, not abs: every other comparison in this
|
||||
// module is saturating because these timestamps come off a
|
||||
// disc and are not trusted. `abs()` is the one exception
|
||||
// and it panics on i64::MIN — which saturating_sub can
|
||||
// produce exactly — taking down the mux thread on one bad
|
||||
// frame instead of comparing false like its neighbours.
|
||||
(raw_ns.saturating_sub(next_in)).saturating_abs() <= CLIP_START_TOLERANCE_NS
|
||||
} else {
|
||||
raw_ns >= next_in.saturating_sub(CLIP_START_TOLERANCE_NS)
|
||||
&& raw_ns <= self.clips[clip + 1].out_ns
|
||||
|
||||
Reference in New Issue
Block a user