From d499b0c20e85a7ad71f9d893f8cad0562f2688a9 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 11:15:05 -0400 Subject: [PATCH] =?UTF-8?q?vp9:=20never=20crash=20=E2=80=94=20no=20unwrap/?= =?UTF-8?q?expect/panic=20in=20the=20decoder,=20deny=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Transforms, 8-tap filters, probability tables: fixed-size views through `first_chunk(_mut)` instead of slice + `try_into().unwrap()/expect()` (same code on the happy path). - Tile sizes read with `get()`; a short tile header is `Error::Invalid`. - Buffer pool tolerates a poisoned lock. - A failed post-processing thread spawn no longer panics: the job reaches the thread over a channel, so it is still in hand and runs inline. - Without the `threads` feature an unpublished reference band reads as empty instead of panicking. - `#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]` outside tests. Co-Authored-By: Claude Opus 5.5 --- crates/vp9/src/boolcoder.rs | 3 +- crates/vp9/src/decoder.rs | 42 +++++++++++------- crates/vp9/src/frame.rs | 10 +++-- crates/vp9/src/inter.rs | 24 ++++++++--- crates/vp9/src/lib.rs | 1 + crates/vp9/src/probs.rs | 2 +- crates/vp9/src/transform.rs | 85 ++++++++++++++++++++++--------------- 7 files changed, 104 insertions(+), 63 deletions(-) diff --git a/crates/vp9/src/boolcoder.rs b/crates/vp9/src/boolcoder.rs index 8584e5cc6..de56d5c15 100644 --- a/crates/vp9/src/boolcoder.rs +++ b/crates/vp9/src/boolcoder.rs @@ -29,8 +29,7 @@ impl<'a> BoolDecoder<'a> { fn fill(&mut self) { // Room for whole bytes below the top 8 bits and the `count` valid bits. let mut shift = 48 - self.count; - if self.pos + 8 <= self.data.len() { - let bytes: [u8; 8] = self.data[self.pos..self.pos + 8].try_into().expect("8 bytes"); + if let Some(&bytes) = self.data.get(self.pos..).and_then(|d| d.first_chunk::<8>()) { let v = u64::from_be_bytes(bytes); let n = ((shift + 8) / 8) as usize; // bytes that fit let n = n.min(7); diff --git a/crates/vp9/src/decoder.rs b/crates/vp9/src/decoder.rs index cfb97969d..e385d3f56 100644 --- a/crates/vp9/src/decoder.rs +++ b/crates/vp9/src/decoder.rs @@ -331,8 +331,10 @@ impl Decoder { let sz = if last { data.len() - pos } else { - ensure!(pos + 4 <= data.len(), "tile size beyond frame data"); - let s = u32::from_be_bytes(data[pos..pos + 4].try_into().expect("4 bytes")) as usize; + let Some(&b) = data.get(pos..).and_then(|d| d.first_chunk::<4>()) else { + return Err(Error::Invalid("tile size beyond frame data".into())); + }; + let s = u32::from_be_bytes(b) as usize; pos += 4; ensure!(s <= data.len() - pos, "tile size {s} beyond frame data"); s @@ -456,7 +458,7 @@ impl Decoder { /// per band for the samples they read), else right here. fn run_post(&mut self, job: PostJob) { #[cfg(all(feature = "threads", not(target_arch = "wasm32")))] - if self.frame_threads() { + let job = if self.frame_threads() { // Bound the post stages in flight (each is one thread). while self.jobs.front().is_some_and(|j| j.is_finished()) { let _ = self.jobs.pop_front().map(|j| j.join()); @@ -467,26 +469,34 @@ impl Decoder { } let shown = job.out.is_some(); let (tx, rx) = std::sync::mpsc::channel(); + // The job reaches the thread over a channel, so it is still here if no thread can + // be spawned and then runs inline below. + let (job_tx, job_rx) = std::sync::mpsc::channel::(); let spawned = std::thread::Builder::new().name("vp9-post".into()).spawn(move || { - if let Some(p) = job.run() { + if let Ok(job) = job_rx.recv() + && let Some(p) = job.run() + { let _ = tx.send(p); } }); match spawned { - Ok(handle) => { - self.jobs.push_back(handle); - if shown { - self.pending.push_back(Pending::Job(rx)); + Ok(handle) => match job_tx.send(job) { + Ok(()) => { + self.jobs.push_back(handle); + if shown { + self.pending.push_back(Pending::Job(rx)); + } + return; } - return; - } - Err(e) => { - // No thread available: the job (moved into the failed closure) is gone, - // which cannot happen in practice; report as a decoding failure. - panic!("cannot spawn VP9 post-processing thread: {e}"); - } + // The thread is gone already: take the job back and run it here. + Err(std::sync::mpsc::SendError(back)) => back, + }, + // No thread available: run it here. + Err(_) => job, } - } + } else { + job + }; if let Some(p) = job.run() { self.pending.push_back(Pending::Ready(p)); } diff --git a/crates/vp9/src/frame.rs b/crates/vp9/src/frame.rs index f8f539c07..aaf881cb7 100644 --- a/crates/vp9/src/frame.rs +++ b/crates/vp9/src/frame.rs @@ -154,7 +154,9 @@ impl Frame { } #[cfg(not(feature = "threads"))] { - self.bands[r].get().expect("reference band decoded before use") + // Bands are decoded in order before use; an unpublished one reads as empty. + static EMPTY: Band = Band { planes: [Vec::new(), Vec::new(), Vec::new()] }; + self.bands[r].get().unwrap_or(&EMPTY) } } @@ -246,7 +248,7 @@ impl Pool { /// `fill` only initialises fresh memory). pub fn take_any(&self, len: usize, fill: T) -> Vec { let found = { - let mut b = self.bufs.lock().expect("buffer pool"); + let mut b = self.bufs.lock().unwrap_or_else(std::sync::PoisonError::into_inner); b.iter().position(|v| v.len() >= len).map(|i| b.swap_remove(i)) }; match found { @@ -261,7 +263,7 @@ impl Pool { /// A buffer of `len` copies of `fill` (recycled memory when available). pub fn take(&self, len: usize, fill: T) -> Vec { let found = { - let mut b = self.bufs.lock().expect("buffer pool"); + let mut b = self.bufs.lock().unwrap_or_else(std::sync::PoisonError::into_inner); b.iter().position(|v| v.capacity() >= len).map(|i| b.swap_remove(i)) }; match found { @@ -278,7 +280,7 @@ impl Pool { if v.capacity() == 0 { return; } - let mut b = self.bufs.lock().expect("buffer pool"); + let mut b = self.bufs.lock().unwrap_or_else(std::sync::PoisonError::into_inner); if b.len() < self.max { b.push(v); } diff --git a/crates/vp9/src/inter.rs b/crates/vp9/src/inter.rs index 10a00339b..ce9e21a44 100644 --- a/crates/vp9/src/inter.rs +++ b/crates/vp9/src/inter.rs @@ -141,12 +141,16 @@ fn filter_h(s: &[u16], f: &[i16], max: i32, o: &mut [u16]) { #[inline(always)] fn filter_h_n(s: &[u16], f: &[i16], max: i32, o: &mut [u16]) { - let o: &mut [u16; W] = (&mut o[..W]).try_into().expect("row"); + let Some(o) = o.first_chunk_mut::() else { + return; + }; if max == 255 { let mut acc = [64u16; W]; for t in 0..8 { let c = f[t] as u16; - let p: &[u16; W] = s[t..t + W].try_into().expect("row"); + let Some(p) = s.get(t..).and_then(|s| s.first_chunk::()) else { + return; + }; for j in 0..W { acc[j] = acc[j].wrapping_add(c.wrapping_mul(p[j])); } @@ -157,7 +161,9 @@ fn filter_h_n(s: &[u16], f: &[i16], max: i32, o: &mut [u16]) { let mut acc = [64i32; W]; for t in 0..8 { let c = f[t] as i32; - let p: &[u16; W] = s[t..t + W].try_into().expect("row"); + let Some(p) = s.get(t..).and_then(|s| s.first_chunk::()) else { + return; + }; for j in 0..W { acc[j] += c * p[j] as i32; } @@ -199,12 +205,16 @@ fn filter_v(s: &[u16], ss: usize, f: &[i16], max: i32, o: &mut [u16]) { #[inline(always)] fn filter_v_n(s: &[u16], ss: usize, f: &[i16], max: i32, o: &mut [u16]) { - let o: &mut [u16; W] = (&mut o[..W]).try_into().expect("row"); + let Some(o) = o.first_chunk_mut::() else { + return; + }; if max == 255 { let mut acc = [64u16; W]; for t in 0..8 { let c = f[t] as u16; - let p: &[u16; W] = s[t * ss..t * ss + W].try_into().expect("row"); + let Some(p) = s.get(t * ss..).and_then(|s| s.first_chunk::()) else { + return; + }; for j in 0..W { acc[j] = acc[j].wrapping_add(c.wrapping_mul(p[j])); } @@ -215,7 +225,9 @@ fn filter_v_n(s: &[u16], ss: usize, f: &[i16], max: i32, o: &mut let mut acc = [64i32; W]; for t in 0..8 { let c = f[t] as i32; - let p: &[u16; W] = s[t * ss..t * ss + W].try_into().expect("row"); + let Some(p) = s.get(t * ss..).and_then(|s| s.first_chunk::()) else { + return; + }; for j in 0..W { acc[j] += c * p[j] as i32; } diff --git a/crates/vp9/src/lib.rs b/crates/vp9/src/lib.rs index 4525dc6fd..f280da808 100644 --- a/crates/vp9/src/lib.rs +++ b/crates/vp9/src/lib.rs @@ -18,6 +18,7 @@ //! ``` #![allow(clippy::needless_range_loop, clippy::too_many_arguments)] +#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable))] mod boolcoder; mod decoder; diff --git a/crates/vp9/src/probs.rs b/crates/vp9/src/probs.rs index 6ed83a30d..5ac3a3941 100644 --- a/crates/vp9/src/probs.rs +++ b/crates/vp9/src/probs.rs @@ -34,7 +34,7 @@ pub struct FrameContext { } fn arr(s: &[u8]) -> [u8; N] { - s.try_into().expect("table size") + s.first_chunk::().copied().unwrap_or([128; N]) } impl Default for FrameContext { diff --git a/crates/vp9/src/transform.rs b/crates/vp9/src/transform.rs index 7a4a2bd70..a9d60c19c 100644 --- a/crates/vp9/src/transform.rs +++ b/crates/vp9/src/transform.rs @@ -57,32 +57,39 @@ fn iwht4(x: [i32; 4], shift: u32) -> [i32; 4] { fn tx1d_narrow(buf: &mut [i32], n: usize, adst: bool) { match (n, adst) { (4, false) => { - let r = narrow::idct4(buf[..4].try_into().unwrap()); - buf[..4].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<4>() { + *a = narrow::idct4(*a); + } } (4, true) => { - let r = iadst4_narrow(buf[..4].try_into().unwrap()); - buf[..4].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<4>() { + *a = iadst4_narrow(*a); + } } (8, false) => { - let r = narrow::idct8(buf[..8].try_into().unwrap()); - buf[..8].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<8>() { + *a = narrow::idct8(*a); + } } (8, true) => { - let r = narrow::iadst8(buf[..8].try_into().unwrap()); - buf[..8].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<8>() { + *a = narrow::iadst8(*a); + } } (16, false) => { - let r = narrow::idct16(buf[..16].try_into().unwrap()); - buf[..16].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<16>() { + *a = narrow::idct16(*a); + } } (16, true) => { - let r = narrow::iadst16(buf[..16].try_into().unwrap()); - buf[..16].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<16>() { + *a = narrow::iadst16(*a); + } } _ => { - let r = narrow::idct32(buf[..32].try_into().unwrap()); - buf[..32].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<32>() { + *a = narrow::idct32(*a); + } } } } @@ -91,32 +98,39 @@ fn tx1d_narrow(buf: &mut [i32], n: usize, adst: bool) { fn tx1d_wide(buf: &mut [i64], n: usize, adst: bool) { match (n, adst) { (4, false) => { - let r = wide::idct4(buf[..4].try_into().unwrap()); - buf[..4].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<4>() { + *a = wide::idct4(*a); + } } (4, true) => { - let r = iadst4_wide(buf[..4].try_into().unwrap()); - buf[..4].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<4>() { + *a = iadst4_wide(*a); + } } (8, false) => { - let r = wide::idct8(buf[..8].try_into().unwrap()); - buf[..8].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<8>() { + *a = wide::idct8(*a); + } } (8, true) => { - let r = wide::iadst8(buf[..8].try_into().unwrap()); - buf[..8].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<8>() { + *a = wide::iadst8(*a); + } } (16, false) => { - let r = wide::idct16(buf[..16].try_into().unwrap()); - buf[..16].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<16>() { + *a = wide::idct16(*a); + } } (16, true) => { - let r = wide::iadst16(buf[..16].try_into().unwrap()); - buf[..16].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<16>() { + *a = wide::iadst16(*a); + } } _ => { - let r = wide::idct32(buf[..32].try_into().unwrap()); - buf[..32].copy_from_slice(&r); + if let Some(a) = buf.first_chunk_mut::<32>() { + *a = wide::idct32(*a); + } } } } @@ -137,7 +151,8 @@ pub fn inverse_transform_add(coefs: &[i32], tx_size: u8, tx_type: u8, lossless: if lossless { let mut t = [0i32; 16]; for i in 0..4 { - let r = iwht4(coefs[i * 4..i * 4 + 4].try_into().unwrap(), 2); + let c = coefs.get(i * 4..).and_then(|c| c.first_chunk::<4>()).copied().unwrap_or_default(); + let r = iwht4(c, 2); t[i * 4..i * 4 + 4].copy_from_slice(&r); } for j in 0..4 { @@ -205,12 +220,12 @@ fn itx_narrow(coefs: &[i32], p: &TxParams, dst: &mut [u16], stri // Column pass across 4 / 8 columns at once (the same butterflies on lane vectors), then // the rounding, residual add and clipping per row of lanes. if N == 4 { - let x: [Cols<4>; N] = std::array::from_fn(|i| Cols(t[i][..4].try_into().expect("4 columns"))); + let x: [Cols<4>; N] = std::array::from_fn(|i| Cols(t[i].first_chunk::<4>().copied().unwrap_or_default())); let y = col_tx4(x, p.col_adst); add_cols(&y, 0, p, dst, stride); } else { for c0 in (0..N).step_by(8) { - let x: [Cols<8>; N] = std::array::from_fn(|i| Cols(t[i][c0..c0 + 8].try_into().expect("8 columns"))); + let x: [Cols<8>; N] = std::array::from_fn(|i| Cols(t[i].get(c0..).and_then(|r| r.first_chunk::<8>()).copied().unwrap_or_default())); let y = col_tx8(x, p.col_adst); add_cols(&y, c0, p, dst, stride); } @@ -221,7 +236,9 @@ fn itx_narrow(coefs: &[i32], p: &TxParams, dst: &mut [u16], stri #[inline(always)] fn add_cols(y: &[Cols; N], c0: usize, p: &TxParams, dst: &mut [u16], stride: usize) { for (i, v) in y.iter().enumerate() { - let d: &mut [u16; W] = (&mut dst[i * stride + c0..i * stride + c0 + W]).try_into().expect("row"); + let Some(d) = dst.get_mut(i * stride + c0..).and_then(|d| d.first_chunk_mut::()) else { + return; + }; for j in 0..W { let r = (v.0[j].wrapping_add(p.round) >> p.shift).clamp(-(1 << 20), 1 << 20); d[j] = (d[j] as i32 + r).max(0).min(p.max) as u16; @@ -295,8 +312,8 @@ fn col_tx8(x: [Cols<8>; N], adst: bool) -> [Cols<8>; N] { /// A slice of known length as an array. #[inline(always)] -fn fixed(x: &[T]) -> [T; M] { - x[..M].try_into().expect("transform size") +fn fixed, const M: usize>(x: &[T]) -> [T; M] { + x.first_chunk::().copied().unwrap_or([T::from(0); M]) } /// ADST4 (8.7.1.6) on 4 columns.