90a7fe2ff122bb4233c7ba822e503647628cf176
13
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matthew Jackson
|
5559987325 |
test(mux): replace a false-green discontinuity test with the property it named
a_signalled_discontinuity_survives_a_backstop_discard asserted that a SOURCE-signalled discontinuity on discarded bytes still reaches the AU that follows. It did not test that. Deleting the disc_marks push, or the mark-retirement loop inside discard_gap_before, left it passing. The mechanism: the `discontinuity = true` rode the FIRST over-cap push, which still has the next AU's delimiter at buf[0] — so it force-flushes as an over-long AU rather than discarding, and THAT AU consumes the mark. The assertion's `.find(|x| x.data.contains(&0x22))` then filters it out, and the flag it reads comes entirely from `pending_gap`, set by the second push's backstop. Behaviourally identical to the test 40 lines above it, under a name promising something else. I wrote it this morning, in the same commit that fixed a different test for having a fixture that never reached the code it named, while cataloguing that exact shape. Third instance today of writing the bug I was hunting. The two mechanisms cannot be isolated in one fixture — a fragment that trips the backstop sets pending_gap regardless — so they now get one test each. The replacement drives disc_marks end to end with no backstop involved: a flagged fragment that carries a complete AU and is emitted, not discarded. Nothing else pinned that path. Removing the disc_marks push reds it. Found by the round-9 opus escalation over test quality, dispatched because the sonnet pass over the same 17,000 lines of new test code returned zero findings. |
||
|
Matthew Jackson
|
944e6a8b09 |
fix: align the Linux fsync error with macOS, and clear three stale docs
Round 9 findings, triaged and verified against the pinned tree.
writeback_file: a bounded-fsync WorkerLost returned bare ErrorKind::Other
on Linux where macOS returns EIO. Round 8 fixed the Linux arm to return
Err at all — the right fix — but stopped short of matching the value, so
a consumer distinguishing timeout / halt / lost-worker had nothing to
branch on for the third case on one platform. Now EIO on both.
Three doc comments described the pre-fix behaviour, one of them for
longer than the bug existed:
linux.rs durable_sync still said "all three fallbacks return Ok(())"
mod.rs sync_all still said Linux silently swallows fsync failures and
callers must not treat Ok(()) as a durability barrier
mod.rs SequentialSink::finish repeated the same caveat
All three now say what the code does: a bounded-fsync failure is an Err
on every platform, so Ok(()) IS a durability barrier. A doc that
describes a fixed bug is worse than no doc — it tells a caller to write
a workaround for something that no longer exists.
au_assembly: discard_gap_before duplicated drop_marks_before's
mark-retirement body verbatim and added one statement. Mine, from
earlier today. It now calls it. Two copies of the same retirement loop
is exactly how the two call sites would drift back together.
clpi: ClpiStream's audio_format / audio_rate / video_format / video_rate
are decoded from untrusted on-disc bytes on every parse and read by
nothing. The identically-named fields consumed in disc/bluray.rs belong
to mpls::StreamEntry, not to this struct — checked, because an earlier
round wrongly called a live function dead. Deleted, along with the seven
test assertions that pinned them; the tests that pin pid, coding_type
and language remain. Also removed a section-header comment orphaned by
the get_extents deletion, describing a fixture that no longer exists.
|
||
|
Matthew Jackson
|
e99b634635 |
fix(mux): a backstop discard is a discontinuity; a stream-start trim is not
drop_marks_before retired discontinuity marks alongside timing marks at both of its call sites. At stream start that is right. At the MAX_AU_BUFFER backstop it is not, and the two are now separate. The backstop fires when 8 MiB accumulate with no AU start code in them — corrupt or hostile input — and throws the run away. There IS a prior AU in that case, and whatever emits next definitively does not continue it: a decoder handed that picture resolves its references against frames separated from it by megabytes of discarded data. Retiring the flag meant the resync gate (resync.rs, driven from mux/disc.rs) never armed, so the broken picture went out looking sound. Silent corruption is the one class of loss this crate refuses to have. At stream start the opposite holds. Bytes ahead of the first access-unit delimiter are the tail of an AU that began before sync, and there is no prior AU to be discontinuous from. Marking it would arm the gate at the head of every title and drop its opening GOP. That risk is why this was a decision rather than a fix, and splitting the call sites is what avoids paying it. Recorded as a sticky flag, not an offset mark. A mark placed at the new base is retired moments later by the pre-sync trim that follows resync — the gap has to outlive the bytes that caused it. I found that by writing the test first and watching it fail with the mark approach. The discard is a discontinuity whether or not the source signalled one, and a signalled one on discarded bytes still reaches the AU that follows; both directions are tested. Note the first over-cap run is NOT a discard: the next AU's delimiter is still at buf[0], so it force-flushes as an over-long access unit and loses nothing. Only a run with no opener at all reaches the backstop. The tests push twice for that reason — the single-push version passes without the fix. Swapping either call site for the other fails: reverting the backstop reds the two gap tests, and arming the gate at stream start reds the third. |
||
|
Matthew Jackson
|
9de88969ca |
test: constrain the DiscStream loss surface and the empty-title guards
Second mutation pass over src/mux/. 26 survivors killed, no production change. Verified on HEAD before landing: each mutation below passes all 1,237 mux tests unmutated-suite. The priority item was the honest-loss-reporting surface. Both DiscStream::errors and DiscStream::lost_bytes could return a constant with nothing failing — a rip that lost sectors would report zero loss to the caller. This project has already shipped one defect of that shape (a total decryption failure reported as an empty title, exit 0). Driven now through two short-read fills so both land on values that are neither 0 nor 1 and differ from each other; no constant and no field swap survives. MkvStream::finish -> Ok(()) also survived. MkvMuxer::finish has the zero-frame MkvInvalid guard and two tests cover it, but the Stream wrapper above it could return Ok unconditionally and bypass the guard entirely — the empty-title defence was one layer thinner than it looked. au_assembly: pinned au_opener_from behaviourally to the normative byte values for all four modes, with negative cases for codes that are explicitly not openers (MPEG-2 slice 0x01..0xAF, user data 0xB2, extension 0xB5, sequence end 0xB7 per 13818-2 Table 6-1; VC-1 0x0A/0x0B/0x0C; H.264 SPS/PPS/IDR-slice). au_assembly and codec/ hold independent copies of these constants; they agree today, and comparing constants would not catch logic drifting apart, so both sides are now pinned to the spec instead of to each other. demux_sink::sanitize: every filename component demux:// writes comes from disc-controlled text, so the path-separator arm is a traversal guard. Deleting it now fails, including an end-to-end case where base = "../evil/Title" must produce exactly one file inside the chosen directory. stts_and_ctts_expand renamed to stts_expands_runs_to_per_sample_deltas_in_order and given runs with distinct deltas AND distinct lengths. Its old name claimed ctts coverage it never had, which is why the composition-time chain went unconstrained for eight rounds; the doc comment now points at the tests that do cover ctts. Correction to the previous pass: codec/truehd.rs flush -> vec![] IS equivalent. Applied it, full mux suite green. TrueHD buffers across PES but parse emits every complete unit immediately, so a residual buffer at EOF is a truncated access unit and is correctly discarded. The vec![Default::default()] variants are genuinely different and are killed. Deliberately not constrained: mkv::set_opening_capture (diagnostics behind a process-global tracing check, flaky under the parallel runner), and the three stdio.rs header paths (StdioStream holds concrete io::Stdin/Stdout and cannot be driven without a production refactor to injectable Read/Write). |
||
|
Matthew Jackson
|
0bbceed985 |
Round 4: fix 26 defects across crypto, resource use and codec paths
Twenty-six confirmed findings from the fourth audit round, landed as one cluster because they were found by agents working over disjoint file sets. The one worth calling out is a pair of AACS tests that could not fail. Both asserted CBC behaviour against a hand-rolled expectation that happened to be IV-independent, so replacing AACS_IV with sixteen zero bytes left them passing — they were pinning the code's own arithmetic, not the published constant. Replaced with a literal witness of the published IV plus the NIST SP 800-38A F.2.2 CBC-AES128 vector, and verified the other way round: zeroing AACS_IV now fails three tests. The rest are allocation and correctness work on hot paths: the Annex-B writer in demux_sink allocated and freed a whole-frame Vec per frame, which for a UHD title is ~200,000 allocations over the mmap threshold plus the page faults to first-touch each one; it now reuses a buffer on the writer, and still takes the NAL prefix width from the configuration record rather than assuming four. Six findings whose real fix lives in a consumer crate are recorded for re-filing rather than patched here. |
||
|
Matthew Jackson
|
a32373ff40 |
Fix fifteen defects across perf, resource, panics and key hygiene
All 21 findings held up under verification; 15 fixed here, 6 deferred to files another agent held this round, 0 rejected. **A defect in my own round-2 probe fix.** CHUNK_SECTORS was 1024, and 1024 % 3 == 1 — verified — so every chunk after the first was misaligned against the 6144-byte AACS aligned unit and would be REJECTED by DecryptingSectorSource's alignment gate. On an encrypted disc the forced-subtitle probe I added last round would have read almost nothing past its first chunk. Now 1023 sectors (341 aligned units) with a const assertion that fails the build if it stops dividing, plus set_unit_base per extent so the source's gate is anchored where the extent actually starts. **The same probe skipped sectors on a short read**, advancing by the REQUESTED count rather than the bytes actually returned, so a partial read silently left a gap in the middle of the evidence. It now advances by n/SECTOR_BYTES and clamps n to the buffer. **Its cache key omitted the PGS PID set**, so a playlist declaring an extra subtitle PID got another playlist's verdict for a track that had never been probed. And the key was the whole extent list, so partial clip sharing missed entirely. Both fixed by keying (start_lba, sector_count, pid) — and per-extent keying was shown SOUND rather than assumed: ForcedTracker is two monotone booleans, so per-extent evidence composes by field-wise OR, order- and grouping-independently. Making that honest required per-extent demux state, so an extent's evidence comes only from its own bytes, and memoising only extents whose read reached a designed stop. **A reachable panic in the timeline.** mkvstream::parse_block accepts a TimestampScale up to i64::MAX, so a video frame can set high_ns = i64::MAX and the next passive frame panicked adding the backstep. In release it wrapped negative instead, firing the straggler clamp for essentially every passive frame — audio and subtitles rewritten onto the wrong point of the output timeline. All four sites saturate. **A public constructor divided by zero**: PrefetchedSectorSource::new_with_events with unit_align == 0. Now InvalidInput, matching its batch_sectors sibling. **Two Debug impls printed key material.** DiscInputs (volume_id, mkb, unit_key_ro, samples) and UnitKeyFile both derived Debug. Nothing logs them today — fixed as prevention, because the next tracing::debug! someone adds is the leak. A doc claim that DiscInputs "contains no secrets" was false and is corrected. **An env-var multiply could overflow** in file_sector_source; now bounded at 64 GiB like its writeback sibling, with the parse split out so the bound is testable without touching process env. **The mp4 demuxer allowed one sample per file byte** — ~64x RAM amplification. Now file_len/16, since only vide/soun tracks are indexed and the shortest legal AC-3 frame is 128 bytes. **Two pipeline concurrency defects**: a consumer apply() error was invisible to the producer, and abandon/finalise had a TOCTOU where a caller could report an unfinalised output. Both fixed with compare-exchange state rather than a bool. **Two per-frame copies removed**, both MEASURED rather than reasoned: the AU assembler now hands its allocation to the frame (same pointer, unchanged capacity, proven by asserting the pointer) and tsmux reuses one Annex-B buffer across frames. Both keep capacity deliberately — a naive split_off would have cost more than it saved. **A comment pointed at the wrong file** for a mirrored constant; the mirror is now compiler-enforced with a const assertion converting 90 kHz ticks to ns, so drift fails the build. Deferred to another agent's files, all confirmed: detect_rate's fractional-twin snap, the mp4 reserve's u32 truncation, round_up_grain's overflow, the quadratic base-key gap fill, and MkvStream's frame cap counting frames rather than bytes. Every fix verified red by reverting it. Also noted for later: DecodeSampleSet still derives Debug over multi-MB of on-disc ciphertext. |
||
|
Matthew Jackson
|
270f9d88b3 |
audit: drop dead DTS marks cap, lazy passthrough buf, doc corrections
Round-9 findings from the 10-phase release audit (no HIGH): - Remove the MAX_PTS_MARKS backstop and its tautological test: an empty DTS PES returns before recording a mark, and a non-empty run is already bounded by the MAX_AU_BYTES buffer clear (which clears pts_marks) — so the deque cannot grow unbounded and the cap was dead code. - AuAssembler::for_codec no longer reserves 256 KiB for a Passthrough stream (audio/subtitle, and every TS/BD stream) whose buf is never written; only the reassembling modes reserve. - Correct the scan comment that claimed region is computed (it is a Region-free stub until region detection lands) and drop a public-repo reference to internal "private refactor notes" in the mkb module doc. |
||
|
Matthew Jackson
|
7d852419b5 |
audit: byte caps on GOP buffers, opener-scan resume, honest video codec
Round-6 findings from the 10-phase release audit: - Wire the documented MAX_PENDING_BYTES byte cap into the MPEG-2 GOP buffer (it was dead code) and add an equivalent MAX_GOP_BYTES cap to the sparse-PTS reorder, so a crafted stream of few-but-huge access units cannot over-allocate — both were bounded only by frame count before. - probe_evo_streams defaulted an unsniffable HD-DVD video stream to H.264, which mis-parses a VC-1 (or still-encrypted) clip into a corrupt track. Emit the video stream only when the codec is actually identified — the honest outcome, matching the audio path (a real clear clip always carries its sequence header at the head). - Resume the AU-opener search from a cursor (like the boundary search), so a long unsynced junk run is O(bytes), not O(buffer) per push. - Mark mpeg2's now-dead MAX_AU_BUFFER test-only; restore #[doc(hidden)] on the aacs probe harness module. - Add regression tests: the 0xFD video-routing guard, the FMTS-is-UHD key state, and the GOP byte caps. |
||
|
Matthew Jackson
|
9066433c29 |
audit: guard 0xFD video routing, carry frame duration, add cap tests
Round-5 findings from the 10-phase release audit: - collect_es routed EVERY extended-stream-id (0xFD) PES into the video ES buffer, so a 0xFD HD-audio sub-stream (MLP/TrueHD) could pollute the video sample and — if it preceded the video PES — stamp the video track with the audio PID, losing the video. Only the VC-1 extension (0x55) is now treated as video; routing 0xFD audio to its own track is deferred to the HD-DVD program-chain follow-up. - The sparse-PTS reorder now carries its calibrated per-frame duration onto each frame, so the muxer emits a BlockDuration and the back-patched Segment Duration covers the final frame instead of understating it. - Add regression tests for the MAX_MARKS and MAX_VTI_HITS caps (promote MAX_VTI_HITS to module scope); make the differential-test factory array a named type; drop an identity-op in a reorder test. |
||
|
Matthew Jackson
|
c81a6e05cd |
audit: fix AU mark-field loss, VTI tie determinism, and mark/perf issues
Round-4 findings from the 10-phase release audit (the first fully clean round; it dug into the new #22/#18 refactor code): - AuAssembler closed each AU from only the FRONT mark's fields, so when one PES fragment carried the source and a later fragment of the same AU carried the PTS, the second field was dropped — a regression vs the old separate pts/source mark deques. Now merge the first Some of each field across all in-range marks. - parse_vti_clip_order picked the largest residue bucket with HashMap::into_values().max_by_key(), nondeterministic on a size tie (randomized HashMap iteration) — could select a different clip table run-to-run. Break ties by smallest offset. - Bound the marks/disc_marks deques (MAX_MARKS): the buf-size cap prunes marks only when bytes accumulate, so a run of zero-length timed fragments could grow them without bound on hostile input. - Add push_owned so the PS path moves the PES payload into a passthrough AU with no copy (MPEG-2 video + all audio), removing a per-PES malloc+memcpy the refactor had introduced on the DVD path. - Back-patch the MKV duration from the block END (start + its own duration) so it covers the final frame instead of understating by one. - Add direct tests for the MKB record-framing walker; drop a stale drain_complete_aus doc comment left on process_au. |
||
|
Matthew Jackson
|
b5a5138569 |
mux: resume AU-boundary scans from a cursor (O(n) not O(n²))
AuAssembler::drain rescanned the whole buffered access unit from a fixed offset on every push, so reassembling one AU split across N program-stream PES fragments cost O(bytes²/fragment) — amplified on the HD-DVD PS path where H.264/HEVC/VC-1 frames are large and now flow through this shared assembler (unlike the TS path, which delivers one AU per PES). Carry a scan_pos cursor (and, for the stateful VC-1/MPEG-2 rules, a seen_unit flag) so each push resumes the boundary search where the last one stopped instead of restarting. Total scan work for one AU is now O(AU bytes). The from-scratch scanners are retained as a #[cfg(test)] oracle; a new differential test asserts the resumable path yields byte-identical AUs at every fragment granularity for all three modes. |
||
|
Matthew Jackson
|
3633882d6c |
mux: reassemble MPEG-2 access units via the shared AuAssembler
The MPEG-2 parser hand-rolled its own PES reassembly — a byte buffer plus parallel PTS / source / discontinuity mark queues keyed by absolute offset — duplicating what AuAssembler already does for H.264/HEVC/VC-1. Add a Mode::Mpeg2 to AuAssembler (picture 0x00 with preceding sequence 0xB3 / GOP 0xB8 headers — the same headers-precede-picture shape as the VC-1 mode) and have the MPEG-2 parser own one via AuAssembler::mpeg2(). parse() now feeds fragments to the assembler and processes each complete access unit; the buffer, base offset, and three mark queues are gone. The GOP-buffered temporal_reference reorder and PTS origin-locking are unchanged. The parser's external contract is unchanged, so all existing MPEG-2 parser tests pass as-is; new AuAssembler tests cover the MPEG-2 boundary rule directly. |
||
|
Matthew Jackson
|
48bec4cc03 |
mux: HD-DVD VC-1 demux via extended stream id 0xFD
VC-1 HD-DVDs (e.g. Shaun of the Dead) carry video on MPEG-PS extended stream id 0xFD, with the real stream selector in stream_id_extension inside the PES extension. Parse that field so the video routes to a distinct track (pid 0xFD00|ext) instead of being dropped. Reframe VC-1 access units in AuAssembler with a dedicated Mode::Vc1: an AU is delimited by the next frame BDU (0x0D) once a frame has already been seen, so the sequence (0x0F) and entry-point (0x0E) headers that precede an I-frame stay attached to the frame they describe. The old single-start-code split stranded those headers on the prior AU, which the decoder reported as bits-overconsumption and hard decode failures. hddvd probe now tracks the video pid it detects and emits VC-1 on 0xFD. |