From 6c97c00add1ce12c4acbdf53c1a31adc48ce34c5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 24 Jul 2026 23:13:33 +0200 Subject: [PATCH] fix(encode/pyrowave): the RDO block-index cap must cover 4:2:0, not just 4:4:4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vendored rate controller packs the 32x32 block index into the low 16 bits of `RDOperation::block_offset_saving` (pyrowave-sys patches/0002). Past u16::MAX the index collides with the `saving` field, the resolve over-credits, and the emitted payload can overshoot the buffer `pyrowave_encoder_packetize` writes into — whose only bounds check is an `assert` that the Release (NDEBUG) vendored build compiles out. There is no Rust-side guard behind it. All three callers of `pyrowave_mode_fits_rdo` hardcoded 4:4:4, leaving 4:2:0 unchecked — but 4:2:0 overflows too, just later: 8192x6144 is 73728 blocks and 8192x8192 is 98304, while `Codec::PyroWave.max_dimension()` permits 8192 per axis, so both are reachable from a client-requested `mode=WxHxFPS`. Worse, the host's only use of the helper (native/handshake.rs) is a 4:4:4 -> 4:2:0 downgrade, so an oversized mode was actively routed INTO the unguarded branch. The cap now lives in `validate_dimensions`, checked against 4:2:0 — the most permissive chroma, so a mode that cannot fit there fits no PyroWave session at any chroma. That is the single chokepoint both the negotiator (handshake.rs:180) and `open_video_backend` already run. Both encoder opens additionally check the chroma actually being opened: the 4:4:4 half, plus defence in depth for the `PUNKTFUNK_ENCODER=pyrowave` lab override. Not dormant: punktfunk-host has `default = ["pyrowave"]` and no build passes `--no-default-features`, so these backends ship in every host binary. Tests pin the arithmetic (8192x6144 = 73728, 8192x8192 = 98304, 7680x4320 still fits) and assert the cap does not leak to H.265/AV1, which have no such controller. Co-Authored-By: Claude Opus 5 (1M context) --- crates/pf-encode/src/enc/codec.rs | 40 ++++++++++++++++++++ crates/pf-encode/src/enc/linux/pyrowave.rs | 17 ++++++--- crates/pf-encode/src/enc/pyrowave_wire.rs | 11 ++++++ crates/pf-encode/src/enc/windows/pyrowave.rs | 13 ++++--- 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/crates/pf-encode/src/enc/codec.rs b/crates/pf-encode/src/enc/codec.rs index a9093662..520b9b2d 100644 --- a/crates/pf-encode/src/enc/codec.rs +++ b/crates/pf-encode/src/enc/codec.rs @@ -454,6 +454,26 @@ pub fn validate_dimensions(codec: Codec, width: u32, height: u32) -> Result<()> (use HEVC/AV1 above 4096, or lower the client resolution)" ); } + // PyroWave's vendored rate controller packs the 32×32 block index into the low 16 bits of + // `RDOperation::block_offset_saving` (pyrowave-sys `patches/0002-rdo-saving-clamp.patch`). + // Past `u16::MAX` blocks the index collides with the `saving` field, the resolve over-credits, + // and the emitted payload can overshoot the buffer `pyrowave_encoder_packetize` writes into — + // whose only bounds check is an `assert` that the Release (NDEBUG) vendored build compiles out. + // So this is a hard cap, not a quality knob. + // + // Checked against 4:2:0, the *most permissive* chroma: a mode that cannot fit even there can + // fit no PyroWave session at all, so it belongs at this single chokepoint (which both the + // negotiator and `open_video_backend` run) rather than only in the per-backend opens. 4:4:4 + // has twice the block count and is checked again at open, where the real chroma is known — + // and the negotiator's 4:4:4 → 4:2:0 downgrade means an oversized mode arrives at the encoder + // as 4:2:0, which is exactly the case the old open-time guard skipped. + #[cfg(feature = "pyrowave")] + if codec == Codec::PyroWave && !crate::pyrowave_mode_fits_rdo(width, height, false) { + anyhow::bail!( + "invalid PyroWave resolution {width}x{height}: exceeds the rate controller's 16-bit \ + block index (pyrowave-sys patches/0002) — lower the client resolution" + ); + } Ok(()) } @@ -477,6 +497,26 @@ mod tests { assert!(validate_dimensions(Codec::H264, 3840, 4098).is_err()); } + /// PyroWave's hard cap is the rate controller's 16-bit block index, not just + /// `max_dimension()`. Checked at 4:2:0 (the most permissive chroma), because a mode that + /// cannot fit there cannot fit at any chroma — and because the negotiator's 4:4:4 → 4:2:0 + /// downgrade delivers oversized modes to the encoder AS 4:2:0. HEVC/AV1 at the same + /// dimensions must stay unaffected. + #[cfg(feature = "pyrowave")] + #[test] + fn pyrowave_rejects_modes_past_the_rdo_block_index() { + // Fits: 8K 4:2:0 is 49125 blocks. + assert!(validate_dimensions(Codec::PyroWave, 7680, 4320).is_ok()); + // Does not fit at 4:2:0 (73728 / 98304 blocks) — must be refused even though both are + // within `Codec::PyroWave.max_dimension()` (8192). + assert!(validate_dimensions(Codec::PyroWave, 8192, 6144).is_err()); + assert!(validate_dimensions(Codec::PyroWave, 8192, 8192).is_err()); + // The same modes remain legal for the H.26x/AV1 codecs, which have no such rate + // controller — the cap must not leak across codecs. + assert!(validate_dimensions(Codec::H265, 8192, 8192).is_ok()); + assert!(validate_dimensions(Codec::Av1, 8192, 6144).is_ok()); + } + #[test] fn hevc_and_av1_allow_up_to_8192() { for c in [Codec::H265, Codec::Av1] { diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index d2d04169..00e6777f 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -227,12 +227,19 @@ impl PyroWaveEncoder { if !chroma.is_444() && (width % 2 != 0 || height % 2 != 0) { bail!("pyrowave 4:2:0 needs even dimensions (got {width}x{height})"); } - if chroma.is_444() && !crate::pyrowave_mode_fits_rdo(width, height, true) { - // The negotiator downgrades these modes to 4:2:0 pre-Welcome; refuse if one - // slips through (e.g. the lab override) rather than wrap the RDO block index. + // Checked against the chroma actually being opened, NOT hardcoded 4:4:4. The 4:2:0 block + // count is ~half of 4:4:4's but still unbounded (8192×6144 4:2:0 = 73728 > u16::MAX), and + // the negotiator's 4:4:4 → 4:2:0 downgrade hands oversized modes to this open AS 4:2:0 — + // so a `chroma.is_444()`-gated check is skipped exactly when it is needed. Wrapping the + // index lets the resolve over-credit and `packetize` overshoot our bitstream buffer + // (its own bounds `assert` is compiled out by the Release vendored build). + // `validate_dimensions` rejects the impossible-at-any-chroma modes earlier; this is the + // 4:4:4-specific half plus defence in depth for the lab override. + if !crate::pyrowave_mode_fits_rdo(width, height, chroma.is_444()) { bail!( - "pyrowave 4:4:4 at {width}x{height} exceeds the rate controller's 16-bit \ - block index (see pyrowave-sys patches/0002 note) — use 4:2:0 at this size" + "pyrowave {} at {width}x{height} exceeds the rate controller's 16-bit block \ + index (see pyrowave-sys patches/0002 note) — lower the resolution", + if chroma.is_444() { "4:4:4" } else { "4:2:0" } ); } // SAFETY: `open_inner` only issues Vulkan/pyrowave calls whose preconditions it diff --git a/crates/pf-encode/src/enc/pyrowave_wire.rs b/crates/pf-encode/src/enc/pyrowave_wire.rs index ce0bbf09..0c747959 100644 --- a/crates/pf-encode/src/enc/pyrowave_wire.rs +++ b/crates/pf-encode/src/enc/pyrowave_wire.rs @@ -246,6 +246,17 @@ mod tests { assert!(block_count_32x32(3840, 2160, true) <= u16::MAX as u32); assert!(block_count_32x32(7680, 4320, true) > u16::MAX as u32); assert!(block_count_32x32(7680, 4320, false) <= u16::MAX as u32); + // …and 4:2:0 wraps it too, just later — the hole the old 4:4:4-only open guard left. + // `Codec::max_dimension()` allows PyroWave 8192px per axis, so these modes were + // reachable from a client-requested `mode=WxHxFPS`, and the negotiator's 4:4:4 → 4:2:0 + // downgrade routed oversized modes straight into the unguarded branch. + // `validate_dimensions` now rejects them against this 4:2:0 count. + assert_eq!(block_count_32x32(8192, 6144, false), 73728); + assert_eq!(block_count_32x32(8192, 8192, false), 98304); + assert!(block_count_32x32(8192, 6144, false) > u16::MAX as u32); + assert!(block_count_32x32(8192, 8192, false) > u16::MAX as u32); + // The largest 4:2:0 mode that still fits, for the boundary the validator enforces. + assert!(block_count_32x32(7680, 4320, false) <= u16::MAX as u32); } #[test] diff --git a/crates/pf-encode/src/enc/windows/pyrowave.rs b/crates/pf-encode/src/enc/windows/pyrowave.rs index 93001490..249ebacc 100644 --- a/crates/pf-encode/src/enc/windows/pyrowave.rs +++ b/crates/pf-encode/src/enc/windows/pyrowave.rs @@ -189,12 +189,15 @@ impl PyroWaveEncoder { if !chroma444 && (width % 2 != 0 || height % 2 != 0) { bail!("pyrowave 4:2:0 needs even dimensions (got {width}x{height})"); } - if chroma444 && !crate::pyrowave_mode_fits_rdo(width, height, true) { - // The negotiator downgrades these modes to 4:2:0 pre-Welcome; refuse if one - // slips through (e.g. the lab override) rather than wrap the RDO block index. + // Checked against the chroma actually being opened, NOT hardcoded 4:4:4 — see the Linux + // twin (`enc/linux/pyrowave.rs`) for the full rationale: the negotiator's 4:4:4 → 4:2:0 + // downgrade hands oversized modes to this open AS 4:2:0, so the old `chroma444`-gated + // check was skipped exactly when it was needed. + if !crate::pyrowave_mode_fits_rdo(width, height, chroma444) { bail!( - "pyrowave 4:4:4 at {width}x{height} exceeds the rate controller's 16-bit \ - block index (see pyrowave-sys patches/0002 note) — use 4:2:0 at this size" + "pyrowave {} at {width}x{height} exceeds the rate controller's 16-bit block \ + index (see pyrowave-sys patches/0002 note) — lower the resolution", + if chroma444 { "4:4:4" } else { "4:2:0" } ); } let fps = fps.max(1);