From e20b614059120c66fcc24946f385854582db52b0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 23:57:59 +0200 Subject: [PATCH 1/5] chore(safety): PF_SAN sanitizer gate for the C ABI harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PF_SAN=address builds the punktfunk-core staticlib on nightly with -Zsanitizer/-Zbuild-std and the C harness with clang -fsanitize, so ASAN instruments both sides of the boundary at once and LSAN (detect_leaks=1) becomes the first automated check on abi.rs's Box::into_raw/from_raw leak contract. Verified on the .25 box: green run passes byte-exact; deleting one punktfunk_session_free() in the harness makes LSAN report the 308 Rust-side allocations behind the handle and the script exit 1. The harness binary moves from mktemp to target/ — a debug+ASAN static binary can exceed a tmpfs /tmp (it did, on .25's 3.6G tmpfs). --- crates/punktfunk-core/tests/c/run.sh | 41 +++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/crates/punktfunk-core/tests/c/run.sh b/crates/punktfunk-core/tests/c/run.sh index dd0b368e..0cb39ab0 100755 --- a/crates/punktfunk-core/tests/c/run.sh +++ b/crates/punktfunk-core/tests/c/run.sh @@ -11,24 +11,51 @@ 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" + 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 -- 2.54.0 From c3b57438e1398ce36034bc951e373e89a44eb2bd Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 12 Aug 2026 00:02:26 +0200 Subject: [PATCH 2/5] =?UTF-8?q?chore(ci):=20c-abi-asan=20job=20in=20audit.?= =?UTF-8?q?yml=20=E2=80=94=20the=20harness=20under=20ASAN+LSAN,=20weekly?= =?UTF-8?q?=20+=20on=20demand?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same shape as the miri job (dated nightly, own san- cache prefixes, non-blocking day one via a step-level ::warning::, a proved-it-ran grep). run.sh gains PF_SAN_TOOLCHAIN so CI can pin its dated nightly — bare +nightly would ask for the rolling channel the job never installs. Both the pinned and vanilla paths re-verified green on .25. --- .gitea/workflows/audit.yml | 82 ++++++++++++++++++++++++++++ crates/punktfunk-core/tests/c/run.sh | 3 +- 2 files changed, 84 insertions(+), 1 deletion(-) 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/punktfunk-core/tests/c/run.sh b/crates/punktfunk-core/tests/c/run.sh index 0cb39ab0..81f494e4 100755 --- a/crates/punktfunk-core/tests/c/run.sh +++ b/crates/punktfunk-core/tests/c/run.sh @@ -22,7 +22,8 @@ target_args="" target_sub="" if [ -n "$san" ]; then san_target="x86_64-unknown-linux-gnu" - toolchain="+nightly" + # -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}" -- 2.54.0 From e8c306b9c06122965b517979f398d5683d23b635 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 12 Aug 2026 00:12:00 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(punktfunk-host):=20WP3c/3d=20=E2=80=94?= =?UTF-8?q?=20align=20the=20TOKEN=5FUSER=20buffer,=20make=20EqualSid=20fai?= =?UTF-8?q?l=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3c: forming &TOKEN_USER (align 8) out of a bare [u8; 256] (align 1) was UB by the validity rule whenever the stack slot landed misaligned — shipped codegen happened to 8-align it, which is luck, not a contract. Fixed with a repr(align(8)) wrapper that keeps the buffer at 256 BYTES; the comment records why [u64; 32] is the wrong shape (len() would silently become 32 and misclassify every hand-run host as SYSTEM via ERROR_INSUFFICIENT_BUFFER, invisibly to a SYSTEM-side test). Length arg now size_of_val. 3d: EqualSid().is_ok() read BOTH 'SIDs differ' and 'EqualSid failed' as Err, so a genuine failure yielded 'not SYSTEM' — the fail-OPEN direction, contradicting the documented fail-closed contract. Now split three ways on the last-error code, with SetLastError(0) cleared first so a stale value cannot misclassify. Gate: cargo check -p punktfunk-host + cargo clippy --release -D warnings both green on .133 (real MSVC, fresh extraction, sentinel-verified). --- crates/punktfunk-host/src/hooks.rs | 35 ++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 7 deletions(-) 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 + } } } -- 2.54.0 From 9a59504ba4cdfecc6727dd9da22482d38b0699bd Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 12 Aug 2026 00:12:01 +0200 Subject: [PATCH 4/5] fix(punktfunk-core): validate InputKind before forming &InputEvent in the C ABI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit abi.rs's two send-input entry points built &InputEvent straight out of caller memory with ev.as_ref(); InputKind is repr(u8) with 16 valid discriminants, so a C embedder writing ev->kind = 42 was immediate UB the moment the reference formed — in a file whose stated principle is that failures become status codes. New read_input_event() checks null, reads the tag as a raw byte, validates through the same InputKind::from_u8 the wire path uses, and only then forms the reference; bad tags return InvalidArg. Every other field is a plain integer, valid for any pattern. Test stages the event in MaybeUninit storage so the test itself never holds a reference to the invalid value. 380 lib tests + the C harness round-trip + clippy -D warnings green on .25; header regenerated. --- crates/punktfunk-core/src/abi.rs | 79 ++++++++++++++++++++++++++------ include/punktfunk_core.h | 8 +++- 2 files changed, 71 insertions(+), 16 deletions(-) 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/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 -- 2.54.0 From 500284973724b8a5cf247a4b129b4c52e22204c0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 12 Aug 2026 00:31:14 +0200 Subject: [PATCH 5/5] =?UTF-8?q?feat(pf-encode):=20WP4=20=E2=80=94=20AvFram?= =?UTF-8?q?e/AvSwsContext=20RAII=20across=20all=20three=20libav=20backends?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two newtypes beside AvBuffer/AvFilterGraph (same house shape: alloc/from_raw rejects the allocator's null once, as_ptr lends, Drop frees, no Clone; NonNull inside so the Options get a niche). 8 av_frame_alloc + 3 sws_getContext sites converted; all 22 hand-placed av_frame_free and 5 sws_freeContext calls are gone, and the three hand-written Drop impls (CpuInner, SystemInner, NvencEncoder) with them. The live defect this closes: ZeroCopyInner::submit (ffmpeg_win) leaked the frame AND one pooled hwframe surface on each of three ? exits between the pool pull and the send — under a SAFETY comment asserting no leak — and with POOL=8, eight such failures starved the pool and wedged the encoder with no error naming the cause. Every exit now returns the surface. Drop-order care (the hidden cost the survey flagged): NvencEncoder's sws_csc moved to field #1 (its hand-Drop freed it before all fields; this path runs on every stall-watchdog recovery via *self = fresh); CpuInner's nv12/sws declaration order flipped to match its hand-Drop; SystemInner's already agreed. Pinned by FIELD ORDER comments, not offset_of asserts — the survey's assert suggestion is the wrong tool: offset_of measures repr(Rust) memory layout, which the compiler may reorder independently of the declaration order that drop order actually follows. The dmabuf path keeps its early descriptor release via an explicit drop() at the exact point the hand-written free sat. Gates: .25 clippy -D warnings + tests green (nvenc,vulkan-encode,pyrowave); .133 check --all-targets + clippy --release -D warnings + 80 tests green (nvenc,amf-qsv,qsv; test step needs ffmpeg\bin on Path — 0xC0000135 otherwise). Owed on hardware: the #[ignore]d alloc/drop cycles on .136/.116/.173/.47 and the pool-exhaustion assertion (9th submit succeeds after 8 forced failures). --- crates/pf-encode/src/enc/libav.rs | 82 +++++++ crates/pf-encode/src/enc/linux/mod.rs | 135 ++++++------ crates/pf-encode/src/enc/linux/vaapi.rs | 202 ++++++++--------- .../pf-encode/src/enc/windows/ffmpeg_win.rs | 206 ++++++++---------- 4 files changed, 323 insertions(+), 302 deletions(-) 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(()) } -- 2.54.0