From 86f463cf7104ce20871670e088610585e3d90db1 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 11 Jun 2026 23:04:23 +0000 Subject: [PATCH] fix(housekeeping): unaligned read UB + recv-drop parity; dedup mmsghdr; doc fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a bug-hunt + unsafe-audit pass (4 reviewers + adversarial verify). It confirmed ZERO real bugs in the recent batched/paced data-plane work — these are the surfaced cleanups + one genuine soundness fix: - SOUNDNESS (reduce unsafe): inject/gamepad.rs::pump_ff did `ptr::read` of an InputEventRaw (align 8, holds a timeval) out of a 1-aligned [u8; N] buffer — UB per the reference (x86_64 tolerates it, but it can miscompile under LTO). Use ptr::read_unaligned + a SAFETY note. Zero behavior change. - recv parity: recv_batch (recvmmsg) didn't drop an oversized/truncated datagram the way scalar recv does — poll_frame now skips a message whose len fills the buffer (> MAX_DATAGRAM_BYTES), matching recv's `n >= RECV_BUF` drop. (AEAD already rejected these on encrypted sessions; this restores the documented invariant on the batched path.) - dedup unsafe FFI: factor the identical mmsghdr-from-iovec construction out of send_batch + recv_batch into one `mmsghdrs()` helper — the raw-pointer scaffolding + its lifetime SAFETY note now live in one place. - docs: TARGET_SOCKBUF no longer calls paced sending future work (it landed, m3.rs::paced_submit); gamescope.rs input is no longer "(TODO)" (wired + live-validated); the PUNKTFUNK_PERF `wire_mbps` field is renamed `tx_mbps` and noted as attempted/sealed bytes (send_dropped shows what didn't reach the wire). Full suite (35 + loopback round-trip + 6) + clippy + fmt green. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/punktfunk-core/src/session.rs | 6 +++ crates/punktfunk-core/src/transport/udp.rs | 40 ++++++++++--------- crates/punktfunk-host/src/inject/gamepad.rs | 6 ++- crates/punktfunk-host/src/m3.rs | 5 ++- .../punktfunk-host/src/vdisplay/gamescope.rs | 3 +- 5 files changed, 37 insertions(+), 23 deletions(-) diff --git a/crates/punktfunk-core/src/session.rs b/crates/punktfunk-core/src/session.rs index 17c8ee7..3e89313 100644 --- a/crates/punktfunk-core/src/session.rs +++ b/crates/punktfunk-core/src/session.rs @@ -231,6 +231,12 @@ impl Session { let i = self.recv_idx; self.recv_idx += 1; let len = self.recv_lens[i]; + // An oversized datagram fills the whole buffer (recvmmsg truncates + caps msg_len at the + // buffer size) — drop it rather than hand up a truncated, corrupt packet, mirroring the + // scalar `recv`'s `n >= RECV_BUF` check. + if len > MAX_DATAGRAM_BYTES { + continue; + } let pkt = match self.open_from_wire(&self.recv_scratch[i][..len]) { Ok(p) => p, Err(_) => continue, diff --git a/crates/punktfunk-core/src/transport/udp.rs b/crates/punktfunk-core/src/transport/udp.rs index 70cb87a..cb9c4f1 100644 --- a/crates/punktfunk-core/src/transport/udp.rs +++ b/crates/punktfunk-core/src/transport/udp.rs @@ -16,6 +16,23 @@ use std::net::UdpSocket; /// silently truncating it. const RECV_BUF: usize = MAX_DATAGRAM_BYTES + 1; +/// Build one `mmsghdr` per `iovec` (each a single-buffer message) for `sendmmsg`/`recvmmsg`. Shared +/// by `send_batch` + `recv_batch` so the raw-pointer scaffolding lives in exactly one place. +/// +/// SAFETY (caller's): each returned header holds a raw pointer into `iovs`; the caller MUST keep +/// `iovs` alive and unmoved for as long as the headers are passed to the syscall. +#[cfg(target_os = "linux")] +fn mmsghdrs(iovs: &mut [libc::iovec]) -> Vec { + iovs.iter_mut() + .map(|iov| { + let mut h: libc::mmsghdr = unsafe { std::mem::zeroed() }; + h.msg_hdr.msg_iov = iov; + h.msg_hdr.msg_iovlen = 1; + h + }) + .collect() +} + pub struct UdpTransport { socket: UdpSocket, } @@ -31,7 +48,8 @@ impl UdpTransport { /// Sized for 1 Gbps+: at ~1.2 Gbps on the wire an 8 MB buffer is only ~49 ms of steady state, /// and a single multi-MB IDR keyframe (~4 MB ≈ 3300 packets) instantly fills most of it. 32 MB /// gives ~200 ms of headroom and absorbs a keyframe burst without EAGAIN drops. (Paced sending - /// will reduce the buffer actually needed once it lands — see the 1 Gbps roadmap work.) + /// — `m3.rs::paced_submit` — now spreads a big frame's overflow, so this buffer mostly absorbs + /// the immediate microburst rather than a whole unpaced frame.) const TARGET_SOCKBUF: usize = 32 * 1024 * 1024; /// Bind `local` and `connect` to `peer`, so `send`/`recv` need no address and the @@ -109,15 +127,7 @@ impl Transport for UdpTransport { iov_len: p.len(), }) .collect(); - let mut hdrs: Vec = iovs - .iter_mut() - .map(|iov| { - let mut h: libc::mmsghdr = unsafe { std::mem::zeroed() }; - h.msg_hdr.msg_iov = iov; - h.msg_hdr.msg_iovlen = 1; - h - }) - .collect(); + let mut hdrs = mmsghdrs(&mut iovs); let n = unsafe { libc::sendmmsg(fd, hdrs.as_mut_ptr(), hdrs.len() as libc::c_uint, 0) }; if n < 0 { let err = std::io::Error::last_os_error(); @@ -172,15 +182,7 @@ impl Transport for UdpTransport { iov_len: b.len(), }) .collect(); - let mut hdrs: Vec = iovs - .iter_mut() - .map(|iov| { - let mut h: libc::mmsghdr = unsafe { std::mem::zeroed() }; - h.msg_hdr.msg_iov = iov; - h.msg_hdr.msg_iovlen = 1; - h - }) - .collect(); + let mut hdrs = mmsghdrs(&mut iovs); let n = unsafe { libc::recvmmsg( fd, diff --git a/crates/punktfunk-host/src/inject/gamepad.rs b/crates/punktfunk-host/src/inject/gamepad.rs index 49f38cb..0d24fd4 100644 --- a/crates/punktfunk-host/src/inject/gamepad.rs +++ b/crates/punktfunk-host/src/inject/gamepad.rs @@ -357,7 +357,11 @@ impl VirtualPad { if n != buf.len() as isize { break; // EAGAIN / short read — queue drained } - let ev: InputEventRaw = unsafe { std::ptr::read(buf.as_ptr() as *const _) }; + // SAFETY: `buf` is exactly `size_of::()` bytes and fully written by the + // `read` above. `read_unaligned` (not `read`) because the `[u8]` buffer is 1-aligned but + // `InputEventRaw` needs 8 (it holds a `timeval`) — a plain `ptr::read` would be UB. + let ev: InputEventRaw = + unsafe { std::ptr::read_unaligned(buf.as_ptr() as *const InputEventRaw) }; match (ev.type_, ev.code) { (EV_UINPUT, UI_FF_UPLOAD) => { let mut up: UinputFfUpload = unsafe { std::mem::zeroed() }; diff --git a/crates/punktfunk-host/src/m3.rs b/crates/punktfunk-host/src/m3.rs index b0844fd..e88402c 100644 --- a/crates/punktfunk-host/src/m3.rs +++ b/crates/punktfunk-host/src/m3.rs @@ -1578,9 +1578,10 @@ fn virtual_stream( if perf && last_perf.elapsed() >= std::time::Duration::from_secs(2) { let s = session.stats(); let secs = last_perf.elapsed().as_secs_f64(); - let wire_mbps = (s.bytes_sent - last_bytes) as f64 * 8.0 / secs / 1_000_000.0; + // Attempted (sealed) transmit rate; `send_dropped` below is what didn't reach the wire. + let tx_mbps = (s.bytes_sent - last_bytes) as f64 * 8.0 / secs / 1_000_000.0; tracing::info!( - wire_mbps = format!("{wire_mbps:.0}"), + tx_mbps = format!("{tx_mbps:.0}"), frames = sent, send_dropped = s.packets_send_dropped - last_send_dropped, send_dropped_total = s.packets_send_dropped, diff --git a/crates/punktfunk-host/src/vdisplay/gamescope.rs b/crates/punktfunk-host/src/vdisplay/gamescope.rs index f08c013..0c22039 100644 --- a/crates/punktfunk-host/src/vdisplay/gamescope.rs +++ b/crates/punktfunk-host/src/vdisplay/gamescope.rs @@ -11,7 +11,8 @@ //! Requirements: gamescope built with PipeWire + libei input emulation (distro packages are); //! a usable Vulkan device (the NVIDIA render node). Headless capture on the proprietary NVIDIA //! driver is plausible-by-architecture but not a well-trodden path — validate empirically. -//! Input is a gamescope-specific libei/EIS socket (`LIBEI_SOCKET`), wired separately (TODO). +//! Input uses gamescope's own libei/EIS socket (`LIBEI_SOCKET`), relayed to the libei backend (see +//! `inject/libei.rs`) — wired and live-validated. use super::{Mode, VirtualDisplay, VirtualOutput}; use anyhow::{anyhow, Context, Result};