From 5317c583ab5d5cd52119c9658d97ff144ddc4a5e Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 27 Aug 2026 18:06:31 +0200 Subject: [PATCH 1/2] fix(bitstream/av1): an over-declared OBU is a parse error, not a decode-thread panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `obu_size` is a leb128 read straight out of the stream — bounded only by `u32::MAX`, and tied to nothing about how many bytes are actually present. `read_obu` then built the OBU with an unchecked `&data[start_offset..start_offset + obu_size]`, so any access unit whose last OBU declared more payload than remained panicked with `range end index .. out of range for slice of length ..`. That is a bounds check rather than arithmetic, so it panics in release too, and it aborts whichever thread is decoding. It reaches every native AV1 rung: pf-vkdecode, pf-dxvadec and pf-vaadec are all re-exports of `pf_bitstream::av1::Av1Planner`, whose `plan_au` hands raw access-unit bytes straight to this function. `PUNKTFUNK_AU_FAULT=truncate` produces the shape, and so does any AU delivered short over the wire. This was a hole in an otherwise consistent posture, not a missing idea: `plan_au` degrades every other malformation to `TruncatedAu`/`Parse`, and pf-vkdecode already re-validates `obu.end > au.len()` a layer up. Bound it once, where all three rungs route through. The checked end is reused for `bytes_used`, so the slice and the advance can no longer disagree. Both tests fail without the guard — the parser one reproduces the original panic verbatim (`range end index 10600 out of range for slice of length 5293`). Recorded as PROVENANCE.md deviation 14. Not filed upstream. --- crates/pf-bitstream/src/av1.rs | 52 ++++++++++++++++ .../vendor/cros-codecs/PROVENANCE.md | 28 +++++++++ .../cros-codecs/src/codec/av1/parser.rs | 62 ++++++++++++++++++- 3 files changed, 139 insertions(+), 3 deletions(-) diff --git a/crates/pf-bitstream/src/av1.rs b/crates/pf-bitstream/src/av1.rs index 7dee0185..7e51fda2 100644 --- a/crates/pf-bitstream/src/av1.rs +++ b/crates/pf-bitstream/src/av1.rs @@ -1187,4 +1187,56 @@ mod tests { Some(PlanError::NoFrame) ); } + + /// A truncated access unit never panics the decode thread. + /// + /// `plan_au` degrades every malformation it knows about to [`PlanWarning::TruncatedAu`] + /// or [`PlanError`], and `pf-vkdecode` re-validates OBU ranges on top — but the AV1 + /// `obu_size` bound lives in the vendored parser, and until PROVENANCE.md deviation 14 + /// it was missing: an AU cut mid-OBU leaves a final OBU declaring more payload than + /// remains, and the unchecked slice aborted the calling thread. That reaches all three + /// native rungs, which re-export this planner, and is exactly the shape + /// `PUNKTFUNK_AU_FAULT=truncate` injects. + /// + /// The contract asserted here is the crate's stated posture, not a specific verdict: + /// a short AU is a plan error or a warning, and whatever plans do come back stay + /// inside the bytes handed in. + #[test] + fn a_truncated_access_unit_is_a_plan_error_not_a_panic() { + let mut planned = 0usize; + let mut rejected = 0usize; + + for packet in IvfIterator::new(AV1_25FPS).take(12) { + for denom in [2usize, 3, 4, 8] { + let cut = packet.len() - packet.len() / denom; + // A fresh planner per cut: the claim is that a short unit fails cleanly on + // its own terms, not that a planner carries state across one. + let mut planner = Av1Planner::new(); + match planner.plan_au(&packet[..cut]) { + Ok(plans) => { + planned += 1; + for plan in &plans { + for tile in &plan.tiles { + assert!( + tile.data.start <= tile.data.end && tile.data.end <= cut, + "tile range {:?} escapes a {cut}-byte truncated unit", + tile.data + ); + } + } + } + Err(_) => rejected += 1, + } + } + } + + assert!( + planned + rejected == 48, + "every cut must reach a verdict; got {planned} planned + {rejected} rejected" + ); + assert!( + rejected > 0, + "no truncated unit was rejected - the test proves nothing" + ); + } } diff --git a/crates/pf-bitstream/vendor/cros-codecs/PROVENANCE.md b/crates/pf-bitstream/vendor/cros-codecs/PROVENANCE.md index 16719b98..81d18c80 100644 --- a/crates/pf-bitstream/vendor/cros-codecs/PROVENANCE.md +++ b/crates/pf-bitstream/vendor/cros-codecs/PROVENANCE.md @@ -208,5 +208,33 @@ in the future." (`a_picture_whose_macroblock_count_overflows_is_a_parse_error_not_a_panic`). **Report upstream — not yet filed.** +14. `src/codec/av1/parser.rs` — `read_obu`: bound `obu_size` against the buffer before it is + used to slice. `obu_size` is a leb128 read out of the stream (`read_leb128()? as usize`, + so anything up to `u32::MAX`) and nothing ties it to the bytes actually present; the OBU + was then built with an unchecked `&data[start_offset..start_offset + obu_size]`. Any + access unit whose last OBU declares more payload than remains — a truncated AU, or simply + an over-declared size — panicked with `range end index .. out of range for slice of length + ..`. That is a bounds check, not arithmetic, so it panics in release too (the workspace + leaves `overflow-checks` off, which is why the parser's other unchecked accumulations + merely wrap), and it aborts whichever thread is decoding. + + Blast radius is every native AV1 rung: `pf-vkdecode`, `pf-dxvadec` and `pf-vaadec` are all + re-exports of `pf_bitstream::av1::Av1Planner`, whose `plan_au` hands raw access-unit bytes + straight to this function. Reachable from the project's own `PUNKTFUNK_AU_FAULT=truncate` + injector — whose `FaultMode::Truncate` docs reason only about Annex-B, where a NALU carries + no length, while AV1 OBUs do — and from any AU delivered short over the wire. + + This was a gap in an otherwise consistent posture rather than a missing idea: `plan_au` + degrades every *other* malformation to `PlanWarning::TruncatedAu` or `PlanError::Parse`, + and `pf-vkdecode` re-validates `obu.end > au.len()` one layer up. Guarded with + `checked_add` plus a length compare, returning the same `String` error the rest of the + parser uses; the computed end is reused for `bytes_used` so the slice and the advance can + no longer disagree. Regression-tested in the file's own test module + (`an_obu_declaring_more_bytes_than_are_present_is_a_parse_error_not_a_panic`, which + reproduces the original panic exactly when the guard is reverted) and at the planner + boundary in `pf-bitstream` + (`av1::tests::a_truncated_access_unit_is_a_plan_error_not_a_panic`). + **Not filed upstream.** + Re-sync procedure: fetch the AOSP tree, re-apply this trim, diff `codec/` + `bitstream_utils.rs` (expect near-zero conflicts), update the commit pin above. diff --git a/crates/pf-bitstream/vendor/cros-codecs/src/codec/av1/parser.rs b/crates/pf-bitstream/vendor/cros-codecs/src/codec/av1/parser.rs index e99e4744..062aa28a 100644 --- a/crates/pf-bitstream/vendor/cros-codecs/src/codec/av1/parser.rs +++ b/crates/pf-bitstream/vendor/cros-codecs/src/codec/av1/parser.rs @@ -1850,10 +1850,25 @@ impl Parser { assert!(reader.0.position() % 8 == 0); let start_offset: usize = (reader.0.position() / 8).try_into().unwrap(); + // `obu_size` was read off the wire as a leb128 and is bounded only by `u32::MAX`; nothing + // ties it to how many bytes are actually present. Bound it against the buffer BEFORE it is + // used to slice, or a truncated (or simply over-declared) OBU panics with `range end index + // .. out of range` — an abort of whatever thread is decoding. See PROVENANCE.md deviation 14. + let obu_end = start_offset + .checked_add(obu_size) + .ok_or::("obu_size overflows the access unit offset".into())?; + if obu_end > data.len() { + return Err(format!( + "obu_size {} overruns the access unit: {} bytes present after the OBU header", + obu_size, + data.len().saturating_sub(start_offset) + )); + } + log::debug!( "Identified OBU type {:?}, data size: {}, obu_size: {}", header.obu_type, - start_offset + obu_size, + obu_end, obu_size ); @@ -1872,8 +1887,8 @@ impl Parser { Ok(ObuAction::Process(Obu { header, - data: Cow::from(&data[start_offset..start_offset + obu_size]), - bytes_used: start_offset + obu_size, + data: Cow::from(&data[start_offset..obu_end]), + bytes_used: obu_end, })) } @@ -4334,4 +4349,45 @@ mod tests { .unwrap_err(); assert!(err.starts_with("Invalid tile_rows"), "{err}"); } + + /// An OBU whose declared `obu_size` runs past the bytes present is a parse error, + /// not a panic (PROVENANCE.md deviation 14). + /// + /// `obu_size` is a leb128 read straight out of the stream and bounded only by + /// `u32::MAX`; nothing ties it to the length of the buffer handed in. Cutting a real + /// access unit mid-OBU therefore leaves a final OBU declaring more payload than + /// remains, and the unchecked slice used to abort the calling thread with + /// `range end index .. out of range for slice of length ..`. + #[test] + fn an_obu_declaring_more_bytes_than_are_present_is_a_parse_error_not_a_panic() { + let mut overruns = 0usize; + + for packet in IvfIterator::new(STREAM_TEST_25_FPS).take(8) { + // Three cuts per unit so the walk is guaranteed to land inside an OBU + // rather than exactly on a boundary. + for denom in [2usize, 3, 4] { + let cut = packet.len() - packet.len() / denom; + let mut parser = Parser::default(); + let mut consumed = 0usize; + + while consumed < cut { + match parser.read_obu(&packet[..cut][consumed..]) { + Ok(ObuAction::Process(obu)) => consumed += obu.bytes_used, + Ok(ObuAction::Drop(n)) => consumed += usize::try_from(n).unwrap(), + Err(e) => { + if e.contains("overruns the access unit") { + overruns += 1; + } + break; + } + } + } + } + } + + assert!( + overruns > 0, + "no cut reached the obu_size bound - the test proves nothing" + ); + } } -- 2.54.0 From 86fbd8121bcafd04c28d5958f3a162d3156f0cf5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 27 Aug 2026 18:30:07 +0200 Subject: [PATCH 2/2] fix(host/input): a pad's OS identity is host-wide, not the client's wire index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every OS-level name a virtual pad needs is derived from a pad index and nothing else: the `Global\pfxusb-boot-` / `Global\pfds-boot-` bootstrap mailboxes, the `SwDeviceCreate` instance ids (`pf_xusb_`, `pf_pad_`, `pf_ds4_`, `pf_xbox_`), and on Linux the DualSense pairing MAC, the Deck serial and the Switch MAC — the last three documented as needing to be unique per pad, because hid-playstation adopts the MAC as the HID `uniq` and SDL/Steam dedup by that serial. The host serves up to DEFAULT_MAX_CONCURRENT sessions of the same desktop, each with its own input thread and its own router, and every client numbers its first controller wire pad 0. So two paired clients each holding a controller collide on all of them. On Windows the second session's `Shm::create_named` sees ERROR_ALREADY_EXISTS on all five retries and never gets a pad for the whole session — and the create-failure hint tells the operator to restart the service, which would kill both sessions, when no other process is involved at all. On Linux nothing errors: both mint the same DualSense MAC and SDL merges the two pads into one controller. The wire index is a session's own numbering and cannot be an OS identity. New `pf_inject::pad_pool` makes the OS slot host-wide — claimed on a pad's first present frame, released when it goes away, freed wholesale when the session drops — and `Pads` translates once on the way in. Because only the NUMBER changes and not the name format, the drivers (which read the index back out of `pszDeviceLocation`) need no change. Slots are claimed lazily rather than as fixed per-session windows, so a single session still reaches all MAX_PADS pads; two sessions share the range. An exhausted host now declines with an honest line instead of retrying against a name it can never win. Feedback reverses the same map: a backend tags rumble and rich HID output with the OS slot it created the device under, so `Pads::pump` maps it back to the client's wire index — otherwise the fix would have delivered one client's rumble to another's pad. `HidOutput::pad`/`with_pad` keep that translation in one place, so a seventh variant cannot silently forget it. Verified: pf-inject 40 tests (8 new, covering the collision, single-session reach, release, drop, exhaustion, mask and reverse-map), punktfunk-core 502 with `--features quic`, and `cargo check -p punktfunk-host` clean on linux/amd64 in punktfunk-rust-ci — punktfunk-host does not build on macOS, so that check is the only compile signal for this file and it was not skipped. --- crates/pf-inject/src/inject/pad_pool.rs | 326 +++++++++++++++++++++ crates/pf-inject/src/lib.rs | 13 + crates/punktfunk-core/src/quic/datagram.rs | 115 ++++++++ crates/punktfunk-host/src/native/input.rs | 103 ++++++- 4 files changed, 556 insertions(+), 1 deletion(-) create mode 100644 crates/pf-inject/src/inject/pad_pool.rs diff --git a/crates/pf-inject/src/inject/pad_pool.rs b/crates/pf-inject/src/inject/pad_pool.rs new file mode 100644 index 00000000..af44325c --- /dev/null +++ b/crates/pf-inject/src/inject/pad_pool.rs @@ -0,0 +1,326 @@ +//! Host-wide allocation of the OS-level virtual-pad slots ([`PadSlotPool`]), and the per-session +//! wire-index → slot mapping built on it ([`PadSlotMap`]). +//! +//! # Why this exists +//! +//! Every OS-level name a virtual pad needs is derived from a pad index and nothing else: +//! +//! - the bootstrap mailboxes `Global\pfxusb-boot-` and `Global\pfds-boot-` +//! (`pf_driver_proto::gamepad::xusb_boot_name` / `pf_driver_proto::gamepad::pad_boot_name`); +//! - the `SwDeviceCreate` instance ids — `pf_xusb_`, `pf_pad_`, `pf_ds4_`, `pf_xbox_`; +//! - on Linux the DualSense pairing MAC, the Steam Deck serial and the Switch Pro MAC — the last +//! three *explicitly required to be unique per pad*, because `hid-playstation` adopts the MAC as +//! the HID `uniq` and SDL/Steam dedup controllers by that serial. +//! +//! The host serves several sessions at once (`native::DEFAULT_MAX_CONCURRENT`), each with its own +//! input thread and its own pad router, and **every client numbers its first controller wire pad +//! 0**. Those two facts together mean two clients each holding a controller collide on every name +//! above. On Windows the second session's `Shm::create_named` sees `ERROR_ALREADY_EXISTS` for all +//! five retries and reports [`crate::pad_slots::PadCreateFault::IndexOwnedElsewhere`] — whose +//! remedy tells the operator to restart the service, which would kill both sessions, and no other +//! process is even involved. On Linux there is no error at all: both sessions mint a DualSense +//! with the same pairing MAC, `hid-playstation` writes it into `HID_UNIQ` for both, and SDL/Steam +//! merge the two pads into one controller. +//! +//! # The fix +//! +//! Stop treating the wire index as an OS identity. A session's wire indices are its own business; +//! the **OS slot is host-wide**, claimed on a pad's first frame and released when that pad goes +//! away. One translation, performed once in the session's pad router, makes every name above +//! unique — and because the *format* of those names is unchanged, the drivers (which read the +//! index back out of `pszDeviceLocation`) need no change at all. +//! +//! Slots are claimed lazily rather than handed out as fixed per-session windows, so the common +//! single-session case still reaches all [`MAX_PADS`] pads; two sessions simply share the range +//! between them. +//! +//! # Why a process-global pool +//! +//! The names being protected are process-wide (`Global\…` kernel objects, PnP instance ids), so +//! the thing that arbitrates them is process-wide too — there is no configuration in which two +//! pools within one host would be correct. A global also keeps the fix off every session +//! signature between here and the accept loop. Collisions with a *separate* live process are a +//! different problem and remain [`crate::pad_slots::PadCreateFault::IndexOwnedElsewhere`]'s. + +use punktfunk_core::input::MAX_PADS; +use std::sync::Mutex; + +/// The set of OS pad slots currently spoken for, host-wide. +#[derive(Debug)] +pub struct PadSlotPool { + /// Bit `i` set = OS slot `i` is claimed. `MAX_PADS <= 16` is asserted in [`crate::pad_slots`]. + taken: Mutex, +} + +impl Default for PadSlotPool { + fn default() -> Self { + Self::new() + } +} + +impl PadSlotPool { + pub const fn new() -> Self { + Self { + taken: Mutex::new(0), + } + } + + /// Claim the lowest free OS slot, or `None` when the host already holds [`MAX_PADS`] pads. + /// + /// Lowest-free rather than round-robin so a single-session host keeps the slot numbering it + /// has always had — pad 0 is slot 0 — and so a field log reads the same as it used to. + pub fn claim(&self) -> Option { + let mut taken = self.lock(); + (0..MAX_PADS).find_map(|i| { + (*taken & (1 << i) == 0).then(|| { + *taken |= 1 << i; + i as u8 + }) + }) + } + + /// Hand `slot` back. Releasing a slot that was never claimed is a no-op, so a double release + /// on a teardown path cannot free somebody else's pad. + pub fn release(&self, slot: u8) { + if (slot as usize) < MAX_PADS { + *self.lock() &= !(1u16 << slot); + } + } + + /// A poisoned pool must not wedge every future pad on the host: one session panicking while + /// holding the lock says nothing about whether the *bitmap* is usable, and it is — a `u16` + /// has no torn state. + fn lock(&self) -> std::sync::MutexGuard<'_, u16> { + self.taken.lock().unwrap_or_else(|e| e.into_inner()) + } + + #[cfg(test)] + fn taken_mask(&self) -> u16 { + *self.lock() + } +} + +/// The process-wide pool — see the module docs for why this is a global. +pub fn global() -> &'static PadSlotPool { + static POOL: PadSlotPool = PadSlotPool::new(); + &POOL +} + +/// One session's wire-index → OS-slot mapping, drawn from a [`PadSlotPool`]. +/// +/// Dropping releases every slot the session still holds, so a session that ends abruptly — a +/// panicking input thread included — cannot strand an OS name for the life of the host. +#[derive(Debug)] +pub struct PadSlotMap<'a> { + pool: &'a PadSlotPool, + slot: [Option; MAX_PADS], +} + +impl PadSlotMap<'static> { + /// A mapping against the process-wide pool — what a real session uses. + pub fn new() -> Self { + Self::with_pool(global()) + } +} + +impl Default for PadSlotMap<'static> { + fn default() -> Self { + Self::new() + } +} + +impl<'a> PadSlotMap<'a> { + /// A mapping against a caller-supplied pool. Exists so the allocation policy is testable + /// without touching host-wide state. + pub fn with_pool(pool: &'a PadSlotPool) -> Self { + Self { + pool, + slot: [None; MAX_PADS], + } + } + + /// This session's OS slot for `wire`, claiming one on first use. + /// + /// `None` means the wire index is out of range, or the host is already holding [`MAX_PADS`] + /// pads across all its sessions — in which case no device may be created for it. + pub fn claim_for(&mut self, wire: usize) -> Option { + if wire >= MAX_PADS { + return None; + } + if let Some(slot) = self.slot[wire] { + return Some(slot); + } + let slot = self.pool.claim()?; + self.slot[wire] = Some(slot); + Some(slot) + } + + /// This session's OS slot for `wire` **without** claiming one. + pub fn slot_of(&self, wire: usize) -> Option { + self.slot.get(wire).copied().flatten() + } + + /// The wire index this session has mapped to `slot` — the reverse direction, needed because + /// every backend reports feedback (rumble, rich HID output) tagged with the OS index it was + /// created under, while the client only knows its own wire numbering. + pub fn wire_of(&self, slot: u8) -> Option { + self.slot.iter().position(|s| *s == Some(slot)) + } + + /// Release `wire`'s slot back to the pool, if it holds one. + pub fn release(&mut self, wire: usize) { + if let Some(slot) = self.slot.get_mut(wire).and_then(Option::take) { + self.pool.release(slot); + } + } + + /// Translate a wire-space active mask into OS-slot space. + /// + /// The managers' unplug sweep walks this mask against the slots they actually created, so it + /// has to speak the same numbering the devices were created under. A wire bit with no slot + /// contributes nothing — it names a pad this session never got a device for. + pub fn os_mask(&self, wire_mask: u16) -> u16 { + (0..MAX_PADS) + .filter(|w| wire_mask & (1 << w) != 0) + .filter_map(|w| self.slot[w]) + .fold(0u16, |m, slot| m | (1u16 << slot)) + } +} + +impl Drop for PadSlotMap<'_> { + fn drop(&mut self) { + for wire in 0..MAX_PADS { + self.release(wire); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The defect this module exists for: two sessions, each numbering its first pad 0, must not + /// land on the same OS slot — every pad name on both platforms is derived from that number. + #[test] + fn two_sessions_numbering_their_first_pad_zero_get_different_os_slots() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + let mut b = PadSlotMap::with_pool(&pool); + + assert_eq!(a.claim_for(0), Some(0)); + assert_eq!(b.claim_for(0), Some(1), "session B must not reuse slot 0"); + assert_eq!(a.claim_for(1), Some(2)); + assert_eq!(b.claim_for(1), Some(3)); + + // And the claim is stable: asking again is not a second allocation. + assert_eq!(a.claim_for(0), Some(0)); + assert_eq!(pool.taken_mask(), 0b1111); + } + + /// A single session still reaches every pad — the fix must not cost the common case. + #[test] + fn one_session_still_reaches_every_pad() { + let pool = PadSlotPool::new(); + let mut only = PadSlotMap::with_pool(&pool); + for wire in 0..MAX_PADS { + assert_eq!(only.claim_for(wire), Some(wire as u8), "wire {wire}"); + } + assert_eq!(pool.taken_mask(), u16::MAX); + } + + #[test] + fn a_released_slot_goes_back_to_the_pool() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + let mut b = PadSlotMap::with_pool(&pool); + + assert_eq!(a.claim_for(0), Some(0)); + assert_eq!(b.claim_for(0), Some(1)); + a.release(0); + assert_eq!(a.slot_of(0), None); + // The freed slot is the lowest one now, so the next claim takes it. + assert_eq!(b.claim_for(1), Some(0)); + + // Releasing twice must not free a slot somebody else now holds. + a.release(0); + assert_eq!(pool.taken_mask(), 0b11); + } + + /// A session that ends — abruptly included — strands nothing. + #[test] + fn dropping_a_session_returns_every_slot_it_held() { + let pool = PadSlotPool::new(); + { + let mut s = PadSlotMap::with_pool(&pool); + s.claim_for(0); + s.claim_for(3); + s.claim_for(7); + assert_eq!(pool.taken_mask(), 0b111); + } + assert_eq!( + pool.taken_mask(), + 0, + "a dropped session must free its slots" + ); + } + + /// An exhausted host refuses honestly instead of handing out a colliding slot. + #[test] + fn an_exhausted_pool_refuses_rather_than_colliding() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + for wire in 0..MAX_PADS { + assert!(a.claim_for(wire).is_some()); + } + let mut b = PadSlotMap::with_pool(&pool); + assert_eq!(b.claim_for(0), None, "no slot left, and none may be shared"); + } + + #[test] + fn an_out_of_range_wire_index_claims_nothing() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + assert_eq!(a.claim_for(MAX_PADS), None); + assert_eq!(a.claim_for(usize::MAX), None); + assert_eq!( + pool.taken_mask(), + 0, + "a rejected index must not consume a slot" + ); + } + + /// The sweep mask has to speak the numbering the devices were created under. + #[test] + fn the_active_mask_is_translated_into_slot_space() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + let mut b = PadSlotMap::with_pool(&pool); + a.claim_for(0); // slot 0 + b.claim_for(0); // slot 1 + b.claim_for(1); // slot 2 + + // B holds wire 0 and 1; in slot space that is bits 1 and 2, never bit 0 (A's pad). + assert_eq!(b.os_mask(0b11), 0b110); + // A wire bit with no device contributes nothing. + assert_eq!(a.os_mask(0b11), 0b1); + assert_eq!(a.os_mask(0), 0); + } + + /// Feedback comes back tagged with the OS slot; it has to reach the right wire pad. + #[test] + fn feedback_maps_back_to_the_wire_pad_that_owns_it() { + let pool = PadSlotPool::new(); + let mut a = PadSlotMap::with_pool(&pool); + let mut b = PadSlotMap::with_pool(&pool); + a.claim_for(0); + b.claim_for(0); + + assert_eq!(a.wire_of(0), Some(0)); + assert_eq!( + a.wire_of(1), + None, + "B's pad must not resolve to a wire pad of A's - that is rumble on the wrong client" + ); + assert_eq!(b.wire_of(1), Some(0)); + } +} diff --git a/crates/pf-inject/src/lib.rs b/crates/pf-inject/src/lib.rs index 9fd58dde..1e1f3b98 100644 --- a/crates/pf-inject/src/lib.rs +++ b/crates/pf-inject/src/lib.rs @@ -603,6 +603,19 @@ pub mod mouse_windows; /// run on a developer machine at all. See [`pad_slots`]. #[path = "inject/pad_gate.rs"] pub mod pad_gate; +/// Host-wide allocation of the OS-level pad slots ([`pad_pool::PadSlotPool`]) and the per-session +/// wire-index → slot mapping over it ([`pad_pool::PadSlotMap`]). +/// +/// Every OS name a virtual pad needs — the `Global\pf…-boot-` mailboxes, the `SwDeviceCreate` +/// instance ids, the DualSense pairing MAC, the Deck serial, the Switch MAC — is derived from a +/// pad index, while every client numbers its first controller wire pad 0 and the host serves +/// several sessions at once. This is what keeps those two facts from colliding. +/// +/// Built on every target, like [`pad_gate`] and [`pad_slots`]: it is a bitmap and an array, it +/// touches no OS pad API, and the collision it prevents is one only a multi-session host box can +/// demonstrate — so the policy either has tests that run everywhere, or none that anyone runs. +#[path = "inject/pad_pool.rs"] +pub mod pad_pool; /// Shared virtual-pad slot table + creation lifecycle ([`pad_slots::PadSlots`]) — the /// `Vec>` table, `active_mask` unplug sweep, and gate-checked create every backend /// manager used to copy-paste (G12). diff --git a/crates/punktfunk-core/src/quic/datagram.rs b/crates/punktfunk-core/src/quic/datagram.rs index 8c67faa7..fbde2b29 100644 --- a/crates/punktfunk-core/src/quic/datagram.rs +++ b/crates/punktfunk-core/src/quic/datagram.rs @@ -586,6 +586,72 @@ pub enum HidOutput { } impl HidOutput { + /// The pad this output is addressed to. + /// + /// `u16` because [`HidOutput::AudioCtl`] carries one; every other variant's `pad` is a `u8` + /// on the wire and widens losslessly. + pub fn pad(&self) -> u16 { + match self { + HidOutput::Led { pad, .. } + | HidOutput::PlayerLeds { pad, .. } + | HidOutput::Trigger { pad, .. } + | HidOutput::TrackpadHaptic { pad, .. } + | HidOutput::HidRaw { pad, .. } => u16::from(*pad), + HidOutput::AudioCtl { pad, .. } => *pad, + } + } + + /// The same output, re-addressed to `pad`. + /// + /// The host needs this because a virtual pad's OS identity is host-wide while each session + /// numbers its own pads from zero (`pf_inject::pad_pool`): a backend reports feedback tagged + /// with the OS slot it created the device under, and that has to become the client's own wire + /// index before it goes out. Doing it here rather than at the call site keeps the variant list + /// in one place — a seventh variant that forgot to translate would be a silent mis-address. + /// + /// Pad indices are bounded by `input::MAX_PADS` (16), well inside the `u8` the narrow variants + /// carry; the assert pins that rather than trusting it. + pub fn with_pad(self, pad: u16) -> Self { + debug_assert!( + pad <= u16::from(u8::MAX), + "pad index {pad} does not fit the wire's u8 variants" + ); + let narrow = pad as u8; + match self { + HidOutput::Led { r, g, b, .. } => HidOutput::Led { + pad: narrow, + r, + g, + b, + }, + HidOutput::PlayerLeds { bits, .. } => HidOutput::PlayerLeds { pad: narrow, bits }, + HidOutput::Trigger { which, effect, .. } => HidOutput::Trigger { + pad: narrow, + which, + effect, + }, + HidOutput::TrackpadHaptic { + side, + amplitude, + period, + count, + .. + } => HidOutput::TrackpadHaptic { + pad: narrow, + side, + amplitude, + period, + count, + }, + HidOutput::HidRaw { kind, data, .. } => HidOutput::HidRaw { + pad: narrow, + kind, + data, + }, + HidOutput::AudioCtl { flags, raw, .. } => HidOutput::AudioCtl { pad, flags, raw }, + } + } + pub fn encode(&self) -> Vec { let mut out = vec![HIDOUT_MAGIC]; match self { @@ -1772,4 +1838,53 @@ mod tests { assert_eq!(decode_cursor_state_datagram(&bad), None); } } + + /// Every variant must re-address, because the host translates a backend's OS pad slot into + /// the client's wire index on the way out (`pf_inject::pad_pool`). A variant that ignored + /// `with_pad` would deliver another pad's rumble to this one, silently. + #[test] + fn with_pad_re_addresses_every_hid_output_variant() { + let every = [ + HidOutput::Led { + pad: 0, + r: 1, + g: 2, + b: 3, + }, + HidOutput::PlayerLeds { + pad: 0, + bits: 0b101, + }, + HidOutput::Trigger { + pad: 0, + which: 1, + effect: vec![1, 2, 3], + }, + HidOutput::TrackpadHaptic { + pad: 0, + side: 1, + amplitude: 7, + period: 8, + count: 9, + }, + HidOutput::HidRaw { + pad: 0, + kind: HID_RAW_OUTPUT, + data: vec![0xAA, 0xBB], + }, + HidOutput::AudioCtl { + pad: 0, + flags: 0b1, + raw: [1, 2, 3, 4, 5, 6], + }, + ]; + + for ev in every { + let before = ev.clone(); + let moved = ev.with_pad(9); + assert_eq!(moved.pad(), 9, "{before:?} did not re-address"); + // Re-addressing changes the pad and nothing else: putting it back is the identity. + assert_eq!(moved.with_pad(0), before, "{before:?} lost a field"); + } + } } diff --git a/crates/punktfunk-host/src/native/input.rs b/crates/punktfunk-host/src/native/input.rs index caca778e..5691a1cf 100644 --- a/crates/punktfunk-host/src/native/input.rs +++ b/crates/punktfunk-host/src/native/input.rs @@ -95,6 +95,18 @@ const MAX_WIRE_PADS: usize = punktfunk_core::input::MAX_PADS; /// [`resolve_pad_kind`] folds any kind a platform can't build into one it can, so this never /// constructs a manager the build lacks. struct Pads { + /// This session's wire index → host-wide OS pad slot ([`crate::inject::pad_pool`]). + /// + /// The number handed to a backend BECOMES the pad's OS identity — the `Global\pf…-boot-` + /// mailbox, the `SwDeviceCreate` instance id, the DualSense pairing MAC, the Deck serial, the + /// Switch MAC. Every client numbers its own first controller wire pad 0 and the host serves + /// several sessions at once, so the wire index cannot be that identity: two clients each + /// holding a controller would collide on all of them. Slots are claimed on a pad's first + /// present frame and released when it goes away, so this is also what stops a session from + /// stranding an OS name after it ends. + slots: crate::inject::pad_pool::PadSlotMap<'static>, + /// One warn per session when the host has no OS slot left — not one per frame. + slots_exhausted_warned: bool, /// Declared (and host-resolved) kind per pad index; `default` until a `GamepadArrival` lands. kinds: [GamepadPref; MAX_WIRE_PADS], /// The kind of the manager that currently OWNS a built device at each index (`None` = no @@ -156,6 +168,8 @@ impl Pads { "gamepad backends: per-pad router (session default)" ); Pads { + slots: crate::inject::pad_pool::PadSlotMap::new(), + slots_exhausted_warned: false, kinds: [default; MAX_WIRE_PADS], owner: [None; MAX_WIRE_PADS], xbox360: None, @@ -227,9 +241,74 @@ impl Pads { if idx >= MAX_WIRE_PADS { return; } + // Wire index → host-wide OS slot, the ONE place the translation happens. Claim on a + // present frame; a removal can only concern a slot this session already holds, and asking + // for one there would mint a device-shaped name for a pad that is going away. + let slot = if present { + self.slots.claim_for(idx) + } else { + self.slots.slot_of(idx) + }; + let Some(slot) = slot else { + if present && !self.slots_exhausted_warned { + self.slots_exhausted_warned = true; + tracing::warn!( + pad = idx, + max = MAX_WIRE_PADS, + "no host pad slot left — every OS slot is held by a live session, so this pad \ + gets no device. It appears when a slot frees (another session ending, or one \ + of its pads unplugging)." + ); + } + return; + }; let (kind, new_owner) = route_decision(self.owner[idx], self.kinds[idx], present); self.owner[idx] = new_owner; - self.route_handle(kind, ev); + self.route_handle(kind, &self.re_index(ev, slot)); + if !present { + // The removal has reached the backend, so the OS name is the next session's to take. + // + // The device itself lingers for `pad_slots::SWEEP_GRACE` (300 ms) before the devnode + // actually goes, so a session claiming this slot inside that window can still lose the + // create race — on Windows that is one `IndexOwnedElsewhere` and the existing backoff, + // which heals on its own once the grace expires. Holding the slot until the sweep + // fired would trade that transient for a permanent leak on any session that unplugs a + // pad it never re-plugs, which is the worse of the two. + self.slots.release(idx); + } + } + + /// Re-address an event from this session's wire numbering into the host-wide slot numbering + /// the backends create their devices under. + /// + /// The `active_mask` is translated too, and has to be: every manager's unplug sweep walks that + /// mask against the slots it actually created, so a wire-space mask would sweep away another + /// session's pad — or spare one this session had dropped. + fn re_index( + &self, + ev: &punktfunk_core::input::GamepadEvent, + slot: u8, + ) -> punktfunk_core::input::GamepadEvent { + use punktfunk_core::input::GamepadEvent; + match ev { + GamepadEvent::State(f) => { + let mut f = *f; + f.index = i16::from(slot); + f.active_mask = self.slots.os_mask(f.active_mask); + GamepadEvent::State(f) + } + GamepadEvent::Arrival { + kind, + capabilities, + audio_caps, + .. + } => GamepadEvent::Arrival { + index: slot, + kind: *kind, + capabilities: *capabilities, + audio_caps: *audio_caps, + }, + } } /// Dispatch a decoded event to the manager for `kind`, creating it lazily. @@ -475,6 +554,28 @@ impl Pads { mut rumble: impl FnMut(u16, u16, u16, u16, u16), mut hidout: impl FnMut(punktfunk_core::quic::HidOutput), ) { + // The reverse of `re_index`. A backend tags its feedback with the OS slot it created the + // device under; the client only ever knew its own wire index, so rumble and rich HID + // output have to come back the other way or they land on the wrong pad — and, with two + // sessions live, on the wrong client's pad. + // + // Snapshotted into a plain array first because the callbacks below are handed to + // `&mut self.`; borrowing `self.slots` inside them would not compile. + let mut wire_of = [None; MAX_WIRE_PADS]; + for (slot, wire) in wire_of.iter_mut().enumerate() { + *wire = self.slots.wire_of(slot as u8); + } + // A slot with no wire index is not this session's pad; dropping its feedback is the point. + let mut rumble = |pad: u16, low, high, lt, rt| { + if let Some(wire) = wire_of.get(pad as usize).copied().flatten() { + rumble(wire as u16, low, high, lt, rt); + } + }; + let mut hidout = |h: punktfunk_core::quic::HidOutput| { + if let Some(wire) = wire_of.get(h.pad() as usize).copied().flatten() { + hidout(h.with_pad(wire as u16)); + } + }; if let Some(m) = &mut self.xbox360 { m.pump_rumble(&mut rumble); // the X-Box pad has no rich-feedback plane } -- 2.54.0