fix(vkdecode): the address the driver keeps is now the address we keep
The AV1 use-after-free fix (cdd1f3ef) stabilised the wrong half. NVIDIA was
measured retaining pColorConfig, so StoredParamsAv1 boxed the colour and timing
blocks — but OwnedStdAv1SequenceHeader kept the Std struct ITSELF inline, so the
pStdSequenceHeader we handed vkCreateVideoSessionParametersKHR was a stack
address inside ensure_parameters, dead the moment it returned. The fix worked
because of WHICH pointer that driver happened to hold. A driver retaining the
outer one instead — no more of a spec violation than retaining pColorConfig was —
reproduces the original bug exactly: plausible pictures, wrong content, no error
and no counter moved.
The same shape was in the shipping codecs, one step further from evidence: the
H.264 and H.265 create paths pointed pStdSPSs/pStdPPSs/pStdVPSs at function-local
Vecs, and both Add paths handed over the wrapper's inline std field and then moved
the wrapper. Those are spec-legal — the object stores copies — and have never
misbehaved on the fleet. They are fixed anyway, because that is precisely what was
true of H.264/H.265 before the same class of bug was found in them, and a
correctness argument that reduces to which vendor we tested is not one.
So: the Std struct is boxed inside each owning wrapper (one level out from what
_color_backing already did), and the contiguous create-time arrays are now fields
of the stored parameters, assembled at their final address. Identical bytes at
identical offsets — only where they live changed.
The line drawn deliberately, in prose at session.rs:29: Std DATA is pinned; the
VkVideoSessionParametersCreateInfoKHR chain itself is not. Retention there would
be a different and far more extreme class of driver bug, and pinning it needs a
self-referential struct over lifetime-parameterised builders.
⚠ NOT hardware-verified. No GPU has run this — the fleet is unreachable and the
250/250 parity that proved this code bit-exact cannot be re-run. That is why the
change is constrained to address stability alone, and why it ships five CPU-only
tests instead: three capture the pointer handed to Vulkan, perform the real move,
and assert it survives — each verified FAILING first, with genuinely differing
addresses, not a tautology. Two more pin the create-array ownership; those fail
before the fix as compile errors rather than assertions, because the pre-fix bug
there is a dangling pointer and asserting on it is UB.
Also: caps.rs claimed the borrow checker pins a profile chain between wire() and
its last use. False at exactly one site — decoder.rs took a raw *const, ending the
borrow, leaving nothing but inspection to stop a future editor moving the chain
before create_query_pool. Correct today, guarded by prose, which is how the first
bug shipped. It is now compiler-enforced: the pointer write and the create call
live inside one helper that takes the profile by reference, so the borrow is held
across both by the signature. An audit cleared the chains otherwise — no entry
point we pass one to retains it.
Gates: fmt clean; clippy -D warnings over pf-vkdecode AND pf-client-core in the
Linux container (its only real consumer, which cannot build on macOS at all —
wol.rs uses deps its manifest gates to linux/windows, so workspace clippy has
never passed there and does not now); 187 lib tests green on Linux, up from 182.
This commit is contained in:
@@ -496,17 +496,9 @@ impl OpRing {
|
||||
) -> Result<Self, vk::Result> {
|
||||
let query_pool = if dev.result_status_queries() {
|
||||
let mut chain = decode_profile.chain();
|
||||
let profile = chain.wire();
|
||||
let mut query_ci = vk::QueryPoolCreateInfo::default()
|
||||
.query_type(vk::QueryType::RESULT_STATUS_ONLY_KHR)
|
||||
.query_count(query_count);
|
||||
// Chained manually: `push_next` would clobber the profile's own
|
||||
// `p_next` (its H264 half) — the encoder's exact precedent.
|
||||
query_ci.p_next = (profile as *const vk::VideoProfileInfoKHR<'_>).cast();
|
||||
// SAFETY: live device; `query_ci` roots the wired chain for the call.
|
||||
// The video profile chained in satisfies the "same profile as the
|
||||
// session" rule for queries used inside a coding scope.
|
||||
Some(unsafe { dev.ash().create_query_pool(&query_ci, None)? })
|
||||
// SAFETY: fn contract. `chain` outlives the call, and the helper's
|
||||
// SIGNATURE — not a comment — is what keeps it immobile across it.
|
||||
Some(unsafe { Self::create_status_query_pool(dev, chain.wire(), query_count)? })
|
||||
} else {
|
||||
debug!(
|
||||
"decode family lacks queryResultStatusSupport — no per-op status \
|
||||
@@ -554,6 +546,39 @@ impl OpRing {
|
||||
cmds,
|
||||
})
|
||||
}
|
||||
|
||||
/// The RESULT_STATUS query pool, created against `profile`.
|
||||
///
|
||||
/// Split out for the BORROW rather than for tidiness. `VkQueryPoolCreateInfo`
|
||||
/// has no codec-aware builder here — `push_next` would clobber the profile's
|
||||
/// own `p_next` (its codec half), so the chain is written as a raw `*const`, and
|
||||
/// a raw pointer ends the borrow the moment it is taken. Inline, only inspection
|
||||
/// stopped a later edit from moving or dropping the chain between that write and
|
||||
/// `vkCreateQueryPool`; taking `&vk::VideoProfileInfoKHR<'_>` as a PARAMETER
|
||||
/// makes the compiler hold the borrow across the whole call instead
|
||||
/// ([`crate::caps::H264ProfileChain`]'s contract, which used to claim the
|
||||
/// borrow checker covered this site and did not).
|
||||
///
|
||||
/// # Safety
|
||||
///
|
||||
/// `dev` wraps live handles ([`DeviceHandles`] contract).
|
||||
unsafe fn create_status_query_pool(
|
||||
dev: &DecodeDevice,
|
||||
profile: &vk::VideoProfileInfoKHR<'_>,
|
||||
query_count: u32,
|
||||
) -> Result<vk::QueryPool, vk::Result> {
|
||||
let mut query_ci = vk::QueryPoolCreateInfo::default()
|
||||
.query_type(vk::QueryType::RESULT_STATUS_ONLY_KHR)
|
||||
.query_count(query_count);
|
||||
// Chained manually: `push_next` would clobber the profile's own `p_next`
|
||||
// — the encoder's exact precedent.
|
||||
query_ci.p_next = std::ptr::from_ref(profile).cast();
|
||||
// SAFETY: fn contract; `query_ci` roots the wired chain for the call, and
|
||||
// `profile` is borrowed for the whole of this body so the chain cannot move
|
||||
// out from under that pointer. The video profile chained in satisfies the
|
||||
// "same profile as the session" rule for queries used inside a coding scope.
|
||||
unsafe { dev.ash().create_query_pool(&query_ci, None) }
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for OpRing {
|
||||
|
||||
Reference in New Issue
Block a user