From 048f1258794e29aabac417a4e559bfd52e070612 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Sat, 1 Aug 2026 13:35:43 -0700 Subject: [PATCH] Kill codec mutation survivors and unify H.264's duplicated escape stripper The mux/codec parsers (startcode, h264, hevc, dts) had 300 surviving mutants between them, and it turned out to be for the reason you'd fear: the exp-Golomb readers and the AU-boundary bitstream scanners had essentially no direct unit coverage, only indirect exercise through full-frame parse() calls that never touched the actual edge cases. Direct fixes to test gaps: - The shared BitReader's read_ue truncation guard (`leading_zeros > 31`) and skip_start_code's 4-byte-vs-3-byte boundary check had no test at their exact boundary. Added tests that hit the boundary precisely; a `>=`/`==`/`<=` typo either rejects a legal 31-leading- zero code or reads one byte past the buffer. - H.264's private SpsReader duplicates the same read_bits/read_ue shapes with no tests of its own at all (only reached through multi-field SPS parsing, several fields deep). Added direct tests. - HEVC's per-AU trailing-zero strip after the last NAL (no start code following) walks `end` down to trim padding; a wrong-direction typo there walks off the end of the buffer instead of terminating - exactly the "loop must make positive progress on malformed input" class. Added a test with a zero-padded trailing NAL. - HEVC's SEI match guards (`sei_mastering.is_none()` / `sei_content_light.is_none()`) implement "first HDR10 value in the title wins" - untested, and a naive test using both-messages-per-AU can't even exercise the guards because the whole-scan early return above them already handles that case. Split into single-message- per-AU tests that actually reach the arms. - parse_mastering_display/parse_content_light_level's length guards were `< N` with no boundary test; one-byte-short input now confirmed to return None instead of indexing out of bounds. - DTS's drain_front collapses duplicate offset-0 PTS markers after rebasing; untested, and the visible effect (front_pts()) can't tell a working collapse from a broken one since it already returns the right marker either way - the actual defect is unbounded growth of pts_marks over a long recording, so the new test asserts the bound directly across repeated drains. - DTS's dts_core_samples/dts_core_sample_rate header-length guard and next_core_boundary's syncword-length guard got exact-boundary tests the same way; also caught a nblks `<<`/`>>` direction bug candidate in the mutant (confirmed the real code is correct, just untested). Real bug found and fixed, not just a test gap: H.264's parse_sps_high_profile_ext re-implemented emulation-prevention byte stripping inline (a window scan: match `00 00 03` at position i, advance 3, else advance 1) instead of calling the existing unescape_ebsp_prefix used by slice-header parsing. On a run of 3+ real zero bytes ahead of an 0x03 - non-conformant, but this is disc bytes, not a spec-clean encoder - the two disagreed: unescape_ebsp_prefix's cumulative zero counter (matching the H.264 reference decode process and libavcodec's RBSP extractor) treats it as an escape and drops the 0x03; the window scan treats it as real payload and keeps it, corrupting the SPS bits read after it. Extracted the shared rule into `unescape_ebsp` (parameterized on output length so both the 16-byte slice-header prefix and the unbounded SPS case can share it) and pointed both call sites at the one implementation. Added a regression test pinning the shared function's behaviour on the input that used to separate them. All new tests hand-verified against the actual mutation (operator flipped or guard replaced by hand, confirmed red, then restored) per the mutation-testing brief, not just written and trusted. --- src/mux/codec/dts.rs | 120 +++++++++++++++++++++++++++++++++++++ src/mux/codec/h264.rs | 89 +++++++++++++++++++++------ src/mux/codec/hevc.rs | 106 ++++++++++++++++++++++++++++++++ src/mux/codec/startcode.rs | 25 ++++++++ 4 files changed, 321 insertions(+), 19 deletions(-) diff --git a/src/mux/codec/dts.rs b/src/mux/codec/dts.rs index f56f632..238f500 100644 --- a/src/mux/codec/dts.rs +++ b/src/mux/codec/dts.rs @@ -863,6 +863,54 @@ mod tests { // frames advance by. (512 * 1e9 + 24000) / 48000 = 10_666_667 ns. const DTS_CORE_DUR_NS: i64 = (512 * 1_000_000_000 + 48_000 / 2) / 48_000; + // --- stamp_pts: the "same PES, no fresh front timestamp" branch --- + + /// `stamp_pts`'s third arm (`else if front != PTS_UNSET`) is only reached + /// once the first arm's `front != self.last_front_pts` has already failed + /// (i.e. `front == self.last_front_pts`) and no within-PES running cursor + /// is available yet (`next_pts_ns == PTS_UNSET`). In that state the AU must + /// still be stamped with its own (repeated) front timestamp, not silently + /// collapsed to 0 — a `!=` -> `==` typo on the guard would only take this + /// arm when `front` IS the unset sentinel, producing 0 instead here and the + /// sentinel value itself if front really were unset. + #[test] + fn stamp_pts_reuses_front_when_no_running_cursor_yet() { + let mut parser = DtsParser::new(); + parser.last_front_pts = 500; + parser.next_pts_ns = PTS_UNSET; + assert_eq!( + parser.stamp_pts(500, DTS_CORE_DUR_NS), + 500, + "front == last_front_pts and no cursor yet: stamp with front itself" + ); + } + + // --- drain_front: collapsing duplicate offset-0 PTS markers must not leak --- + + /// Every drained access unit that shares its PES with the previous one + /// pushes a new `(0, pts)` marker once rebased; `drain_front` must collapse + /// those down to the single most-recent one each time, or `pts_marks` grows + /// once per access unit for the life of the track — an unbounded allocation + /// on a multi-hour disc. `front_pts()` alone can't observe this: it always + /// finds the last offset-0 entry regardless of how many duplicates precede + /// it, so the leak is invisible unless something checks that the collapse + /// actually ran. + #[test] + fn drain_front_collapses_offset_zero_markers_instead_of_leaking() { + let mut parser = DtsParser::new(); + for i in 0..200i64 { + parser.buf.extend_from_slice(&[0u8; 5]); + parser.pts_marks.push_back((5, i)); + parser.pts_marks.push_back((5, i)); + parser.drain_front(5); + } + assert!( + parser.pts_marks.len() <= 2, + "pts_marks must stay bounded across repeated drains, got {}", + parser.pts_marks.len() + ); + } + /// A real DTS-HD EXSS substream of `total` bytes (short header form), with an /// optional false DTS core syncword embedded in its payload (decoding to a /// plausible core size) — to prove precise sizing, not a payload scan, bounds @@ -1589,6 +1637,78 @@ mod tests { assert_eq!(dts_core_frame_size(&d), 0x3FFF + 1); } + // --- dts_core_samples / dts_core_sample_rate: header-length boundary --- + + #[test] + fn dts_core_samples_reads_nblks_high_bit_from_byte4_bit0() { + // NBLKS is 7 bits: byte4 bit0 is the HIGH bit (<<6), byte5>>2 the low 6. + // With byte4 bit0 set and byte5 = 0, nblks = 64 (not 0), so a `<<` -> `>>` + // typo on the byte4 contribution (which always yields 0, since a 1-bit + // value has nothing to shift right into) collapses samples from 2080 + // down to 32. + let mut d = vec![0u8; CORE_HEADER_MIN_BYTES]; + d[4] = 0x01; + d[5] = 0x00; + assert_eq!(dts_core_samples(&d), (64 + 1) * 32); + } + + #[test] + fn dts_core_samples_and_sample_rate_decode_at_exact_header_min_length() { + // The length guard is `data.len() < CORE_HEADER_MIN_BYTES`; exactly + // CORE_HEADER_MIN_BYTES (10) bytes must still be read as a real header + // (index 8, SFREQ, is in bounds), not treated as truncated. Use NBLKS + // and SFREQ values that differ from the truncated-header fallbacks + // (512 samples, 48 kHz) so a `<` -> `<=` typo that takes the fallback + // path one byte early is visible in the output, not masked by a + // coincidental match. + let mut d = vec![0u8; CORE_HEADER_MIN_BYTES]; + d[4] = 0x00; + d[5] = 0x00; // nblks = 0 -> 32 samples, not the 512 fallback + d[8] = 6u8 << 2; // SFREQ = 6 -> 11_025 Hz, not the 48_000 fallback + assert_eq!(d.len(), CORE_HEADER_MIN_BYTES); + assert_eq!(dts_core_samples(&d), 32); + assert_eq!(dts_core_sample_rate(&d), 11_025); + } + + #[test] + fn dts_core_samples_and_sample_rate_decode_past_header_min_length_too() { + // A `<` -> `>` typo on the same guard takes the FALLBACK path once + // `data.len()` exceeds CORE_HEADER_MIN_BYTES instead of never — i.e. + // every real, normally-sized core frame (always well past 10 bytes) + // would silently report the 512-sample/48kHz fallback. Use a buffer + // twice the minimum with non-fallback NBLKS/SFREQ values. + let mut d = vec![0u8; CORE_HEADER_MIN_BYTES * 2]; + d[4] = 0x00; + d[5] = 0x00; // nblks = 0 -> 32 samples + d[8] = 6u8 << 2; // SFREQ = 6 -> 11_025 Hz + assert!(d.len() > CORE_HEADER_MIN_BYTES); + assert_eq!(dts_core_samples(&d), 32); + assert_eq!(dts_core_sample_rate(&d), 11_025); + } + + // --- next_core_boundary: SYNCWORD_BYTES length guard --- + + /// Only the 4-byte EXSS syncword is buffered after the core frame — no + /// header fields at all. The guard `buf.len() < pos + SYNCWORD_BYTES` must + /// be strict `<`: at exactly `pos + SYNCWORD_BYTES` the sync bytes ARE + /// fully present, so the function proceeds to identify them as an + /// extension sync (then fails to size the header and falls back, ending + /// in `NextCore::None` here since nothing after it looks like a core sync + /// either). A `<=` typo would instead return `NeedMore` at this exact + /// length without ever inspecting what the 4 buffered bytes are. + #[test] + fn next_core_boundary_exact_syncword_length_is_not_need_more() { + let core = make_dts_core(MIN_CORE_FRAME_BYTES); + let mut buf = core.clone(); + buf.extend_from_slice(&DTS_HD_EXT_SYNC); // exactly 4 bytes, nothing more + let result = next_core_boundary(&buf, core.len()); + assert!( + matches!(result, NextCore::None), + "expected None (recognized-but-unsizeable extension sync with \ + nothing after it), got a different variant" + ); + } + // --- find_sync --- #[test] diff --git a/src/mux/codec/h264.rs b/src/mux/codec/h264.rs index bea17f6..ca65868 100644 --- a/src/mux/codec/h264.rs +++ b/src/mux/codec/h264.rs @@ -141,10 +141,29 @@ thread_local! { /// off the per-frame allocation path for anything larger. fn unescape_ebsp_prefix(ebsp: &[u8]) -> Vec { const PREFIX_OCTETS: usize = 16; - let mut out = Vec::with_capacity(PREFIX_OCTETS); + unescape_ebsp(ebsp, PREFIX_OCTETS) +} + +/// Copy `ebsp` with emulation-prevention bytes removed, stopping after at +/// most `max_octets` OUTPUT bytes (pass `ebsp.len()` for "no cap"). +/// +/// The zero run-length counter is cumulative across the whole scan and is +/// reset only when an escape byte is actually dropped — matching the +/// reference decoding process (ITU-T H.264 §7.3.1 / the libavcodec RBSP +/// extractor), which discards a 0x03 following ANY run of two-or-more 0x00 +/// bytes, not just an exact `00 00 03` sliding window. A window-based scanner +/// that advances by 3 on a match and by 1 otherwise disagrees with this on a +/// run of 3+ real zero bytes ahead of an 0x03: `00 00 00 03` decodes here as +/// `00 00 00` (the run is escaped, matching every other H.264 decoder), where +/// a window scan starting fresh after each non-match would keep the 0x03 as +/// real payload. Only one implementation of this rule may exist in this +/// module — see `parse_sps_high_profile_ext`, which used to keep its own +/// window-scan copy that disagreed with this one on exactly that input. +fn unescape_ebsp(ebsp: &[u8], max_octets: usize) -> Vec { + let mut out = Vec::with_capacity(max_octets.min(ebsp.len())); let mut zeros = 0usize; for &b in ebsp { - if out.len() == PREFIX_OCTETS { + if out.len() == max_octets { break; } // Drop the escape octet itself, but only in the 00 00 03 position. @@ -493,23 +512,12 @@ impl CodecParser for H264Parser { /// omits the extension in that case. fn parse_sps_high_profile_ext(sps: &[u8]) -> Option<(u8, u8, u8)> { // Strip emulation-prevention bytes: 00 00 03 xx → 00 00 xx (drop the 03). - // We skip byte 0 (NAL header) and start the RBSP from byte 1. - let rbsp: Vec = { - let raw = &sps[1..]; // skip NAL header byte - let mut out = Vec::with_capacity(raw.len()); - let mut i = 0; - while i < raw.len() { - if i + 2 < raw.len() && raw[i] == 0x00 && raw[i + 1] == 0x00 && raw[i + 2] == 0x03 { - out.push(0x00); - out.push(0x00); - i += 3; // skip the 0x03 emulation-prevention byte - } else { - out.push(raw[i]); - i += 1; - } - } - out - }; + // We skip byte 0 (NAL header) and start the RBSP from byte 1. Shares + // `unescape_ebsp` with the slice-header prefix reader rather than + // re-deriving the same rule — see that function's doc comment for why a + // second, window-based copy of this rule used to disagree with it. + let raw = &sps[1..]; // skip NAL header byte + let rbsp: Vec = unescape_ebsp(raw, raw.len()); // RBSP layout after stripping the NAL header byte: // [0] profile_idc (already checked by caller) @@ -747,6 +755,26 @@ mod tests { ); } + /// Regression: `parse_sps_high_profile_ext` used to re-derive the + /// emulation-prevention rule with its own window scanner (match `00 00 + /// 03` at position i, advance by 3; else advance by 1) instead of calling + /// the shared `unescape_ebsp`. On a run of 3+ real 0x00 bytes ahead of an + /// 0x03 — non-conformant, but this is untrusted disc input, not a + /// spec-clean encoder — the two disagreed: `unescape_ebsp`'s cumulative + /// zero counter (the same rule libavcodec's RBSP extractor and the H.264 + /// reference decoding process use) drops the 0x03 as an escape, while a + /// fresh 3-byte window starting right after the non-matching first byte + /// kept it as real payload. Pin the shared function's behaviour here so a + /// second hand-rolled copy doesn't quietly reappear. + #[test] + fn unescape_ebsp_drops_escape_after_a_run_of_three_zeros() { + assert_eq!( + super::unescape_ebsp(&[0x00, 0x00, 0x00, 0x03, 0x42], 5), + vec![0x00, 0x00, 0x00, 0x42], + "the 0x03 after a 3-zero run is an escape byte, not payload" + ); + } + // --- keyframe parameter-set re-assert: exact bytes + no whole-frame copy --- /// The keyframe SPS/PPS re-assert must produce EXACTLY these bytes: the @@ -2011,4 +2039,27 @@ mod tests { expected_len ); } + + /// `SpsReader::read_bits` shifts each new bit into the low end of the + /// accumulator (`val << 1 | bit`). Pins the direction directly: a + /// `<<` -> `>>` typo would leave every accumulated bit shifted out and + /// the result would collapse towards 0 instead of building up the value. + #[test] + fn sps_reader_read_bits_builds_value_msb_first() { + // 0b1011_0000 read 4 bits MSB-first -> 0b1011 = 11. + let mut r = super::SpsReader::new(&[0b1011_0000]); + assert_eq!(r.read_bits(4), Some(0b1011)); + } + + /// `SpsReader::read_ue`'s truncation guard is `leading_zeros > 31`: 31 + /// leading zero bits is the longest legal code and must decode, not + /// abort. Mirrors the equivalent guard in the shared `BitReader` in + /// `startcode.rs`, kept here because `SpsReader` is a separate, + /// unshared implementation used only for SPS parsing. + #[test] + fn sps_reader_read_ue_thirty_one_leading_zeros_is_still_valid() { + let data = [0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00]; + let mut r = super::SpsReader::new(&data); + assert_eq!(r.read_ue(), Some(u32::MAX >> 1)); + } } diff --git a/src/mux/codec/hevc.rs b/src/mux/codec/hevc.rs index 3b8d45e..a30d170 100644 --- a/src/mux/codec/hevc.rs +++ b/src/mux/codec/hevc.rs @@ -1250,6 +1250,76 @@ mod tests { assert_eq!(h.max_pic_average_light_level, maxfall); } + /// The `scan_sei` match arms are guarded with `self.sei_mastering.is_none()` + /// / `self.sei_content_light.is_none()`, so the FIRST mastering-display and + /// content-light SEI a title carries wins and later repeats (every AU of a + /// real HDR10 stream repeats both) are ignored. A splice or a corrupt later + /// AU that carries different (or garbage) HDR10 numbers must not overwrite + /// the value already latched from the title's first AU. + /// + /// Each AU below carries only ONE of the two messages, so `sei_mastering` + /// and `sei_content_light` are never BOTH `Some` at once — the whole-scan + /// early return a few lines above these two match arms + /// (`if self.sei_mastering.is_some() && self.sei_content_light.is_some()`) + /// would otherwise fire on the second AU and make the per-arm guards + /// unreachable, proving nothing about them. + #[test] + fn hevc_hdr10_sei_keeps_the_first_value_and_ignores_later_repeats() { + let pps = { + let mut v = vec![0x00, 0x00, 0x01]; + v.extend_from_slice(&hevc_nal_header(NAL_PPS)); + v.push(0xC0); + v + }; + let idr = { + let mut v = vec![0x00, 0x00, 0x01]; + v.extend_from_slice(&hevc_nal_header(19)); + v.push(0xEC); + v + }; + let mastering_au = |max_lum: u32| { + let mut data = pps.clone(); + data.extend_from_slice(&sei_nal(&[sei_message( + SEI_MASTERING_DISPLAY_COLOUR_VOLUME, + &mastering_payload([1, 2, 3], [4, 5, 6], 7, 8, max_lum, 10), + )])); + data.extend_from_slice(&idr); + data + }; + let cll_au = |maxcll: u16, maxfall: u16| { + let mut data = pps.clone(); + data.extend_from_slice(&sei_nal(&[sei_message( + SEI_CONTENT_LIGHT_LEVEL_INFO, + &cll_payload(maxcll, maxfall), + )])); + data.extend_from_slice(&idr); + data + }; + + let mut mastering_only = HevcParser::new(); + mastering_only.parse(&make_pes(mastering_au(10_000_000), Some(0))); + mastering_only.parse(&make_pes(mastering_au(1), Some(3750))); + assert_eq!( + mastering_only + .sei_mastering + .map(|m| m.max_display_mastering_luminance), + Some(10_000_000), + "the SECOND AU's mastering-luminance must be ignored, not adopted" + ); + + let mut cll_only = HevcParser::new(); + cll_only.parse(&make_pes(cll_au(1000, 400), Some(0))); + cll_only.parse(&make_pes(cll_au(9999, 9999), Some(3750))); + assert_eq!( + cll_only.sei_content_light, + Some(ContentLightLevel { + max_content_light_level: 1000, + max_pic_average_light_level: 400 + }), + "the SECOND AU's content-light numbers must be ignored, not adopted" + ); + } + /// MEASURED, not reasoned: an HDR10 stream carries a prefix SEI per access /// unit, and `scan_sei` allocated + byte-copied the whole SEI RBSP through /// `strip_emulation_prevention` on EVERY one — including after both HDR10 @@ -2520,6 +2590,26 @@ mod tests { assert_eq!(len + 4, fd.len(), "exactly one NAL in frame data"); } + /// The trailing-zero strip after the last NAL in the buffer (`while end > + /// nal_start && data[end - 1] == 0x00 { end -= 1; }`) must make strictly + /// positive progress: this NAL has no start code after it (`next` falls + /// back to `data.len()`), so a step in the wrong direction walks `end` + /// past the end of `data` and the very next iteration indexes + /// `data[end - 1]` out of bounds. Zero-padding after the last NAL is + /// exactly what a damaged trailing sector on disc looks like. + #[test] + fn trailing_zero_strip_on_last_nal_does_not_run_past_the_buffer() { + let mut parser = HevcParser::new(); + let mut data = vec![0x00, 0x00, 0x01]; + data.extend_from_slice(&hevc_nal_header(NAL_AUD)); + data.push(0x00); // damaged/zero-padded trailing byte, no start code follows + let frames = parser.parse(&make_pes(data, Some(0))); + // AUD is dropped and the payload is otherwise empty once the padding + // is stripped, so there is nothing to emit — the assertion that + // matters is that `parse` returned at all instead of panicking. + assert!(frames.is_empty()); + } + // --- empty PES --- #[test] @@ -3189,6 +3279,22 @@ mod tests { assert!(parse_sps_chroma(&[0x42, 0x01]).is_none()); } + /// A mastering-display SEI payload one byte short of the fixed 24-byte + /// layout must be rejected, not read out of bounds. This is the guard a + /// crafted/truncated SEI on a damaged disc hits directly. + #[test] + fn parse_mastering_display_one_byte_short_is_none() { + assert!(parse_mastering_display(&[0u8; 23]).is_none()); + assert!(parse_mastering_display(&[0u8; 24]).is_some()); + } + + /// Same guard, content-light-level's 4-byte layout. + #[test] + fn parse_content_light_level_one_byte_short_is_none() { + assert!(parse_content_light_level(&[0u8; 3]).is_none()); + assert!(parse_content_light_level(&[0u8; 4]).is_some()); + } + #[test] fn hvcc_falls_back_to_8bit_420_on_unparseable_sps() { // An SPS whose RBSP is truncated mid-parse (can't reach the bit depths) diff --git a/src/mux/codec/startcode.rs b/src/mux/codec/startcode.rs index e742328..e8d71eb 100644 --- a/src/mux/codec/startcode.rs +++ b/src/mux/codec/startcode.rs @@ -315,4 +315,29 @@ mod tests { let data = [0x00, 0x01, 0x01, 0x65]; assert_eq!(skip_start_code(&data, 0), None); } + + #[test] + fn skip_start_code_three_zeros_no_terminator_is_not_a_code() { + // `00 00 00` with nothing after it is neither a 3-byte code (3rd byte + // isn't 0x01) nor a complete 4-byte code (there is no 4th byte at all). + // Pins the `pos + 3 < data.len()` guard: on a 3-byte all-zero buffer + // `pos + 3 == data.len()`, so the guard must be strict `<` and reject + // the 4-byte branch before it would index one past the end. A `<=` + // (or a `pos * 3` typo, which also evaluates to a value not exceeding + // `data.len()` here) lets the 4-byte branch run and read `data[pos+3]` + // out of bounds. + let data = [0x00, 0x00, 0x00]; + assert_eq!(skip_start_code(&data, 0), None); + } + + #[test] + fn read_ue_thirty_one_leading_zeros_is_still_a_valid_code() { + // Exactly 31 leading zero bits, a stop bit, then 31 zero info bits: + // a legal (if enormous) ue(v) code with code_num = 2^31 - 1. The + // truncation guard is `leading_zeros > 31`, so 31 must NOT trip it — + // only 32 does. A guard mutated to `>= 31` or `== 31` aborts one bit + // early and returns None instead of the real value. + let data = [0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00]; + assert_eq!(BitReader::new(&data).read_ue(), Some(u32::MAX >> 1)); + } }