From aa070f2d550d19c88ab102df4abd64e4e4662f48 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 29 Jul 2026 01:31:41 +0200 Subject: [PATCH] feat(ffi): hand-mirrored C structs are now layout-checked at compile time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sharpest memory-safety risk left in this codebase is not an `unsafe` block — it is a hand-written `#[repr(C)]` mirror of an external C struct. Get a field offset wrong and nothing fails to compile and nothing reliably crashes: the library reads a pointer, a length or a pitch out of the wrong bytes. Eleven such structs across five files had NO check at all. Guarded here, each next to the struct it protects: * `AVCUDADeviceContext`, and `AVD3D11VADeviceContext`/`AVD3D11VAFramesContext`. `ffmpeg-sys-next` binds none of them, so these mirrors are the only definitions — and we WRITE through them (`cuda_ctx`, `device`, `bind_flags`). ⚠ The D3D11VA pair is duplicated VERBATIM in two crates (pf-encode's `ffmpeg_win.rs`, pf-client-core's `video_d3d11.rs`) because neither can depend on the other; they must agree with libav and with each other, and now a drift in either is a build error. * The six cuda.h structs. Three were already asserted — but only in `#[cfg(test)]`, so the check ran when someone ran the tests and never in a release build. They are `const` now. The other three, including `CUDA_MEMCPY2D` which is filled on EVERY zero-copy frame, had nothing. * `MsghdrX`, Darwin's `msghdr_x`, which `libc` does not expose. Its layout is not reviewable by eye: the 32-bit fields force padding before each following pointer, so `msg_iov` sits at 16 and not 12. `sendmsg_x`/`recvmsg_x` take the pointer and length from it. * `IPolicyConfigVtbl` — the sharpest of the set. It mirrors an UNDOCUMENTED COM interface, and `set_default_endpoint` is called by SLOT INDEX through a ten-entry `_reserved` gap that carries no names to anchor a review. A field added or resized above it does not break the build; it calls a different function pointer through a mismatched signature. Every assertion is `const _: () = assert!(..)`, so it holds on every build including release and cannot be skipped. The compiler verified the numbers — the sizes and offsets asserted here are the ones the target actually produces, on each platform that compiles the struct. Verified: Linux .21 fmt + both CI clippy steps rc=0 (CUDA + libav CUDA mirrors); Windows .47 full CI clippy set rc=0 + pf-capture tests (D3D11VA pair, COM vtable); macOS `cargo check -p punktfunk-core` (MsghdrX — the only platform that compiles it). --- crates/pf-client-core/src/video_d3d11.rs | 26 +++++++++ crates/pf-encode/src/enc/linux/mod.rs | 13 +++++ .../pf-encode/src/enc/windows/ffmpeg_win.rs | 26 +++++++++ crates/pf-zerocopy/src/imp/cuda.rs | 30 ++--------- crates/pf-zerocopy/src/imp/cuda/ffi.rs | 53 +++++++++++++++++++ .../punktfunk-core/src/transport/udp/apple.rs | 18 +++++++ .../src/audio/windows/audio_control.rs | 20 +++++++ 7 files changed, 159 insertions(+), 27 deletions(-) diff --git a/crates/pf-client-core/src/video_d3d11.rs b/crates/pf-client-core/src/video_d3d11.rs index 6609c22f..45f975be 100644 --- a/crates/pf-client-core/src/video_d3d11.rs +++ b/crates/pf-client-core/src/video_d3d11.rs @@ -149,6 +149,32 @@ struct AVD3D11VAFramesContext { texture_infos: *mut c_void, // AVD3D11FrameDescriptor* (FFmpeg-managed) } +// Hand-written mirrors of libav's `AVD3D11VADeviceContext` / `AVD3D11VAFramesContext` +// (hwcontext_d3d11va.h) — `ffmpeg-sys-next` binds neither, and we WRITE `device` / `bind_flags` +// through them, so a wrong offset is silent corruption of libav's context rather than a compile +// error. ⚠ These two structs are duplicated in the other crate that talks to the same libav +// contexts (pf-encode's `ffmpeg_win.rs` and pf-client-core's `video_d3d11.rs`); they must agree +// with libav AND with each other, and these assertions are what makes a drift in either a build +// failure instead of a runtime mystery. +const _: () = { + use std::mem::{offset_of, size_of}; + type P = *mut c_void; + assert!(size_of::() == 7 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, device) == 0); + assert!(offset_of!(AVD3D11VADeviceContext, device_context) == size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, video_device) == 2 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, video_context) == 3 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, lock) == 4 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, unlock) == 5 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, lock_ctx) == 6 * size_of::

()); + // ptr, u32, u32, ptr — the two 32-bit flags pack into one pointer-sized slot with no padding. + assert!(size_of::() == 3 * size_of::

