From 4b54d72c23478f9a53b6b660d13565f2b7e94d07 Mon Sep 17 00:00:00 2001 From: SusieeTheFemboy <96988330+SusieeTheLinuxUser@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:00:32 +0200 Subject: [PATCH 1/2] Video: clamp H.264 IDCT inputs and temporal-direct mvs to the spec ranges The audit's L-27 (issue #928): scale4/scale8 and the DC dequantisations deliberately wrap (huge levels must not panic there), so the values reaching idct4_add/idct8_1d can be any i32, where the plain + / - of the butterfly arithmetic panicked in debug and overflow-checks builds on crafted streams. The spec leaves the inverse-transform output unspecified when a dequantised r falls outside -2^15 to 2^15 - 1 (8.5.12.1, likewise 8.5.13.1), so the IDCT now clamps its input to that range: bit-exact for every stream whose r stays in range, defined instead of panicking or wrapping to garbage otherwise. Temporal direct (8.4.1.2.1) had the same class of bug one layer up: the mvL0 - mvCol subtraction overflowed i16 for extreme co-located mvs (stored mvs wrap by design). The derivation moves into temporal_direct_mvs, runs in i32 and clamps to the spec's -2^15 to 2^15 - 1 motion-vector range. Regression tests feed i32::MIN/MAX coefficients into idct4_add/idct8_add and i16 extremes through temporal_direct_mvs; they panicked with overflow before the fix. Decoded output for well-formed streams is unchanged. Co-Authored-By: GLM-5.3-Flash --- crates/video/src/h264/slicedec.rs | 58 ++++++++++++++++++++++++++---- crates/video/src/h264/transform.rs | 48 ++++++++++++++++++++++--- 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/crates/video/src/h264/slicedec.rs b/crates/video/src/h264/slicedec.rs index dfab85b5..f80db0f2 100644 --- a/crates/video/src/h264/slicedec.rs +++ b/crates/video/src/h264/slicedec.rs @@ -566,6 +566,24 @@ impl<'a> SliceDecoder<'a> { (refs, mvs, false) } + /// Temporal direct motion vectors (8.4.1.2.1): mvL0 = (mvCol * DistScaleFactor + 128) >> 8 + /// and mvL1 = mvL0 − mvCol. Motion vectors are always within the spec's −2^15 to 2^15 − 1 + /// range, which crafted streams leave (stored mvs wrap by design), so the derivation runs in + /// i32 and clamps to that range instead of overflowing. + fn temporal_direct_mvs(dsf: i32, mv_col: [i16; 2]) -> ([i16; 2], [i16; 2]) { + const MV_MIN: i32 = -(1 << 15); + const MV_MAX: i32 = (1 << 15) - 1; + let scaled = |c: i16| ((dsf * i32::from(c) + 128) >> 8).clamp(MV_MIN, MV_MAX) as i16; + let m0 = [scaled(mv_col[0]), scaled(mv_col[1])]; + ( + m0, + [ + (i32::from(m0[0]) - i32::from(mv_col[0])).clamp(MV_MIN, MV_MAX) as i16, + (i32::from(m0[1]) - i32::from(mv_col[1])).clamp(MV_MIN, MV_MAX) as i16, + ], + ) + } + /// Derive direct-mode motion for the 8x8 blocks in `b8_mask` of the current MB. fn derive_direct(&mut self, b8_mask: u8) -> Result<()> { let spatial = self.sh.direct_spatial_mv_pred; @@ -600,13 +618,7 @@ impl<'a> SliceDecoder<'a> { } }; let (dsf, ltr) = self.tdirect.get(ref0 as usize).copied().unwrap_or((256, true)); - let (mv0, mv1) = if ltr { - (mv_col, [0, 0]) - } else { - let s = |c: i16| ((dsf * c as i32 + 128) >> 8) as i16; - let m0 = [s(mv_col[0]), s(mv_col[1])]; - (m0, [m0[0] - mv_col[0], m0[1] - mv_col[1]]) - }; + let (mv0, mv1) = if ltr { (mv_col, [0, 0]) } else { Self::temporal_direct_mvs(dsf, mv_col) }; ([ref0, 0], [mv0, mv1]) }; self.set_motion(x, y, unit, unit, refs, mvs); @@ -1467,3 +1479,35 @@ impl<'a> SliceDecoder<'a> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Temporal direct (8.4.1.2.1) with co-located mvs at the i16 extremes: crafted streams get + /// there because stored mvs wrap by design (mvd is truncated, the mvp add wraps), so the + /// derivation must stay within the spec's −2^15 to 2^15 − 1 motion-vector range instead of + /// overflowing (it panicked in debug/overflow-checks builds). + #[test] + fn temporal_direct_mvs_at_i16_extremes_stay_in_the_spec_range() { + // DistScaleFactor extremes from the clamp in SliceDecoder::new (±1024/1023). + for dsf in [-1024, -1, 0, 1, 1023] { + for c0 in [i16::MAX, i16::MIN, 0, 40, -4] { + for c1 in [i16::MAX, i16::MIN, 0, 40, -4] { + let (m0, m1) = SliceDecoder::temporal_direct_mvs(dsf, [c0, c1]); + for mv in m0.into_iter().chain(m1) { + assert!((-(1 << 15)..=(1 << 15) - 1).contains(&i32::from(mv))); + } + } + } + } + // mvL0 = (mvCol * DistScaleFactor + 128) >> 8, mvL1 = mvL0 − mvCol (8.4.1.2.1). + let (m0, m1) = SliceDecoder::temporal_direct_mvs(-1024, [-32768, 32767]); + assert_eq!(m0, [32767, -32768]); + assert_eq!(m1, [32767, -32768]); + // DistScaleFactor 256 (tb == td) reproduces the co-located mv: mvL1 = 0. + let (m0, m1) = SliceDecoder::temporal_direct_mvs(256, [40, -4]); + assert_eq!(m0, [40, -4]); + assert_eq!(m1, [0, 0]); + } +} diff --git a/crates/video/src/h264/transform.rs b/crates/video/src/h264/transform.rs index 1ad55f7b..de01c09c 100644 --- a/crates/video/src/h264/transform.rs +++ b/crates/video/src/h264/transform.rs @@ -91,6 +91,14 @@ fn clip_u8(v: i32) -> u8 { v.clamp(0, 255) as u8 } +/// Clamp a dequantised coefficient to the spec's r range: the inverse-transform output is +/// unspecified when r falls outside −2^15 to 2^15 − 1 (8.5.12.1, likewise 8.5.13.1), and crafted +/// streams get there because scale4/scale8 wrap. Clamping keeps the butterflies below overflow. +#[inline(always)] +fn r_clamp(v: i32) -> i32 { + v.clamp(-(1 << 15), (1 << 15) - 1) +} + /// Inverse 4x4 transform of raster-order `d` and add to `dst` (8.5.12.2, 8.5.14). /// /// Rows first, then columns (the spec's order: the `>> 1` steps make it matter). The column @@ -99,10 +107,11 @@ pub fn idct4_add(d: &[i32; 16], dst: &mut [u8], stride: usize) { let mut t = [[0i32; 4]; 4]; for (i, row) in t.iter_mut().enumerate() { let r = &d[i * 4..i * 4 + 4]; - let e = r[0] + r[2]; - let f = r[0] - r[2]; - let g = (r[1] >> 1) - r[3]; - let h = r[1] + (r[3] >> 1); + let (r0, r1, r2, r3) = (r_clamp(r[0]), r_clamp(r[1]), r_clamp(r[2]), r_clamp(r[3])); + let e = r0 + r2; + let f = r0 - r2; + let g = (r1 >> 1) - r3; + let h = r1 + (r3 >> 1); *row = [e + h, f + g, f - g, e - h]; } let mut o = [[0i32; 4]; 4]; @@ -163,7 +172,7 @@ fn idct8_1d(d: [i32; 8]) -> [i32; 8] { pub fn idct8_add(d: &[i32; 64], dst: &mut [u8], stride: usize) { let mut t = [[0i32; 8]; 8]; for (row, src) in t.iter_mut().zip(d.as_chunks::<8>().0) { - *row = idct8_1d(*src); + *row = idct8_1d(std::array::from_fn(|k| r_clamp(src[k]))); } let mut o = [[0i32; 8]; 8]; for j in 0..8 { @@ -292,6 +301,35 @@ mod tests { assert_eq!(scale8(1, 320, 30), (320 + 1) >> 1); } + /// Crafted streams scale levels far outside the spec's r range (scale4/scale8 wrap by + /// design, so r can be any i32): the spec leaves the inverse-transform output unspecified + /// when r falls outside −2^15 to 2^15 − 1 (8.5.12.1), so the IDCT must clamp its input into + /// the defined range instead of overflowing the butterfly arithmetic (it panicked in + /// debug/overflow-checks builds). + #[test] + fn idct4_huge_coefficients_are_clamped_to_the_spec_r_range() { + let mut d = [0i32; 16]; + for (i, v) in d.iter_mut().enumerate() { + *v = if i % 2 == 0 { i32::MAX } else { i32::MIN }; + } + let (mut a, mut b) = ([0u8; 16], [0u8; 16]); + idct4_add(&[i32::MAX; 16], &mut a, 4); + idct4_add(&[32767; 16], &mut b, 4); + assert_eq!(a, b); + let (mut a, mut b) = ([0u8; 16], [0u8; 16]); + idct4_add(&d, &mut a, 4); + idct4_add(&std::array::from_fn(|i| if i % 2 == 0 { 32767 } else { -32768 }), &mut b, 4); + assert_eq!(a, b); + } + + #[test] + fn idct8_huge_coefficients_are_clamped_to_the_spec_r_range() { + let (mut a, mut b) = ([0u8; 64], [0u8; 64]); + idct8_add(&[i32::MIN; 64], &mut a, 8); + idct8_add(&[-32768; 64], &mut b, 8); + assert_eq!(a, b); + } + /// Corrupt streams can carry huge coefficient levels: dequantisation wraps (as release /// builds always did) instead of panicking with "attempt to multiply with overflow" in /// debug builds (found by mutation fuzzing an H.264 transport stream). From 8b34dc533ff39e0f03936a8327e58408a289c88f Mon Sep 17 00:00:00 2001 From: SusieeTheFemboy <96988330+SusieeTheLinuxUser@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:00:00 +0200 Subject: [PATCH 2/2] Video: derive temporal-direct mvL1 from the unclamped mvL0 mvL1 = mvL0 - mvCol (8.4.1.2.3) was computed from mvL0 after it had been clamped to the motion-vector range, so a crafted stream with dsf 512 and mvCol 32767 got mvL1 = 0 instead of 32767. Both vectors are now derived in i32 and only the results are clamped; in-range streams are unchanged. Co-Authored-By: Claude Opus 5.5 --- crates/video/src/h264/slicedec.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/video/src/h264/slicedec.rs b/crates/video/src/h264/slicedec.rs index f80db0f2..a118b964 100644 --- a/crates/video/src/h264/slicedec.rs +++ b/crates/video/src/h264/slicedec.rs @@ -573,15 +573,10 @@ impl<'a> SliceDecoder<'a> { fn temporal_direct_mvs(dsf: i32, mv_col: [i16; 2]) -> ([i16; 2], [i16; 2]) { const MV_MIN: i32 = -(1 << 15); const MV_MAX: i32 = (1 << 15) - 1; - let scaled = |c: i16| ((dsf * i32::from(c) + 128) >> 8).clamp(MV_MIN, MV_MAX) as i16; - let m0 = [scaled(mv_col[0]), scaled(mv_col[1])]; - ( - m0, - [ - (i32::from(m0[0]) - i32::from(mv_col[0])).clamp(MV_MIN, MV_MAX) as i16, - (i32::from(m0[1]) - i32::from(mv_col[1])).clamp(MV_MIN, MV_MAX) as i16, - ], - ) + // mvL1 comes from the unclamped scaled value, as in the spec; only the results are clamped. + let m0 = mv_col.map(|c| (dsf * i32::from(c) + 128) >> 8); + let clamp = |v: i32| v.clamp(MV_MIN, MV_MAX) as i16; + (m0.map(clamp), [clamp(m0[0] - i32::from(mv_col[0])), clamp(m0[1] - i32::from(mv_col[1]))]) } /// Derive direct-mode motion for the 8x8 blocks in `b8_mask` of the current MB. @@ -1505,6 +1500,10 @@ mod tests { let (m0, m1) = SliceDecoder::temporal_direct_mvs(-1024, [-32768, 32767]); assert_eq!(m0, [32767, -32768]); assert_eq!(m1, [32767, -32768]); + // mvL1 is derived from the unclamped mvL0: (512 * 32767 + 128) >> 8 = 65534, minus 32767. + let (m0, m1) = SliceDecoder::temporal_direct_mvs(512, [32767, 0]); + assert_eq!(m0, [32767, 0]); + assert_eq!(m1, [32767, 0]); // DistScaleFactor 256 (tb == td) reproduces the co-located mv: mvL1 = 0. let (m0, m1) = SliceDecoder::temporal_direct_mvs(256, [40, -4]); assert_eq!(m0, [40, -4]);