fix(pf-encode): the Vulkan extension probe walked a driver-filled array with no bound
ci / web (pull_request) Successful in 1m20s
apple / swift (pull_request) Successful in 1m38s
apple / screenshots (pull_request) Skipped
windows-drivers / driver-build (pull_request) Successful in 1m49s
windows-drivers / probe-and-proto (pull_request) Successful in 27s
ci / docs-site (pull_request) Successful in 1m22s
ci / rust-arm64 (pull_request) Successful in 2m50s
ci / bun-nix (pull_request) Successful in 24s
android / android (pull_request) Successful in 4m37s
ci / rust (pull_request) Successful in 10m25s
ci / web (pull_request) Successful in 1m20s
apple / swift (pull_request) Successful in 1m38s
apple / screenshots (pull_request) Skipped
windows-drivers / driver-build (pull_request) Successful in 1m49s
windows-drivers / probe-and-proto (pull_request) Successful in 27s
ci / docs-site (pull_request) Successful in 1m22s
ci / rust-arm64 (pull_request) Successful in 2m50s
ci / bun-nix (pull_request) Successful in 24s
android / android (pull_request) Successful in 4m37s
ci / rust (pull_request) Successful in 10m25s
`ext_advertised` did `CStr::from_ptr(e.extension_name.as_ptr())` over a
driver-filled `[c_char; VK_MAX_EXTENSION_NAME_SIZE]`, and `vk_build.rs` open-coded
the identical call a second time. Neither had an in-Rust bound: a driver that
fills all 256 bytes without a NUL runs the walk into the NEXT
`ExtensionProperties`, and on the LAST element past the allocation.
The SAFETY comment asserted the spec guarantee ("a spec-guaranteed NUL-terminated
byte array") instead of enforcing it. That is the defect class this programme
keeps finding: a proof that restates what the other side promised rather than
checking it. Vulkan drivers are exactly the other side.
The bounded answer already shipped in the same crate — `pyrowave.rs:210` uses
ash's `extension_name_as_c_str()` for the identical job. It stops at
VK_MAX_EXTENSION_NAME_SIZE and returns Err when there is no terminator, so a
malformed entry is a non-match instead of an overrun. Both sites now route
through the one helper, which is no longer unsafe at all.
Deletes 2 unsafe operations and one duplicated walk.
⚠ The pre-existing test could not have caught this: it only ever built
well-formed, NUL-terminated entries. Added a case whose LAST element is 256
non-NUL bytes — the exact shape that used to leave the array — and a
prefix-match case, so the bound is now asserted rather than assumed.
Verified on 192.168.1.25 (Ubuntu, cargo 1.96.0 — the pinned toolchain):
cargo check -p pf-encode --features vulkan-encode,pyrowave --locked ok
cargo test -p pf-encode --features vulkan-encode,pyrowave ext_advertised
2 passed / 0 failed
cargo clippy -p pf-encode --all-targets --locked
--features vulkan-encode,pyrowave -- -D warnings clean
Linux-only code (`enc/linux/`), so the Windows leg is unaffected.
This commit is contained in:
@@ -17,7 +17,7 @@
|
||||
// child-module shape. External imports are this file's own; `vk_util` is a crate-root sibling,
|
||||
// so the path is `crate::`, not the parent-relative `super::` the parent uses.
|
||||
use super::*;
|
||||
use crate::vk_util::{find_mem, make_plain_image, make_view};
|
||||
use crate::vk_util::{ext_advertised, find_mem, make_plain_image, make_view};
|
||||
use anyhow::{bail, Result};
|
||||
use ash::vk;
|
||||
use std::ffi::c_void;
|
||||
@@ -53,10 +53,10 @@ pub(super) unsafe fn probe_rgb_direct(
|
||||
let Ok(exts) = instance.enumerate_device_extension_properties(pd) else {
|
||||
return Err("probe-failed(ext-enum)");
|
||||
};
|
||||
if !exts
|
||||
.iter()
|
||||
.any(|e| std::ffi::CStr::from_ptr(e.extension_name.as_ptr()) == vrgb::EXTENSION_NAME)
|
||||
{
|
||||
// Route through `vk_util::ext_advertised` rather than open-coding the walk a second time:
|
||||
// this copy used the same unbounded `CStr::from_ptr` and had the same read-past-the-array
|
||||
// hazard on a driver that fills all VK_MAX_EXTENSION_NAME_SIZE bytes without a NUL.
|
||||
if !ext_advertised(&exts, vrgb::EXTENSION_NAME) {
|
||||
return Err("no-ext(mesa<26.0-or-no-efc)");
|
||||
}
|
||||
// 2. Feature bit.
|
||||
|
||||
@@ -19,11 +19,15 @@ use pf_frame::PixelFormat;
|
||||
/// barriers were used without the extension ever being enabled; `pf-presenter/dmabuf.rs` is the
|
||||
/// in-repo precedent that enables it).
|
||||
pub(super) fn ext_advertised(exts: &[vk::ExtensionProperties], name: &std::ffi::CStr) -> bool {
|
||||
exts.iter().any(|e| {
|
||||
// SAFETY: `extension_name` is a spec-guaranteed NUL-terminated UTF-8 byte array inside
|
||||
// the driver-filled `VkExtensionProperties` (VK_MAX_EXTENSION_NAME_SIZE bound).
|
||||
unsafe { std::ffi::CStr::from_ptr(e.extension_name.as_ptr()) == name }
|
||||
})
|
||||
// `extension_name_as_c_str()` is ash's BOUNDED accessor: it stops at
|
||||
// `VK_MAX_EXTENSION_NAME_SIZE` and returns `Err` when the array holds no NUL, so a
|
||||
// malformed driver entry is a non-match rather than a read past the array. The previous
|
||||
// `CStr::from_ptr(e.extension_name.as_ptr())` had no in-Rust bound at all — its SAFETY
|
||||
// comment asserted the spec guarantee instead of enforcing it, so a driver that filled all
|
||||
// 256 bytes without a terminator ran the walk into the NEXT `ExtensionProperties` and, on
|
||||
// the last element, past the allocation. Same accessor `pyrowave.rs` already uses for the
|
||||
// identical job. No unsafe, no unchecked read, same answer on every well-formed driver.
|
||||
exts.iter().any(|e| e.extension_name_as_c_str() == Ok(name))
|
||||
}
|
||||
|
||||
pub(crate) fn color_range(layer: u32) -> vk::ImageSubresourceRange {
|
||||
@@ -453,6 +457,29 @@ mod tests {
|
||||
));
|
||||
}
|
||||
|
||||
/// A driver entry with NO terminator anywhere in `extension_name` must be a non-match, not a
|
||||
/// read past the array.
|
||||
///
|
||||
/// This is the case the old `CStr::from_ptr(e.extension_name.as_ptr())` could not survive:
|
||||
/// with every one of VK_MAX_EXTENSION_NAME_SIZE bytes non-NUL it walked into the NEXT
|
||||
/// `ExtensionProperties`, and on the LAST element past the allocation entirely. The old test
|
||||
/// only ever built well-formed, NUL-terminated entries, so it proved nothing about the bound
|
||||
/// — which is why the hazard survived a SAFETY comment that asserted the spec guarantee
|
||||
/// rather than enforcing it.
|
||||
#[test]
|
||||
fn ext_advertised_rejects_unterminated_name_without_overrunning() {
|
||||
let mut bad = ash::vk::ExtensionProperties::default();
|
||||
bad.extension_name.fill(b'A' as std::ffi::c_char);
|
||||
// Deliberately LAST, so an unbounded walk would leave the whole array.
|
||||
let exts = [ash::vk::ExtensionProperties::default(), bad];
|
||||
assert!(!super::ext_advertised(
|
||||
&exts,
|
||||
ash::ext::queue_family_foreign::NAME
|
||||
));
|
||||
// And a name that is a prefix of the garbage still must not match.
|
||||
assert!(!super::ext_advertised(&exts, c"AAAA"));
|
||||
}
|
||||
|
||||
use super::*;
|
||||
|
||||
/// CSC mode (`bgra_target = false`): the 3→4 expand is a pure byte shuffle — no channel
|
||||
|
||||
Reference in New Issue
Block a user