Safety round 2: ASAN+LSAN over the C ABI boundary, two soundness fixes, and WP4's AvFrame RAII #172

Merged

enricobuehler merged 5 commits from worktree-safety-round-2 into main 2026-08-12 05:57:18 +00:00

Conversation 0 Commits 5 Files Changed 9

+539 -332

enricobuehler commented 2026-08-12 05:08:37 +00:00

Owner

Copy Link

Copy Source

Round 2 of the rust-safety programme (design/rust-safety-programme.md), five commits, each independently gated before it was committed.

1–2. §6.1 rank-1 tooling — the C ABI harness under ASAN+LSAN (e20b6140, c3b57438)

PF_SAN=address in crates/punktfunk-core/tests/c/run.sh rebuilds both sides of the boundary instrumented — the staticlib on nightly with -Zsanitizer/-Zbuild-std (std included), the harness with clang -fsanitize — and LSAN (detect_leaks=1) becomes the first automated check on abi.rs's Box::into_raw/from_raw leak contract. A c-abi-asan job joins audit.yml in the miri job's shape: same dated nightly pin (one date to bump for both), own san- cache prefixes, non-blocking day one via a step-level ::warning::, and a proved-it-ran grep so a silently-skipped harness can't read as green.

Proven to fail on .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; the restored tree exits 0.

Two traps encoded in the diff: PF_SAN_TOOLCHAIN pins the script's cargo +<toolchain> to the dated nightly CI installs (bare +nightly would ask for the rolling channel the job never installs), and the harness binary moved from mktemp to target/ — a debug+ASAN static binary can exceed a tmpfs /tmp (it did, on a 3.6 GB one).

3. WP3c/3d — hooks.rs (e8c306b9)

• 3c: &TOKEN_USER (align 8) was formed out of a bare [u8; 256] (align 1) — UB by the validity rule whenever the stack slot lands misaligned; shipped codegen happened to 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 (its len() of 32 would misclassify every hand-run host as SYSTEM via ERROR_INSUFFICIENT_BUFFER, invisibly to a SYSTEM-side test).
• 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 fn's own documented contract. Now a three-way split on the last-error code, with SetLastError(0) cleared first so a stale value can't misclassify.

Gate: cargo check + cargo clippy --release -- -D warnings on .133 (real MSVC, fresh extraction, sentinel-verified). Still owed (runbook C): a hand-run hooks-fire check on .173.

4. abi.rs — validate InputKind before &InputEvent exists (9a59504b)

The two send-input entry points built &InputEvent straight out of caller memory; 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 the 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. The test stages the event in MaybeUninit storage so the test itself never holds a reference to an invalid value. Header regenerated (doc-comment drift only).

5. WP4 — AvFrame / AvSwsContext RAII, all three libav backends (50028497)

Two newtypes beside AvBuffer/AvFilterGraph, same house shape (NonNull inside, so the Options get a niche). All 22 hand-placed av_frame_free and 5 sws_freeContext calls are gone, and the three hand-written Drop impls with them.

The live defect this closes: ZeroCopyInner::submit 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 starve the pool and wedge the encoder with no error naming the cause.