()); + assert!(offset_of!(AVD3D11VAFramesContext, texture) == 0); + assert!(offset_of!(AVD3D11VAFramesContext, bind_flags) == size_of::

()); + assert!(offset_of!(AVD3D11VAFramesContext, misc_flags) == size_of::

() + 4); + assert!(offset_of!(AVD3D11VAFramesContext, texture_infos) == 2 * size_of::

()); +}; + fn averr(what: &str, code: i32) -> anyhow::Error { anyhow!("{what}: {}", ffmpeg::Error::from(code)) } diff --git a/crates/pf-encode/src/enc/linux/mod.rs b/crates/pf-encode/src/enc/linux/mod.rs index 8e9f7586..af9e5b65 100644 --- a/crates/pf-encode/src/enc/linux/mod.rs +++ b/crates/pf-encode/src/enc/linux/mod.rs @@ -58,6 +58,19 @@ struct AVCUDADeviceContext { internal: *mut std::ffi::c_void, // filled by ctx_init } +// A hand-written mirror of libav's `AVCUDADeviceContext` (hwcontext_cuda.h) — `ffmpeg-sys-next` +// does not bind it, so this is the only definition, and `CudaHw::new` WRITES `cuda_ctx` through it. +// A wrong offset here would not fail to compile or crash; it would scribble over libav's internal +// pointer. Nothing checked that until this block: the realistic failure is someone adding or +// reordering a field here, which these assertions turn into a build error. +const _: () = { + use std::mem::{offset_of, size_of}; + assert!(size_of::() == 3 * size_of::<*mut std::ffi::c_void>()); + assert!(offset_of!(AVCUDADeviceContext, cuda_ctx) == 0); + assert!(offset_of!(AVCUDADeviceContext, stream) == size_of::<*mut std::ffi::c_void>()); + assert!(offset_of!(AVCUDADeviceContext, internal) == 2 * size_of::<*mut std::ffi::c_void>()); +}; + /// CUDA hardware-frame contexts that wrap our shared `CUcontext`, so `hevc_nvenc` reads the /// imported device buffer directly. Owns two `AVBufferRef`s, unref'd on drop. struct CudaHw { diff --git a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs index 01cfa90b..5ed6f4ac 100644 --- a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs +++ b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs @@ -92,6 +92,32 @@ struct AVD3D11VAFramesContext { texture_infos: *mut c_void, // AVD3D11FrameDescriptor* (FFmpeg-owned; we never touch it) } +// Hand-written mirrors of libav's `AVD3D11VADeviceContext` / `AVD3D11VAFramesContext` +// (hwcontext_d3d11va.h) — `ffmpeg-sys-next` binds neither, and we WRITE `device` / `bind_flags` +// through them, so a wrong offset is silent corruption of libav's context rather than a compile +// error. ⚠ These two structs are duplicated in the other crate that talks to the same libav +// contexts (pf-encode's `ffmpeg_win.rs` and pf-client-core's `video_d3d11.rs`); they must agree +// with libav AND with each other, and these assertions are what makes a drift in either a build +// failure instead of a runtime mystery. +const _: () = { + use std::mem::{offset_of, size_of}; + type P = *mut c_void; + assert!(size_of::() == 7 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, device) == 0); + assert!(offset_of!(AVD3D11VADeviceContext, device_context) == size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, video_device) == 2 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, video_context) == 3 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, lock) == 4 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, unlock) == 5 * size_of::

()); + assert!(offset_of!(AVD3D11VADeviceContext, lock_ctx) == 6 * size_of::

()); + // ptr, u32, u32, ptr — the two 32-bit flags pack into one pointer-sized slot with no padding. + assert!(size_of::() == 3 * size_of::

