From 029979e8247e7bf3e4ff3dfa1929574a6c3a6f2e Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 16:55:54 +0200 Subject: [PATCH] fix(vdisplay/windows): own the device handle, prove the ladder once, and fix two proofs that were wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` (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`. Its guard also compared the required size against `size_of::()` while stamping `size_of::()` 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) --- crates/pf-vdisplay/src/vdisplay/ddc.rs | 17 +++-- .../src/vdisplay/windows/manager.rs | 44 +++++++------ .../src/vdisplay/windows/pf_vdisplay.rs | 65 ++++++++++--------- 3 files changed, 71 insertions(+), 55 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/ddc.rs b/crates/pf-vdisplay/src/vdisplay/ddc.rs index a88bfe4f..b281a5a3 100644 --- a/crates/pf-vdisplay/src/vdisplay/ddc.rs +++ b/crates/pf-vdisplay/src/vdisplay/ddc.rs @@ -61,10 +61,19 @@ fn active_monitors() -> Vec { let out = unsafe { &mut *(data.0 as *mut Vec) }; let mut info = MONITORINFOEXW::default(); info.monitorInfo.cbSize = std::mem::size_of::() as u32; - // SAFETY: `hmon` is the live monitor handle the enumeration just handed us; `info` is a - // properly-sized MONITORINFOEXW local whose cbSize is set, which GetMonitorInfoW requires - // to safely write the extended (szDevice) variant. - if unsafe { GetMonitorInfoW(hmon, &mut info.monitorInfo) }.as_bool() { + // The pointer is derived from the WHOLE `MONITORINFOEXW`, not from its `monitorInfo` field. + // `cbSize` promises the OS 104 bytes, but `&mut info.monitorInfo` is a pointer to the + // 40-byte `MONITORINFO` prefix: everything the OS writes past byte 40 — which is `szDevice`, + // 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::(); + // 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 .szDevice .iter() diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index 106f60d8..4f43ad29 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -183,6 +183,25 @@ enum ShrinkAction { /// 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. /// 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 { + 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 { if ccd_exclusive { ShrinkAction::Reisolate @@ -1049,32 +1068,19 @@ impl VirtualDisplayManager { // 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 // early poll, so finer sampling shaves ~150 ms off every stage crossing. - 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); - } + if let Some(n) = poll_gdi_name(target_id) { + return Some(n); } // SAFETY: `force_extend_topology` only calls `SetDisplayConfig` (CCD) with no borrowed memory. unsafe { force_extend_topology() }; - for _ in 0..60 { - thread::sleep(Duration::from_millis(50)); - // SAFETY: as the resolve loop above. - if let Some(n) = unsafe { resolve_gdi_name(target_id) } { - return Some(n); - } + if let Some(n) = poll_gdi_name(target_id) { + return Some(n); } // 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. if unsafe { pf_win_display::win_display::activate_target_path(target_id) } { - for _ in 0..60 { - thread::sleep(Duration::from_millis(50)); - // SAFETY: as the resolve loops above. - if let Some(n) = unsafe { resolve_gdi_name(target_id) } { - return Some(n); - } + if let Some(n) = poll_gdi_name(target_id) { + return Some(n); } } None diff --git a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs index c4884d57..60d4fa30 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs @@ -29,7 +29,7 @@ use windows::Win32::Devices::DeviceAndDriverInstallation::{ SetupDiGetDeviceInterfaceDetailW, DIGCF_DEVICEINTERFACE, DIGCF_PRESENT, HDEVINFO, SPINT_ACTIVE, 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::{ 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 { +/// 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 { // SAFETY: plain SetupAPI enumeration call; the returned list is solely owned by the RAII wrapper. let hdev = DevInfoList( unsafe { @@ -369,14 +376,21 @@ unsafe fn open_device() -> Result { let _ = unsafe { SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, None, 0, Some(&mut required), None) }; - if (required as usize) < size_of::() { + // Against the struct's own size, not `u32`'s: the value stamped into `cbSize` below is + // `size_of::()`, so that is what the buffer must hold. + if (required as usize) < size_of::() { 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` 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::())]; 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 - // the API fill up to `required` bytes stays in bounds; `detail` aliases `buf` only within - // this iteration, and the `DevicePath` pointer is read before `buf` is dropped. + // SAFETY: `buf` is at least `required` bytes and aligned to 8 (so also to the struct's 4), + // so stamping `cbSize` and letting the API fill up to `required` bytes stays in bounds; + // `detail` aliases `buf` only within this iteration, and the `DevicePath` pointer is read + // before `buf` is dropped. let opened = unsafe { (*detail).cbSize = size_of::() as u32; SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, Some(detail), required, None, None) @@ -395,7 +409,10 @@ unsafe fn open_device() -> Result { }) }; 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. 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)> { - // SAFETY: `open_device` is `unsafe` only because it issues SetupAPI enumeration + `CreateFileW` - // 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() } { + let device = match open_device() { Ok(d) => d, Err(first) => { // 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); for _ in 0..8 { std::thread::sleep(std::time::Duration::from_millis(500)); - // SAFETY: as above — plain SetupAPI + CreateFileW FFI, no preconditions. - match unsafe { open_device() } { + match open_device() { Ok(d) => { reopened = Ok(d); break; @@ -445,11 +458,9 @@ impl VdisplayDriver for PfVdisplayDriver { reopened.context("pf-vdisplay interface still absent after an adapter cycle")? } }; - // Wrap IMMEDIATELY: every `?` below must close the device exactly once — the old - // wrap-on-success-only shape leaked the raw handle whenever GET_INFO itself failed. - // SAFETY: `device` is the valid handle `open_device` just returned; ownership transfers into - // the `OwnedHandle` (single owner, `CloseHandle` on drop). - let device = unsafe { OwnedHandle::from_raw_handle(device.0 as _) }; + // `open_device` hands back an `OwnedHandle`, so every `?` below closes the device exactly + // once by construction — the shape this used to reach by wrapping the raw handle here, and + // which leaked whenever GET_INFO itself failed before that wrap was moved up. let raw = HANDLE(device.as_raw_handle()); // HARD protocol-version check (unlike SudoVDA's best-effort log): a mismatched host/driver pair // 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? pub fn probe() -> Result<()> { - // SAFETY: `open_device` is `unsafe` only for its SetupAPI + `CreateFileW` FFI; no arguments, returns - // an owned raw `HANDLE` (or `Err`). - 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(()) + // The handle closes on drop. + open_device().map(|_| ()) } /// Is the pf-vdisplay driver present (device interface enumerable)? pub fn is_available() -> bool { - // SAFETY: `open_device` returns an owned raw `HANDLE`; on `Ok(h)` the handle is moved into the - // 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() } + open_device().is_ok() } /// [`is_available`], with self-heal: an interface-less driver whose adapter devnode EXISTS is the