Drop-order care (the survey's "hidden cost"): NvencEncoder.sws_csc moved to field #1 (its hand-Drop freed it before all fields, and 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. One correction to the programme doc en route: its suggested offset_of! drop-order asserts are 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 — so the constraint is pinned the way AvBuffer's doc already does it, in FIELD ORDER comments. The dmabuf path keeps its early descriptor release via an explicit drop() at the exact point the hand-written free sat.

Verification matrix

gate	box	result
ASAN+LSAN harness, green + forced-leak + vanilla path	.25	pass / fails correctly / pass
punktfunk-core: 380 lib tests + C harness + clippy -D warnings (quic)	.25	green
punktfunk-host: check + clippy --release -D warnings	.133	green
pf-encode: clippy -D warnings + tests (nvenc,vulkan-encode,pyrowave)	.25	green
pf-encode: check --all-targets + clippy --release + 80 tests (nvenc,amf-qsv,qsv)	.133	green

Still owed after merge (tracked in design/rust-safety-verification-runbooks.md): the two human runbooks (WP0 Moonlight on .133, WP3b forced-fallback on .181), the #[ignore]d WP4 alloc/drop cycles on real hardware (.136/.116/.173/.47) plus the pool-exhaustion assertion, and the first live c-abi-asan run (dispatch audit.yml once this lands).

Round 2 of the rust-safety programme (`design/rust-safety-programme.md`), five commits, each independently gated before it was committed. ## 1–2. §6.1 rank-1 tooling — the C ABI harness under ASAN+LSAN (`e20b6140`, `c3b57438`) `PF_SAN=address` in `crates/punktfunk-core/tests/c/run.sh` rebuilds **both sides of the boundary instrumented** — the staticlib on nightly with `-Zsanitizer`/`-Zbuild-std` (std included), the harness with `clang -fsanitize` — and LSAN (`detect_leaks=1`) becomes the first automated check on `abi.rs`'s `Box::into_raw`/`from_raw` leak contract. A `c-abi-asan` job joins `audit.yml` in the miri job's shape: same dated nightly pin (one date to bump for both), own `san-` cache prefixes, non-blocking day one via a step-level `::warning::`, and a proved-it-ran grep so a silently-skipped harness can't read as green. **Proven to fail on `.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; the restored tree exits 0. Two traps encoded in the diff: `PF_SAN_TOOLCHAIN` pins the script's `cargo +<toolchain>` to the dated nightly CI installs (bare `+nightly` would ask for the rolling channel the job never installs), and the harness binary moved from `mktemp` to `target/` — a debug+ASAN static binary can exceed a tmpfs `/tmp` (it did, on a 3.6 GB one). ## 3. WP3c/3d — `hooks.rs` (`e8c306b9`) - **3c**: `&TOKEN_USER` (align 8) was formed out of a bare `[u8; 256]` (align 1) — UB by the validity rule whenever the stack slot lands misaligned; shipped codegen happened to 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 (its `len()` of 32 would misclassify every hand-run host as SYSTEM via `ERROR_INSUFFICIENT_BUFFER`, invisibly to a SYSTEM-side test). - **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 fn's own documented contract. Now a three-way split on the last-error code, with `SetLastError(0)` cleared first so a stale value can't misclassify. Gate: `cargo check` + `cargo clippy --release -- -D warnings` on `.133` (real MSVC, fresh extraction, sentinel-verified). Still owed (runbook C): a hand-run hooks-fire check on `.173`. ## 4. `abi.rs` — validate `InputKind` before `&InputEvent` exists (`9a59504b`) The two send-input entry points built `&InputEvent` straight out of caller memory; `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 the 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`. The test stages the event in `MaybeUninit` storage so the test itself never holds a reference to an invalid value. Header regenerated (doc-comment drift only). ## 5. WP4 — `AvFrame` / `AvSwsContext` RAII, all three libav backends (`50028497`) Two newtypes beside `AvBuffer`/`AvFilterGraph`, same house shape (`NonNull` inside, so the `Option`s get a niche). All **22 hand-placed `av_frame_free`** and **5 `sws_freeContext`** calls are gone, and the three hand-written `Drop` impls with them. The live defect this closes: `ZeroCopyInner::submit` 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 starve the pool and wedge the encoder with no error naming the cause. Drop-order care (the survey's "hidden cost"): `NvencEncoder.sws_csc` moved to field #1 (its hand-`Drop` freed it before all fields, and 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. **One correction to the programme doc en route**: its suggested `offset_of!` drop-order asserts are 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 — so the constraint is pinned the way `AvBuffer`'s doc already does it, in FIELD ORDER comments. The dmabuf path keeps its early descriptor release via an explicit `drop()` at the exact point the hand-written free sat. ## Verification matrix | gate | box | result | |---|---|---| | ASAN+LSAN harness, green + forced-leak + vanilla path | `.25` | pass / fails correctly / pass | | punktfunk-core: 380 lib tests + C harness + clippy `-D warnings` (quic) | `.25` | green | | punktfunk-host: check + clippy `--release -D warnings` | `.133` | green | | pf-encode: clippy `-D warnings` + tests (`nvenc,vulkan-encode,pyrowave`) | `.25` | green | | pf-encode: check `--all-targets` + clippy `--release` + **80 tests** (`nvenc,amf-qsv,qsv`) | `.133` | green | Still owed after merge (tracked in `design/rust-safety-verification-runbooks.md`): the two human runbooks (WP0 Moonlight on `.133`, WP3b forced-fallback on `.181`), the `#[ignore]`d WP4 alloc/drop cycles on real hardware (`.136`/`.116`/`.173`/`.47`) plus the pool-exhaustion assertion, and the first live `c-abi-asan` run (dispatch `audit.yml` once this lands).

enricobuehler added 5 commits 2026-08-12 05:08:40 +00:00

chore(safety): PF_SAN sanitizer gate for the C ABI harness ... e20b614059

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).

chore(ci): c-abi-asan job in audit.yml — the harness under ASAN+LSAN, weekly + on demand ... c3b57438e1

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.

fix(punktfunk-host): WP3c/3d — align the TOKEN_USER buffer, make EqualSid fail closed ... e8c306b9c0

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).

fix(punktfunk-core): validate InputKind before forming &InputEvent in the C ABI ... 9a59504ba4

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.

feat(pf-encode): WP4 — AvFrame/AvSwsContext RAII across all three libav backends ... 5002849737

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).

enricobuehler merged commit 66ba61b12c into main 2026-08-12 05:57:18 +00:00

enricobuehler deleted branch worktree-safety-round-2 2026-08-12 05:57:22 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

FernandoMX enricobuehler (Enrico Bühler) kszlim nannku neoney sassycrown

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unom/punktfunk#172