()); + assert!(offset_of!(AVD3D11VAFramesContext, texture) == 0); + assert!(offset_of!(AVD3D11VAFramesContext, bind_flags) == size_of::

()); + assert!(offset_of!(AVD3D11VAFramesContext, misc_flags) == size_of::

() + 4); + assert!(offset_of!(AVD3D11VAFramesContext, texture_infos) == 2 * size_of::

()); +}; + /// AMD AMF vs Intel QSV — the two libavcodec vendor backends this module covers. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum WinVendor { diff --git a/crates/pf-zerocopy/src/imp/cuda.rs b/crates/pf-zerocopy/src/imp/cuda.rs index 5059e4cf..5d5a0a47 100644 --- a/crates/pf-zerocopy/src/imp/cuda.rs +++ b/crates/pf-zerocopy/src/imp/cuda.rs @@ -1401,30 +1401,6 @@ pub fn copy_pitched_nv12_to_buffer( } } -#[cfg(test)] -mod tests { - use super::*; - use std::mem::{offset_of, size_of}; - - /// The external-semaphore param structs are hand-flattened from cuda.h unions — assert the - /// layout against the C definitions so a transcription slip fails in CI, not in the driver. - #[test] - fn external_semaphore_struct_layouts_match_cuda_h() { - // CUDA_EXTERNAL_SEMAPHORE_HANDLE_DESC: type(4)+pad(4)+union(16)+flags(4)+reserved(64) = 96. - assert_eq!(size_of::(), 96); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_HANDLE_DESC, handle), 8); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_HANDLE_DESC, flags), 24); - - // CUDA_EXTERNAL_SEMAPHORE_SIGNAL_PARAMS: params{fence(8)+nvSciSync(8)+keyedMutex(8)+ - // reserved[12](48)} = 72, flags at 72, reserved[16] → 144 total. - assert_eq!(size_of::(), 144); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_SIGNAL_PARAMS, value), 0); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_SIGNAL_PARAMS, flags), 72); - - // CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS: params{fence(8)+nvSciSync(8)+keyedMutex(16, - // tail-padded)+reserved[10](40)} = 72, flags at 72, reserved[16] → 144 total. - assert_eq!(size_of::(), 144); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS, value), 0); - assert_eq!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS, flags), 72); - } -} +// The cuda.h struct layouts these calls depend on are asserted at COMPILE time in `ffi.rs` +// (`const _`), which covers all six structs and every build — the runtime test that used to live +// here checked three of them and only when someone ran it. diff --git a/crates/pf-zerocopy/src/imp/cuda/ffi.rs b/crates/pf-zerocopy/src/imp/cuda/ffi.rs index 626aa627..f77d4932 100644 --- a/crates/pf-zerocopy/src/imp/cuda/ffi.rs +++ b/crates/pf-zerocopy/src/imp/cuda/ffi.rs @@ -139,6 +139,59 @@ pub struct CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS { /// to try". pub const CU_EXTERNAL_SEMAPHORE_HANDLE_TYPE_TIMELINE_SEMAPHORE_FD: c_uint = 9; +// LAYOUT GUARDS for the six hand-flattened cuda.h structs above. +// +// These are filled here and handed straight to the CUDA driver, which reads them by offset. A +// transcription slip does not fail to compile and does not reliably crash — it makes the driver read +// a pitch, a size or an fd from the wrong bytes. Three of the six were already asserted, but only in +// `#[cfg(test)]`, so the check ran when someone ran the tests and never in a release build; these +// are `const`, so they hold on every compilation and cannot be skipped. The other three +// (`CUDA_MEMCPY2D` and the two external-memory descs) had no check at all until now, and +// `CUDA_MEMCPY2D` is the one used on every zero-copy frame. +const _: () = { + use std::mem::{offset_of, size_of}; + + // 16 fields, all pointer-sized or padded to it: 16 * 8 = 128. The two `c_uint` memory-type + // fields are each followed by padding to align the pointer after them, which is the part a + // hand-transcription gets wrong. + assert!(size_of::() == 128); + assert!(offset_of!(CUDA_MEMCPY2D, srcMemoryType) == 16); + assert!(offset_of!(CUDA_MEMCPY2D, srcHost) == 24); // padded past srcMemoryType + assert!(offset_of!(CUDA_MEMCPY2D, srcPitch) == 48); + assert!(offset_of!(CUDA_MEMCPY2D, dstMemoryType) == 72); + assert!(offset_of!(CUDA_MEMCPY2D, dstHost) == 80); // padded past dstMemoryType + assert!(offset_of!(CUDA_MEMCPY2D, dstPitch) == 104); + assert!(offset_of!(CUDA_MEMCPY2D, WidthInBytes) == 112); + assert!(offset_of!(CUDA_MEMCPY2D, Height) == 120); + + // type(4)+pad(4)+union(16)+size(8)+flags(4)+reserved[16](64) = 104. + assert!(size_of::() == 104); + assert!(offset_of!(CUDA_EXTERNAL_MEMORY_HANDLE_DESC, handle) == 8); + assert!(offset_of!(CUDA_EXTERNAL_MEMORY_HANDLE_DESC, size) == 24); + assert!(offset_of!(CUDA_EXTERNAL_MEMORY_HANDLE_DESC, flags) == 32); + + // offset(8)+size(8)+flags(4)+reserved[16](64), tail-padded to 8 = 88. + assert!(size_of::() == 88); + assert!(offset_of!(CUDA_EXTERNAL_MEMORY_BUFFER_DESC, size) == 8); + assert!(offset_of!(CUDA_EXTERNAL_MEMORY_BUFFER_DESC, flags) == 16); + + // type(4)+pad(4)+union(16)+flags(4)+reserved[16](64) = 96. No `size` — a semaphore has none. + assert!(size_of::() == 96); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_HANDLE_DESC, handle) == 8); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_HANDLE_DESC, flags) == 24); + + // params{fence(8)+nvSciSync(8)+keyedMutex(8)+reserved[12](48)} = 72, flags at 72 → 144. + assert!(size_of::() == 144); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_SIGNAL_PARAMS, value) == 0); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_SIGNAL_PARAMS, flags) == 72); + + // As signal, but `keyedMutex` is `{u64 key; u32 timeoutMs;}` = 16 with tail padding, hence the + // explicit pad word before the 10 reserved words. Still 72 to `flags` → 144. + assert!(size_of::() == 144); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS, value) == 0); + assert!(offset_of!(CUDA_EXTERNAL_SEMAPHORE_WAIT_PARAMS, flags) == 72); +}; + /// `CUipcMemHandle` (cuda.h): an opaque 64-byte struct identifying a device allocation across /// processes. Produced by `cuIpcGetMemHandle` in the exporting process, consumed by /// `cuIpcOpenMemHandle` in the importer — passed **by value**, matching the C diff --git a/crates/punktfunk-core/src/transport/udp/apple.rs b/crates/punktfunk-core/src/transport/udp/apple.rs index 8b980be0..a01de5bd 100644 --- a/crates/punktfunk-core/src/transport/udp/apple.rs +++ b/crates/punktfunk-core/src/transport/udp/apple.rs @@ -49,6 +49,24 @@ struct MsghdrX { msg_datalen: libc::size_t, } +// A hand-written mirror of Darwin's `struct msghdr_x` (sys/socket.h), which `libc` does not expose. +// `sendmsg_x`/`recvmsg_x` read and write through it, so a wrong offset would hand the kernel the +// wrong pointer or length — the two fields it uses to decide how much memory to touch. The layout is +// not obvious by inspection because the 32-bit fields force padding before the pointers that follow +// them, which is exactly the kind of thing an edit gets wrong silently. +const _: () = { + use std::mem::{offset_of, size_of}; + assert!(size_of::() == 56); + assert!(offset_of!(MsghdrX, msg_name) == 0); + assert!(offset_of!(MsghdrX, msg_namelen) == 8); + assert!(offset_of!(MsghdrX, msg_iov) == 16); // 4 bytes of padding after msg_namelen + assert!(offset_of!(MsghdrX, msg_iovlen) == 24); + assert!(offset_of!(MsghdrX, msg_control) == 32); // padding after msg_iovlen + assert!(offset_of!(MsghdrX, msg_controllen) == 40); + assert!(offset_of!(MsghdrX, msg_flags) == 44); + assert!(offset_of!(MsghdrX, msg_datalen) == 48); +}; + #[cfg(target_vendor = "apple")] extern "C" { /// Darwin batched receive: up to `cnt` datagrams in one syscall; returns the count received and diff --git a/crates/punktfunk-host/src/audio/windows/audio_control.rs b/crates/punktfunk-host/src/audio/windows/audio_control.rs index 711c9324..785373bb 100644 --- a/crates/punktfunk-host/src/audio/windows/audio_control.rs +++ b/crates/punktfunk-host/src/audio/windows/audio_control.rs @@ -310,6 +310,26 @@ struct IPolicyConfigVtbl { // SetEndpointVisibility follows — unused. } +// This mirrors the vtable of the UNDOCUMENTED `IPolicyConfig` COM interface, so there is no header +// to check it against and no `windows-rs` binding to fall back on. `set_default_endpoint` is called +// by INDEX — `((*vtbl).set_default_endpoint)(..)` is really "the 14th function pointer in this +// table" — so a field added, removed or resized above it does not fail to compile: it silently calls +// a DIFFERENT function through a mismatched signature, which is arbitrary-code territory rather +// than a wrong answer. The `_reserved` gap is what makes that easy to get wrong, since its ten slots +// carry no names to anchor a review. These assertions pin the two things the call actually depends +// on: the slot index of `set_default_endpoint`, and the size of the table up to it. +const _: () = { + use std::mem::{offset_of, size_of}; + type P = *const c_void; + // 3 IUnknown slots + 10 reserved = `set_default_endpoint` is slot 13 (0-based). + assert!(offset_of!(IPolicyConfigVtbl, query_interface) == 0); + assert!(offset_of!(IPolicyConfigVtbl, add_ref) == size_of::

()); + assert!(offset_of!(IPolicyConfigVtbl, release) == 2 * size_of::

()); + assert!(offset_of!(IPolicyConfigVtbl, _reserved) == 3 * size_of::

()); + assert!(offset_of!(IPolicyConfigVtbl, set_default_endpoint) == 13 * size_of::

()); + assert!(size_of::() == 14 * size_of::

()); +}; + /// Set `device_id` as the default audio endpoint for eConsole/eMultimedia/eCommunications via the /// undocumented `IPolicyConfig::SetDefaultEndpoint` (the call `mmsys.cpl` makes). Errs if any role /// fails.