fix(vdisplay/windows): own the device handle, prove the ladder once, and fix two proofs that were wrong
Three of Phase 5's four items; 5.4 is held for a decision (see below). `open_device` is a SAFE fn returning an `OwnedHandle`. It never had a caller obligation — it takes no arguments and every precondition is internal — so the `unsafe fn` it used to be pushed a proof burden onto four call sites with nothing to prove, and handed back a raw `HANDLE` that two of them re-owned by hand with `CloseHandle`. That shape has already leaked once in this file (the wrap-IMMEDIATELY comment in `open` records it). Ownership is now a `Drop`, `probe()` is `open_device().map(|_| ())`, `is_available()` is `open_device().is_ok()`, and `CloseHandle` is no longer imported here at all. `resolve_target_gdi` ran the same 60 × 50 ms poll three times verbatim, and the 2nd and 3rd copies documented themselves as "SAFETY: as the resolve loop above" — a pointer to a proof rather than a proof, which rots silently the moment the block it points at moves. One `poll_gdi_name`, one proof, and the three-stage ladder now reads as the three stages its doc describes. Two proofs were simply wrong, both discharged everything except the one obligation that mattered: * The SetupAPI detail buffer was a `Vec<u8>` (align 1) written through as `SP_DEVICE_INTERFACE_DETAIL_DATA_W` (align 4). The SAFETY comment proved bounds and aliasing and was silent on alignment. Now a `Vec<u64>`. Its guard also compared the required size against `size_of::<u32>()` while stamping `size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>()` into `cbSize` — checking a different number than the one it promises the OS. * `GetMonitorInfoW` was handed `&mut info.monitorInfo`: `cbSize` promises 104 bytes but that pointer's provenance covers the 40-byte `MONITORINFO` prefix, so everything the OS writes past byte 40 — which is `szDevice`, the only field the caller reads — is out of bounds for it. A compiler may assume those bytes were untouched and fold the zeroed initializer into the reads, i.e. return an EMPTY device name. That name is what `panel_off_except`'s exclusion compares against, so an empty one darkens the panel it is meant to spare. The pointer now carries the full struct's provenance. pf_vdisplay.rs 42 -> 34 `unsafe`, manager.rs 58 -> 56. Windows runner: clippy --all-targets clean, 48 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -61,10 +61,19 @@ fn active_monitors() -> Vec<ActiveMonitor> {
|
|||||||
let out = unsafe { &mut *(data.0 as *mut Vec<ActiveMonitor>) };
|
let out = unsafe { &mut *(data.0 as *mut Vec<ActiveMonitor>) };
|
||||||
let mut info = MONITORINFOEXW::default();
|
let mut info = MONITORINFOEXW::default();
|
||||||
info.monitorInfo.cbSize = std::mem::size_of::<MONITORINFOEXW>() as u32;
|
info.monitorInfo.cbSize = std::mem::size_of::<MONITORINFOEXW>() as u32;
|
||||||
// SAFETY: `hmon` is the live monitor handle the enumeration just handed us; `info` is a
|
// The pointer is derived from the WHOLE `MONITORINFOEXW`, not from its `monitorInfo` field.
|
||||||
// properly-sized MONITORINFOEXW local whose cbSize is set, which GetMonitorInfoW requires
|
// `cbSize` promises the OS 104 bytes, but `&mut info.monitorInfo` is a pointer to the
|
||||||
// to safely write the extended (szDevice) variant.
|
// 40-byte `MONITORINFO` prefix: everything the OS writes past byte 40 — which is `szDevice`,
|
||||||
if unsafe { GetMonitorInfoW(hmon, &mut info.monitorInfo) }.as_bool() {
|
// the only field this function reads — is then out of bounds for that pointer's provenance.
|
||||||
|
// A compiler is entitled to assume those bytes were untouched and fold the zeroed
|
||||||
|
// initializer into the reads below, i.e. hand back an EMPTY device name. That name is what
|
||||||
|
// `panel_off_except`'s exclusion compares against, so an empty one would darken the very
|
||||||
|
// panel it is meant to spare.
|
||||||
|
let p = (&raw mut info).cast::<windows::Win32::Graphics::Gdi::MONITORINFO>();
|
||||||
|
// SAFETY: `hmon` is the live monitor handle the enumeration just handed us. `p` carries the
|
||||||
|
// provenance of the full `MONITORINFOEXW`, so the OS may write all `cbSize` bytes it was
|
||||||
|
// promised; `info` is a live local that outlives this synchronous call.
|
||||||
|
if unsafe { GetMonitorInfoW(hmon, p) }.as_bool() {
|
||||||
let len = info
|
let len = info
|
||||||
.szDevice
|
.szDevice
|
||||||
.iter()
|
.iter()
|
||||||
|
|||||||
@@ -183,6 +183,25 @@ enum ShrinkAction {
|
|||||||
/// snapshot too (from `set_virtual_primary_ccd`), so keying on the snapshot ran the EXCLUSIVE
|
/// snapshot too (from `set_virtual_primary_ccd`), so keying on the snapshot ran the EXCLUSIVE
|
||||||
/// isolate on a Primary group — clearing `DISPLAYCONFIG_PATH_ACTIVE` on every non-kept path, i.e.
|
/// isolate on a Primary group — clearing `DISPLAYCONFIG_PATH_ACTIVE` on every non-kept path, i.e.
|
||||||
/// blanking the very physical displays `Primary` exists to keep lit.
|
/// blanking the very physical displays `Primary` exists to keep lit.
|
||||||
|
/// One stage of [`ManagerInner::resolve_target_gdi`]'s ladder: poll for the target's GDI name until
|
||||||
|
/// the 3 s ceiling. 50 ms sampling (latency plan P0.5) — a typical activation resolves on an early
|
||||||
|
/// poll, so finer sampling shaves ~150 ms off every stage crossing.
|
||||||
|
///
|
||||||
|
/// Extracted because the ladder ran this loop three times verbatim, and the 2nd and 3rd copies
|
||||||
|
/// documented themselves as "SAFETY: as the resolve loop above" — a pointer to a proof rather than
|
||||||
|
/// a proof, which silently rots the moment the block it points at moves.
|
||||||
|
fn poll_gdi_name(target_id: u32) -> Option<String> {
|
||||||
|
for _ in 0..60 {
|
||||||
|
thread::sleep(Duration::from_millis(50));
|
||||||
|
// SAFETY: `resolve_gdi_name` is `unsafe` for its CCD FFI; it takes a plain `Copy` `u32`
|
||||||
|
// target id by value and returns an owned `String`, so no caller memory is borrowed.
|
||||||
|
if let Some(n) = unsafe { resolve_gdi_name(target_id) } {
|
||||||
|
return Some(n);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
None
|
||||||
|
}
|
||||||
|
|
||||||
fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction {
|
fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction {
|
||||||
if ccd_exclusive {
|
if ccd_exclusive {
|
||||||
ShrinkAction::Reisolate
|
ShrinkAction::Reisolate
|
||||||
@@ -1049,32 +1068,19 @@ impl VirtualDisplayManager {
|
|||||||
// structure encodes real failure modes (headless auto-activate, integrated-panel clone,
|
// structure encodes real failure modes (headless auto-activate, integrated-panel clone,
|
||||||
// lid-closed path activation) and is untouched — but a typical activation resolves on an
|
// lid-closed path activation) and is untouched — but a typical activation resolves on an
|
||||||
// early poll, so finer sampling shaves ~150 ms off every stage crossing.
|
// early poll, so finer sampling shaves ~150 ms off every stage crossing.
|
||||||
for _ in 0..60 {
|
if let Some(n) = poll_gdi_name(target_id) {
|
||||||
thread::sleep(Duration::from_millis(50));
|
return Some(n);
|
||||||
// SAFETY: `resolve_gdi_name` is `unsafe` for its CCD FFI; it takes a plain `Copy` `u32`
|
|
||||||
// target id by value and returns an owned `String`, so no caller memory is borrowed.
|
|
||||||
if let Some(n) = unsafe { resolve_gdi_name(target_id) } {
|
|
||||||
return Some(n);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
// SAFETY: `force_extend_topology` only calls `SetDisplayConfig` (CCD) with no borrowed memory.
|
// SAFETY: `force_extend_topology` only calls `SetDisplayConfig` (CCD) with no borrowed memory.
|
||||||
unsafe { force_extend_topology() };
|
unsafe { force_extend_topology() };
|
||||||
for _ in 0..60 {
|
if let Some(n) = poll_gdi_name(target_id) {
|
||||||
thread::sleep(Duration::from_millis(50));
|
return Some(n);
|
||||||
// SAFETY: as the resolve loop above.
|
|
||||||
if let Some(n) = unsafe { resolve_gdi_name(target_id) } {
|
|
||||||
return Some(n);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
// SAFETY: `activate_target_path` runs the CCD query/apply FFI with owned local buffers; the
|
// SAFETY: `activate_target_path` runs the CCD query/apply FFI with owned local buffers; the
|
||||||
// `Copy` target id is passed by value, under the `state` lock — the sole topology mutator.
|
// `Copy` target id is passed by value, under the `state` lock — the sole topology mutator.
|
||||||
if unsafe { pf_win_display::win_display::activate_target_path(target_id) } {
|
if unsafe { pf_win_display::win_display::activate_target_path(target_id) } {
|
||||||
for _ in 0..60 {
|
if let Some(n) = poll_gdi_name(target_id) {
|
||||||
thread::sleep(Duration::from_millis(50));
|
return Some(n);
|
||||||
// SAFETY: as the resolve loops above.
|
|
||||||
if let Some(n) = unsafe { resolve_gdi_name(target_id) } {
|
|
||||||
return Some(n);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
None
|
None
|
||||||
|
|||||||
@@ -29,7 +29,7 @@ use windows::Win32::Devices::DeviceAndDriverInstallation::{
|
|||||||
SetupDiGetDeviceInterfaceDetailW, DIGCF_DEVICEINTERFACE, DIGCF_PRESENT, HDEVINFO, SPINT_ACTIVE,
|
SetupDiGetDeviceInterfaceDetailW, DIGCF_DEVICEINTERFACE, DIGCF_PRESENT, HDEVINFO, SPINT_ACTIVE,
|
||||||
SP_DEVICE_INTERFACE_DATA, SP_DEVICE_INTERFACE_DETAIL_DATA_W,
|
SP_DEVICE_INTERFACE_DATA, SP_DEVICE_INTERFACE_DETAIL_DATA_W,
|
||||||
};
|
};
|
||||||
use windows::Win32::Foundation::{CloseHandle, HANDLE, LUID};
|
use windows::Win32::Foundation::{HANDLE, LUID};
|
||||||
use windows::Win32::Storage::FileSystem::{
|
use windows::Win32::Storage::FileSystem::{
|
||||||
CreateFileW, FILE_FLAGS_AND_ATTRIBUTES, FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_EXISTING,
|
CreateFileW, FILE_FLAGS_AND_ATTRIBUTES, FILE_SHARE_READ, FILE_SHARE_WRITE, OPEN_EXISTING,
|
||||||
};
|
};
|
||||||
@@ -325,7 +325,14 @@ impl Drop for DevInfoList {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
unsafe fn open_device() -> Result<HANDLE> {
|
/// Open the pf-vdisplay control device.
|
||||||
|
///
|
||||||
|
/// SAFE, and owning. It has no caller obligation — it takes no arguments and every precondition is
|
||||||
|
/// internal — so the `unsafe fn` it used to be pushed a proof burden onto four call sites that had
|
||||||
|
/// nothing to prove; two of them then re-established ownership by hand with `CloseHandle`, a shape
|
||||||
|
/// this file has already leaked from once (see the wrap-IMMEDIATELY comment in `open`). Returning an
|
||||||
|
/// `OwnedHandle` makes the close a `Drop`, so there is exactly one way to get it wrong: not at all.
|
||||||
|
fn open_device() -> Result<OwnedHandle> {
|
||||||
// SAFETY: plain SetupAPI enumeration call; the returned list is solely owned by the RAII wrapper.
|
// SAFETY: plain SetupAPI enumeration call; the returned list is solely owned by the RAII wrapper.
|
||||||
let hdev = DevInfoList(
|
let hdev = DevInfoList(
|
||||||
unsafe {
|
unsafe {
|
||||||
@@ -369,14 +376,21 @@ unsafe fn open_device() -> Result<HANDLE> {
|
|||||||
let _ = unsafe {
|
let _ = unsafe {
|
||||||
SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, None, 0, Some(&mut required), None)
|
SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, None, 0, Some(&mut required), None)
|
||||||
};
|
};
|
||||||
if (required as usize) < size_of::<u32>() {
|
// Against the struct's own size, not `u32`'s: the value stamped into `cbSize` below is
|
||||||
|
// `size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>()`, so that is what the buffer must hold.
|
||||||
|
if (required as usize) < size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>() {
|
||||||
continue; // sizing failed — never stamp a cbSize through an under-sized buffer
|
continue; // sizing failed — never stamp a cbSize through an under-sized buffer
|
||||||
}
|
}
|
||||||
let mut buf = vec![0u8; required as usize];
|
// `u64`, not `u8`: this buffer is written through as `SP_DEVICE_INTERFACE_DETAIL_DATA_W`,
|
||||||
|
// which needs 4-byte alignment, and a `Vec<u8>` only promises 1. The old SAFETY comment
|
||||||
|
// proved bounds and aliasing and was silent on alignment — the one obligation the code did
|
||||||
|
// not actually discharge.
|
||||||
|
let mut buf = vec![0u64; (required as usize).div_ceil(size_of::<u64>())];
|
||||||
let detail = buf.as_mut_ptr() as *mut SP_DEVICE_INTERFACE_DETAIL_DATA_W;
|
let detail = buf.as_mut_ptr() as *mut SP_DEVICE_INTERFACE_DETAIL_DATA_W;
|
||||||
// SAFETY: `buf` is `required` bytes (>= 4, checked above), so stamping `cbSize` and letting
|
// SAFETY: `buf` is at least `required` bytes and aligned to 8 (so also to the struct's 4),
|
||||||
// the API fill up to `required` bytes stays in bounds; `detail` aliases `buf` only within
|
// so stamping `cbSize` and letting the API fill up to `required` bytes stays in bounds;
|
||||||
// this iteration, and the `DevicePath` pointer is read before `buf` is dropped.
|
// `detail` aliases `buf` only within this iteration, and the `DevicePath` pointer is read
|
||||||
|
// before `buf` is dropped.
|
||||||
let opened = unsafe {
|
let opened = unsafe {
|
||||||
(*detail).cbSize = size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>() as u32;
|
(*detail).cbSize = size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>() as u32;
|
||||||
SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, Some(detail), required, None, None)
|
SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, Some(detail), required, None, None)
|
||||||
@@ -395,7 +409,10 @@ unsafe fn open_device() -> Result<HANDLE> {
|
|||||||
})
|
})
|
||||||
};
|
};
|
||||||
match opened {
|
match opened {
|
||||||
Ok(h) => return Ok(h),
|
// SAFETY: `h` is the handle `CreateFileW` just returned to THIS call and nothing else
|
||||||
|
// holds it, so transferring it into the `OwnedHandle` gives it a single owner that
|
||||||
|
// closes it exactly once on drop.
|
||||||
|
Ok(h) => return Ok(unsafe { OwnedHandle::from_raw_handle(h.0 as _) }),
|
||||||
// A raced-away or wedged device — remember the error, try the next interface.
|
// A raced-away or wedged device — remember the error, try the next interface.
|
||||||
Err(e) => last_err = Some(e),
|
Err(e) => last_err = Some(e),
|
||||||
}
|
}
|
||||||
@@ -418,10 +435,7 @@ impl VdisplayDriver for PfVdisplayDriver {
|
|||||||
}
|
}
|
||||||
|
|
||||||
unsafe fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)> {
|
unsafe fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)> {
|
||||||
// SAFETY: `open_device` is `unsafe` only because it issues SetupAPI enumeration + `CreateFileW`
|
let device = match open_device() {
|
||||||
// FFI; it takes no arguments and returns an owned raw `HANDLE` (or `Err`). Called here on the
|
|
||||||
// backend-init thread, with no precondition beyond a valid thread context.
|
|
||||||
let device = match unsafe { open_device() } {
|
|
||||||
Ok(d) => d,
|
Ok(d) => d,
|
||||||
Err(first) => {
|
Err(first) => {
|
||||||
// No openable interface. If a WUDFHost crash left the devnode a hostless zombie
|
// No openable interface. If a WUDFHost crash left the devnode a hostless zombie
|
||||||
@@ -433,8 +447,7 @@ impl VdisplayDriver for PfVdisplayDriver {
|
|||||||
let mut reopened = Err(first);
|
let mut reopened = Err(first);
|
||||||
for _ in 0..8 {
|
for _ in 0..8 {
|
||||||
std::thread::sleep(std::time::Duration::from_millis(500));
|
std::thread::sleep(std::time::Duration::from_millis(500));
|
||||||
// SAFETY: as above — plain SetupAPI + CreateFileW FFI, no preconditions.
|
match open_device() {
|
||||||
match unsafe { open_device() } {
|
|
||||||
Ok(d) => {
|
Ok(d) => {
|
||||||
reopened = Ok(d);
|
reopened = Ok(d);
|
||||||
break;
|
break;
|
||||||
@@ -445,11 +458,9 @@ impl VdisplayDriver for PfVdisplayDriver {
|
|||||||
reopened.context("pf-vdisplay interface still absent after an adapter cycle")?
|
reopened.context("pf-vdisplay interface still absent after an adapter cycle")?
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
// Wrap IMMEDIATELY: every `?` below must close the device exactly once — the old
|
// `open_device` hands back an `OwnedHandle`, so every `?` below closes the device exactly
|
||||||
// wrap-on-success-only shape leaked the raw handle whenever GET_INFO itself failed.
|
// once by construction — the shape this used to reach by wrapping the raw handle here, and
|
||||||
// SAFETY: `device` is the valid handle `open_device` just returned; ownership transfers into
|
// which leaked whenever GET_INFO itself failed before that wrap was moved up.
|
||||||
// the `OwnedHandle` (single owner, `CloseHandle` on drop).
|
|
||||||
let device = unsafe { OwnedHandle::from_raw_handle(device.0 as _) };
|
|
||||||
let raw = HANDLE(device.as_raw_handle());
|
let raw = HANDLE(device.as_raw_handle());
|
||||||
// HARD protocol-version check (unlike SudoVDA's best-effort log): a mismatched host/driver pair
|
// HARD protocol-version check (unlike SudoVDA's best-effort log): a mismatched host/driver pair
|
||||||
// fails loudly here rather than corrupting the IOCTL stream.
|
// fails loudly here rather than corrupting the IOCTL stream.
|
||||||
@@ -828,23 +839,13 @@ impl VirtualDisplay for PfVdisplayDisplay {
|
|||||||
|
|
||||||
/// Readiness probe: can we open the pf-vdisplay control device?
|
/// Readiness probe: can we open the pf-vdisplay control device?
|
||||||
pub fn probe() -> Result<()> {
|
pub fn probe() -> Result<()> {
|
||||||
// SAFETY: `open_device` is `unsafe` only for its SetupAPI + `CreateFileW` FFI; no arguments, returns
|
// The handle closes on drop.
|
||||||
// an owned raw `HANDLE` (or `Err`).
|
open_device().map(|_| ())
|
||||||
let h = unsafe { open_device()? };
|
|
||||||
// SAFETY: `h` is the handle just opened by `open_device` in this function, owned here and not yet
|
|
||||||
// handed anywhere else, so this closes it exactly once — no double-close, no use-after-close.
|
|
||||||
unsafe {
|
|
||||||
let _ = CloseHandle(h);
|
|
||||||
}
|
|
||||||
Ok(())
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Is the pf-vdisplay driver present (device interface enumerable)?
|
/// Is the pf-vdisplay driver present (device interface enumerable)?
|
||||||
pub fn is_available() -> bool {
|
pub fn is_available() -> bool {
|
||||||
// SAFETY: `open_device` returns an owned raw `HANDLE`; on `Ok(h)` the handle is moved into the
|
open_device().is_ok()
|
||||||
// closure (sole owner) and closed exactly once via `CloseHandle`, on `Err` there is nothing to
|
|
||||||
// close — so no double-close and no leak of an opened handle. The `unsafe` covers both FFI calls.
|
|
||||||
unsafe { open_device().map(|h| CloseHandle(h)).is_ok() }
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// [`is_available`], with self-heal: an interface-less driver whose adapter devnode EXISTS is the
|
/// [`is_available`], with self-heal: an interface-less driver whose adapter devnode EXISTS is the
|
||||||
|
|||||||
Reference in New Issue
Block a user