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" + ); + } } 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 }