fix(encode): close the two teardown memory-safety holes in the reset paths
apple / swift (push) Successful in 1m17s
apple / screenshots (push) Successful in 6m14s
android / android (push) Has been cancelled
arch / build-publish (push) Has been cancelled
ci / web (push) Has been cancelled
ci / rust (push) Has been cancelled
ci / docs-site (push) Has been cancelled
ci / bench (push) Has been cancelled
deb / build-publish-host (push) Has been cancelled
deb / build-publish (push) Has been cancelled
decky / build-publish (push) Has been cancelled
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Has been cancelled
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Has been cancelled
docker / deploy-docs (push) Has been cancelled
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Has been cancelled
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Has been cancelled
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Has been cancelled
windows-host / package (push) Has been cancelled
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 10s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Has been cancelled
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Has been cancelled
apple / swift (push) Successful in 1m17s
apple / screenshots (push) Successful in 6m14s
android / android (push) Has been cancelled
arch / build-publish (push) Has been cancelled
ci / web (push) Has been cancelled
ci / rust (push) Has been cancelled
ci / docs-site (push) Has been cancelled
ci / bench (push) Has been cancelled
deb / build-publish-host (push) Has been cancelled
deb / build-publish (push) Has been cancelled
decky / build-publish (push) Has been cancelled
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Has been cancelled
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Has been cancelled
docker / deploy-docs (push) Has been cancelled
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Has been cancelled
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Has been cancelled
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Has been cancelled
windows-host / package (push) Has been cancelled
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 10s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Has been cancelled
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Has been cancelled
Both surfaced in a post-refactor quality sweep of pf-encode and were then verified against the source (and, for pyrowave, against the C side). - PyroWave (BOTH platforms): `reset()` destroyed the encoder and, when the rebuild failed, returned `false` leaving `pw_enc` pointing at the freed object — `Drop` then destroyed it a second time. `pyrowave_encoder_destroy` is a plain `delete` (pyrowave_c.cpp:1184, which also reads `encoder->device` afterwards) with no null check, so this is a real double free. The failure branch is not vacuous: the rebuild fails when the device is lost/OOM, which is exactly the state that makes the stall watchdog call `reset()` in the first place, so the host corrupts its heap on the path that runs when things are already going wrong. Now nulls `pw_enc` before the fallible create, publishes only on success, and null-guards both `Drop` and `encode_frame` (the Windows `Drop` already guarded `sync` this way). - QSV: `reset()` dropped `pending` — each entry owning the `Box<BsBuf>` the runtime writes into asynchronously — BEFORE `MFXVideoENCODE_Close` aborted those operations, so the VPL runtime could write into freed heap. The preceding drain is best-effort and bails on the first `Err`, i.e. precisely the wedged-encoder case that triggers the reset. Fixed by ordering: Close, then clear. The full-teardown path was already correct (`Inner` declares `session` before `pending`, and fields drop in declaration order). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -865,6 +865,13 @@ impl PyroWaveEncoder {
|
||||
/// One frame, synchronously: ingest → CSC → pyrowave encode (recorded into our command
|
||||
/// buffer) → submit + fence wait (sub-ms) → packetize into an `EncodedFrame`.
|
||||
unsafe fn encode_frame(&mut self, frame: &CapturedFrame) -> Result<()> {
|
||||
// A failed `reset()` leaves the encoder destroyed and null. Callers today turn that into
|
||||
// a session error and never resubmit, but a null here would be a use-after-free inside
|
||||
// pyrowave rather than a clean error — so fail loudly instead of relying on that.
|
||||
anyhow::ensure!(
|
||||
!self.pw_enc.is_null(),
|
||||
"pyrowave: encode after a failed reset (encoder was destroyed and not rebuilt)"
|
||||
);
|
||||
let dev = self.device.clone();
|
||||
let (w, h) = (self.width, self.height);
|
||||
dev.begin_command_buffer(
|
||||
@@ -1195,6 +1202,12 @@ impl Encoder for PyroWaveEncoder {
|
||||
unsafe {
|
||||
self.device.device_wait_idle().ok();
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
// Publish the null IMMEDIATELY: the create below is fallible, and its failure path
|
||||
// must not leave a freed pointer in the field. `pyrowave_encoder_destroy` is a plain
|
||||
// `delete` (pyrowave_c.cpp) with no null check, so `Drop` running on a stale handle
|
||||
// is a double free — the exact shape this reset hits when the rebuild fails because
|
||||
// the device is already lost, which is the state that made the watchdog fire.
|
||||
self.pw_enc = std::ptr::null_mut();
|
||||
let einfo = pw::pyrowave_encoder_create_info {
|
||||
device: self.pw_dev,
|
||||
width: self.width as i32,
|
||||
@@ -1209,6 +1222,10 @@ impl Encoder for PyroWaveEncoder {
|
||||
let r = pw::pyrowave_encoder_create(&einfo, &mut enc);
|
||||
if r != pw::pyrowave_result_PYROWAVE_SUCCESS {
|
||||
tracing::error!(result = ?r, "pyrowave: encoder rebuild failed");
|
||||
// `pw_enc` stays null — `Drop` and `encode_frame` both guard on it. The queued
|
||||
// AUs are forfeit either way (the caller turns a false reset into a session
|
||||
// error), so drop them rather than shipping output from a dead encoder.
|
||||
self.pending.clear();
|
||||
return false;
|
||||
}
|
||||
self.pw_enc = enc;
|
||||
@@ -1254,7 +1271,11 @@ impl Drop for PyroWaveEncoder {
|
||||
// before the VkDevice they borrow (encoder before device, per pyrowave.h).
|
||||
unsafe {
|
||||
self.device.device_wait_idle().ok();
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
// Null when a failed `reset()` already destroyed it — `pyrowave_encoder_destroy`
|
||||
// is not null-safe.
|
||||
if !self.pw_enc.is_null() {
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
}
|
||||
pw::pyrowave_device_destroy(self.pw_dev);
|
||||
for (_, _, i, m, v) in self.import_cache.drain(..) {
|
||||
self.device.destroy_image_view(v, None);
|
||||
|
||||
@@ -417,6 +417,12 @@ impl PyroWaveEncoder {
|
||||
/// # Safety
|
||||
/// Runs on the single encode thread; all pyrowave calls take handles this struct owns.
|
||||
unsafe fn encode_frame(&mut self, frame: &CapturedFrame) -> Result<()> {
|
||||
// A failed `reset()` leaves the encoder destroyed and null — fail cleanly rather than
|
||||
// handing null to pyrowave (see the Linux twin).
|
||||
anyhow::ensure!(
|
||||
!self.pw_enc.is_null(),
|
||||
"pyrowave: encode after a failed reset (encoder was destroyed and not rebuilt)"
|
||||
);
|
||||
let FramePayload::D3d11(d3d) = &frame.payload else {
|
||||
bail!("pyrowave (Windows) needs a D3D11 frame (the capturer must be in pyrowave mode)")
|
||||
};
|
||||
@@ -639,6 +645,10 @@ impl Encoder for PyroWaveEncoder {
|
||||
// SAFETY: encode is synchronous (no work in flight); the device outlives the swapped encoder.
|
||||
unsafe {
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
// Publish the null IMMEDIATELY — see the Linux twin. The create below is fallible and
|
||||
// `pyrowave_encoder_destroy` is a plain `delete` with no null check, so leaving the
|
||||
// freed pointer in the field makes `Drop` a double free.
|
||||
self.pw_enc = std::ptr::null_mut();
|
||||
let einfo = pw::pyrowave_encoder_create_info {
|
||||
device: self.pw_dev,
|
||||
width: self.width as i32,
|
||||
@@ -655,6 +665,8 @@ impl Encoder for PyroWaveEncoder {
|
||||
let r = pw::pyrowave_encoder_create(&einfo, &mut enc);
|
||||
if r != pw::pyrowave_result_PYROWAVE_SUCCESS {
|
||||
tracing::error!(result = ?r, "pyrowave: encoder rebuild failed");
|
||||
// `pw_enc` stays null — `Drop` and `encode_frame` both guard on it.
|
||||
self.pending.clear();
|
||||
return false;
|
||||
}
|
||||
self.pw_enc = enc;
|
||||
@@ -698,7 +710,11 @@ impl Drop for PyroWaveEncoder {
|
||||
// SAFETY: owned handles, destroyed exactly once; pyrowave objects (encoder, images, sync) go
|
||||
// before the device they borrow (per pyrowave.h).
|
||||
unsafe {
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
// Null when a failed `reset()` already destroyed it — `pyrowave_encoder_destroy`
|
||||
// is not null-safe (same guard `sync` below has had all along).
|
||||
if !self.pw_enc.is_null() {
|
||||
pw::pyrowave_encoder_destroy(self.pw_enc);
|
||||
}
|
||||
for (_, img) in self.y_images.drain(..).chain(self.cbcr_images.drain(..)) {
|
||||
pw::pyrowave_image_destroy(img);
|
||||
}
|
||||
|
||||
@@ -1418,15 +1418,21 @@ impl Encoder for QsvEncoder {
|
||||
let inner = self.inner.as_mut().expect("checked above");
|
||||
// Best-effort settle of in-flight operations (Close aborts them anyway).
|
||||
while sync_one(inner, 5).ok().flatten().is_some() {}
|
||||
inner.pending.clear();
|
||||
inner.ready.clear();
|
||||
inner.frames_submitted = 0;
|
||||
inner.first_au_logged = false;
|
||||
// Close BEFORE dropping `pending`. Each `Pending` owns the `Box<BsBuf>` the runtime
|
||||
// is writing into asynchronously (and a `Box<FrameCtrl>` it reads), so clearing first
|
||||
// frees that heap while the operation is still live — a use-after-free by the VPL
|
||||
// runtime. The drain above is best-effort and bails on the first `Err`, which is
|
||||
// exactly the wedged-encoder case that triggers this reset, so it cannot be relied on
|
||||
// to have retired everything. Close aborts the operations; only then is the drop safe.
|
||||
// SAFETY: the session is live on this thread; Close on a wedged encoder is legal
|
||||
// (result deliberately ignored) and re-Init happens through `init_encode`.
|
||||
unsafe {
|
||||
let _ = vpl::MFXVideoENCODE_Close(inner.session.0);
|
||||
}
|
||||
inner.pending.clear();
|
||||
inner.ready.clear();
|
||||
inner.frames_submitted = 0;
|
||||
inner.first_au_logged = false;
|
||||
inner.session.0
|
||||
};
|
||||
match self.init_encode(rebuilt) {
|
||||
|
||||
Reference in New Issue
Block a user