fix(vkdecode): H.264 and H.265 session parameters own what the driver keeps
The same use-after-free the AV1 rung was just fixed for, closed in the two rungs that ship. session.rs and session_h265.rs handed their Std parameter sets to vkCreateVideoSessionParametersKHR and dropped the backings when the call returned; NVIDIA 610.57.04 was measured retaining such a pointer to decode-record time, which is what made AV1 diverge on 250 of 250 frames. Nothing was known to be broken here — both rungs are bit-exact on four drivers — but that was luck rather than correctness: the freed blocks happen to still hold the right bytes in that window. The native Vulkan rung sits in the auto ladder above FFmpeg-Vulkan on shipping clients, so this was live code, and its failure mode is silent wrong pixels rather than a crash. StoredParams and StoredParamsH265 hold the parameters object together with every wrapper it points at, so an object whose backing is gone cannot be built. create_parameters_object takes the wrappers by value; the Add arm adopts them only after a successful update, so a failed update drops what it never stored; the Recreate arm replaces, destroys the old object, then drops its backings, written explicitly so the ordering survives later edits. The Add-vs-Recreate decision table and the VPS ledger are untouched — only ownership moved. params.rs still carried the refuted claim as a type-level contract, that Vulkan "copies all parameter data before returning" and keeping the wrapper alive across the call "is the whole obligation". Corrected to the measured truth. The tests are what stop this returning, and each was verified by sabotage: inlining the H.264 PPS box fails at pps pScalingLists, inlining the H.265 SPS DPB box fails at sps pDecPicBufMgr, and making either adopt drop instead of store fails both session tests. Two lessons are recorded in them. Pointer equality cannot be the assertion, because the Std struct carries pointers by value and a stale one compares equal — the read-back is the discriminator, so the tests clobber the dead stack first to make a dangling read deterministic rather than lucky. And the first H.265 draft read six of eight pointers and let the sabotage through, so it now reads every one with a labelled assert. ⚠ One site of this class remains, deliberately: the VkVideoProfileInfoKHR chains, where wire()'s borrow dies with its enclosing block while the object created from it lives on — three session creates, an image, a buffer, and a query pool built from a raw pointer into a stack chain. It spans six modules and all three codecs, and a profile is enums a driver resolves at create time with no per-frame deref, so the risk is materially lower. It wants its own pass with its own hardware verification. Gates: macOS fmt/clippy/196 tests, container clippy -D warnings, pf-vkdecode 182/182 and pf-client-core 140/140. On the RTX 5070 Ti, all 8 gpu_parity legs re-verified green after the change — H.264, H.265, Main 10 and AV1 all still bit-identical to libavcodec.
This commit is contained in:
@@ -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();
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<OwnedStdSps>,
|
||||
pps: Vec<OwnedStdPps>,
|
||||
}
|
||||
|
||||
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<OwnedStdSps>, pps: Option<OwnedStdPps>) {
|
||||
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<vk::DeviceMemory>,
|
||||
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<vk::VideoSessionParametersKHR, SessionError> {
|
||||
sps: Vec<OwnedStdSps>,
|
||||
pps: Vec<OwnedStdPps>,
|
||||
) -> Result<StoredParams, SessionError> {
|
||||
// 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<hh::StdVideoH264SequenceParameterSet> =
|
||||
sps.iter().map(|o| *o.std()).collect();
|
||||
let std_pps: Vec<hh::StdVideoH264PictureParameterSet> =
|
||||
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);
|
||||
|
||||
@@ -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<OwnedStdH265Vps>,
|
||||
sps: Vec<OwnedStdH265Sps>,
|
||||
pps: Vec<OwnedStdH265Pps>,
|
||||
}
|
||||
|
||||
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<OwnedStdH265Vps>,
|
||||
sps: Option<OwnedStdH265Sps>,
|
||||
pps: Option<OwnedStdH265Pps>,
|
||||
) {
|
||||
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<vk::DeviceMemory>,
|
||||
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<vk::VideoSessionParametersKHR, SessionError> {
|
||||
vps: Vec<OwnedStdH265Vps>,
|
||||
sps: Vec<OwnedStdH265Sps>,
|
||||
pps: Vec<OwnedStdH265Pps>,
|
||||
) -> Result<StoredParamsH265, SessionError> {
|
||||
// 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<hh::StdVideoH265VideoParameterSet> =
|
||||
vps.iter().map(|o| *o.std()).collect();
|
||||
let std_sps: Vec<hh::StdVideoH265SequenceParameterSet> =
|
||||
sps.iter().map(|o| *o.std()).collect();
|
||||
let std_pps: Vec<hh::StdVideoH265PictureParameterSet> =
|
||||
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);
|
||||
|
||||
Reference in New Issue
Block a user