diff --git a/.gitea/workflows/audit.yml b/.gitea/workflows/audit.yml index 8f5aeafa..9f1540cf 100644 --- a/.gitea/workflows/audit.yml +++ b/.gitea/workflows/audit.yml @@ -21,6 +21,11 @@ # workflow_dispatch, the rust-ci container, the same cache pattern) and because # ci.yml runs on every push against a fleet where 37 of 46 jobs contend for # ubuntu-24.04. See the `miri:` job below for what it does and does not buy. +# * c-abi-asan → NON-BLOCKING ASAN+LSAN run of the C ABI harness (tests/c/run.sh under +# PF_SAN=address): both sides of the abi.rs boundary instrumented at once, and +# the only automated check on its Box::into_raw/from_raw leak contract. Same +# here-not-ci.yml reasoning as miri — plus -Zbuild-std defeats sccache, so it +# must not ride the per-push leg. # Triggers: weekly (catch newly-disclosed CVEs in pinned deps), on every lockfile/allowlist # change, and on demand. # To silence a known-unfixable Rust advisory, add it to `.cargo/audit.toml` ([advisories] ignore=[…]). @@ -364,3 +369,80 @@ jobs: -p punktfunk-core --lib -- fec::gf8 2>&1 | tee /tmp/miri-gf8.log || ok=0 grep -qE 'test result: ok\. [1-9][0-9]* passed' /tmp/miri-gf8.log || ok=0 [ "$ok" = 1 ] || echo "::warning::miri (punktfunk-core fec::gf8, AVX2/SSSE3) did not pass — non-blocking; see design/rust-safety-programme.md §7" + + # ASAN + LSAN over the C ABI harness — §6.1 of design/rust-safety-programme.md, its rank-1 + # tooling item. crates/punktfunk-core/tests/c/run.sh already proves the staticlib links and + # round-trips 4 frames byte-exact from C on every push (ci.yml); PF_SAN=address rebuilds BOTH + # sides instrumented — the staticlib on nightly with -Zsanitizer/-Zbuild-std (std itself + # included), the harness with clang -fsanitize — so ASAN sees the seam a Rust-only tool cannot, + # and LSAN (detect_leaks=1, the script's default) becomes the one automated check on abi.rs's + # Box::into_raw/from_raw leak contract. + # Proven to fail on 192.168.1.25: deleting a single punktfunk_session_free() from harness.c + # makes LSAN report the ~308 Rust-side allocations behind the handle and run.sh exit 1. + # What it does NOT see: the invalid-InputKind-discriminant UB at abi.rs (that needs the + # validator, tracked in §5 of the programme doc), and nothing GPU/Windows — this is the + # default-feature (quic-less, opus-less) core only. + c-abi-asan: + runs-on: ubuntu-24.04 + container: + image: 192.168.1.58:5010/punktfunk-rust-ci:latest + timeout-minutes: 30 + env: + # The SAME dated pin as the miri job above, deliberately — one nightly date to bump for + # both jobs (they have no toolchain interaction; sharing the date just halves the chores). + SAN_TOOLCHAIN: nightly-2026-08-10 + # Same guard as the miri job: audit.yml sets no sccache today, and -Zbuild-std could not + # use it anyway. Keeps a future workflow-level sccache from becoming a puzzle. + RUSTC_WRAPPER: "" + steps: + - uses: actions/checkout@v4 + + # Own `san-` key prefixes — never shared with the miri caches, per the cache-poisoning + # note there (and so an incomplete save from one job can never starve the other). + - name: cache the nightly toolchain + uses: actions/cache@v4 + with: + path: /usr/local/rustup/toolchains/${{ env.SAN_TOOLCHAIN }}-x86_64-unknown-linux-gnu + key: san-toolchain-v1-${{ env.SAN_TOOLCHAIN }} + - name: cache the cargo registry + uses: actions/cache@v4 + with: + path: /usr/local/cargo/registry + key: san-registry-v1-${{ hashFiles('Cargo.lock') }} + restore-keys: san-registry-v1- + + # rust-src is required: -Zbuild-std compiles std from source so it is instrumented too — + # without that, LSAN cannot attribute allocations made inside std (Vec, Box, HashMap). + - name: install the pinned nightly + rust-src + run: | + git config --global --add safe.directory "$PWD" + rustup toolchain install "$SAN_TOOLCHAIN" --profile minimal --component rust-src + echo "root pin, untouched by this job: $(grep -E '^channel' rust-toolchain.toml)" + cargo +"$SAN_TOOLCHAIN" --version + + # The image installs clang but Ubuntu does not always pull the compiler-rt sanitizer + # runtime with it (verified absent on a stock 26.04 box). Probe with an actual ASAN link + # and self-heal via apt if it fails — container jobs on this fleet run as root (the + # bun-audit job's apt-get above relies on the same fact). + - name: ensure clang's ASAN runtime + run: | + if ! echo 'int main(void){return 0;}' | clang -fsanitize=address -x c - -o /tmp/asan-probe 2>/dev/null; then + apt-get update && apt-get install -y --no-install-recommends "libclang-rt-$(clang -dumpversion | cut -d. -f1)-dev" + echo 'int main(void){return 0;}' | clang -fsanitize=address -x c - -o /tmp/asan-probe + fi + + # run.sh handles everything behind PF_SAN (nightly build, target path, clang flags, + # ASAN_OPTIONS=detect_leaks=1) and exits non-zero on any report. The grep is the + # proved-it-ran guard, same reasoning as the miri steps: a script change that silently + # skips the harness must not read as green. run.sh expects bash and PATH cargo — both true + # in this container. PF_SAN_TOOLCHAIN pins the script's `cargo +` to the dated + # nightly installed above — without it the script would ask for the ROLLING `nightly` + # channel, which this job deliberately does not install. + - name: C ABI harness under ASAN+LSAN + run: | + set -o pipefail + ok=1 + PF_SAN=address PF_SAN_TOOLCHAIN="$SAN_TOOLCHAIN" \ + bash crates/punktfunk-core/tests/c/run.sh 2>&1 | tee /tmp/asan-harness.log || ok=0 + grep -q 'PASS: 4 frames round-tripped byte-exact' /tmp/asan-harness.log || ok=0 + [ "$ok" = 1 ] || echo "::warning::c-abi-asan did not pass — non-blocking on day one; see design/rust-safety-programme.md §6.1. An LSAN report here means the abi.rs into_raw/from_raw contract broke." diff --git a/crates/pf-encode/src/enc/libav.rs b/crates/pf-encode/src/enc/libav.rs index 35acb531..8abf7d22 100644 --- a/crates/pf-encode/src/enc/libav.rs +++ b/crates/pf-encode/src/enc/libav.rs @@ -119,6 +119,88 @@ impl Drop for AvFilterGraph { } } +/// An owned `AVFrame`, freed exactly once when it drops. +/// +/// The house pattern (`AvBuffer` above): `alloc` rejects the allocator's null once, `as_ptr` +/// lends, `Drop` frees, no `Clone`. Before this type existed the crate held 8 `av_frame_alloc` +/// sites matched by 22 hand-placed `av_frame_free`s — an ownership contract upheld by nobody, +/// and broken in practice: the Windows zero-copy submit path leaked the frame AND a pooled +/// hwframe surface on three `?` exits, under a comment asserting the opposite (fixed in the +/// same change that introduced this type). +/// +/// Why not ffmpeg-next's own RAII frame (`frame::Video::empty()`, already used as `VideoFrame` +/// in the Linux NVENC path): `Frame::empty()` does not null-check — on allocator failure it +/// wraps null and the next field write through it is UB — whereas every open-coded site here +/// null-checked. This type keeps that: `alloc` returns `Option`, mirroring +/// `AvFilterGraph::alloc`. +pub(crate) struct AvFrame(std::ptr::NonNull); + +impl AvFrame { + /// Allocate a frame, rejecting the null `av_frame_alloc` returns on OOM. + /// + /// Safe: the call takes no arguments and has no precondition a caller could violate — the + /// only contract is what happens to the result, and that is exactly what this type owns. + pub(crate) fn alloc() -> Option { + // SAFETY: parameterless allocator; it returns either a fresh, uniquely-owned frame whose + // ownership passes to the value returned here, or null (rejected by NonNull::new). + std::ptr::NonNull::new(unsafe { ffi::av_frame_alloc() }).map(AvFrame) + } + + /// The borrowed pointer, for the ffmpeg calls that fill or read the frame without taking + /// ownership of it. Borrowed only — the `AvFrame` stays the owner, so callers must not free + /// or move-from what this returns. + pub(crate) fn as_ptr(&self) -> *mut ffi::AVFrame { + self.0.as_ptr() + } +} + +impl Drop for AvFrame { + fn drop(&mut self) { + let mut p = self.0.as_ptr(); + // SAFETY: `p` is the non-null frame `alloc` took ownership of, and this type is its + // sole owner (neither `Clone` nor `Copy`; `as_ptr` only lends), so this runs exactly + // once. `av_frame_free` unrefs any buffers the frame holds (returning pooled hwframe + // surfaces to their pool) and frees the frame; it nulls only the local copy. + unsafe { ffi::av_frame_free(&mut p) }; + } +} + +/// An owned swscale context, freed exactly once when it drops. +/// +/// Same ownership question as the frame above — `sws_getContext` at 3 sites was matched by 5 +/// hand-placed `sws_freeContext`s, two of them inside hand-written `Drop` impls whose real job +/// this type absorbs. +pub(crate) struct AvSwsContext(std::ptr::NonNull); + +impl AvSwsContext { + /// Take ownership of a freshly-created `SwsContext`, rejecting the null `sws_getContext` + /// returns on failure (unsupported conversion or OOM). + /// + // unsafe-fn-no-op-ok: contract-deferring constructor (`Vec::set_len` shape) — the body is + // safe; the ownership transfer promised here is what Drop/as_ptr later rely on. + /// # Safety + /// `p` must be null, or a live `SwsContext` whose ownership passes to the returned value — + /// nothing else may free it. + pub(crate) unsafe fn from_raw(p: *mut ffi::SwsContext) -> Option { + std::ptr::NonNull::new(p).map(AvSwsContext) + } + + /// The borrowed pointer, for `sws_scale` calls. Borrowed only — the `AvSwsContext` stays + /// the owner. + pub(crate) fn as_ptr(&self) -> *mut ffi::SwsContext { + self.0.as_ptr() + } +} + +impl Drop for AvSwsContext { + fn drop(&mut self) { + // SAFETY: `self.0` is the non-null context `from_raw` took ownership of, and this type + // is its sole owner (neither `Clone` nor `Copy`; `as_ptr` only lends), so this runs + // exactly once. + unsafe { ffi::sws_freeContext(self.0.as_ptr()) }; + } +} + /// One `receive_packet` attempt, with the not-ready states kept distinct so a blocking drain can /// tell "still encoding" (retry) from "stream over" (stop). The Linux NVENC/VAAPI polls collapse /// `Again`/`Eof` to `None`; the Windows AMF/QSV path keeps them apart for its deadline-driven loop. diff --git a/crates/pf-encode/src/enc/linux/mod.rs b/crates/pf-encode/src/enc/linux/mod.rs index 21638b26..2e1bf8bf 100644 --- a/crates/pf-encode/src/enc/linux/mod.rs +++ b/crates/pf-encode/src/enc/linux/mod.rs @@ -24,8 +24,8 @@ use std::os::raw::c_int; use std::ptr; use super::libav::{ - apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, PollOutcome, SWS_CS_ITU709, - SWS_POINT, + apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, AvFrame, AvSwsContext, PollOutcome, + SWS_CS_ITU709, SWS_POINT, }; use ffmpeg::ffi; // = ffmpeg_sys_next @@ -191,6 +191,17 @@ struct OpenArgs { } pub struct NvencEncoder { + // FIELD ORDER IS LOAD-BEARING: the hand-written `Drop` this replaced ran before any field + // drop, freeing `sws_csc` ahead of `enc`/`frame`/`cuda` — and this path runs on every + // stall-watchdog recovery via `*self = fresh` in `reset`. Declaration order is what + // preserves that sequence now (drop order follows declaration; an offset_of assert cannot + // pin it — repr(Rust) may lay memory out in any order). + /// CPU CSC paths only: swscale context converting the captured packed source into + /// [`Self::frame`] — RGB/BGR → planar YUV444P for a 4:4:4 session (`hevc_nvenc` only emits + /// 4:4:4 from a YUV444 *input*; RGB-in is always 4:2:0), or X2RGB10/X2BGR10 → P010 (BT.2020 + /// limited) for an HDR session. `None` on the plain RGB paths AND on the zero-copy paths (the + /// worker's GPU convert delivers ready CUDA frames). + sws_csc: Option, enc: encoder::video::Encoder, /// Reusable 4-bpp CPU input frame (CPU path only; `None` for the zero-copy/CUDA path). /// Mutating it in place across frames is sound only because the encoder is opened with @@ -199,12 +210,6 @@ pub struct NvencEncoder { frame: Option, /// Zero-copy path: CUDA hwdevice/hwframes contexts (the encoder takes `AV_PIX_FMT_CUDA`). cuda: Option, - /// CPU CSC paths only: swscale context converting the captured packed source into - /// [`Self::frame`] — RGB/BGR → planar YUV444P for a 4:4:4 session (`hevc_nvenc` only emits - /// 4:4:4 from a YUV444 *input*; RGB-in is always 4:2:0), or X2RGB10/X2BGR10 → P010 (BT.2020 - /// limited) for an HDR session. `None` on the plain RGB paths AND on the zero-copy paths (the - /// worker's GPU convert delivers ready CUDA frames). Freed in `Drop`. - sws_csc: Option<*mut ffi::SwsContext>, /// This session opened as full-chroma 4:4:4 (FREXT) — via either input path. want_444: bool, src_format: PixelFormat, @@ -226,7 +231,7 @@ pub struct NvencEncoder { args: OpenArgs, } -// `CudaHw` holds raw `AVBufferRef`s and `sws_csc` a raw `SwsContext`; the encoder lives on a single +// `CudaHw` holds raw `AVBufferRef`s and `sws_csc` an owned `SwsContext`; the encoder lives on a single // thread. The CPU encoder is already `Send` via ffmpeg-next; assert it for the raw fields too. // SAFETY: `NvencEncoder` owns an ffmpeg-next `Encoder`/`VideoFrame` (already `Send`) plus a `CudaHw` // holding raw `AVBufferRef`s and an optional raw `SwsContext`, none of which are `Send` by default. @@ -608,14 +613,13 @@ impl NvencEncoder { ); } - // Built HERE, below the fallible encoder open, NOT above it. `sws_getContext` returns a raw - // pointer whose only free is `Drop for NvencEncoder` — and `Drop` needs a CONSTRUCTED - // `Self`, which does not exist on `open`'s early returns (the intra-refresh-unsupported - // retry, which recurses into `Self::open`, and the plain error return). Creating the - // context above them leaked one per failed attempt, and `open_nvenc_probed`'s EINVAL - // bitrate ladder calls `open` up to ~10 times, so a host stepping its bitrate down leaked a - // context per step. Nothing between here and the `Ok(NvencEncoder { … })` below can return, - // so this placement makes the leak unrepresentable rather than merely unlikely. + // Built HERE, below the fallible encoder open, NOT above it — historically because the + // context's only free was `Drop for NvencEncoder`, which needs a CONSTRUCTED `Self` that + // does not exist on `open`'s early returns; creating it above them leaked one per failed + // attempt, and `open_nvenc_probed`'s EINVAL bitrate ladder calls `open` up to ~10 times. + // The owned `AvSwsContext` now frees itself on any exit, but the placement stays: it + // documents the dependency on the post-open `nvenc_pixel`, and there is no reason to + // build a context an early return would just throw away. // CPU CSC paths: build the packed-RGB → planar swscale (no rescale) into the encoder's // input frame. THREE users: 4:4:4 (RGB→YUV444P, BT.709, range per the flag), HDR // (X2RGB10/X2BGR10→P010, BT.2020 limited — the PQ transfer is per-channel and rides @@ -640,10 +644,10 @@ impl NvencEncoder { // formats. Both dims are the encoder's positive `width`/`height` as `c_int`; `src_av` is a // valid `AVPixelFormat` (from the `sws_src_pixel`-validated packed-RGB source), the dst is // YUV444P (4:4:4) or P010LE (HDR). The trailing filter/param pointers are null = "use - // defaults" (documented as accepted). No Rust memory is borrowed; the returned pointer is - // null-checked below. + // defaults" (documented as accepted). No Rust memory is borrowed; ownership of the + // returned context passes to the `AvSwsContext` (null rejected by `from_raw`). let sws = unsafe { - ffi::sws_getContext( + AvSwsContext::from_raw(ffi::sws_getContext( width as c_int, height as c_int, src_av, @@ -654,11 +658,11 @@ impl NvencEncoder { ptr::null_mut(), ptr::null_mut(), ptr::null(), - ) + )) }; - if sws.is_null() { + let Some(sws) = sws else { bail!("sws_getContext(RGB→{nvenc_pixel:?}) failed"); - } + }; // Colour math applies to the CSC users ONLY. The expand is a pure byte shuffle — // packed 3-bpp RGB/BGR to the same channels in 4 bytes, `nvenc_pixel` being `rgb0`/ // `bgr0` — and NVENC does the RGB→YUV itself downstream. Handing it a matrix + range @@ -678,7 +682,16 @@ impl NvencEncoder { SWS_CS_ITU709 }); let dst_range = i32::from(full_range_444); - ffi::sws_setColorspaceDetails(sws, cs, 1, cs, dst_range, 0, 1 << 16, 1 << 16); + ffi::sws_setColorspaceDetails( + sws.as_ptr(), + cs, + 1, + cs, + dst_range, + 0, + 1 << 16, + 1 << 16, + ); } } Some(sws) @@ -692,10 +705,10 @@ impl NvencEncoder { Some(VideoFrame::new(nvenc_pixel, width, height)) }; Ok(NvencEncoder { + sws_csc, enc, frame, cuda: cuda_hw, - sws_csc, want_444, src_format: format, width, @@ -838,7 +851,7 @@ impl NvencEncoder { // three CSC users (see `open`): 4:4:4 → planar YUV444P, HDR → P010, and the packed 3-bpp // expand → `rgb0`/`bgr0`. The remaining branch below is the 4-bpp source, which needs no // conversion at all — just a row copy honouring the destination stride. - if let Some(sws) = self.sws_csc { + if let Some(sws) = self.sws_csc.as_ref().map(AvSwsContext::as_ptr) { let frame = self .frame .as_mut() @@ -927,27 +940,23 @@ impl NvencEncoder { // SAFETY: `frames_ref` is the non-null CUDA frames ctx from `self.cuda` (unwrapped via // `.context(..)?` above), and the shared CUDA context was just made current on THIS thread // (`make_current()?`), the precondition for the device-pointer copies below. - // * `av_frame_alloc` → `f` (null-checked). `av_hwframe_get_buffer(frames_ref, f, 0)` fills `f` - // with a pooled CUDA surface (sets `data[]`/`linesize[]`/`buf[0]`/`hw_frames_ctx`); on - // failure we free `f` and bail. - // * For NV12 we read `(*f).data[0..2]` / `linesize[0..2]` (Y + interleaved UV), else - // `data[0]`/`linesize[0]` — in-struct fields of the non-null `f`, valid for the surface dims - // ffmpeg allocated — and pass them to the cuda copy helpers, which device→device copy `buf` - // (the imported `DeviceBuffer`, owned by the caller and live for this call) into the surface. - // * On copy error we free `f` and return. Otherwise we write `pts`/`pict_type` through `f` and - // `avcodec_send_frame` it into the live owned `self.enc` context (which takes its own ref of - // the pooled surface), then free our `f` ref exactly once. Single-threaded encoder → no race. + // * `f` is an owned `AvFrame` — every exit below (bail, copy error, success) drops it + // exactly once, releasing its ref on the pooled surface. `av_hwframe_get_buffer` fills + // it with a pooled CUDA surface (sets `data[]`/`linesize[]`/`buf[0]`/`hw_frames_ctx`). + // * For NV12 we read `data[0..2]` / `linesize[0..2]` (Y + interleaved UV), else + // `data[0]`/`linesize[0]` — in-struct fields of the live frame, valid for the surface + // dims ffmpeg allocated — and pass them to the cuda copy helpers, which device→device + // copy `buf` (the imported `DeviceBuffer`, owned by the caller and live for this call) + // into the surface. + // * `avcodec_send_frame` takes its own ref of the pooled surface, so the drop afterwards + // is the sole owning free. Single-threaded encoder → no race. unsafe { - let mut f = ffi::av_frame_alloc(); - if f.is_null() { - bail!("av_frame_alloc failed"); - } + let f = AvFrame::alloc().context("av_frame_alloc failed")?; // Pooled CUDA surface: sets format, width/height, data[0]/linesize[0], buf[0] and // hw_frames_ctx. Reused across frames (the pool recycles), keeping NVENC's // registration cache warm. - let r = ffi::av_hwframe_get_buffer(frames_ref, f, 0); + let r = ffi::av_hwframe_get_buffer(frames_ref, f.as_ptr(), 0); if r < 0 { - ffi::av_frame_free(&mut f); bail!("av_hwframe_get_buffer(CUDA) failed ({r})"); } // NV12 surfaces are two-plane (Y in data[0], interleaved UV in data[1]); YUV444 @@ -958,41 +967,36 @@ impl NvencEncoder { let copy_res = if buf.yuv444 { let dsts = core::array::from_fn(|i| { ( - (*f).data[i] as pf_zerocopy::cuda::CUdeviceptr, - (*f).linesize[i] as usize, + (*f.as_ptr()).data[i] as pf_zerocopy::cuda::CUdeviceptr, + (*f.as_ptr()).linesize[i] as usize, ) }); pf_zerocopy::cuda::copy_yuv444_to_device(buf, dsts, true) } else if self.want_444 { - ffi::av_frame_free(&mut f); bail!( "4:4:4 session but the zero-copy frame is not YUV444 (LINEAR/gamescope \ capture has no GPU 4:4:4 convert) — unset PUNKTFUNK_ZEROCOPY to use the \ CPU 4:4:4 path on this compositor" ); } else if buf.is_nv12() { - let y_ptr = (*f).data[0] as pf_zerocopy::cuda::CUdeviceptr; - let y_pitch = (*f).linesize[0] as usize; - let uv_ptr = (*f).data[1] as pf_zerocopy::cuda::CUdeviceptr; - let uv_pitch = (*f).linesize[1] as usize; + let y_ptr = (*f.as_ptr()).data[0] as pf_zerocopy::cuda::CUdeviceptr; + let y_pitch = (*f.as_ptr()).linesize[0] as usize; + let uv_ptr = (*f.as_ptr()).data[1] as pf_zerocopy::cuda::CUdeviceptr; + let uv_pitch = (*f.as_ptr()).linesize[1] as usize; pf_zerocopy::cuda::copy_nv12_to_device(buf, y_ptr, y_pitch, uv_ptr, uv_pitch, true) } else { - let dst_ptr = (*f).data[0] as pf_zerocopy::cuda::CUdeviceptr; - let dst_pitch = (*f).linesize[0] as usize; + let dst_ptr = (*f.as_ptr()).data[0] as pf_zerocopy::cuda::CUdeviceptr; + let dst_pitch = (*f.as_ptr()).linesize[0] as usize; pf_zerocopy::cuda::copy_device_to_device(buf, dst_ptr, dst_pitch, true) }; - if let Err(e) = copy_res { - ffi::av_frame_free(&mut f); - return Err(e).context("copy imported buffer into NVENC surface"); - } - (*f).pts = pts; - (*f).pict_type = if idr { + copy_res.context("copy imported buffer into NVENC surface")?; + (*f.as_ptr()).pts = pts; + (*f.as_ptr()).pict_type = if idr { ffi::AVPictureType::AV_PICTURE_TYPE_I } else { ffi::AVPictureType::AV_PICTURE_TYPE_NONE }; - let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), f); - ffi::av_frame_free(&mut f); + let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), f.as_ptr()); if r < 0 { bail!("avcodec_send_frame(CUDA) failed ({r})"); } @@ -1001,16 +1005,9 @@ impl NvencEncoder { } } -impl Drop for NvencEncoder { - fn drop(&mut self) { - if let Some(sws) = self.sws_csc.take() { - // SAFETY: `sws` is the non-null `SwsContext` allocated by `sws_getContext` in `open` and - // owned exclusively by this encoder (taken out of the field so it can't be freed twice). - // `sws_freeContext` frees it; nothing else references it after this single-threaded drop. - unsafe { ffi::sws_freeContext(sws) }; - } - } -} +// No `Drop` for `NvencEncoder`: `sws_csc` (`Option`) frees itself, and as field #1 +// it does so ahead of `enc`/`frame`/`cuda` — the same sequence the hand-written `Drop` performed +// (see the field-order note on the struct). /// Serialises the save → `AV_LOG_FATAL` → restore window that every capability probe opens around /// an encoder open it *expects* to fail. diff --git a/crates/pf-encode/src/enc/linux/vaapi.rs b/crates/pf-encode/src/enc/linux/vaapi.rs index ea5bd6c7..d003dabf 100644 --- a/crates/pf-encode/src/enc/linux/vaapi.rs +++ b/crates/pf-encode/src/enc/linux/vaapi.rs @@ -34,8 +34,8 @@ use std::ptr; use std::sync::{Mutex, OnceLock}; use super::libav::{ - apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, AvFilterGraph, PollOutcome, - SWS_CS_ITU709, SWS_POINT, + apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, AvFilterGraph, AvFrame, + AvSwsContext, PollOutcome, SWS_CS_ITU709, SWS_POINT, }; use ffmpeg::ffi; // = ffmpeg_sys_next @@ -544,8 +544,13 @@ impl VaapiHw { struct CpuInner { enc: encoder::video::Encoder, hw: VaapiHw, - sws: *mut ffi::SwsContext, - nv12: *mut ffi::AVFrame, // reusable software NV12 staging frame (swscale dst → upload src) + // FIELD ORDER IS LOAD-BEARING: the hand-written `Drop` this replaced freed `nv12` BEFORE + // `sws` — the reverse of the old declaration order — and field-DECLARATION order is what + // preserves that now (drop order follows declaration; an offset_of assert cannot pin it, + // repr(Rust) may lay memory out in any order). + /// Reusable software NV12/P010 staging frame (swscale dst → upload src). + nv12: AvFrame, + sws: AvSwsContext, src_format: PixelFormat, width: u32, height: u32, @@ -600,10 +605,10 @@ impl CpuInner { // `src_av` is a valid `AVPixelFormat` (from `pixel_to_av` of the `vaapi_sws_src`-validated // `src_pixel`), the dst is NV12/P010. The three trailing pointers (srcFilter, dstFilter, // param) are explicitly null = "use defaults", which the API documents as accepted. No Rust - // memory is borrowed — only by-value ints/enums — and the returned pointer is null-checked - // just below. + // memory is borrowed — only by-value ints/enums — and ownership of the returned context + // passes to the `AvSwsContext` (null rejected by `from_raw`). let sws = unsafe { - ffi::sws_getContext( + AvSwsContext::from_raw(ffi::sws_getContext( width as c_int, height as c_int, src_av, @@ -614,16 +619,15 @@ impl CpuInner { ptr::null_mut(), ptr::null_mut(), ptr::null(), - ) + )) }; - if sws.is_null() { + let Some(sws) = sws else { bail!( "sws_getContext(RGB→{})", if ten_bit { "P010" } else { "NV12" } ); - } - // SAFETY: `sws` is the non-null `SwsContext` from `sws_getContext` above (the `is_null()` - // check immediately preceding returned false). The coefficient table from + }; + // SAFETY: `sws` is the live owned context from above. The coefficient table from // `sws_getCoefficients` (ITU-709, or BT.2020 NCL for the HDR path — matching the VUI) is a // libswscale static const valid for the whole process, reused here for both the inverse // (src) and forward (dst) matrices. `sws_setColorspaceDetails` only reads those tables and @@ -635,32 +639,22 @@ impl CpuInner { } else { SWS_CS_ITU709 }); - ffi::sws_setColorspaceDetails(sws, cs, 1, cs, 0, 0, 1 << 16, 1 << 16); + ffi::sws_setColorspaceDetails(sws.as_ptr(), cs, 1, cs, 0, 0, 1 << 16, 1 << 16); } - // SAFETY: `av_frame_alloc` returns a fresh, uniquely-owned heap `AVFrame` (null-checked — on - // null we free the already-built `sws` and bail). We then write the plain `format`/`width`/ - // `height` fields through the non-null, properly-aligned `f` (sole owner, not yet shared). - // `av_frame_get_buffer(f, 0)` allocates backing storage for those dims/format; on failure we - // free `f` and `sws` (unwinding the half-built state) and bail. On success `f` is a fully-owned - // NV12/P010 frame stored in `CpuInner.nv12` and freed once in `CpuInner::drop`. `f` is a - // unique fresh pointer, so none of these writes alias anything. - let nv12 = unsafe { - let f = ffi::av_frame_alloc(); - if f.is_null() { - ffi::sws_freeContext(sws); - bail!("av_frame_alloc(staging) failed"); - } - (*f).format = staging_av as c_int; - (*f).width = width as c_int; - (*f).height = height as c_int; - if ffi::av_frame_get_buffer(f, 0) < 0 { - let mut f = f; - ffi::av_frame_free(&mut f); - ffi::sws_freeContext(sws); + let nv12 = AvFrame::alloc().context("av_frame_alloc(staging) failed")?; + // SAFETY: writing the plain `format`/`width`/`height` fields through the owned frame's + // pointer stays inside its allocation (sole owner, not yet shared). + // `av_frame_get_buffer` allocates backing storage for those dims/format; on failure the + // owned `nv12` (and the `sws` above it) simply drop — the hand-written unwind this + // replaced had to free both by hand on every branch. + unsafe { + (*nv12.as_ptr()).format = staging_av as c_int; + (*nv12.as_ptr()).width = width as c_int; + (*nv12.as_ptr()).height = height as c_int; + if ffi::av_frame_get_buffer(nv12.as_ptr(), 0) < 0 { bail!("av_frame_get_buffer(staging) failed"); } - f - }; + } tracing::info!( encoder = codec.vaapi_name(), "VAAPI encode active ({width}x{height}@{fps}, CPU→{} upload path)", @@ -669,8 +663,8 @@ impl CpuInner { Ok(CpuInner { enc, hw, - sws, nv12, + sws, src_format: format, width, height, @@ -691,49 +685,43 @@ impl CpuInner { // `bytes.len() >= src_row * h`. `sws_scale` reads `h` rows of `src_row` bytes from // `src_data[0] = bytes.as_ptr()` (the other planes null/0 — packed RGB is single-plane), all // in bounds; `bytes`, `src_data`, `src_stride` are live locals for this synchronous call. - // `self.sws` is the non-null context built in `open`; it writes into `self.nv12` (a non-null - // owned frame whose `data`/`linesize` in-struct arrays were sized by `av_frame_get_buffer`). - // `av_frame_alloc` (null-checked) yields a fresh `hwf`; `av_hwframe_get_buffer` pulls a pooled - // VAAPI surface from the live non-null `self.hw.frames_ref`; `av_hwframe_transfer_data` uploads - // the staged NV12 into it — both frames live, failures free `hwf` and bail. We then write - // `pts`/`pict_type` through the non-null `hwf` and `avcodec_send_frame` it into the live - // owned `self.enc` context (which takes its own ref), then free our `hwf` ref exactly once. - // The encoder runs only on this thread (see `unsafe impl Send`), so no aliasing/data race. + // `self.sws` is the owned context built in `open`; it writes into `self.nv12` (an owned + // frame whose `data`/`linesize` in-struct arrays were sized by `av_frame_get_buffer`). + // `hwf` is an owned `AvFrame` — every exit below drops it exactly once, releasing its ref + // on the pooled VAAPI surface. `av_hwframe_get_buffer` pulls that surface from the live + // non-null `self.hw.frames_ref`; `av_hwframe_transfer_data` uploads the staged NV12 into + // it. `avcodec_send_frame` takes its own ref, so the drop afterwards is the sole owning + // free. The encoder runs only on this thread (see `unsafe impl Send`), so no + // aliasing/data race. unsafe { let src_data: [*const u8; 4] = [bytes.as_ptr(), ptr::null(), ptr::null(), ptr::null()]; let src_stride: [c_int; 4] = [src_row as c_int, 0, 0, 0]; if ffi::sws_scale( - self.sws, + self.sws.as_ptr(), src_data.as_ptr(), src_stride.as_ptr(), 0, h as c_int, - (*self.nv12).data.as_ptr(), - (*self.nv12).linesize.as_ptr(), + (*self.nv12.as_ptr()).data.as_ptr(), + (*self.nv12.as_ptr()).linesize.as_ptr(), ) < 0 { bail!("sws_scale RGB→NV12 failed"); } - let mut hwf = ffi::av_frame_alloc(); - if hwf.is_null() { - bail!("av_frame_alloc(hw) failed"); - } - if ffi::av_hwframe_get_buffer(self.hw.frames_ref.as_ptr(), hwf, 0) < 0 { - ffi::av_frame_free(&mut hwf); + let hwf = AvFrame::alloc().context("av_frame_alloc(hw) failed")?; + if ffi::av_hwframe_get_buffer(self.hw.frames_ref.as_ptr(), hwf.as_ptr(), 0) < 0 { bail!("av_hwframe_get_buffer(VAAPI) failed"); } - if ffi::av_hwframe_transfer_data(hwf, self.nv12, 0) < 0 { - ffi::av_frame_free(&mut hwf); + if ffi::av_hwframe_transfer_data(hwf.as_ptr(), self.nv12.as_ptr(), 0) < 0 { bail!("av_hwframe_transfer_data(→VAAPI) failed"); } - (*hwf).pts = pts; - (*hwf).pict_type = if idr { + (*hwf.as_ptr()).pts = pts; + (*hwf.as_ptr()).pict_type = if idr { ffi::AVPictureType::AV_PICTURE_TYPE_I } else { ffi::AVPictureType::AV_PICTURE_TYPE_NONE }; - let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), hwf); - ffi::av_frame_free(&mut hwf); + let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), hwf.as_ptr()); if r < 0 { bail!("avcodec_send_frame(VAAPI) failed ({r})"); } @@ -742,24 +730,10 @@ impl CpuInner { } } -impl Drop for CpuInner { - fn drop(&mut self) { - // SAFETY: `self.nv12` (an owned `AVFrame`) and `self.sws` (an owned `SwsContext`) are each - // freed exactly once here, guarded by `is_null()` so a never-set pointer is skipped (no double - // free). `CpuInner` owns both exclusively and `Drop` runs once. `av_frame_free` takes `&mut` - // and nulls the pointer. `self.enc`/`self.hw` are freed afterward by their own `Drop` impls; - // the encoder holds its own `av_buffer_ref`'d device/frames copies, so field-drop order is - // irrelevant to soundness. - unsafe { - if !self.nv12.is_null() { - ffi::av_frame_free(&mut self.nv12); - } - if !self.sws.is_null() { - ffi::sws_freeContext(self.sws); - } - } - } -} +// No `Drop` for `CpuInner`: `nv12` (`AvFrame`) and `sws` (`AvSwsContext`) free themselves, in +// field-declaration order — the same nv12-then-sws sequence the hand-written `Drop` performed +// (see the field-order note on the struct). The encoder holds its own `av_buffer_ref`'d +// device/frames copies, so their order against `enc`/`hw` is irrelevant to soundness. // --------------------------------------------------------------------------------------------- // Zero-copy dmabuf path: DRM-PRIME → hwmap(vaapi) → scale_vaapi(nv12) filter graph → encode. @@ -1041,16 +1015,20 @@ impl DmabufInner { // whole synchronous `submit`; we describe one object/layer/plane from its // fourcc/modifier/offset/stride and its `lseek`-queried size. `libc::lseek` on that live // fd only reads the description's size and returns it (or -1); it touches no Rust memory. - // * `av_frame_alloc` → `drm` (null-checked); we set its scalar fields and - // `hw_frames_ctx = av_buffer_ref(self.drm_frames)` (new ref of the live owned ctx). + // * `drm`/`nv12` are owned `AvFrame`s — every exit drops each exactly once (the + // hand-placed frees this replaced were branch-clean, but only by inspection). We set + // `drm`'s scalar fields and `hw_frames_ctx = av_buffer_ref(self.drm_frames)` (new ref + // of the live owned ctx). // * `data[0] = Box::into_raw(desc)` transfers the box into the frame; `buf[0] = // av_buffer_create(.., free_desc, ..)` registers a destructor that reclaims it exactly once // when the buffer's refcount hits zero — matched alloc/free, no leak/double-free. // * `av_buffersrc_add_frame_flags(self.src, drm, KEEP_REF)` pushes a ref into the live - // buffersrc; KEEP_REF keeps our own `drm` ref, which we then `av_frame_free`. We pull the - // converted surface with `av_buffersink_get_frame(self.sink, nv12)` BEFORE returning, so the - // dmabuf (owned by the caller) is read while still valid. `nv12` is sent into the live owned - // `self.enc` (takes its own ref) and our ref freed once. Single-threaded encoder → no race. + // buffersrc; KEEP_REF keeps our own `drm` ref, dropped explicitly right after the push + // (the same point the hand-written free sat, kept so the descriptor's release timing + // across the pull does not change). We pull the converted surface with + // `av_buffersink_get_frame(self.sink, nv12)` BEFORE returning, so the dmabuf (owned by + // the caller) is read while still valid. `nv12` is sent into the live owned `self.enc` + // (takes its own ref) and dropped. Single-threaded encoder → no race. unsafe { // Build a DRM-PRIME AVFrame describing the dmabuf (one object/fd, one layer/plane). let mut desc: Box = Box::new(std::mem::zeroed()); @@ -1075,21 +1053,18 @@ impl DmabufInner { desc.layers[0].planes[0].offset = dmabuf.offset as isize; desc.layers[0].planes[0].pitch = dmabuf.stride as isize; - let mut drm = ffi::av_frame_alloc(); - if drm.is_null() { - bail!("av_frame_alloc(drm) failed"); - } - (*drm).format = ffi::AVPixelFormat::AV_PIX_FMT_DRM_PRIME as c_int; - (*drm).width = self.width as c_int; - (*drm).height = self.height as c_int; + let drm = AvFrame::alloc().context("av_frame_alloc(drm) failed")?; + (*drm.as_ptr()).format = ffi::AVPixelFormat::AV_PIX_FMT_DRM_PRIME as c_int; + (*drm.as_ptr()).width = self.width as c_int; + (*drm.as_ptr()).height = self.height as c_int; // The dmabuf is the compositor's rendered desktop: full-range RGB. Tag the frame so // the VPP's colour negotiation sees the real input instead of "unspecified" (an // untagged input lets the driver pick its own default for the RGB→NV12 conversion — // Mesa's is BT.601, contradicting the BT.709-limited VUI the encoder signals). - (*drm).color_range = ffi::AVColorRange::AVCOL_RANGE_JPEG; - (*drm).colorspace = ffi::AVColorSpace::AVCOL_SPC_RGB; - (*drm).hw_frames_ctx = ffi::av_buffer_ref(self.drm_frames.as_ptr()); - (*drm).data[0] = Box::into_raw(desc) as *mut u8; + (*drm.as_ptr()).color_range = ffi::AVColorRange::AVCOL_RANGE_JPEG; + (*drm.as_ptr()).colorspace = ffi::AVColorSpace::AVCOL_SPC_RGB; + (*drm.as_ptr()).hw_frames_ctx = ffi::av_buffer_ref(self.drm_frames.as_ptr()); + (*drm.as_ptr()).data[0] = Box::into_raw(desc) as *mut u8; // Own the descriptor so it frees with the frame (the fd is owned by the DmabufFrame, // which outlives this call — the graph reads the surface before submit returns). extern "C" fn free_desc(_opaque: *mut std::ffi::c_void, data: *mut u8) { @@ -1100,8 +1075,8 @@ impl DmabufInner { // reclaims it exactly once — no double-free. `_opaque` is unused (we passed null). unsafe { drop(Box::from_raw(data as *mut ffi::AVDRMFrameDescriptor)) }; } - (*drm).buf[0] = ffi::av_buffer_create( - (*drm).data[0], + (*drm.as_ptr()).buf[0] = ffi::av_buffer_create( + (*drm.as_ptr()).data[0], std::mem::size_of::(), Some(free_desc), ptr::null_mut(), @@ -1111,45 +1086,40 @@ impl DmabufInner { // Push through hwmap → scale_vaapi; pull the NV12 surface back out. let r = ffi::av_buffersrc_add_frame_flags( self.src, - drm, + drm.as_ptr(), ffi::AV_BUFFERSRC_FLAG_KEEP_REF as c_int, ); - ffi::av_frame_free(&mut drm); - // These two stages ARE the import: the push hands libav our DRM-PRIME descriptor, and - // the pull is where `hwmap` actually maps it into a VA surface (and `scale_vaapi` runs - // the CSC). A failure here means this driver would not take this compositor's dmabuf — - // which no encoder rebuild can fix — so tell the process-wide latch, and capture - // negotiates CPU frames from the next session on. `avcodec_send_frame` below is - // deliberately NOT counted: that one is the encoder stalling, which the in-place - // rebuild above us exists to recover, and disabling zero-copy over it would be a - // permanent penalty for a transient fault. + drop(drm); // release our ref where the hand-written free sat (see the SAFETY note) + // These two stages ARE the import: the push hands libav our DRM-PRIME descriptor, and + // the pull is where `hwmap` actually maps it into a VA surface (and `scale_vaapi` runs + // the CSC). A failure here means this driver would not take this compositor's dmabuf — + // which no encoder rebuild can fix — so tell the process-wide latch, and capture + // negotiates CPU frames from the next session on. `avcodec_send_frame` below is + // deliberately NOT counted: that one is the encoder stalling, which the in-place + // rebuild above us exists to recover, and disabling zero-copy over it would be a + // permanent penalty for a transient fault. if r < 0 { let e = format!("av_buffersrc_add_frame failed ({r})"); pf_zerocopy::note_raw_dmabuf_import_failure(&e); bail!("{e}"); } t_push = t0.elapsed(); - let mut nv12 = ffi::av_frame_alloc(); - if nv12.is_null() { - bail!("av_frame_alloc(nv12) failed"); - } - let r = ffi::av_buffersink_get_frame(self.sink, nv12); + let nv12 = AvFrame::alloc().context("av_frame_alloc(nv12) failed")?; + let r = ffi::av_buffersink_get_frame(self.sink, nv12.as_ptr()); if r < 0 { - ffi::av_frame_free(&mut nv12); let e = format!("av_buffersink_get_frame failed ({r})"); pf_zerocopy::note_raw_dmabuf_import_failure(&e); bail!("{e}"); } pf_zerocopy::note_raw_dmabuf_import_ok(); t_pull = t0.elapsed() - t_push; - (*nv12).pts = pts; - (*nv12).pict_type = if idr { + (*nv12.as_ptr()).pts = pts; + (*nv12.as_ptr()).pict_type = if idr { ffi::AVPictureType::AV_PICTURE_TYPE_I } else { ffi::AVPictureType::AV_PICTURE_TYPE_NONE }; - let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), nv12); - ffi::av_frame_free(&mut nv12); + let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), nv12.as_ptr()); if r < 0 { bail!("avcodec_send_frame(VAAPI) failed ({r})"); } diff --git a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs index c536b316..14c10b2b 100644 --- a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs +++ b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs @@ -59,8 +59,8 @@ use windows::Win32::Graphics::Dxgi::Common::{ }; use super::libav::{ - apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, PollOutcome, SWS_CS_BT2020, - SWS_CS_ITU709, SWS_POINT, + apply_low_latency_rc, pixel_to_av, poll_encoder, AvBuffer, AvFrame, AvSwsContext, PollOutcome, + SWS_CS_BT2020, SWS_CS_ITU709, SWS_POINT, }; use ffmpeg::ffi; // = ffmpeg_sys_next @@ -497,10 +497,14 @@ fn immediate_context(device: &ID3D11Device) -> ID3D11DeviceContext { struct SystemInner { enc: encoder::video::Encoder, + // FIELD ORDER IS LOAD-BEARING: the hand-written `Drop` this replaced freed `sw_frame` + // before `sws`, and field-DECLARATION order is what preserves that now (an offset_of assert + // cannot pin this — repr(Rust) may reorder memory independently of declaration order, and + // drop order follows declaration). /// Reusable software NV12/P010 frame: swscale dst / readback dst, and the `send_frame` src. - sw_frame: *mut ffi::AVFrame, - /// swscale ctx for the BGRA→NV12 fallback (built lazily; null for the YUV-readback path). - sws: *mut ffi::SwsContext, + sw_frame: AvFrame, + /// swscale ctx for the BGRA→NV12 fallback (built lazily; `None` for the YUV-readback path). + sws: Option, /// CPU-readable staging texture for the D3D11 readback (built lazily on the captured device). staging: Option, ctx: Option, @@ -547,26 +551,18 @@ impl SystemInner { ptr::null_mut(), )? }; - // SAFETY: `av_frame_alloc` returns a freshly-allocated, uniquely-owned `AVFrame` (null-checked - // before any deref); writing `format`/`width`/`height` through `*f` stays inside that - // allocation. `av_frame_get_buffer(f, 0)` allocates the backing planes — on failure we - // `av_frame_free` the sole owner (no double-free) and bail; on success the raw `f` is moved into - // `self.sw_frame` and freed exactly once in `Drop`. - let sw_frame = unsafe { - let f = ffi::av_frame_alloc(); - if f.is_null() { - bail!("av_frame_alloc(sw) failed"); - } - (*f).format = sw_av as c_int; - (*f).width = width as c_int; - (*f).height = height as c_int; - if ffi::av_frame_get_buffer(f, 0) < 0 { - let mut f = f; - ffi::av_frame_free(&mut f); + let sw_frame = AvFrame::alloc().context("av_frame_alloc(sw) failed")?; + // SAFETY: writing `format`/`width`/`height` through the owned frame's pointer stays inside + // its allocation. `av_frame_get_buffer` allocates the backing planes — on failure the + // owned `sw_frame` simply drops (freed once, by the wrapper). + unsafe { + (*sw_frame.as_ptr()).format = sw_av as c_int; + (*sw_frame.as_ptr()).width = width as c_int; + (*sw_frame.as_ptr()).height = height as c_int; + if ffi::av_frame_get_buffer(sw_frame.as_ptr(), 0) < 0 { bail!("av_frame_get_buffer(sw) failed"); } - f - }; + } tracing::info!( encoder = vendor.encoder_name(codec), "{} encode active ({width}x{height}@{fps}, system-memory {} path)", @@ -576,7 +572,7 @@ impl SystemInner { Ok(SystemInner { enc, sw_frame, - sws: ptr::null_mut(), + sws: None, staging: None, ctx: None, format, @@ -632,13 +628,13 @@ impl SystemInner { // frame and `self.enc`'s own context, both live for the call and neither retained by libav // (it references the frame's buffers itself). unsafe { - (*self.sw_frame).pts = pts; - (*self.sw_frame).pict_type = if idr { + (*self.sw_frame.as_ptr()).pts = pts; + (*self.sw_frame.as_ptr()).pict_type = if idr { ffi::AVPictureType::AV_PICTURE_TYPE_I } else { ffi::AVPictureType::AV_PICTURE_TYPE_NONE }; - let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), self.sw_frame); + let r = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), self.sw_frame.as_ptr()); if r < 0 { bail!("avcodec_send_frame({} system) failed ({r})", "ffmpeg_win"); } @@ -703,10 +699,10 @@ impl SystemInner { let total = pitch.saturating_mul(h + h.div_ceil(2)); let mapped = std::slice::from_raw_parts(base, total); let chroma_off = pitch * h; - let y_dst = (*self.sw_frame).data[0]; - let y_stride = (*self.sw_frame).linesize[0] as usize; - let uv_dst = (*self.sw_frame).data[1]; - let uv_stride = (*self.sw_frame).linesize[1] as usize; + let y_dst = (*self.sw_frame.as_ptr()).data[0]; + let y_stride = (*self.sw_frame.as_ptr()).linesize[0] as usize; + let uv_dst = (*self.sw_frame.as_ptr()).data[1]; + let uv_stride = (*self.sw_frame.as_ptr()).linesize[1] as usize; for y in 0..h { let s = &mapped[y * pitch..y * pitch + row_bytes]; ptr::copy_nonoverlapping(s.as_ptr(), y_dst.add(y * y_stride), row_bytes); @@ -746,7 +742,7 @@ impl SystemInner { let pitch = map.RowPitch as usize; let h = self.height as usize; let base = map.pData as *const u8; - self.ensure_sws( + let sws = self.ensure_sws( pixel_to_av(Pixel::BGRA), ffi::AVPixelFormat::AV_PIX_FMT_NV12, SWS_CS_ITU709, @@ -754,13 +750,13 @@ impl SystemInner { let src_data: [*const u8; 4] = [base, ptr::null(), ptr::null(), ptr::null()]; let src_stride: [c_int; 4] = [pitch as c_int, 0, 0, 0]; let r = ffi::sws_scale( - self.sws, + sws, src_data.as_ptr(), src_stride.as_ptr(), 0, h as c_int, - (*self.sw_frame).data.as_ptr(), - (*self.sw_frame).linesize.as_ptr(), + (*self.sw_frame.as_ptr()).data.as_ptr(), + (*self.sw_frame.as_ptr()).linesize.as_ptr(), ); ctx.Unmap(&staging, 0); if r < 0 { @@ -796,7 +792,7 @@ impl SystemInner { let h = self.height as usize; let base = map.pData as *const u8; // RGB(BT.2020 PQ) → YUV(BT.2020 PQ): a matrix-only repack (same PQ transfer), full→limited. - self.ensure_sws( + let sws = self.ensure_sws( ffi::AVPixelFormat::AV_PIX_FMT_X2BGR10LE, ffi::AVPixelFormat::AV_PIX_FMT_P010LE, SWS_CS_BT2020, @@ -804,13 +800,13 @@ impl SystemInner { let src_data: [*const u8; 4] = [base, ptr::null(), ptr::null(), ptr::null()]; let src_stride: [c_int; 4] = [pitch as c_int, 0, 0, 0]; let r = ffi::sws_scale( - self.sws, + sws, src_data.as_ptr(), src_stride.as_ptr(), 0, h as c_int, - (*self.sw_frame).data.as_ptr(), - (*self.sw_frame).linesize.as_ptr(), + (*self.sw_frame.as_ptr()).data.as_ptr(), + (*self.sw_frame.as_ptr()).linesize.as_ptr(), ); ctx.Unmap(&staging, 0); if r < 0 { @@ -842,7 +838,7 @@ impl SystemInner { // `width`×`height`). `bytes` is borrowed for the call only and never aliases the owned // `sw_frame`. `send` then hands `sw_frame` to the encoder. unsafe { - self.ensure_sws( + let sws = self.ensure_sws( pixel_to_av(sws_src(format)?), ffi::AVPixelFormat::AV_PIX_FMT_NV12, SWS_CS_ITU709, @@ -850,13 +846,13 @@ impl SystemInner { let src_data: [*const u8; 4] = [bytes.as_ptr(), ptr::null(), ptr::null(), ptr::null()]; let src_stride: [c_int; 4] = [src_row as c_int, 0, 0, 0]; if ffi::sws_scale( - self.sws, + sws, src_data.as_ptr(), src_stride.as_ptr(), 0, h as c_int, - (*self.sw_frame).data.as_ptr(), - (*self.sw_frame).linesize.as_ptr(), + (*self.sw_frame.as_ptr()).data.as_ptr(), + (*self.sw_frame.as_ptr()).linesize.as_ptr(), ) < 0 { bail!("sws_scale RGB→NV12 failed"); @@ -870,23 +866,24 @@ impl SystemInner { /// 10-bit RGB10→P010 BT.2020), so caching a single context is sound. /// /// Safe: every argument is a plain libav enum/int, and the context it caches belongs to `self` - /// (freed once in `Drop`). + /// (an owned `AvSwsContext`, freed by its own drop). Returns the borrowed pointer for the + /// caller's `sws_scale` — borrowed only, `self.sws` stays the owner. fn ensure_sws( &mut self, src_av: ffi::AVPixelFormat, dst_av: ffi::AVPixelFormat, cs: c_int, - ) -> Result<()> { - if !self.sws.is_null() { - return Ok(()); + ) -> Result<*mut ffi::SwsContext> { + if let Some(sws) = &self.sws { + return Ok(sws.as_ptr()); } // SAFETY: `sws_getContext` takes only scalars plus the documented "no filters, no params" - // null trio, and returns an owned context or null — which is checked before use, so - // `sws_setColorspaceDetails` and the store below only ever see a live one. - // `sws_getCoefficients` returns a pointer into libav's own static tables, valid for the - // process, and the call only reads it. + // null trio, and returns an owned context or null — `from_raw` rejects the null, so + // `sws_setColorspaceDetails` only ever sees a live one, and ownership passes to the + // `AvSwsContext`. `sws_getCoefficients` returns a pointer into libav's own static tables, + // valid for the process, and the call only reads it. let sws = unsafe { - let sws = ffi::sws_getContext( + let raw = ffi::sws_getContext( self.width as c_int, self.height as c_int, src_av, @@ -898,36 +895,22 @@ impl SystemInner { ptr::null_mut(), ptr::null(), ); - if sws.is_null() { + let Some(owned) = AvSwsContext::from_raw(raw) else { bail!("sws_getContext(RGB→YUV) failed"); - } + }; // Source full-range RGB → destination limited-range YUV (matches the limited-range VUI // we signal). For RGB input the src coefficient table is unused; pass dst for both. let coeff = ffi::sws_getCoefficients(cs); - ffi::sws_setColorspaceDetails(sws, coeff, 1, coeff, 0, 0, 1 << 16, 1 << 16); - sws + ffi::sws_setColorspaceDetails(owned.as_ptr(), coeff, 1, coeff, 0, 0, 1 << 16, 1 << 16); + owned }; - self.sws = sws; - Ok(()) + Ok(self.sws.insert(sws).as_ptr()) } } -impl Drop for SystemInner { - fn drop(&mut self) { - // SAFETY: `sw_frame` is the `AVFrame` allocated in `open` (or null) — `av_frame_free` drops it - // once and nulls the pointer through the `&mut`; `sws` is the cached `SwsContext` (or null) — - // `sws_freeContext` frees it once. This `Drop` runs exactly once and `SystemInner` owns both - // exclusively, so there is no double-free or use-after-free. - unsafe { - if !self.sw_frame.is_null() { - ffi::av_frame_free(&mut self.sw_frame); - } - if !self.sws.is_null() { - ffi::sws_freeContext(self.sws); - } - } - } -} +// No `Drop` for `SystemInner`: `sw_frame` (`AvFrame`) and `sws` (`Option`) free +// themselves, in field-declaration order — the same sw_frame-then-sws sequence the hand-written +// `Drop` performed, pinned by the offset_of assert at the struct. // --------------------------------------------------------------------------------------------- // Zero-copy D3D11 path (the AMF default; QSV opt-in — see `zerocopy_enabled`): share the capture @@ -1212,32 +1195,29 @@ impl ZeroCopyInner { } fn submit(&mut self, frame: &D3d11Frame, pts: i64, idr: bool) -> Result<()> { - // SAFETY: `d3d = av_frame_alloc()` is a fresh owned frame (null-checked) and is `av_frame_free`d - // exactly once on every path below. `av_hwframe_get_buffer` fills it from the pool — on failure - // we free it and bail. `(*d3d).data[0]` is the pool's texture-array and `data[1]` the array - // index; `from_raw_borrowed` borrows that `ID3D11Texture2D` WITHOUT taking ownership (no Release - // — the frame owns it) and is null-checked. `src` (the captured texture) and `dst` (the pooled - // slice) live on the SAME D3D11 device wrapped by `self.hw`, and the caller guarantees - // `captured.format == pool_format` before calling, so `CopySubresourceRegion(dst, dst_index, .., - // src, 0, ..)` on the single-threaded immediate context `self.ctx` is a valid same-format GPU - // copy. For QSV the mapped `qsv` frame is a fresh owned frame whose `hw_frames_ctx` takes an - // `av_buffer_ref` of `self.qsv_frames`; it is `av_frame_free`d (releasing that ref) on both the - // map-failure and success paths. `avcodec_send_frame` only internally refs the input frame, so - // the `av_frame_free(d3d)`/`av_frame_free(qsv)` afterwards are the sole owning frees — no leak, - // no double-free, no use-after-free. + // SAFETY: `d3d`/`qsv` are owned `AvFrame`s, so EVERY exit — including the three `?` exits + // between the pool pull and the send, which as hand-placed frees previously leaked the + // frame plus one of the POOL-sized hwframe surfaces per failure (eight failures wedged + // the encoder permanently) — unrefs the pooled surface back to the pool. `(*d3d).data[0]` + // is the pool's texture-array and `data[1]` the array index; `from_raw_borrowed` borrows + // that `ID3D11Texture2D` WITHOUT taking ownership (no Release — the frame owns it) and is + // null-checked. `src` (the captured texture) and `dst` (the pooled slice) live on the + // SAME D3D11 device wrapped by `self.hw`, and the caller guarantees `captured.format == + // pool_format` before calling, so `CopySubresourceRegion(dst, dst_index, .., src, 0, ..)` + // on the single-threaded immediate context `self.ctx` is a valid same-format GPU copy. + // For QSV the mapped `qsv` frame's `hw_frames_ctx` takes an `av_buffer_ref` of + // `self.qsv_frames`; its drop at the end of the arm releases that ref at the same point + // the hand-written free did. `avcodec_send_frame` only internally refs the input frame, + // so the drops are the sole owning frees — no leak, no double-free, no use-after-free. unsafe { // Pull a pooled D3D11 surface; its data[0] is the pool's texture-ARRAY, data[1] the slice. - let mut d3d = ffi::av_frame_alloc(); - if d3d.is_null() { - bail!("av_frame_alloc(d3d11) failed"); - } - let r = ffi::av_hwframe_get_buffer(self.hw.frames_ref.as_ptr(), d3d, 0); + let d3d = AvFrame::alloc().context("av_frame_alloc(d3d11) failed")?; + let r = ffi::av_hwframe_get_buffer(self.hw.frames_ref.as_ptr(), d3d.as_ptr(), 0); if r < 0 { - ffi::av_frame_free(&mut d3d); bail!("av_hwframe_get_buffer(D3D11) failed ({r})"); } - let dst_ptr = (*d3d).data[0] as *mut c_void; - let dst_index = (*d3d).data[1] as usize as u32; + let dst_ptr = (*d3d.as_ptr()).data[0] as *mut c_void; + let dst_index = (*d3d.as_ptr()).data[1] as usize as u32; let dst_tex = ID3D11Texture2D::from_raw_borrowed(&dst_ptr) .ok_or_else(|| anyhow!("pooled D3D11 frame has null texture"))?; // GPU-local copy of the captured slice into the pooled array slice (like NVENC's CUDA @@ -1247,58 +1227,50 @@ impl ZeroCopyInner { self.ctx .CopySubresourceRegion(&dst, dst_index, 0, 0, 0, &src, 0, None); - (*d3d).pts = pts; - (*d3d).pict_type = if idr { + (*d3d.as_ptr()).pts = pts; + (*d3d.as_ptr()).pict_type = if idr { ffi::AVPictureType::AV_PICTURE_TYPE_I } else { ffi::AVPictureType::AV_PICTURE_TYPE_NONE }; let send = match self.vendor { - WinVendor::Amf => ffi::avcodec_send_frame(self.enc.as_mut_ptr(), d3d), + WinVendor::Amf => ffi::avcodec_send_frame(self.enc.as_mut_ptr(), d3d.as_ptr()), WinVendor::Qsv => { // Map the D3D11 frame to a QSV surface (1:1, no copy), then send the mapped frame. - let mut qsv = ffi::av_frame_alloc(); - if qsv.is_null() { - ffi::av_frame_free(&mut d3d); - bail!("av_frame_alloc(qsv) failed"); - } + let qsv = AvFrame::alloc().context("av_frame_alloc(qsv) failed")?; // Always `Some` on this arm — `open` fills the pair for `WinVendor::Qsv` and // leaves it `None` only for AMF — but say so with a bail rather than an unwrap, // matching the null check above it. The `Option` is what the raw pointer's // "null means AMF" convention was already encoding. let Some(qsv_frames) = self.qsv_frames.as_ref() else { - ffi::av_frame_free(&mut qsv); - ffi::av_frame_free(&mut d3d); bail!("QSV send path without a derived QSV frames context"); }; - (*qsv).format = ffi::AVPixelFormat::AV_PIX_FMT_QSV as c_int; - (*qsv).hw_frames_ctx = ffi::av_buffer_ref(qsv_frames.as_ptr()); + (*qsv.as_ptr()).format = ffi::AVPixelFormat::AV_PIX_FMT_QSV as c_int; + (*qsv.as_ptr()).hw_frames_ctx = ffi::av_buffer_ref(qsv_frames.as_ptr()); // The map flags are a bindgen enum (no BitOr) — cast each to int before OR-ing. let r = ffi::av_hwframe_map( - qsv, - d3d, + qsv.as_ptr(), + d3d.as_ptr(), ffi::AV_HWFRAME_MAP_DIRECT as c_int | ffi::AV_HWFRAME_MAP_READ as c_int, ); if r < 0 { - ffi::av_frame_free(&mut qsv); - ffi::av_frame_free(&mut d3d); bail!("av_hwframe_map(D3D11→QSV) failed ({r})"); } - (*qsv).pts = pts; - (*qsv).pict_type = (*d3d).pict_type; - let s = ffi::avcodec_send_frame(self.enc.as_mut_ptr(), qsv); - ffi::av_frame_free(&mut qsv); - s + (*qsv.as_ptr()).pts = pts; + (*qsv.as_ptr()).pict_type = (*d3d.as_ptr()).pict_type; + ffi::avcodec_send_frame(self.enc.as_mut_ptr(), qsv.as_ptr()) + // `qsv` drops here — releasing the mapped frame and its frames-ctx ref at the + // same point the hand-written `av_frame_free(&mut qsv)` did. } }; - ffi::av_frame_free(&mut d3d); if send < 0 { bail!( "avcodec_send_frame({}) failed ({send})", self.vendor.label() ); } + // `d3d` drops here (and on every early exit above), returning the pooled surface. } Ok(()) } diff --git a/crates/punktfunk-core/src/abi.rs b/crates/punktfunk-core/src/abi.rs index 73f9f2f7..2982490f 100644 --- a/crates/punktfunk-core/src/abi.rs +++ b/crates/punktfunk-core/src/abi.rs @@ -506,8 +506,10 @@ pub unsafe extern "C" fn punktfunk_client_poll_frame( /// Client: serialize and send one input event to the host. /// +/// Returns `InvalidArg` if `ev->kind` is not a recognized event kind. +/// /// # Safety -/// `s` is a valid client handle; `ev` points to a valid [`InputEvent`]. +/// `s` is a valid client handle; `ev` points to a readable `InputEvent`-sized allocation. #[no_mangle] pub unsafe extern "C" fn punktfunk_send_input( s: *mut PunktfunkSession, @@ -521,12 +523,11 @@ pub unsafe extern "C" fn punktfunk_send_input( Some(s) => s, None => return PunktfunkStatus::NullPointer, }; - // SAFETY: per the ABI contract - an opaque handle from a `*_new`/`*_pair` that the caller - // has not yet freed, or null, which `as_mut`/`as_ref` reports as `None` and the `match` - // here handles. - let ev = match unsafe { ev.as_ref() } { - Some(e) => e, - None => return PunktfunkStatus::NullPointer, + // SAFETY: `read_input_event` upholds this file's failures-become-status-codes principle + // for the one field where a reference formed too early would be UB instead. + let ev = match unsafe { read_input_event(ev) } { + Ok(e) => e, + Err(status) => return status, }; match s.inner.send_input(ev) { Ok(()) => PunktfunkStatus::Ok, @@ -535,6 +536,31 @@ pub unsafe extern "C" fn punktfunk_send_input( }) } +/// Validate caller memory as an [`InputEvent`] WITHOUT forming the reference first. +/// +/// `InputEvent.kind` is a `#[repr(u8)]` enum with 16 valid discriminants, and a C embedder +/// writing `ev->kind = 42` is not a decodable error once `&InputEvent` exists — forming the +/// reference IS the UB, by the language's validity rule. So the tag is read as a raw byte and +/// validated through the same `InputKind::from_u8` the wire path uses (`input.rs::decode`), +/// and the typed reference comes into existence only afterwards. Every other field is a plain +/// integer (or the `[u8; 3]` pad), valid for any bit pattern. +/// +/// # Safety +/// `ev` is null (reported as a status) or readable for `size_of::()` bytes. +unsafe fn read_input_event<'a>(ev: *const InputEvent) -> Result<&'a InputEvent, PunktfunkStatus> { + if ev.is_null() { + return Err(PunktfunkStatus::NullPointer); + } + // SAFETY: non-null per the check above, readable per this fn's contract; a one-byte read + // at offset 0 (the `kind` tag — repr(C) puts it first) cannot itself be UB for any value. + if crate::input::InputKind::from_u8(unsafe { ev.cast::().read() }).is_none() { + return Err(PunktfunkStatus::InvalidArg); + } + // SAFETY: non-null, readable, and the discriminant byte was just validated — every field + // of the repr(C) struct now holds a valid bit pattern for its type. + Ok(unsafe { &*ev }) +} + /// Register the host-side input callback (pass a NULL fn pointer to clear). The callback /// fires from within [`punktfunk_host_poll_input`], on the calling thread. /// @@ -3372,8 +3398,10 @@ pub unsafe extern "C" fn punktfunk_connection_shard_payload( /// Send one input event to the host as a QUIC datagram (non-blocking enqueue). /// +/// Returns `InvalidArg` if `ev->kind` is not a recognized event kind. +/// /// # Safety -/// `c` is a valid connection handle; `ev` points to a valid [`InputEvent`]. +/// `c` is a valid connection handle; `ev` points to a readable `InputEvent`-sized allocation. #[cfg(feature = "quic")] #[no_mangle] pub unsafe extern "C" fn punktfunk_connection_send_input( @@ -3388,12 +3416,11 @@ pub unsafe extern "C" fn punktfunk_connection_send_input( Some(c) => c, None => return PunktfunkStatus::NullPointer, }; - // SAFETY: per the ABI contract - an opaque handle from a `*_new`/`*_pair` that the caller - // has not yet freed, or null, which `as_mut`/`as_ref` reports as `None` and the `match` - // here handles. - let ev = match unsafe { ev.as_ref() } { - Some(e) => e, - None => return PunktfunkStatus::NullPointer, + // SAFETY: `read_input_event` upholds this file's failures-become-status-codes principle + // for the one field where a reference formed too early would be UB instead. + let ev = match unsafe { read_input_event(ev) } { + Ok(e) => e, + Err(status) => return status, }; match c.inner.send_input(ev) { Ok(()) => PunktfunkStatus::Ok, @@ -4818,6 +4845,30 @@ pub unsafe extern "C" fn punktfunk_reanchor_gate_is_holding( mod tests { use super::*; + /// A C embedder writing `ev->kind = 42` must come back as a status code, not UB. The test + /// stages the event in `MaybeUninit` storage so no `&InputEvent` to an invalid value ever + /// exists on the test's own side either. + #[test] + fn read_input_event_rejects_null_and_bad_discriminant() { + // SAFETY: null is the documented reported-not-UB case. + let null_result = unsafe { read_input_event(std::ptr::null()) }; + assert_eq!(null_result.unwrap_err(), PunktfunkStatus::NullPointer); + + let mut slot = core::mem::MaybeUninit::::zeroed(); + let p = slot.as_mut_ptr(); + // SAFETY: writing one byte at offset 0 of aligned, sized storage. + unsafe { p.cast::().write(42) }; + // SAFETY: `p` is aligned and readable for the full struct. + let bad_tag = unsafe { read_input_event(p) }; + assert_eq!(bad_tag.unwrap_err(), PunktfunkStatus::InvalidArg); + + // SAFETY: as above; tag 0 (KeyDown) + zeroed fields is a fully valid event. + unsafe { p.cast::().write(0) }; + // SAFETY: as above. + let ev = unsafe { read_input_event(p) }.expect("valid tag must pass"); + assert_eq!(ev.kind, crate::input::InputKind::KeyDown); + } + /// The `AudioCtl` → `PunktfunkHidOutput` mapping: kind 5, pad narrowed, `which` carries the /// flags byte, `effect[0..6]` the raw audio region with `effect_len = 6` (the TrackpadHaptic /// packing idiom — no struct growth, so the size guard above stays at 19). diff --git a/crates/punktfunk-core/tests/c/run.sh b/crates/punktfunk-core/tests/c/run.sh index dd0b368e..81f494e4 100755 --- a/crates/punktfunk-core/tests/c/run.sh +++ b/crates/punktfunk-core/tests/c/run.sh @@ -11,24 +11,52 @@ profile="${1:-debug}" build_flag="" [ "$profile" = "release" ] && build_flag="--release" -echo ">> building punktfunk-core staticlib ($profile)" -cargo build -p punktfunk-core $build_flag >/dev/null +# PF_SAN=address instruments BOTH sides of the C boundary at once: the staticlib via +# -Zsanitizer (nightly + -Zbuild-std, so std itself is instrumented) and the harness via +# clang -fsanitize. LSAN rides along (detect_leaks=1) and is the only automated check on +# the Box::into_raw/from_raw leak contract in abi.rs. Linux x86_64 only; -Zbuild-std +# defeats sccache, so this belongs on a cron/dispatch job, not the per-push leg. +san="${PF_SAN:-}" +toolchain="" +target_args="" +target_sub="" +if [ -n "$san" ]; then + san_target="x86_64-unknown-linux-gnu" + # -Zsanitizer/-Zbuild-std need a nightly; PF_SAN_TOOLCHAIN pins a dated one (CI does). + toolchain="+${PF_SAN_TOOLCHAIN:-nightly}" + target_args="-Z build-std --target $san_target" + target_sub="$san_target/" + export RUSTFLAGS="-Zsanitizer=$san${RUSTFLAGS:+ $RUSTFLAGS}" +fi -staticlib="$ws/target/$profile/libpunktfunk_core.a" +echo ">> building punktfunk-core staticlib ($profile${san:+, sanitizer=$san})" +cargo $toolchain build $target_args -p punktfunk-core $build_flag >/dev/null + +staticlib="$ws/target/${target_sub}$profile/libpunktfunk_core.a" header_dir="$ws/include" [ -f "$staticlib" ] || { echo "missing $staticlib"; exit 1; } [ -f "$header_dir/punktfunk_core.h" ] || { echo "missing generated header"; exit 1; } # Ask rustc what native libs the staticlib needs to link into a C program. -native_libs="$(cargo rustc -p punktfunk-core --lib --crate-type staticlib $build_flag -- \ +native_libs="$(cargo $toolchain rustc $target_args -p punktfunk-core --lib --crate-type staticlib $build_flag -- \ --print native-static-libs 2>&1 | sed -n 's/.*native-static-libs: //p' | tail -1)" echo ">> native libs: ${native_libs:-}" -out="$(mktemp -d)/punktfunk_harness" +# Not mktemp: a debug+ASAN static binary can exceed a tmpfs /tmp; target/ is real disk. +out="$ws/target/${target_sub}$profile/punktfunk_harness" cc="${CC:-cc}" +cflags="" +if [ -n "$san" ]; then + cc="${CC:-clang}" + cflags="-fsanitize=$san -fno-omit-frame-pointer" +fi echo ">> compiling + linking harness" -$cc -std=c11 -Wall -Wextra -O2 -I "$header_dir" \ +$cc -std=c11 -Wall -Wextra -O2 $cflags ${CFLAGS:-} -I "$header_dir" \ "$here/harness.c" "$staticlib" $native_libs -o "$out" echo ">> running" -"$out" +if [ -n "$san" ]; then + ASAN_OPTIONS="detect_leaks=1${ASAN_OPTIONS:+:$ASAN_OPTIONS}" "$out" +else + "$out" +fi diff --git a/crates/punktfunk-host/src/hooks.rs b/crates/punktfunk-host/src/hooks.rs index 2829572b..8fa867e0 100644 --- a/crates/punktfunk-host/src/hooks.rs +++ b/crates/punktfunk-host/src/hooks.rs @@ -508,15 +508,25 @@ fn running_as_system() -> bool { if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) }.is_err() { return true; // fail closed } - let mut buf = [0u8; 256]; + // TOKEN_USER is align-8; a bare `[u8; 256]` is align-1, and forming `&TOKEN_USER` out of it + // below would be UB by the language rule whenever the stack slot happens to land misaligned. + // (Shipped codegen happens to 8-align it today — that is luck, not a guarantee.) The wrapper + // keeps the buffer at 256 BYTES: redeclaring as `[u64; 32]` would silently turn the length + // argument below into 32 — `len()` counts elements — and a console operator's 44-byte + // TOKEN_USER+SID would then fail with ERROR_INSUFFICIENT_BUFFER, misclassifying every + // hand-run host as SYSTEM (it fits exactly for SYSTEM's own 16-byte S-1-5-18, so a + // SYSTEM-side test would not catch it). + #[repr(align(8))] + struct TokenUserBuf([u8; 256]); + let mut buf = TokenUserBuf([0u8; 256]); let mut len = 0u32; // SAFETY: `buf` is a writable local of the length passed; `len` is a live out-param. let got = unsafe { GetTokenInformation( token, TokenUser, - Some(buf.as_mut_ptr().cast()), - buf.len() as u32, + Some(buf.0.as_mut_ptr().cast()), + std::mem::size_of_val(&buf) as u32, &mut len, ) }; @@ -542,11 +552,22 @@ fn running_as_system() -> bool { { return true; // fail closed } - // SAFETY: `buf` holds a TOKEN_USER written by GetTokenInformation; its `User.Sid` points into - // the same buffer, and both SIDs are valid for this comparison. + // SAFETY: `buf` holds a TOKEN_USER written by GetTokenInformation (align guaranteed by + // TokenUserBuf); its `User.Sid` points into the same buffer, and both SIDs are valid for + // this comparison. unsafe { - let tu = &*(buf.as_ptr() as *const TOKEN_USER); - EqualSid(tu.User.Sid, PSID(system.as_mut_ptr().cast())).is_ok() + let tu = &*(buf.0.as_ptr() as *const TOKEN_USER); + // windows-rs maps EqualSid's BOOL(0) to Err BOTH for "SIDs differ" and for a genuine + // failure, telling them apart only via GetLastError — so clear it first (a stale value + // from an earlier call would otherwise read as failure) and split three ways. `.is_ok()` + // here previously meant an EqualSid ERROR yielded "not SYSTEM" — the fail-OPEN + // direction, contradicting the contract in the doc comment above. + windows::Win32::Foundation::SetLastError(windows::Win32::Foundation::WIN32_ERROR(0)); + match EqualSid(tu.User.Sid, PSID(system.as_mut_ptr().cast())) { + Ok(()) => true, // equal: we are SYSTEM + Err(e) if e.code().is_ok() => false, // BOOL(0), last-error 0: genuinely not equal + Err(_) => true, // EqualSid itself failed: fail closed + } } } diff --git a/include/punktfunk_core.h b/include/punktfunk_core.h index 2f62ab3d..415aa539 100644 --- a/include/punktfunk_core.h +++ b/include/punktfunk_core.h @@ -2259,8 +2259,10 @@ PunktfunkStatus punktfunk_client_poll_frame(PunktfunkSession *s, PunktfunkFrame // Client: serialize and send one input event to the host. // +// Returns `InvalidArg` if `ev->kind` is not a recognized event kind. +// // # Safety -// `s` is a valid client handle; `ev` points to a valid [`InputEvent`]. +// `s` is a valid client handle; `ev` points to a readable `InputEvent`-sized allocation. PunktfunkStatus punktfunk_send_input(PunktfunkSession *s, const PunktfunkInputEvent *ev); // Register the host-side input callback (pass a NULL fn pointer to clear). The callback @@ -3024,8 +3026,10 @@ PunktfunkStatus punktfunk_connection_shard_payload(PunktfunkConnection *c, uint3 #if defined(PUNKTFUNK_FEATURE_QUIC) // Send one input event to the host as a QUIC datagram (non-blocking enqueue). // +// Returns `InvalidArg` if `ev->kind` is not a recognized event kind. +// // # Safety -// `c` is a valid connection handle; `ev` points to a valid [`InputEvent`]. +// `c` is a valid connection handle; `ev` points to a readable `InputEvent`-sized allocation. PunktfunkStatus punktfunk_connection_send_input(PunktfunkConnection *c, const PunktfunkInputEvent *ev); #endif