diff --git a/crates/pf-encode/src/enc/linux/pyrowave.rs b/crates/pf-encode/src/enc/linux/pyrowave.rs index 9d9becd5..43ccd6eb 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave.rs @@ -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); diff --git a/crates/pf-encode/src/enc/windows/pyrowave.rs b/crates/pf-encode/src/enc/windows/pyrowave.rs index 82b3defc..a68fd3d9 100644 --- a/crates/pf-encode/src/enc/windows/pyrowave.rs +++ b/crates/pf-encode/src/enc/windows/pyrowave.rs @@ -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); } diff --git a/crates/pf-encode/src/enc/windows/qsv.rs b/crates/pf-encode/src/enc/windows/qsv.rs index e0f72f8a..af52f1f2 100644 --- a/crates/pf-encode/src/enc/windows/qsv.rs +++ b/crates/pf-encode/src/enc/windows/qsv.rs @@ -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` the runtime + // is writing into asynchronously (and a `Box` 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) {