aleleba/soundcraft
1
0
Fork 0
mirror of https://github.com/storytold/soundcraft.git synced 2026-10-24 02:26:46 -06:00
Code Issues Packages Projects Releases Wiki Activity

Video: clamp H.264 IDCT inputs and temporal-direct mvs to the spec ranges

Browse Source 
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 <noreply@z.ai>
This commit is contained in:
SusieeTheFemboy
2026-10-23 23:49:14 +02:00
co-authored by GLM-5.3-Flash
parent 6b84a4f046
commit 4b54d72c23
2 changed files with 94 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/video/src/h264/slicedec.rs
+51 -7
View File
@@ -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]);
}
}
crates/video/src/h264/transform.rs
+43 -5
View File
@@ -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).