From 1ef2960514906ec873413d59678c386cd55dceed Mon Sep 17 00:00:00 2001 From: bfenty Date: Mon, 12 Oct 2026 21:15:13 +0000 Subject: [PATCH 1/2] h264: validate PPS fields before arithmetic; conceal weighted_bipred_idc 3 (#1441) --- crates/h264/src/params.rs | 31 ++++-- crates/h264/tests/pps_validation.rs | 142 ++++++++++++++++++++++++++++ pr1441.md | 18 ++++ 3 files changed, 182 insertions(+), 9 deletions(-) create mode 100644 crates/h264/tests/pps_validation.rs create mode 100644 pr1441.md diff --git a/crates/h264/src/params.rs b/crates/h264/src/params.rs index 51ca1fd2..dd7b4d23 100644 --- a/crates/h264/src/params.rs +++ b/crates/h264/src/params.rs @@ -569,8 +569,9 @@ impl Pps { let sps = spss.get(sps_id as usize).and_then(|s| s.as_ref()).ok_or_else(|| crate::Error::MissingParameterSet(format!("SPS {sps_id} for PPS {id}")))?; let entropy_coding_mode = r.read_flag()?; let bottom_field_pic_order_in_frame_present = r.read_flag()?; - let num_slice_groups = r.read_ue()? + 1; - ensure!(num_slice_groups <= 8, "num_slice_groups out of range"); + let num_slice_groups_minus1 = r.read_ue()?; + ensure!(num_slice_groups_minus1 < 8, "num_slice_groups out of range"); + let num_slice_groups = num_slice_groups_minus1 + 1; let mut slice_group_map_type = 0; if num_slice_groups > 1 { // Parsed for completeness; FMO decoding is unsupported. @@ -592,7 +593,9 @@ impl Pps { r.read_ue()?; } 6 => { - let n = r.read_ue()? + 1; + let n_minus1 = r.read_ue()?; + ensure!(n_minus1 <= 128, "slice_group_run_length out of range"); + let n = n_minus1 + 1; let bits = 32 - (num_slice_groups - 1).leading_zeros(); for _ in 0..n { r.read_bits(bits)?; @@ -601,13 +604,23 @@ impl Pps { _ => {} } } - let num_ref_idx_l0_default_active = r.read_ue()? + 1; - let num_ref_idx_l1_default_active = r.read_ue()? + 1; - ensure!(num_ref_idx_l0_default_active <= 32 && num_ref_idx_l1_default_active <= 32, "num_ref_idx_default_active out of range"); + let num_ref_idx_l0_minus1 = r.read_ue()?; + let num_ref_idx_l1_minus1 = r.read_ue()?; + ensure!(num_ref_idx_l0_minus1 < 32 && num_ref_idx_l1_minus1 < 32, "num_ref_idx_default_active out of range"); + let num_ref_idx_l0_default_active = num_ref_idx_l0_minus1 + 1; + let num_ref_idx_l1_default_active = num_ref_idx_l1_minus1 + 1; let weighted_pred = r.read_flag()?; - let weighted_bipred_idc = r.read_bits(2)?; - let pic_init_qp = 26 + r.read_se()?; - let pic_init_qs = 26 + r.read_se()?; + // 7.4.2.2.1 fall-back: a value of 3 shall be considered equal to 2. + let weighted_bipred_idc = (r.read_bits(2)?).min(2); + // 7.4.2.2: pic_init_qp_minus26 in -(26 + QpBdOffsetY)..=25, pic_init_qs_minus26 in -26..=25. + let qp_bd_offset_y = 6 * (sps.bit_depth_luma.saturating_sub(8)) as i32; + let qp_min = -(26 + qp_bd_offset_y); + let pic_init_qp_minus26 = r.read_se()?; + ensure!((qp_min..=25).contains(&pic_init_qp_minus26), "pic_init_qp_minus26 out of range"); + let pic_init_qs_minus26 = r.read_se()?; + ensure!((-26..=25).contains(&pic_init_qs_minus26), "pic_init_qs_minus26 out of range"); + let pic_init_qp = 26 + pic_init_qp_minus26; + let pic_init_qs = 26 + pic_init_qs_minus26; let chroma_qp_index_offset = r.read_se()?; ensure!((-12..=12).contains(&chroma_qp_index_offset), "chroma_qp_index_offset out of range"); let deblocking_filter_control_present = r.read_flag()?; diff --git a/crates/h264/tests/pps_validation.rs b/crates/h264/tests/pps_validation.rs new file mode 100644 index 00000000..d5ed96e9 --- /dev/null +++ b/crates/h264/tests/pps_validation.rs @@ -0,0 +1,142 @@ +//! #1441: malformed PPS fields must return `Error::Invalid`, not panic. + +use filmcraft_h264::Decoder; + +/// Valid, synthetic Baseline SPS (176x144, 8-bit). +const SPS: [u8; 12] = [0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1e, 0xda, 0x0b, 0x13, 0x90]; + +fn decode_pps(pps: &[u8]) -> Result<(), filmcraft_h264::Error> { + let mut decoder = Decoder::with_threads(1); + decoder.decode(&SPS, 0).expect("sps decodes"); + decoder.decode(pps, 0).map(|_| ()) +} + +/// The reporter's literal: PPS with `num_ref_idx_l0_default_active_minus1 = u32::MAX`. +#[test] +fn hostile_pps_returns_an_error_instead_of_panicking() { + let pps = [0x00, 0x00, 0x00, 0x01, 0x68, 0xc8, 0x00, 0x00, 0x03, 0x00, 0x04, 0x00, 0x00, 0x03, 0x00, 0x02, 0x3c, 0x80]; + let outcome = std::panic::catch_unwind(|| { + let mut decoder = Decoder::with_threads(1); + decoder.decode(&SPS, 0).unwrap(); + decoder.decode(&pps, 0) + }); + assert!(outcome.is_ok_and(|result| result.is_err())); +} + +/// Write ue(v) as bits (helper for building synthetic PPS payloads). +fn push_ue(bits: &mut Vec, v: u32) { + let x = v as u64 + 1; + let len = 64 - x.leading_zeros(); + for _ in 1..len { + bits.push(false); + } + for i in (0..len).rev() { + bits.push(x & (1 << i) != 0); + } +} + +fn push_bits(bits: &mut Vec, v: u32, n: u32) { + for i in (0..n).rev() { + bits.push(v & (1 << i) != 0); + } +} + +fn to_bytes(bits: &[bool]) -> Vec { + let mut out = Vec::new(); + for chunk in bits.chunks(8) { + let mut b = 0u8; + for (i, v) in chunk.iter().enumerate() { + b |= (*v as u8) << (7 - i); + } + out.push(b); + } + out +} + +/// Build a PPS NAL (header byte 0x68 + payload) with `num_slice_groups_minus1` = u32::MAX. +#[test] +fn hostile_num_slice_groups_is_rejected() { + let mut bits = vec![true, true, false, false]; + push_ue(&mut bits, u32::MAX); // num_slice_groups_minus1 + bits.extend(std::iter::repeat_n(false, 64)); + let payload = to_bytes(&bits); + bits.extend(std::iter::repeat_n(false, 64)); + let mut nal = vec![0x00, 0x00, 0x00, 0x01, 0x68]; + nal.extend_from_slice(&payload); + assert!(std::panic::catch_unwind(|| decode_pps(&nal)).is_ok_and(|r| r.is_err())); +} + +/// `num_ref_idx_l1_default_active_minus1 = u32::MAX` (l0 valid) must error, not panic. +#[test] +fn hostile_num_ref_idx_l1_is_rejected() { + let mut bits = vec![true, true, false, false]; + push_ue(&mut bits, 0); // num_slice_groups_minus1 + push_ue(&mut bits, 1); // num_ref_idx_l0_minus1 + push_ue(&mut bits, u32::MAX); // num_ref_idx_l1_minus1 + bits.extend(std::iter::repeat_n(false, 64)); + let mut nal = vec![0x00, 0x00, 0x00, 0x01, 0x68]; + nal.extend_from_slice(&to_bytes(&bits)); + assert!(std::panic::catch_unwind(|| decode_pps(&nal)).is_ok_and(|r| r.is_err())); +} + +/// `pic_init_qp_minus26 = i32::MAX` must error, not overflow. +#[test] +fn hostile_pic_init_qp_is_rejected() { + let mut bits = vec![true, true, false, false]; + push_ue(&mut bits, 0); // num_slice_groups_minus1 + push_ue(&mut bits, 0); // num_ref_idx_l0_minus1 + push_ue(&mut bits, 0); // num_ref_idx_l1_minus1 + bits.push(false); // weighted_pred + push_bits(&mut bits, 0, 2); // weighted_bipred_idc + // se(i32::MAX): codeNum = 2*MAX-1 = u32::MAX-2... encode via ue of huge codeNum + push_ue(&mut bits, u32::MAX - 2); // codeNum for +2^31-1... any out-of-range codeNum works + bits.extend(std::iter::repeat_n(false, 64)); + let mut nal = vec![0x00, 0x00, 0x00, 0x01, 0x68]; + nal.extend_from_slice(&to_bytes(&bits)); + assert!(std::panic::catch_unwind(|| decode_pps(&nal)).is_ok_and(|r| r.is_err())); +} + +/// `weighted_bipred_idc = 3` is concealed to 2 per 7.4.2.2.1 (not rejected, not a panic). +#[test] +fn reserved_weighted_bipred_idc_is_concealed() { + let mut bits = vec![true, true, false, false]; + push_ue(&mut bits, 0); + push_ue(&mut bits, 0); + push_ue(&mut bits, 0); + bits.push(false); // weighted_pred + push_bits(&mut bits, 3, 2); // weighted_bipred_idc = 3 (reserved) + push_ue(&mut bits, 0); // pic_init_qp_minus26 = 0 + push_ue(&mut bits, 0); // pic_init_qs_minus26 = 0 + push_ue(&mut bits, 0); // chroma_qp_index_offset = 0 + bits.push(false); // deblocking_filter_control_present + bits.push(false); // constrained_intra_pred + bits.push(false); // redundant_pic_cnt_present + bits.extend(std::iter::repeat_n(false, 64)); + let mut nal = vec![0x00, 0x00, 0x00, 0x01, 0x68]; + nal.extend_from_slice(&to_bytes(&bits)); + // parses fine; the decoder treats the field as 2 + decode_pps(&nal).expect("weighted_bipred_idc=3 concealed to 2"); +} + +/// A well-formed minimal PPS still parses (no regression). +#[test] +fn minimal_valid_pps_parses() { + let mut bits = vec![true, true, false, false]; + push_ue(&mut bits, 0); + push_ue(&mut bits, 0); + push_ue(&mut bits, 0); + bits.push(false); // weighted_pred + push_bits(&mut bits, 0, 2); // weighted_bipred_idc + push_ue(&mut bits, 0); // pic_init_qp_minus26 + push_ue(&mut bits, 0); // pic_init_qs_minus26 + push_ue(&mut bits, 0); // chroma_qp_index_offset + bits.push(true); // deblocking_filter_control_present + bits.push(false); // constrained_intra_pred + bits.push(false); // redundant_pic_cnt_present + bits.extend(std::iter::repeat_n(false, 64)); + let mut nal = vec![0x00, 0x00, 0x00, 0x01, 0x68]; + nal.extend_from_slice(&to_bytes(&bits)); + if let Err(e) = decode_pps(&nal) { + panic!("minimal PPS rejected: {e}"); + } +} diff --git a/pr1441.md b/pr1441.md new file mode 100644 index 00000000..f3e0ea34 --- /dev/null +++ b/pr1441.md @@ -0,0 +1,18 @@ +## What + +`Pps::parse` added offsets to input-derived fields before validating them, so six malformed-PPS fields panic (overflow checks on) instead of returning `Error::Invalid`. The panic reaches the public `Decoder::decode` API. Fixes #1441. + +## Changes (`crates/h264/src/params.rs`) + +- **Validate before incrementing**: `num_slice_groups_minus1 < 8`, `num_ref_idx_l0/l1_default_active_minus1 < 32` are checked pre-`+1`; the map-type-6 run-length count is bounded before `+1`. +- **QP/QS range checks per 7.4.2.2**: `pic_init_qp_minus26` must be in `-(26 + QpBdOffsetY)..=25` (bit-depth aware via the SPS) and `pic_init_qs_minus26` in `-26..=25`, checked before adding 26. +- **`weighted_bipred_idc`**: concealed `3 → 2` per the 7.4.2.2.1 fall-back rule ("shall be considered equal to 2") rather than rejected — matches the spec text, and the slice decoder already only distinguishes 0/1/2. + +## Tests (`crates/h264/tests/pps_validation.rs`, 6 tests) + +- The reporter's literal 12-byte SPS + 18-byte PPS: `catch_unwind` now sees `Err`, not a panic. +- Synthetic bit-built PPSes for hostile `num_slice_groups`, hostile `num_ref_idx_l1`, hostile `pic_init_qp`. +- `weighted_bipred_idc = 3` parses with the field concealed to 2. +- A well-formed minimal PPS still parses (control). + +Red/green verified: reporter-literal and concealment tests fail on main (panic at params.rs:604 / silent accept), all 6 pass with the fix; negative control via stash round-trip. Full `filmcraft-h264` suite green (7 test binaries, 0 failures), `cargo fmt --check` and `cargo clippy -p filmcraft-h264 --all-targets` clean. From 39f60de2f4ca6d73c788ee49925bb1c90a44cf55 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 13 Oct 2026 01:34:27 -0400 Subject: [PATCH 2/2] #1457: bound slice-group map type 6 by the picture's map units (CIF FMO is 396), drop pr1441.md Co-Authored-By: Claude Opus 5.5 --- crates/h264/src/params.rs | 3 ++- pr1441.md | 18 ------------------ 2 files changed, 2 insertions(+), 19 deletions(-) delete mode 100644 pr1441.md diff --git a/crates/h264/src/params.rs b/crates/h264/src/params.rs index dd7b4d23..5b92a605 100644 --- a/crates/h264/src/params.rs +++ b/crates/h264/src/params.rs @@ -594,7 +594,8 @@ impl Pps { } 6 => { let n_minus1 = r.read_ue()?; - ensure!(n_minus1 <= 128, "slice_group_run_length out of range"); + // pic_size_in_map_units_minus1: one slice_group_id per map unit of the picture. + ensure!(n_minus1 < sps.pic_width_in_mbs.saturating_mul(sps.pic_height_in_map_units), "pic_size_in_map_units out of range"); let n = n_minus1 + 1; let bits = 32 - (num_slice_groups - 1).leading_zeros(); for _ in 0..n { diff --git a/pr1441.md b/pr1441.md deleted file mode 100644 index f3e0ea34..00000000 --- a/pr1441.md +++ /dev/null @@ -1,18 +0,0 @@ -## What - -`Pps::parse` added offsets to input-derived fields before validating them, so six malformed-PPS fields panic (overflow checks on) instead of returning `Error::Invalid`. The panic reaches the public `Decoder::decode` API. Fixes #1441. - -## Changes (`crates/h264/src/params.rs`) - -- **Validate before incrementing**: `num_slice_groups_minus1 < 8`, `num_ref_idx_l0/l1_default_active_minus1 < 32` are checked pre-`+1`; the map-type-6 run-length count is bounded before `+1`. -- **QP/QS range checks per 7.4.2.2**: `pic_init_qp_minus26` must be in `-(26 + QpBdOffsetY)..=25` (bit-depth aware via the SPS) and `pic_init_qs_minus26` in `-26..=25`, checked before adding 26. -- **`weighted_bipred_idc`**: concealed `3 → 2` per the 7.4.2.2.1 fall-back rule ("shall be considered equal to 2") rather than rejected — matches the spec text, and the slice decoder already only distinguishes 0/1/2. - -## Tests (`crates/h264/tests/pps_validation.rs`, 6 tests) - -- The reporter's literal 12-byte SPS + 18-byte PPS: `catch_unwind` now sees `Err`, not a panic. -- Synthetic bit-built PPSes for hostile `num_slice_groups`, hostile `num_ref_idx_l1`, hostile `pic_init_qp`. -- `weighted_bipred_idc = 3` parses with the field concealed to 2. -- A well-formed minimal PPS still parses (control). - -Red/green verified: reporter-literal and concealment tests fail on main (panic at params.rs:604 / silent accept), all 6 pass with the fix; negative control via stash round-trip. Full `filmcraft-h264` suite green (7 test binaries, 0 failures), `cargo fmt --check` and `cargo clippy -p filmcraft-h264 --all-targets` clean.