unom/punktfunk
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases 3 Wiki Activity

fix(host/vaapi): plug two AVBufferRef leaks in DmabufInner::open

Browse Source 
Surfaced while writing the unsafe-soundness proofs (2/N): both are refcount
leaks (sound — never dangling/double-free — so the SAFETY proofs held, but real
bugs on the persistent punktfunk1-host listener that opens a fresh encoder per
session).

1. Per-session leak: `par->hw_frames_ctx = av_buffer_ref(drm_frames)` created a
   second owned ref. `av_buffersrc_parameters_set` takes its OWN ref of
   `par->hw_frames_ctx`, and `av_free(par)` frees only the struct, not the ref —
   so the extra ref leaked every session, pinning the DRM frames ctx + device.
   Fix: assign `drm_frames` borrowed (the standard ffmpeg pattern); our single
   owned ref lives in DmabufInner and is unref'd in Drop.

2. Error-path leak: the final `open_vaapi_encoder(...)?` returned without the
   unref ladder every other error path runs, leaking graph/drm_frames/
   vaapi_device/drm_device on encoder-open failure. Fix: match + clean up before
   returning (nv12_ctx is borrowed from the sink → freed by graph teardown).

cargo clippy -p punktfunk-host --all-targets -- -D warnings clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Enrico Bühler
2026-06-26 09:02:54 +00:00
parent ba68a98873
commit 9777ed7fb3
1 changed files with 22 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/punktfunk-host/src/encode/linux/vaapi.rs
+22 -11
View File
@@ -596,12 +596,12 @@ impl DmabufInner {
num: 1,
den: fps as c_int,
};
// NOTE(leak, not UB): `av_buffersrc_parameters_set` takes its OWN ref of
// `par->hw_frames_ctx`, and `av_free(par)` frees only the struct (not the ref). So this
// `av_buffer_ref(drm_frames)` ref is leaked once per session — assigning `drm_frames`
// borrowed (no extra ref) is the correct ffmpeg pattern. Sound (a refcount leak, never a
// dangling/double-free), but it keeps the DRM frames ctx + device alive past `Drop`.
(*par).hw_frames_ctx = ffi::av_buffer_ref(drm_frames);
// Assign `drm_frames` BORROWED (no extra ref): `av_buffersrc_parameters_set` takes its
// own ref of `par->hw_frames_ctx` (via av_buffer_replace), and `av_free(par)` frees only
// the struct, not the ref. Our single owned `drm_frames` ref is retained, lives in
// `DmabufInner`, and is unref'd in `Drop`. Wrapping it in `av_buffer_ref` here would leak
// that extra ref every session (the persistent listener would accumulate them).
(*par).hw_frames_ctx = drm_frames;
let r = ffi::av_buffersrc_parameters_set(src, par);
ffi::av_free(par as *mut _);
if r < 0 {
@@ -656,10 +656,12 @@ impl DmabufInner {
ffi::av_buffer_unref(&mut drm_device);
bail!("filter sink has no VAAPI frames context");
}
// NOTE(leak, not UB): unlike the error paths above, this `?` returns without unref'ing
// `graph`/`drm_frames`/`vaapi_device`/`drm_device` — so an encoder-open failure leaks them.
// Sound (leak only); only the unhappy path, when the session is failing anyway.
let enc = open_vaapi_encoder(
// On encoder-open failure, free the graph + our owned buffer refs before bailing (matching
// every error path above) so a failed session doesn't leak them. `nv12_ctx` is borrowed
// from the sink (owned by `graph`), so `avfilter_graph_free` reclaims it — don't unref it
// separately. On success the encoder takes its own ref of `vaapi_device`, and `drm_frames`/
// `vaapi_device`/`drm_device`/`graph` move into `DmabufInner` (freed in `Drop`).
let enc = match open_vaapi_encoder(
codec,
width,
height,
@@ -667,7 +669,16 @@ impl DmabufInner {
bitrate_bps,
vaapi_device,
nv12_ctx,
)?;
) {
Ok(enc) => enc,
Err(e) => {
ffi::avfilter_graph_free(&mut graph);
ffi::av_buffer_unref(&mut drm_frames);
ffi::av_buffer_unref(&mut vaapi_device);
ffi::av_buffer_unref(&mut drm_device);
return Err(e);
}
};
tracing::info!(
encoder = codec.vaapi_name(),