diff --git a/crates/pf-vkdecode/src/params.rs b/crates/pf-vkdecode/src/params.rs index d1e4c668..1f15efed 100644 --- a/crates/pf-vkdecode/src/params.rs +++ b/crates/pf-vkdecode/src/params.rs @@ -80,9 +80,12 @@ impl std::error::Error for ParamsError {} /// pointers hold the addresses of. /// - [`Self::std`] hands the struct out by shared reference. The struct is `Copy`; a /// copy taken out of the wrapper still points INTO the wrapper's backing and must -/// not outlive it. The intended use is passing the reference straight into -/// `vkCreateVideoSessionParametersKHR`, which copies all parameter data before -/// returning — keeping the wrapper alive across that call is the whole obligation. +/// not outlive it. ⚠⚠ The obligation is NOT merely "keep the wrapper alive across +/// `vkCreateVideoSessionParametersKHR`", which is what this said and what the +/// spec reads like: NVIDIA 610.57.04 was measured retaining an embedded pointer +/// out of a Std set and dereferencing it at every `vkCmdDecodeVideoKHR` +/// ([`crate::session_av1`]). The wrapper must outlive the parameters OBJECT, and +/// [`crate::session`] is where that is enforced by construction. /// - Nothing exposes mutation of the backing, so for the wrapper's lifetime the /// pointed-to data is immutable and the `*const` aliasing rules hold trivially. /// - Deliberately NOT `Clone`: a derived clone would duplicate the pointer VALUES but @@ -537,6 +540,109 @@ mod tests { assert_eq!(offsets, [2, -1, 4]); } + /// Scribble over the stack the conversion's frames just used. + /// + /// The discriminator in the move tests is READING a block back, and pointer + /// equality cannot stand in for it: the Std struct carries its pointers by + /// VALUE, so a stale one is copied along with the struct and still compares + /// equal. An inlined backing therefore shows up only as wrong CONTENT — and + /// only if the dead slot has actually been reused by then. This makes that + /// certain instead of lucky: after it runs, a pointer into a dead local reads + /// back `0xA5`s rather than, by chance, its old contents. + #[inline(never)] + fn clobber_the_dead_stack() { + let mut scratch = [0xA5u8; 16 * 1024]; + std::hint::black_box(&mut scratch); + } + + /// Both wrappers may be MOVED — into the session's stored parameters, out of a + /// `Result`, into a `Vec` that later reallocates — without disturbing the + /// addresses a driver has already been given. + /// + /// Not a Rust triviality worth skipping: it is the whole reason + /// [`crate::session`] can fix its use-after-free by STORING these values + /// alongside the parameters object rather than by boxing or pinning them. It + /// holds because every backing is `Box`ed; an "optimisation" that inlined any + /// one of them as a field would keep every other test in this crate green, keep + /// compiling, and hand the driver a pointer into a moved-from stack slot. The + /// H.264 parity leg would catch it on hardware — this catches it in ordinary CI. + /// (`params_av1::moving_the_wrapper_leaves_the_driver_s_pointers_put` is the + /// same test one codec over; `params_h265`'s is the third.) + #[test] + fn moving_the_wrapper_leaves_the_driver_s_pointers_put() { + // An SPS carrying BOTH of its embedded pointers: the POC-type-1 offset + // array and the scaling lists. (The vendored `Sps` is not `Clone`, so the + // fixture is a builder rather than a value.) + let pointer_bearing_sps = || { + let mut sps = full_sps(); + sps.pic_order_cnt_type = 1; + sps.num_ref_frames_in_pic_order_cnt_cycle = 3; + sps.offset_for_ref_frame[0] = 2; + sps.offset_for_ref_frame[1] = -1; + sps.offset_for_ref_frame[2] = 4; + sps.seq_scaling_matrix_present_flag = true; + sps.scaling_lists_4x4 = std::array::from_fn(|i| [10 + i as u8; 16]); + sps + }; + // And a PPS carrying its one. + let mut pps = full_pps(pointer_bearing_sps()); + pps.pic_scaling_matrix_present_flag = true; + pps.scaling_lists_4x4 = std::array::from_fn(|i| [60 + i as u8; 16]); + + let owned_sps = sps_to_std(&pointer_bearing_sps()).expect("a High-profile SPS converts"); + let owned_pps = pps_to_std(&pps).expect("its PPS converts"); + let (offsets, sps_lists) = ( + owned_sps.std().pOffsetForRefFrame, + owned_sps.std().pScalingLists, + ); + let pps_lists = owned_pps.std().pScalingLists; + assert!(!offsets.is_null(), "POC type 1 attaches the offset array"); + assert!(!sps_lists.is_null(), "the SPS declares scaling lists"); + assert!(!pps_lists.is_null(), "so does the PPS"); + + // Every move the session's stored parameters put them through: out of the + // conversion, into a `Vec`, through a reallocation of that `Vec` as later + // Adds push more sets in, and along with the whole `StoredParams` value as + // it is installed by `mem::replace`. + let stored_sps = vec![owned_sps]; + let mut stored_pps = vec![owned_pps]; + for id in 1..crate::session::MAX_STD_PPS as u8 { + let mut more = full_pps(pointer_bearing_sps()); + more.pic_parameter_set_id = id; + stored_pps.push(pps_to_std(&more).expect("converts")); + } + assert!( + stored_pps.capacity() > 1, + "the pushes reallocated, which is the case being pinned" + ); + let stored = (stored_sps, stored_pps, 0u8); + let (stored_sps, stored_pps, _) = stored; + + assert_eq!(stored_sps[0].std().pOffsetForRefFrame, offsets); + assert_eq!(stored_sps[0].std().pScalingLists, sps_lists); + assert_eq!(stored_pps[0].std().pScalingLists, pps_lists); + + // The assertions that actually bite. Pointer equality above cannot fail — + // the Std struct carries the value, so a stale pointer is copied along with + // it — but an inlined backing leaves those pointers addressing dead locals + // in `sps_to_std`/`pps_to_std`'s returned frames, which this has just + // overwritten. + clobber_the_dead_stack(); + // They still address live blocks holding the fixture's own values, not + // stale copies. + // SAFETY: `stored_sps`/`stored_pps` are alive here and own every block. + let (read_offsets, read_sps_lists, read_pps_lists) = unsafe { + ( + std::slice::from_raw_parts(offsets, 3), + &*sps_lists, + &*pps_lists, + ) + }; + assert_eq!(read_offsets, [2, -1, 4]); + assert_eq!(read_sps_lists.ScalingList4x4[5], [15; 16]); + assert_eq!(read_pps_lists.ScalingList4x4[5], [65; 16]); + } + #[test] fn sps_scaling_lists_convert_when_present_and_stay_absent_when_not() { let mut sps = full_sps(); diff --git a/crates/pf-vkdecode/src/params_h265.rs b/crates/pf-vkdecode/src/params_h265.rs index 906f5ca0..22635041 100644 --- a/crates/pf-vkdecode/src/params_h265.rs +++ b/crates/pf-vkdecode/src/params_h265.rs @@ -1290,6 +1290,187 @@ mod tests { assert_eq!(dpb.max_dec_pic_buffering_minus1[0], 5); } + /// Scribble over the stack the conversion's frames just used. + /// + /// The discriminator in the move tests is READING a block back, and pointer + /// equality cannot stand in for it: the Std struct carries its pointers by + /// VALUE, so a stale one is copied along with the struct and still compares + /// equal. An inlined backing therefore shows up only as wrong CONTENT — and + /// only if the dead slot has actually been reused by then. This makes that + /// certain instead of lucky: after it runs, a pointer into a dead local reads + /// back `0xA5`s rather than, by chance, its old contents. + #[inline(never)] + fn clobber_the_dead_stack() { + let mut scratch = [0xA5u8; 16 * 1024]; + std::hint::black_box(&mut scratch); + } + + /// All three wrappers may be MOVED — into the session's stored parameters, out + /// of a `Result`, into a `Vec` that later reallocates — without disturbing the + /// addresses a driver has already been given. + /// + /// Not a Rust triviality worth skipping: it is the whole reason + /// [`crate::session_h265`] can fix its use-after-free by STORING these values + /// alongside the parameters object rather than by boxing or pinning them. It + /// holds because every backing is `Box`ed; an "optimisation" that inlined any + /// one of them as a field would keep every other test in this crate green, keep + /// compiling, and hand the driver a pointer into a moved-from stack slot. The + /// H.265 and Main 10 parity legs would catch it on hardware — this catches it in + /// ordinary CI. H.265 has the most surface of the three codecs: seven pointers + /// across the SPS alone, and this exercises every one a stream can populate. + /// (`params_av1::moving_the_wrapper_leaves_the_driver_s_pointers_put` and + /// `params::`'s are the same test one codec over.) + #[test] + fn moving_the_wrapper_leaves_the_driver_s_pointers_put() { + // An SPS carrying every embedded pointer it can: profile/tier/level and + // DPB manager (always), plus scaling lists, short-term RPS candidates and + // long-term SPS candidates. + let mut sps = full_sps(); + sps.scaling_list_data_present_flag = true; + sps.scaling_list.scaling_list_4x4 = std::array::from_fn(|i| [10 + i as u8; 16]); + sps.num_short_term_ref_pic_sets = 1; + let mut st = ShortTermRefPicSet { + num_negative_pics: 1, + ..Default::default() + }; + st.delta_poc_s0[0] = -1; + st.used_by_curr_pic_s0[0] = true; + sps.short_term_ref_pic_set = vec![st]; + sps.long_term_ref_pics_present_flag = true; + sps.num_long_term_ref_pics_sps = 1; + sps.lt_ref_pic_poc_lsb_sps[0] = 11; + sps.used_by_curr_pic_lt_sps_flag[0] = true; + // A PPS carrying its one, and a VPS carrying its two. + let mut pps = full_pps(sps.clone()); + pps.scaling_list_data_present_flag = true; + pps.scaling_list.scaling_list_4x4 = std::array::from_fn(|i| [60 + i as u8; 16]); + let vps = Vps { + video_parameter_set_id: 2, + max_sub_layers_minus1: 1, + profile_tier_level: full_sps().profile_tier_level, + // Distinct from the SPS's [5, 6, …] so a read-back names which block + // it came from rather than merely that some block was readable. + max_dec_pic_buffering_minus1: [3, 4, 0, 0, 0, 0, 0], + ..Default::default() + }; + + let owned_vps = vps_to_std_h265(&vps).expect("the VPS converts"); + let owned_sps = sps_to_std_h265(&sps).expect("a Main 10 SPS converts"); + let owned_pps = pps_to_std_h265(&pps).expect("its PPS converts"); + let vps_ptrs = ( + owned_vps.std().pProfileTierLevel, + owned_vps.std().pDecPicBufMgr, + ); + let sps_ptrs = ( + owned_sps.std().pProfileTierLevel, + owned_sps.std().pDecPicBufMgr, + owned_sps.std().pScalingLists, + owned_sps.std().pShortTermRefPicSet, + owned_sps.std().pLongTermRefPicsSps, + ); + let pps_lists = owned_pps.std().pScalingLists; + for (what, ptr) in [ + ("vps pProfileTierLevel", vps_ptrs.0.cast::<()>()), + ("vps pDecPicBufMgr", vps_ptrs.1.cast()), + ("sps pProfileTierLevel", sps_ptrs.0.cast()), + ("sps pDecPicBufMgr", sps_ptrs.1.cast()), + ("sps pScalingLists", sps_ptrs.2.cast()), + ("sps pShortTermRefPicSet", sps_ptrs.3.cast()), + ("sps pLongTermRefPicsSps", sps_ptrs.4.cast()), + ("pps pScalingLists", pps_lists.cast()), + ] { + assert!(!ptr.is_null(), "{what} is attached by this fixture"); + } + + // Every move the session's stored parameters put them through: out of the + // conversion, into a `Vec`, through a reallocation of that `Vec` as later + // Adds push more sets in, and along with the whole `StoredParamsH265` value + // as it is installed by `mem::replace`. + let stored_vps = vec![owned_vps]; + let stored_sps = vec![owned_sps]; + let mut stored_pps = vec![owned_pps]; + for id in 1..crate::session_h265::MAX_STD_PPS as u8 { + let mut more = full_pps(sps.clone()); + more.pic_parameter_set_id = id; + stored_pps.push(pps_to_std_h265(&more).expect("converts")); + } + assert!( + stored_pps.capacity() > 1, + "the pushes reallocated, which is the case being pinned" + ); + let stored = (stored_vps, stored_sps, stored_pps, 0u8); + let (stored_vps, stored_sps, stored_pps, _) = stored; + + let moved_vps = stored_vps[0].std(); + assert_eq!( + (moved_vps.pProfileTierLevel, moved_vps.pDecPicBufMgr), + vps_ptrs + ); + let moved_sps = stored_sps[0].std(); + assert_eq!( + ( + moved_sps.pProfileTierLevel, + moved_sps.pDecPicBufMgr, + moved_sps.pScalingLists, + moved_sps.pShortTermRefPicSet, + moved_sps.pLongTermRefPicsSps, + ), + sps_ptrs + ); + assert_eq!(stored_pps[0].std().pScalingLists, pps_lists); + + // The assertions that actually bite. Pointer equality above cannot fail — + // the Std struct carries the value, so a stale pointer is copied along with + // it — but an inlined backing leaves those pointers addressing dead locals + // in the conversions' returned frames, which this has just overwritten. + clobber_the_dead_stack(); + // They still address live blocks holding the fixture's own values, not + // stale copies. + // Every one of the eight, so no single backing can be inlined without this + // failing — an earlier draft read only six and let exactly that through. + // SAFETY: `stored_*` are alive here and own every one of these blocks. + let (vps_ptl, vps_dpb) = unsafe { (&*vps_ptrs.0, &*vps_ptrs.1) }; + // SAFETY: as above. + let (sps_ptl, sps_dpb, sps_scaling, sps_st, sps_lt) = unsafe { + ( + &*sps_ptrs.0, + &*sps_ptrs.1, + &*sps_ptrs.2, + &*sps_ptrs.3, + &*sps_ptrs.4, + ) + }; + // SAFETY: as above. + let pps_scaling = unsafe { &*pps_lists }; + assert_eq!( + vps_ptl.general_level_idc, + hh::StdVideoH265LevelIdc_STD_VIDEO_H265_LEVEL_IDC_4_1, + "vps pProfileTierLevel" + ); + assert_eq!( + &vps_dpb.max_dec_pic_buffering_minus1[..2], + &[3, 4], + "vps pDecPicBufMgr" + ); + assert_eq!( + sps_ptl.general_level_idc, + hh::StdVideoH265LevelIdc_STD_VIDEO_H265_LEVEL_IDC_4_1, + "sps pProfileTierLevel" + ); + assert_eq!( + &sps_dpb.max_dec_pic_buffering_minus1[..2], + &[5, 6], + "sps pDecPicBufMgr" + ); + assert_eq!(sps_scaling.ScalingList4x4[5], [15; 16], "sps pScalingLists"); + assert_eq!(sps_st.num_negative_pics, 1, "sps pShortTermRefPicSet"); + assert_eq!( + sps_lt.lt_ref_pic_poc_lsb_sps[0], 11, + "sps pLongTermRefPicsSps" + ); + assert_eq!(pps_scaling.ScalingList4x4[5], [65; 16], "pps pScalingLists"); + } + #[test] fn sps_scaling_lists_convert_verbatim_including_the_32x32_pair_and_dc_values() { let mut sps = full_sps(); diff --git a/crates/pf-vkdecode/src/session.rs b/crates/pf-vkdecode/src/session.rs index 4b6a0426..c55717c3 100644 --- a/crates/pf-vkdecode/src/session.rs +++ b/crates/pf-vkdecode/src/session.rs @@ -14,6 +14,18 @@ //! whole session — `plan_to_vk`'s `CapacityMismatch` is the trigger the decoder //! sees for the DPB half, the extent comparison covers the other. //! +//! ⚠⚠⚠ **The Std sets' heap blocks must outlive the parameters OBJECT, not just the +//! call that hands them over.** Vulkan reads as though parameter data were captured +//! by `vkCreateVideoSessionParametersKHR`, and all three codecs in this crate +//! assumed it. NVIDIA 610.57.04 does not: for AV1 it was measured keeping +//! `StdVideoAV1SequenceHeader::pColorConfig` and dereferencing it when a decode is +//! RECORDED, which decoded every frame against recycled heap ([`crate::session_av1`] +//! carries the measurement). H.264's Std sets embed the same kind of pointer — +//! `pOffsetForRefFrame` and `pScalingLists` on the SPS, `pScalingLists` on the PPS — +//! so [`StoredParams`] holds the object and its backings in ONE value with one +//! lifetime, and both the recreate path and `Drop` destroy the object before that +//! value is released. +//! //! [`ParamsLedger`] is the pure half of that decision table (unit-tested); //! [`VideoSession`] is the thin Vulkan half. @@ -32,6 +44,8 @@ use crate::device::AllocError; use crate::device::DecodeDevice; use crate::params::pps_to_std; use crate::params::sps_to_std; +use crate::params::OwnedStdPps; +use crate::params::OwnedStdSps; use crate::params::ParamsError; use crate::params_av1::ParamsAv1Error; use crate::params_h265::H265ParamsError; @@ -327,13 +341,62 @@ pub(crate) unsafe fn bind_session_memory( Ok(allocated) } +/// A live parameters object **and every Std parameter set it was given**, in one +/// field — because the two may not drift apart. +/// +/// The wrapper is not decoration and not defensive: a driver in this fleet keeps +/// the embedded pointers out of a Std set and dereferences them long after the call +/// that handed them over returned (module docs), so releasing the backing early +/// hands it freed memory. One value rather than two fields makes "an object whose +/// backing is gone" unrepresentable, which is the only shape of this bug — and the +/// shape a `let owned = …;` local silently had. +/// +/// What is pinned, precisely: the wrappers' BOXED blocks, which is what the driver +/// was measured retaining. The contiguous array of outer `StdVideoH264*` structs +/// each call receives is a short-lived temporary, and the driver copies THAT before +/// returning — which is what the AV1 fix itself rests on, its Std header being moved +/// into storage after the create call on a rung that is now 250/250 bit-exact. So +/// moving these wrappers, or reallocating the `Vec`s holding them, disturbs nothing +/// the driver kept; `params::moving_the_wrapper_leaves_the_driver_s_pointers_put` +/// pins the half that matters. +struct StoredParams { + object: vk::VideoSessionParametersKHR, + /// One entry per set the OBJECT stores, held for the object's whole life. + /// Never read by this crate after the create/update call; the DRIVER reads the + /// blocks they own. + sps: Vec, + pps: Vec, +} + +impl StoredParams { + /// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR` + /// ignores a NULL handle, so a [`VideoSession::create`] that fails before the + /// object exists still drops cleanly. + fn none() -> Self { + Self { + object: vk::VideoSessionParametersKHR::null(), + sps: Vec::new(), + pps: Vec::new(), + } + } + + /// Take over sets an `Add` just handed to the live object — they belong to the + /// OBJECT now, so their blocks live as long as it does rather than as long as + /// the update call. Only ever reached after that call SUCCEEDED: a failed + /// update stored nothing, and its wrappers are dropped instead. + fn adopt(&mut self, sps: Option, pps: Option) { + self.sps.extend(sps); + self.pps.extend(pps); + } +} + /// The Vulkan half: session + bound memory + parameters object. pub(crate) struct VideoSession { device: ash::Device, video_queue: ash::khr::video_queue::Device, session: vk::VideoSessionKHR, memory: Vec, - parameters: vk::VideoSessionParametersKHR, + parameters: StoredParams, ledger: ParamsLedger, pub(crate) config: SessionConfig, /// The session has never run a coding scope: the first one records a @@ -388,7 +451,7 @@ impl VideoSession { video_queue: dev.video_queue().clone(), session, memory: Vec::new(), - parameters: vk::VideoSessionParametersKHR::null(), + parameters: StoredParams::none(), ledger: ParamsLedger::default(), config, needs_reset: ResetArm::armed(), @@ -406,36 +469,38 @@ impl VideoSession { return Err(failure.error); } } - built.parameters = built.create_parameters_object(&[], &[])?; + built.parameters = built.create_parameters_object(Vec::new(), Vec::new())?; } Ok(built) } - /// Create a parameters object holding exactly `sps`/`pps` (either may be empty). + /// Create a parameters object holding exactly `sps`/`pps` (either may be + /// empty), **fused with the wrappers whose heap blocks it points at**. + /// + /// Taking the wrappers BY VALUE rather than as Std slices is the point: there is + /// no way to reach `vkCreateVideoSessionParametersKHR` from here without the + /// resulting object taking ownership of everything it will go on dereferencing + /// (module docs, [`StoredParams`]). /// /// # Safety /// - /// Live device + live session; the Std slices' backing (the `OwnedStd*` - /// wrappers) outlives this call. - /// - /// ⚠ "Vulkan copies all parameter data before returning" is what this used to - /// say, and it is **not universally true**: NVIDIA 610.57.04 was measured - /// retaining `StdVideoAV1SequenceHeader::pColorConfig` and dereferencing it at - /// every `vkCmdDecodeVideoKHR` ([`crate::session_av1`]). The H.26x rungs are - /// bit-exact on four drivers with the backings dropped here, so nothing is - /// known to be wrong — but the H.264 and H.265 Std sets carry embedded pointers - /// too (`pScalingLists`, `pSequenceParameterSetVui`, `pOffsetForRefFrame`, and - /// H.265's seven), and this contract rests on a driver behaviour rather than on - /// ownership. Holding them for the object's life, as AV1 now does, is the - /// version of this that cannot rot. + /// Live device + live session. unsafe fn create_parameters_object( &self, - sps: &[hh::StdVideoH264SequenceParameterSet], - pps: &[hh::StdVideoH264PictureParameterSet], - ) -> Result { + sps: Vec, + pps: Vec, + ) -> Result { + // The contiguous Std arrays the call wants. These are COPIES of each + // wrapper's Std struct (it is `Copy`, and the driver copies them again + // before returning); the embedded pointers they carry still address the + // wrappers' own boxed blocks, which are what must outlive the OBJECT. + let std_sps: Vec = + sps.iter().map(|o| *o.std()).collect(); + let std_pps: Vec = + pps.iter().map(|o| *o.std()).collect(); let add = vk::VideoDecodeH264SessionParametersAddInfoKHR::default() - .std_sp_ss(sps) - .std_pp_ss(pps); + .std_sp_ss(&std_sps) + .std_pp_ss(&std_pps); let mut h264 = vk::VideoDecodeH264SessionParametersCreateInfoKHR::default() .max_std_sps_count(MAX_STD_SPS as u32) .max_std_pps_count(MAX_STD_PPS as u32) @@ -443,20 +508,22 @@ impl VideoSession { let ci = vk::VideoSessionParametersCreateInfoKHR::default() .video_session(self.session) .push_next(&mut h264); - let mut parameters = vk::VideoSessionParametersKHR::null(); - // SAFETY: fn contract; `ci` roots locals outliving the call. + let mut object = vk::VideoSessionParametersKHR::null(); + // SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks + // the driver may retain past it are owned by `sps`/`pps`, which are moved + // into the returned value rather than dropped here. let r = unsafe { (self.video_queue.fp().create_video_session_parameters_khr)( self.device.handle(), &ci, std::ptr::null(), - &mut parameters, + &mut object, ) }; if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } - Ok(parameters) + Ok(StoredParams { object, sps, pps }) } /// The ledger's verdict for activating (`sps`, `pps`), without mutating @@ -512,19 +579,22 @@ impl VideoSession { .update_sequence_count(self.ledger.next_update_seq()) .push_next(&mut add); // SAFETY: live device + parameters object; `update` roots locals - // (incl. the OwnedStd backings) outliving the call. See - // `create_parameters_object` on why "and the driver copies them" - // is an observation about this fleet rather than a guarantee. + // (incl. the OwnedStd backings) outliving the call — and the + // backings go on outliving it, adopted below. let r = unsafe { (self.video_queue.fp().update_video_session_parameters_khr)( self.device.handle(), - self.parameters, + self.parameters.object, &update, ) }; if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } + // ⚠ The added sets now belong to the OBJECT, so their heap blocks + // must too: an Add whose wrappers died at the end of this arm would + // be the AV1 use-after-free with an update call in front of it. + self.parameters.adopt(owned_sps, owned_pps); self.ledger.commit(action, sps, pps); Ok(()) } @@ -536,25 +606,30 @@ impl VideoSession { ); let owned_sps = sps_to_std(sps)?; let owned_pps = pps_to_std(pps)?; - // SAFETY: fn contract (the OwnedStd backings live across the call). - let fresh = unsafe { - self.create_parameters_object( - std::slice::from_ref(owned_sps.std()), - std::slice::from_ref(owned_pps.std()), - )? - }; + // SAFETY: fn contract — live device + live session. The wrappers + // are MOVED IN and come back owned by the fresh object, so they + // live as long as it does rather than merely across the call. + let fresh = + unsafe { self.create_parameters_object(vec![owned_sps], vec![owned_pps])? }; + // The old object goes FIRST and its backings with it — installing + // `fresh` through a local keeps the destroy ahead of the free, + // which is the order a driver still holding the old pointers needs. + let old = std::mem::replace(&mut self.parameters, fresh); // SAFETY: the fn-level contract — the caller drained every // in-flight decode before a Recreate reached here (checked via // parameters_action), so no submitted work reads the old object; - // it is this session's own handle. + // it is this session's own handle, on a live device. unsafe { (self.video_queue.fp().destroy_video_session_parameters_khr)( self.device.handle(), - self.parameters, + old.object, std::ptr::null(), ); } - self.parameters = fresh; + // Explicit, because the ORDER is the whole point: every Std block + // `old` owns is released only now, after the object that pointed at + // them is gone. + drop(old); self.ledger.commit(action, sps, pps); Ok(()) } @@ -566,7 +641,7 @@ impl VideoSession { } pub(crate) fn parameters(&self) -> vk::VideoSessionParametersKHR { - self.parameters + self.parameters.object } /// Whether the next coding scope must record the initialization RESET — @@ -612,11 +687,14 @@ impl Drop for VideoSession { // ORDER is load-bearing, not stylistic: memory bound into a session may // not be freed while the session lives, so the session is destroyed first // — which is also why a failed bind hands its allocations back here - // instead of freeing them itself ([`BindFailure`]). + // instead of freeing them itself ([`BindFailure`]). The Std backings are + // freed after both, by the `parameters` field's own drop, which Rust runs + // AFTER this body — the same reason `ensure_parameters` destroys before it + // replaces. unsafe { (self.video_queue.fp().destroy_video_session_parameters_khr)( self.device.handle(), - self.parameters, + self.parameters.object, std::ptr::null(), ); (self.video_queue.fp().destroy_video_session_khr)( @@ -657,6 +735,47 @@ mod tests { (sps, pps) } + /// An `Add` hands NEW Std sets to an EXISTING parameters object, so their heap + /// blocks must live as long as that OBJECT — not as long as the update call + /// that carried them. [`StoredParams::adopt`] is where the transfer happens, + /// and this pins that it genuinely takes ownership: `ensure_parameters` drops + /// its local wrappers the instant this returns, and a driver holding + /// `pScalingLists` would be reading freed heap from the next frame on + /// ([`crate::session_av1`] for the measurement that made this real). + #[test] + fn an_added_set_keeps_its_blocks_alive_past_the_update_call() { + let sps = SpsBuilder::new() + .seq_parameter_set_id(0) + .profile_idc(Profile::Main) + .level_idc(Level::L4) + .frame_mbs_only_flag(true) + .direct_8x8_inference_flag(true) + .max_num_ref_frames(4) + .resolution(64, 64) + // The one SPS pointer a builder can attach. + .seq_scaling_matrix_present_flag(true) + .build(); + let owned = sps_to_std(&sps).expect("converts"); + let lists = owned.std().pScalingLists; + assert!(!lists.is_null(), "the fixture attaches scaling lists"); + + let mut stored = StoredParams::none(); + stored.adopt(Some(owned), None); + assert_eq!( + (stored.sps.len(), stored.pps.len()), + (1, 0), + "the object took the set itself, not a borrow of it" + ); + assert_eq!( + stored.sps[0].std().pScalingLists, + lists, + "and it is the same block the driver was handed" + ); + // SAFETY: `stored` owns the block — which is exactly the property here. + let read_back = unsafe { (*lists).ScalingList4x4[0] }; + assert_eq!(read_back, [0; 16], "the fixture's lists, read back live"); + } + #[test] fn a_reactivated_identical_pair_is_current_even_across_reparses() { let (sps_a, pps_a) = authored(0, 0, 26); diff --git a/crates/pf-vkdecode/src/session_h265.rs b/crates/pf-vkdecode/src/session_h265.rs index df0afbb7..f6c3a40a 100644 --- a/crates/pf-vkdecode/src/session_h265.rs +++ b/crates/pf-vkdecode/src/session_h265.rs @@ -29,6 +29,17 @@ //! real VPS finally arrives, the content differs from the fallback and the object //! RECREATES onto the real one, exactly as any other content change would. //! +//! ⚠⚠⚠ **The Std sets' heap blocks must outlive the parameters OBJECT, not just the +//! call that hands them over.** Vulkan reads as though parameter data were captured +//! by `vkCreateVideoSessionParametersKHR`, and all three codecs in this crate +//! assumed it. NVIDIA 610.57.04 does not: for AV1 it was measured keeping +//! `StdVideoAV1SequenceHeader::pColorConfig` and dereferencing it when a decode is +//! RECORDED, which decoded every frame against recycled heap ([`crate::session_av1`] +//! carries the measurement). H.265 embeds MORE such pointers than any other codec +//! here — the SPS alone carries seven — so [`StoredParamsH265`] holds the object and +//! its backings in ONE value with one lifetime, and both the recreate path and +//! `Drop` destroy the object before that value is released. +//! //! `ParamsLedgerH265` is the pure half of that decision table (unit-tested); //! `VideoSessionH265` is the thin Vulkan half. @@ -47,6 +58,8 @@ use crate::params_h265::pps_to_std_h265; use crate::params_h265::sps_to_std_h265; use crate::params_h265::vps_to_std_h265; use crate::params_h265::H265ParamsError; +use crate::params_h265::OwnedStdH265Pps; +use crate::params_h265::OwnedStdH265Sps; use crate::params_h265::OwnedStdH265Vps; use crate::params_h265::Pps; use crate::params_h265::Sps; @@ -247,13 +260,73 @@ pub struct SessionConfigH265 { pub profile: H265ProfileKey, } +/// A live parameters object **and every Std parameter set it was given**, in one +/// field — because the two may not drift apart. `session::StoredParams` one codec +/// over, with the VPS leg H.264 does not have. +/// +/// The wrapper is not decoration and not defensive: a driver in this fleet keeps +/// the embedded pointers out of a Std set and dereferences them long after the call +/// that handed them over returned (module docs), so releasing the backing early +/// hands it freed memory. One value rather than two fields makes "an object whose +/// backing is gone" unrepresentable, which is the only shape of this bug — and the +/// shape a `let owned = …;` local silently had. H.265 has the most to lose: its Std +/// SPS points at a profile/tier/level block, a DPB-manager block, scaling lists, +/// the short-term RPS candidate array and the long-term SPS candidates. +/// +/// What is pinned, precisely: the wrappers' BOXED blocks, which is what the driver +/// was measured retaining. The contiguous array of outer `StdVideoH265*` structs +/// each call receives is a short-lived temporary, and the driver copies THAT before +/// returning — which is what the AV1 fix itself rests on, its Std header being moved +/// into storage after the create call on a rung that is now 250/250 bit-exact. So +/// moving these wrappers, or reallocating the `Vec`s holding them, disturbs nothing +/// the driver kept; `params_h265::moving_the_wrapper_leaves_the_driver_s_pointers_put` +/// pins the half that matters. +struct StoredParamsH265 { + object: vk::VideoSessionParametersKHR, + /// One entry per set the OBJECT stores, held for the object's whole life. + /// Never read by this crate after the create/update call; the DRIVER reads the + /// blocks they own. + vps: Vec, + sps: Vec, + pps: Vec, +} + +impl StoredParamsH265 { + /// The placeholder a half-built session holds. `vkDestroyVideoSessionParametersKHR` + /// ignores a NULL handle, so a [`VideoSessionH265::create`] that fails before + /// the object exists still drops cleanly. + fn none() -> Self { + Self { + object: vk::VideoSessionParametersKHR::null(), + vps: Vec::new(), + sps: Vec::new(), + pps: Vec::new(), + } + } + + /// Take over sets an `Add` just handed to the live object — they belong to the + /// OBJECT now, so their blocks live as long as it does rather than as long as + /// the update call. Only ever reached after that call SUCCEEDED: a failed + /// update stored nothing, and its wrappers are dropped instead. + fn adopt( + &mut self, + vps: Option, + sps: Option, + pps: Option, + ) { + self.vps.extend(vps); + self.sps.extend(sps); + self.pps.extend(pps); + } +} + /// The Vulkan half: session + bound memory + parameters object. pub(crate) struct VideoSessionH265 { device: ash::Device, video_queue: ash::khr::video_queue::Device, session: vk::VideoSessionKHR, memory: Vec, - parameters: vk::VideoSessionParametersKHR, + parameters: StoredParamsH265, ledger: ParamsLedgerH265, pub(crate) config: SessionConfigH265, /// The session has never run a coding scope: the first one records a @@ -306,7 +379,7 @@ impl VideoSessionH265 { video_queue: dev.video_queue().clone(), session, memory: Vec::new(), - parameters: vk::VideoSessionParametersKHR::null(), + parameters: StoredParamsH265::none(), ledger: ParamsLedgerH265::default(), config, needs_reset: ResetArm::armed(), @@ -324,35 +397,43 @@ impl VideoSessionH265 { return Err(failure.error); } } - built.parameters = built.create_parameters_object(&[], &[], &[])?; + built.parameters = + built.create_parameters_object(Vec::new(), Vec::new(), Vec::new())?; } Ok(built) } /// Create a parameters object holding exactly `vps`/`sps`/`pps` (any may be - /// empty). + /// empty), **fused with the wrappers whose heap blocks it points at**. + /// + /// Taking the wrappers BY VALUE rather than as Std slices is the point: there is + /// no way to reach `vkCreateVideoSessionParametersKHR` from here without the + /// resulting object taking ownership of everything it will go on dereferencing + /// (module docs, [`StoredParamsH265`]). /// /// # Safety /// - /// Live device + live session; the Std slices' backing (the `OwnedStd*` - /// wrappers, INCLUDING the heap blocks their embedded pointers target) - /// outlives this call. - /// - /// ⚠ This used to end "— Vulkan copies all parameter data before returning", - /// and that is **not universally true**: see [`crate::session::VideoSession`]'s - /// twin of this comment and [`crate::session_av1`], where a driver was measured - /// dereferencing a retained pointer long after the create call. H.265's Std SPS - /// carries SEVEN embedded pointers, more than any other set in this crate. + /// Live device + live session. unsafe fn create_parameters_object( &self, - vps: &[hh::StdVideoH265VideoParameterSet], - sps: &[hh::StdVideoH265SequenceParameterSet], - pps: &[hh::StdVideoH265PictureParameterSet], - ) -> Result { + vps: Vec, + sps: Vec, + pps: Vec, + ) -> Result { + // The contiguous Std arrays the call wants. These are COPIES of each + // wrapper's Std struct (it is `Copy`, and the driver copies them again + // before returning); the embedded pointers they carry still address the + // wrappers' own boxed blocks, which are what must outlive the OBJECT. + let std_vps: Vec = + vps.iter().map(|o| *o.std()).collect(); + let std_sps: Vec = + sps.iter().map(|o| *o.std()).collect(); + let std_pps: Vec = + pps.iter().map(|o| *o.std()).collect(); let add = vk::VideoDecodeH265SessionParametersAddInfoKHR::default() - .std_vp_ss(vps) - .std_sp_ss(sps) - .std_pp_ss(pps); + .std_vp_ss(&std_vps) + .std_sp_ss(&std_sps) + .std_pp_ss(&std_pps); let mut h265 = vk::VideoDecodeH265SessionParametersCreateInfoKHR::default() .max_std_vps_count(MAX_STD_VPS as u32) .max_std_sps_count(MAX_STD_SPS as u32) @@ -361,20 +442,27 @@ impl VideoSessionH265 { let ci = vk::VideoSessionParametersCreateInfoKHR::default() .video_session(self.session) .push_next(&mut h265); - let mut parameters = vk::VideoSessionParametersKHR::null(); - // SAFETY: fn contract; `ci` roots locals outliving the call. + let mut object = vk::VideoSessionParametersKHR::null(); + // SAFETY: fn contract; `ci` roots locals outliving the call, and the blocks + // the driver may retain past it are owned by `vps`/`sps`/`pps`, which are + // moved into the returned value rather than dropped here. let r = unsafe { (self.video_queue.fp().create_video_session_parameters_khr)( self.device.handle(), &ci, std::ptr::null(), - &mut parameters, + &mut object, ) }; if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } - Ok(parameters) + Ok(StoredParamsH265 { + object, + vps, + sps, + pps, + }) } /// The ledger's verdict for activating (`vps`, `sps`, `pps`), without mutating @@ -447,19 +535,22 @@ impl VideoSessionH265 { .update_sequence_count(self.ledger.next_update_seq()) .push_next(&mut add); // SAFETY: live device + parameters object; `update` roots locals - // (incl. the OwnedStd backings) outliving the call. See - // `create_parameters_object` on why "and the driver copies them" - // is an observation about this fleet rather than a guarantee. + // (incl. the OwnedStd backings) outliving the call — and the + // backings go on outliving it, adopted below. let r = unsafe { (self.video_queue.fp().update_video_session_parameters_khr)( self.device.handle(), - self.parameters, + self.parameters.object, &update, ) }; if r != vk::Result::SUCCESS { return Err(SessionError::Vk(r)); } + // ⚠ The added sets now belong to the OBJECT, so their heap blocks + // must too: an Add whose wrappers died at the end of this arm would + // be the AV1 use-after-free with an update call in front of it. + self.parameters.adopt(owned_vps, owned_sps, owned_pps); self.ledger.commit(action, vps, sps, pps); Ok(()) } @@ -473,26 +564,35 @@ impl VideoSessionH265 { let owned_vps = vps.to_std()?; let owned_sps = sps_to_std_h265(sps)?; let owned_pps = pps_to_std_h265(pps)?; - // SAFETY: fn contract (the OwnedStd backings live across the call). + // SAFETY: fn contract — live device + live session. The wrappers + // are MOVED IN and come back owned by the fresh object, so they + // live as long as it does rather than merely across the call. let fresh = unsafe { self.create_parameters_object( - std::slice::from_ref(owned_vps.std()), - std::slice::from_ref(owned_sps.std()), - std::slice::from_ref(owned_pps.std()), + vec![owned_vps], + vec![owned_sps], + vec![owned_pps], )? }; + // The old object goes FIRST and its backings with it — installing + // `fresh` through a local keeps the destroy ahead of the free, + // which is the order a driver still holding the old pointers needs. + let old = std::mem::replace(&mut self.parameters, fresh); // SAFETY: the fn-level contract — the caller drained every // in-flight decode before a Recreate reached here (checked via // parameters_action), so no submitted work reads the old object; - // it is this session's own handle. + // it is this session's own handle, on a live device. unsafe { (self.video_queue.fp().destroy_video_session_parameters_khr)( self.device.handle(), - self.parameters, + old.object, std::ptr::null(), ); } - self.parameters = fresh; + // Explicit, because the ORDER is the whole point: every Std block + // `old` owns is released only now, after the object that pointed at + // them is gone. + drop(old); self.ledger.commit(action, vps, sps, pps); Ok(()) } @@ -504,7 +604,7 @@ impl VideoSessionH265 { } pub(crate) fn parameters(&self) -> vk::VideoSessionParametersKHR { - self.parameters + self.parameters.object } /// Whether the next coding scope must record the initialization RESET — @@ -531,11 +631,14 @@ impl Drop for VideoSessionH265 { // ORDER is load-bearing, not stylistic: memory bound into a session may // not be freed while the session lives, so the session is destroyed first // — which is also why a failed bind hands its allocations back here - // instead of freeing them itself (`crate::session::BindFailure`). + // instead of freeing them itself (`crate::session::BindFailure`). The Std + // backings are freed after both, by the `parameters` field's own drop, + // which Rust runs AFTER this body — the same reason `ensure_parameters` + // destroys before it replaces. unsafe { (self.video_queue.fp().destroy_video_session_parameters_khr)( self.device.handle(), - self.parameters, + self.parameters.object, std::ptr::null(), ); (self.video_queue.fp().destroy_video_session_khr)( @@ -632,6 +735,42 @@ mod tests { }) } + /// An `Add` hands NEW Std sets to an EXISTING parameters object, so their heap + /// blocks must live as long as that OBJECT — not as long as the update call + /// that carried them. [`StoredParamsH265::adopt`] is where the transfer + /// happens, and this pins that it genuinely takes ownership: `ensure_parameters` + /// drops its local wrappers the instant this returns, and a driver holding + /// `pDecPicBufMgr` or `pProfileTierLevel` would be reading freed heap from the + /// next frame on ([`crate::session_av1`] for the measurement that made this + /// real). + #[test] + fn an_added_set_keeps_its_blocks_alive_past_the_update_call() { + // The ledger fixtures never convert, so they leave `general_profile_idc` + // at the unmappable 0; Std conversion needs a real one. + let mut base = (*authored_sps(0, 0, 64)).clone(); + base.profile_tier_level.general_profile_idc = 1; // Main + base.max_dec_pic_buffering_minus1 = [5, 0, 0, 0, 0, 0, 0]; + let sps = Rc::new(base); + let owned_vps = VpsSource::for_sps(&sps).to_std().expect("converts"); + let owned_sps = sps_to_std_h265(&sps).expect("converts"); + let vps_ptl = owned_vps.std().pProfileTierLevel; + let sps_dpb = owned_sps.std().pDecPicBufMgr; + assert!(!vps_ptl.is_null() && !sps_dpb.is_null()); + + let mut stored = StoredParamsH265::none(); + stored.adopt(Some(owned_vps), Some(owned_sps), None); + assert_eq!( + (stored.vps.len(), stored.sps.len(), stored.pps.len()), + (1, 1, 0), + "the object took the sets themselves, not borrows of them" + ); + assert_eq!(stored.vps[0].std().pProfileTierLevel, vps_ptl); + assert_eq!(stored.sps[0].std().pDecPicBufMgr, sps_dpb); + // SAFETY: `stored` owns both blocks — which is exactly the property here. + let dpb = unsafe { (*sps_dpb).max_dec_pic_buffering_minus1[0] }; + assert_eq!(dpb, 5, "the fixture's DPB sizing, read back live"); + } + #[test] fn the_first_activation_adds_all_three_sets_in_one_update_call() { let sps = with_vps(&authored_sps(0, 0, 64), 0, 0);