From 50a153b72a7636257c740bd303271ed7fa11fe15 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 17:12:41 +0200 Subject: [PATCH] refactor(win-display): the CCD helpers were `unsafe fn` for serialization, not soundness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pf-win-display`'s 21 CCD/GDI helpers were `pub unsafe fn` without a memory-safety obligation between them. Every one takes `Copy` scalars or borrowed Rust data, returns owned values, and discharges its FFI preconditions internally — `retry_set_display_config` even binds the input desktop itself, which is the one thing a caller could plausibly get wrong. Their own `# Safety` sections say as much: one reads "safe to call from any thread", and the rest say "call under the manager `state` lock". That is a SERIALIZATION requirement — calling one unlocked races the topology mutator and returns a stale answer, a correctness bug, not undefined behaviour. Encoding it with `unsafe fn` cost 55 `unsafe` blocks across THREE crates whose proofs could only restate "this takes a u32 and returns a String". That is how `unsafe` stops meaning anything: the blocks that wrap a real FFI obligation read exactly like the ones that wrap nothing. The requirement is now prose on the functions that have it, and `unsafe` marks only the FFI calls inside the helpers. win_display.rs 97 -> 63 `unsafe`, manager.rs 56 -> 30, pf_vdisplay.rs 34 -> 32, plus 10 sites in pf-capture's IDD-push paths. 55 blocks and 55 orphaned `// SAFETY:` proofs gone; `#![allow(clippy::missing_safety_doc)]` with them, since nothing is `pub unsafe fn` any more. Two things the compiler handed back once the blocks went, both of which existed only to carry a proof: the `let-else` parens in `force_mode_reenumeration` and `apply_source_positions`, and the nested `if` in `wait_mode_settled` — which is `&&` again, still short-circuiting, so the second CCD query on a 25 ms poll is unchanged. Scope note: the sweep and the plan both scoped this as two crates. It is three — pf-capture calls the same helpers from 10 sites, and `unused_unsafe` under `-D warnings` makes that non-optional. Found by re-deriving the count, which the plan asked for because the underlying finding had been REFUTED in verification; the refutation was wrong about the magnitude (22 of manager.rs's 45 blocks, against its claimed 24). Verified on the Windows runner: clippy --all-targets clean over pf-frame, pf-win-display, pf-capture and pf-vdisplay; pf-vdisplay 55 passed, pf-capture 18 passed. Co-Authored-By: Claude Opus 5 (1M context) --- crates/pf-capture/src/windows/idd_push.rs | 17 +- .../src/windows/idd_push/compose_kick.rs | 8 +- .../pf-capture/src/windows/idd_push/cursor.rs | 4 +- .../src/windows/idd_push/cursor_poll.rs | 5 +- .../src/windows/idd_push/descriptor.rs | 8 +- .../pf-capture/src/windows/idd_push/open.rs | 7 +- .../src/vdisplay/windows/manager.rs | 107 +++---------- .../src/vdisplay/windows/pf_vdisplay.rs | 6 +- crates/pf-win-display/src/display_events.rs | 4 +- crates/pf-win-display/src/monitor_devnode.rs | 4 +- crates/pf-win-display/src/win_display.rs | 145 ++++++++---------- 11 files changed, 101 insertions(+), 214 deletions(-) diff --git a/crates/pf-capture/src/windows/idd_push.rs b/crates/pf-capture/src/windows/idd_push.rs index 50d7b94a..54ff1de6 100644 --- a/crates/pf-capture/src/windows/idd_push.rs +++ b/crates/pf-capture/src/windows/idd_push.rs @@ -791,13 +791,8 @@ impl IddPushCapturer { // Reading back immediately can catch a flip that has not settled yet; that costs one // debounce cycle (the poller re-samples in ~250 ms) and never a wrong ring, which is why // this does not block the frame path on a settle poll the way `open_on` does. - // SAFETY: both are `unsafe fn`s over CCD DisplayConfig; each takes a copy of the plain - // `u32` target id (plus a `bool`), forms no lasting borrow, and returns an owned value. - let requested = - unsafe { pf_win_display::win_display::set_advanced_color(self.target_id, want) }; - // SAFETY: as above — a read-only CCD query over a copy of the plain `u32` target id. - let observed = - unsafe { pf_win_display::win_display::advanced_color_enabled(self.target_id) }; + let requested = pf_win_display::win_display::set_advanced_color(self.target_id, want); + let observed = pf_win_display::win_display::advanced_color_enabled(self.target_id); // A failed READ is not evidence of a failed flip — keep the poller's sample then. now.hdr = observed.unwrap_or(now.hdr); if now.hdr != want && !self.hdr_pin_warned { @@ -1129,9 +1124,7 @@ impl IddPushCapturer { if !self.display_hdr { return; } - // SAFETY: `sdr_white_level_scale` is an `unsafe fn` running a read-only CCD query over owned - // local buffers; the `Copy` target id crosses by value and it returns an owned value. - let queried = unsafe { pf_win_display::win_display::sdr_white_level_scale(self.target_id) }; + let queried = pf_win_display::win_display::sdr_white_level_scale(self.target_id); self.sdr_white_scale = queried.unwrap_or(self.sdr_white_scale); tracing::info!( target_id = self.target_id, @@ -1830,9 +1823,7 @@ impl Capturer for IddPushCapturer { // driver's stash (measured: new_fps=0 forever after the re-attach). CDS_RESET forces a // real mode-set at the CURRENT mode — the same lever bring-up's ADD path relies on — // and the ring recreate below then re-attaches after that churn, not before it. - // SAFETY: `resolve_gdi_name` runs the CCD query FFI over a `Copy` target id (owned - // return) — same contract as every sibling call in this file. - match unsafe { pf_win_display::win_display::resolve_gdi_name(self.target_id) } { + match pf_win_display::win_display::resolve_gdi_name(self.target_id) { Some(gdi) => { if !pf_win_display::win_display::force_mode_reset(&gdi) { tracing::warn!( diff --git a/crates/pf-capture/src/windows/idd_push/compose_kick.rs b/crates/pf-capture/src/windows/idd_push/compose_kick.rs index bdbba38a..d7d772d2 100644 --- a/crates/pf-capture/src/windows/idd_push/compose_kick.rs +++ b/crates/pf-capture/src/windows/idd_push/compose_kick.rs @@ -69,17 +69,13 @@ pub(super) fn kick_dwm_compose(target_id: u32) { let mut pos = POINT::default(); // SAFETY: plain FFI; `pos` is a valid out-param for this synchronous call. let have_pos = unsafe { GetCursorPos(&mut pos) }.is_ok(); - // SAFETY: `source_desktop_rect` only runs the CCD QueryDisplayConfig FFI over owned local - // buffers; the `Copy` target id crosses by value. - let rect = unsafe { pf_win_display::win_display::source_desktop_rect(target_id) }; + let rect = pf_win_display::win_display::source_desktop_rect(target_id); // HID-first (see the doc comment): the registered virtual-mouse kick works from any // session/desktop and wakes an off display. Both geometries come from CCD (global database), // NOT per-session GDI metrics, so the aim is right even from a non-console session. Fall // through to SendInput only when the hook isn't registered / the mouse isn't up. if let (Some(kick), Some(rect)) = (crate::HID_COMPOSE_KICK.get(), rect) { - // SAFETY: `desktop_bounds` only runs the CCD QueryDisplayConfig FFI over owned local - // buffers. - let bounds = unsafe { pf_win_display::win_display::desktop_bounds() }; + let bounds = pf_win_display::win_display::desktop_bounds(); if let Some(bounds) = bounds { if kick(rect, bounds) { return; diff --git a/crates/pf-capture/src/windows/idd_push/cursor.rs b/crates/pf-capture/src/windows/idd_push/cursor.rs index fb2780f9..c4df1aed 100644 --- a/crates/pf-capture/src/windows/idd_push/cursor.rs +++ b/crates/pf-capture/src/windows/idd_push/cursor.rs @@ -72,9 +72,7 @@ impl CursorShared { MappedSection { handle: map, view } }; // Desktop origin of this monitor's source — for the desktop→frame coordinate shift. - // SAFETY: `source_desktop_rect` only runs the CCD QueryDisplayConfig FFI over owned - // locals (same call the compose-kick path makes). - let rect = unsafe { pf_win_display::win_display::source_desktop_rect(target_id) }; + let rect = pf_win_display::win_display::source_desktop_rect(target_id); let origin = rect.map(|(x, y, _w, _h)| (x, y)).unwrap_or((0, 0)); Ok(CursorShared { section, diff --git a/crates/pf-capture/src/windows/idd_push/cursor_poll.rs b/crates/pf-capture/src/windows/idd_push/cursor_poll.rs index 00660529..47e8b809 100644 --- a/crates/pf-capture/src/windows/idd_push/cursor_poll.rs +++ b/crates/pf-capture/src/windows/idd_push/cursor_poll.rs @@ -187,10 +187,7 @@ fn run( // transient CCD failure must not park the pointer at a `(0, 0, 0, 0)` rect, which would // report every position invisible. // - // SAFETY: `source_desktop_rect` is an `unsafe fn` running the read-only CCD - // `QueryDisplayConfig` over owned local buffers; the `Copy` target id crosses by value - // and it returns owned `(x, y, w, h)` values, borrowing nothing. - let fresh = unsafe { pf_win_display::win_display::source_desktop_rect(target_id) }; + let fresh = pf_win_display::win_display::source_desktop_rect(target_id); if let Some(fresh) = fresh { if fresh != rect { tracing::info!( diff --git a/crates/pf-capture/src/windows/idd_push/descriptor.rs b/crates/pf-capture/src/windows/idd_push/descriptor.rs index 470e3b6c..0b62417c 100644 --- a/crates/pf-capture/src/windows/idd_push/descriptor.rs +++ b/crates/pf-capture/src/windows/idd_push/descriptor.rs @@ -59,14 +59,10 @@ impl DescriptorPoller { let mut last_slow_log: Option = None; while !stop_t.load(Ordering::Relaxed) { let t = Instant::now(); - // SAFETY: both are read-only CCD queries taking only a copy of the plain `u32` - // target id (see their own SAFETY docs); nothing is borrowed across the calls. - let (hdr, res) = unsafe { - ( + let (hdr, res) = ( pf_win_display::win_display::advanced_color_enabled(target_id), pf_win_display::win_display::active_resolution(target_id), - ) - }; + ); let took = t.elapsed(); if took >= Self::SLOW && last_slow_log.is_none_or(|t| t.elapsed() >= Duration::from_secs(10)) diff --git a/crates/pf-capture/src/windows/idd_push/open.rs b/crates/pf-capture/src/windows/idd_push/open.rs index 7f0d072f..a8ed1470 100644 --- a/crates/pf-capture/src/windows/idd_push/open.rs +++ b/crates/pf-capture/src/windows/idd_push/open.rs @@ -281,11 +281,8 @@ impl IddPushCapturer { // a fullscreen game can hold the virtual display at a different mode (esp. across a reconnect), so // matching the actual mode lets the first frame flow instead of being dropped (game-capture bug // GB1). Falls back to the negotiated mode when the CCD read is unavailable. - // SAFETY: `active_resolution` is an `unsafe fn` (Win32 CCD `QueryDisplayConfig`) that takes only a - // copy of the plain `u32` CCD target id and returns owned `(w, h)` values; it forms no borrows from - // us and validates the id internally, returning `None` on any failure (handled by `unwrap_or`). - let (w, h) = unsafe { pf_win_display::win_display::active_resolution(target.target_id) } - .unwrap_or((pw, ph)); + let (w, h) = + pf_win_display::win_display::active_resolution(target.target_id).unwrap_or((pw, ph)); if (w, h) != (pw, ph) { tracing::info!( target_id = target.target_id, diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index 4f43ad29..eb478c14 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -193,9 +193,7 @@ enum ShrinkAction { 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) } { + if let Some(n) = resolve_gdi_name(target_id) { return Some(n); } } @@ -380,9 +378,7 @@ pub fn force_recommit() -> bool { return false; }; let _guard = m.state.lock().unwrap(); - // SAFETY: `force_mode_reenumeration`'s contract is "call under the manager `state` lock"; - // held above. The call reads + re-applies the current CCD config over owned locals. - unsafe { pf_win_display::win_display::force_mode_reenumeration() } + pf_win_display::win_display::force_mode_reenumeration() } /// Best-effort "is this WUDFHost pid still alive?" — the monitor-liveness probe for the JOIN path. @@ -561,9 +557,7 @@ impl VirtualDisplayManager { // Let the OS finish the ASYNC monitor departure before the next ADD; a back-to-back // REMOVE→ADD races the teardown and the ADD IOCTL is rejected under reconnect churn. // Verified-state wait, ceiling = the old fixed 400 ms settle (latency plan P0.3). - // SAFETY: CCD query FFI over a `Copy` target id, under the held `state` lock. - let departed = - unsafe { wait_target_departed(old_target, Duration::from_millis(400)) }; + let departed = wait_target_departed(old_target, Duration::from_millis(400)); if !departed { tracing::debug!( old_target, @@ -598,8 +592,7 @@ impl VirtualDisplayManager { // is exclusively owned here — no aliasing. unsafe { self.teardown_removed(dev, &mut inner, mon) }; // Same async-departure settle as the reconnect preempt above (verified wait, P0.3). - // SAFETY: CCD query FFI over a `Copy` target id, under the held `state` lock. - let _ = unsafe { wait_target_departed(old_target, Duration::from_millis(400)) }; + let _ = wait_target_departed(old_target, Duration::from_millis(400)); } } @@ -902,10 +895,7 @@ impl VirtualDisplayManager { if keep.is_empty() { continue; } - // SAFETY: `count_other_active` runs the CCD QueryDisplayConfig FFI over a - // borrowed slice of `Copy` target ids (owned result), under the `state` lock - // (`inner` guard held to the end of this iteration). - let survivors = unsafe { count_other_active(&keep) }.unwrap_or(0); + let survivors = count_other_active(&keep).unwrap_or(0); if survivors == 0 { if fighting > 0 { tracing::info!( @@ -947,16 +937,7 @@ impl VirtualDisplayManager { "re-asserting exclusive topology" ), } - // SAFETY: `isolate_displays_ccd` drives the CCD query/apply FFI over a - // borrowed slice of `Copy` target ids, under the `state` lock — the sole - // topology mutator. The returned snapshot is discarded (the group restores - // the FIRST member's). The FULL isolate on purpose — its forced re-commit - // (SDC_FORCE_MODE_ENUMERATION → COMMIT_MODES → ASSIGN_SWAPCHAIN) is - // load-bearing here exactly as at first isolate: after the eviction's - // topology change the OS stops presenting to the virtual display until a - // forced mode re-commit restarts it (on-glass: a gentle supplied-config - // eviction left capture receiving ONE stashed frame and then nothing). - let _ = unsafe { isolate_displays_ccd(&keep) }; + let _ = isolate_displays_ccd(&keep); // That same forced re-commit hands the live IDD path a fresh swap-chain, // orphaning the session's capture ring — announce it so the session rebuilds // its capture attachment (same-mode ring recreate + driver re-attach + fresh @@ -1014,9 +995,7 @@ impl VirtualDisplayManager { .zip(&placements) .map(|(&(_, _, target, _), p)| (target, p.x, p.y)) .collect(); - // SAFETY: `apply_source_positions` only drives the CCD query/apply FFI with owned local - // buffers, under the `state` lock — the sole topology mutator. - unsafe { pf_win_display::win_display::apply_source_positions(&positions) }; + pf_win_display::win_display::apply_source_positions(&positions); for (&(slot, ..), p) in ordered.iter().zip(&placements) { if let Some( SlotState::Active { mon, .. } @@ -1071,14 +1050,11 @@ impl VirtualDisplayManager { 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() }; + force_extend_topology(); 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) } { + if pf_win_display::win_display::activate_target_path(target_id) { if let Some(n) = poll_gdi_name(target_id) { return Some(n); } @@ -1170,11 +1146,7 @@ impl VirtualDisplayManager { if crate::policy::prefs().ddc_power_off() { inner.group.ddc_panels_off = crate::ddc::panel_off_except(n); } - // SAFETY: `isolate_displays_ccd` is `unsafe` for its CCD topology FFI; it - // takes a borrowed slice of `Copy` target ids (alive across the call) and - // returns an owned `SavedConfig`, under the `state` lock — the sole - // topology mutator. - inner.group.ccd_saved = unsafe { isolate_displays_ccd(&keep) }; + inner.group.ccd_saved = isolate_displays_ccd(&keep); // EXPERIMENTAL `pnp_disable_monitors` policy axis: AFTER the isolate took, // additionally disable the deactivated monitors' PnP devnodes (persistent // across hot-plug re-arrival) so a standby monitor/TV's periodic wake @@ -1199,9 +1171,7 @@ impl VirtualDisplayManager { // Grown set: re-isolate so the fresh member joins the composited set // (its auto-activate may have lit nothing extra to deactivate, but the // re-commit also drives COMMIT_MODES for the new path). - // SAFETY: as above — borrowed slice of Copy ids, owned return, under the - // `state` lock. - let snap = unsafe { isolate_displays_ccd(&keep) }; + let snap = isolate_displays_ccd(&keep); // Normally DISCARDED — the group restores the FIRST member's snapshot. // But if the first member's isolate FAILED, there is no first snapshot, // and this one just deactivated the physicals with nothing able to put @@ -1233,10 +1203,8 @@ impl VirtualDisplayManager { // persistence DB, RESETTING a 120 Hz panel to 60 Hz. So force-EXTEND only when the // virtual is currently sole; otherwise skip straight to the reposition, which // re-supplies each physical's QUERIED mode verbatim (preserving its refresh). - // SAFETY: `count_other_active` runs the CCD QueryDisplayConfig FFI (borrowed - // slice of Copy ids, owned result), under the `state` lock. let already_extended = - unsafe { count_other_active(&[added.target_id]) }.unwrap_or(0) > 0; + count_other_active(&[added.target_id]).unwrap_or(0) > 0; if already_extended { tracing::info!( "display topology=primary — a physical display is already active; \ @@ -1244,14 +1212,10 @@ impl VirtualDisplayManager { virtual primary" ); } else { - // SAFETY: `force_extend_topology` drives the CCD topology FFI (no args, no - // borrowed memory), under the `state` lock — the sole topology mutator. - unsafe { force_extend_topology() }; + force_extend_topology(); thread::sleep(Duration::from_millis(300)); } - // SAFETY: `set_virtual_primary_ccd` takes the `Copy` target id by value and returns - // an owned `SavedConfig` (no borrowed memory crosses), under the `state` lock. - inner.group.ccd_saved = unsafe { set_virtual_primary_ccd(added.target_id) }; + inner.group.ccd_saved = set_virtual_primary_ccd(added.target_id); } Topology::Primary => { // A sibling already holds primary (the group's designated member) — the new @@ -1271,10 +1235,7 @@ impl VirtualDisplayManager { // 1500 ms sleep (a rejected mode / slow third-party CCD-lock holder burns the // ceiling and proceeds, exactly like the sleep it replaces). let settle_start = std::time::Instant::now(); - // SAFETY: CCD/GDI query FFI over a `Copy` target id, under the held `state` lock. - let settled = unsafe { - wait_mode_settled(added.target_id, mode, Duration::from_millis(1500)) - }; + let settled = wait_mode_settled(added.target_id, mode, Duration::from_millis(1500)); tracing::info!( settle_ms = settle_start.elapsed().as_millis() as u64, verified = settled, @@ -1391,8 +1352,7 @@ impl VirtualDisplayManager { // SAFETY: `dev` is the live control handle (this fn's contract); `update_modes` // forwards it to a synchronous IOCTL with owned/borrowed locals only. unsafe { self.driver.update_modes(dev, &mon.key, mode) }?; - // SAFETY: CCD query/apply FFI under the held `state` lock (this fn's contract). - unsafe { pf_win_display::win_display::force_mode_reenumeration() }; + pf_win_display::win_display::force_mode_reenumeration(); if !pf_win_display::win_display::wait_mode_advertised( &gdi, mode, @@ -1414,9 +1374,7 @@ impl VirtualDisplayManager { // Verified-state settle (P0.2): the same committed-state predicate as the create paths. A // mode set that did not commit within the ceiling routes to the re-arrival fallback. let settle_start = Instant::now(); - // SAFETY: CCD/GDI query FFI over a `Copy` target id, under the held `state` lock. - let settled = - unsafe { wait_mode_settled(mon.target_id, mode, Duration::from_millis(1500)) }; + let settled = wait_mode_settled(mon.target_id, mode, Duration::from_millis(1500)); if !settled { anyhow::bail!( "in-place mode set did not commit within 1.5s (advertised after {advertised_ms} ms)" @@ -1486,8 +1444,7 @@ impl VirtualDisplayManager { // the old fixed 400 ms settle (latency plan P0.3); the driver's ghost-reap ADD retry remains // the backstop for a departure the CCD reports early. let depart_start = std::time::Instant::now(); - // SAFETY: CCD query FFI over a `Copy` target id, under the held `state` lock. - let departed = unsafe { wait_target_departed(old.target_id, Duration::from_millis(400)) }; + let departed = wait_target_departed(old.target_id, Duration::from_millis(400)); tracing::info!( depart_ms = depart_start.elapsed().as_millis() as u64, verified = departed, @@ -1522,10 +1479,7 @@ impl VirtualDisplayManager { // Topology settle before capture reopens: verified-state wait, ceiling = the old // fixed 1500 ms sleep (latency plan P0.2 — the re-arrival twin). let settle_start = std::time::Instant::now(); - // SAFETY: CCD/GDI query FFI over a `Copy` target id, under the held `state` lock. - let settled = unsafe { - wait_mode_settled(added.target_id, mode, Duration::from_millis(1500)) - }; + let settled = wait_mode_settled(added.target_id, mode, Duration::from_millis(1500)); tracing::info!( settle_ms = settle_start.elapsed().as_millis() as u64, verified = settled, @@ -1574,16 +1528,14 @@ impl VirtualDisplayManager { // snapshot is DISCARDED — the group keeps the first member's (design §6.1). let mut keep = inner.target_ids(); keep.push(new_target); - // SAFETY: borrowed slice of Copy target ids, owned return, under the `state` lock. - let _ = unsafe { isolate_displays_ccd(&keep) }; + let _ = isolate_displays_ccd(&keep); } Topology::Primary => { // Make the new target primary again (its predecessor held primary), preserving the // original restore snapshot: `set_virtual_primary_ccd` recaptures one, so save + restore // the group's around the call. let keep_saved = inner.group.ccd_saved.take(); - // SAFETY: `Copy` target id by value, owned return, under the `state` lock. - let _ = unsafe { set_virtual_primary_ccd(new_target) }; + let _ = set_virtual_primary_ccd(new_target); inner.group.ccd_saved = keep_saved; } Topology::Extend | Topology::Auto => { @@ -1645,14 +1597,7 @@ impl VirtualDisplayManager { // displays. inner.group.ccd_exclusive = false; if let Some(saved) = inner.group.ccd_saved.take() { - // SAFETY: `saved` is a `SavedConfig` this manager captured from - // `isolate_displays_ccd`/`set_virtual_primary_ccd` on this same box (it can be - // produced no other way), so its path+mode arrays are a self-consistent CCD - // topology and are passed with their own lengths. `restore_displays_ccd` binds - // itself to the input desktop internally (`retry_set_display_config`), which is - // the one thing its callers could otherwise get wrong. The `take()` above makes - // this the only consumer of that snapshot, so no second restore can race it. - unsafe { restore_displays_ccd(&saved) }; + restore_displays_ccd(&saved); } // EXPERIMENTAL `ddc_power_off` wake. OUTSIDE the `ccd_saved` gate, for the same reason // `pnp_disabled` is above it: the panels were commanded dark BEFORE the isolate, and @@ -1680,10 +1625,7 @@ impl VirtualDisplayManager { // the group keeps the first member's. ShrinkAction::Reisolate => { let keep = inner.target_ids(); - // SAFETY: `isolate_displays_ccd` only drives the CCD query/apply FFI over a - // borrowed slice of Copy target ids, under the `state` lock — the sole - // topology mutator. - let _ = unsafe { isolate_displays_ccd(&keep) }; + let _ = isolate_displays_ccd(&keep); } // Re-promote a survivor rather than leaving the desktop's primary on a target that // is about to be REMOVEd. Same save/restore-the-snapshot dance as @@ -1692,8 +1634,7 @@ impl VirtualDisplayManager { ShrinkAction::RepromotePrimary => { if let Some(&survivor) = inner.target_ids().first() { let keep_saved = inner.group.ccd_saved.take(); - // SAFETY: `Copy` target id by value, owned return, under the `state` lock. - let _ = unsafe { set_virtual_primary_ccd(survivor) }; + let _ = set_virtual_primary_ccd(survivor); inner.group.ccd_saved = keep_saved; } } diff --git a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs index 60d4fa30..b3777810 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs @@ -910,8 +910,7 @@ mod tests { // Context probe: can this process see the CCD active-path set at all? (`None` = the query // itself fails in this session/window-station — the whole ladder would be blind, and a // "monitor never activated" verdict would be an artifact of the test context.) - // SAFETY: CCD query over an owned empty slice (test-only diagnostics). - let active0 = unsafe { pf_win_display::win_display::count_other_active(&[]) }; + let active0 = pf_win_display::win_display::count_other_active(&[]); println!("spike: CCD active paths visible before create: {active0:?}"); let mut vd = PfVdisplayDisplay::new().expect("open pf-vdisplay"); let first = vd @@ -943,8 +942,7 @@ mod tests { .expect("no capture target") .target_id; let in_place = t1 == t2; - // SAFETY: CCD query over a Copy target id (test-only diagnostics). - let active = unsafe { pf_win_display::win_display::active_resolution(t2) }; + let active = pf_win_display::win_display::active_resolution(t2); println!( "in-place resize spike: in_place={in_place} (target {t1} -> {t2}) took {resize_ms} ms, \ active resolution now {active:?}" diff --git a/crates/pf-win-display/src/display_events.rs b/crates/pf-win-display/src/display_events.rs index 49def5fd..6dbe2d75 100644 --- a/crates/pf-win-display/src/display_events.rs +++ b/crates/pf-win-display/src/display_events.rs @@ -193,9 +193,7 @@ fn push_event(kind: DisplayEventKind, detail: Option) { } fn refresh_inventory() { - // SAFETY: `target_inventory` only runs read-only CCD queries over locals (see its SAFETY doc); - // called from the listener thread, never the capture thread. - let inv = unsafe { crate::win_display::target_inventory() }; + let inv = crate::win_display::target_inventory(); if !inv.is_empty() { state().lock().unwrap().inventory = inv; } diff --git a/crates/pf-win-display/src/monitor_devnode.rs b/crates/pf-win-display/src/monitor_devnode.rs index 15f379fb..0d1df1cd 100644 --- a/crates/pf-win-display/src/monitor_devnode.rs +++ b/crates/pf-win-display/src/monitor_devnode.rs @@ -174,9 +174,7 @@ pub fn disable_for_deactivated( /// active flags it reads are the settled ones. Journals like [`disable_for_deactivated`]; the /// caller merges the returned ids into the same teardown list. pub fn disable_connected_inactive(keep_target_ids: &[u32]) -> Vec { - // SAFETY: `target_inventory` only runs read-only CCD queries over local buffers (see its - // docs); no borrowed memory crosses the call. - let inventory = unsafe { crate::win_display::target_inventory() }; + let inventory = crate::win_display::target_inventory(); let mut targets: Vec<(String, String)> = Vec::new(); for t in &inventory { if t.active || !t.external_physical || keep_target_ids.contains(&t.target_id) { diff --git a/crates/pf-win-display/src/win_display.rs b/crates/pf-win-display/src/win_display.rs index 01e7d197..ca76b83d 100644 --- a/crates/pf-win-display/src/win_display.rs +++ b/crates/pf-win-display/src/win_display.rs @@ -15,12 +15,17 @@ // below — including `restore_displays_ccd`, the call pf-vdisplay's teardown path depends on to give // the operator their physical panels back. #![deny(unsafe_op_in_unsafe_fn)] -// These CCD/GDI FFI helpers were `pub(crate) unsafe fn` before the pf-win-display carve (plan §W6), -// where `missing_safety_doc` stays silent; crossing the crate boundary makes them `pub` and would -// demand a `# Safety` heading on each. Their callers' obligations (call on the right desktop thread -// with a live OS target id) are stated in each fn's prose doc, and this is an internal -// (publish=false) leaf — keep the pre-carve behavior rather than adding 12 formal headings. -#![allow(clippy::missing_safety_doc)] +// The CCD/GDI helpers below are SAFE fns. They were `unsafe fn` for a decade of habit rather than a +// memory-safety obligation: every one takes `Copy` scalars or borrowed Rust data, returns owned +// values, and discharges its own FFI preconditions internally (`retry_set_display_config` even binds +// the input desktop itself). What their `# Safety` sections actually described — "call under the +// manager `state` lock" — is a SERIALIZATION requirement: calling one unlocked races the topology +// mutator and yields a stale answer, which is a correctness bug, not undefined behaviour. +// +// Encoding that with `unsafe fn` cost ~55 `unsafe` blocks across three crates whose proofs could +// only restate "this takes a u32 and returns a String", which is how `unsafe` stopped meaning +// anything here. The requirement is now prose on the functions that have it, and `unsafe` marks +// only the FFI calls inside. // THE CCD CONTRACT, stated once — most `// SAFETY:` proofs below are an instance of it. // @@ -94,7 +99,7 @@ use punktfunk_core::Mode; /// OWN active path, so the rest of bring-up (`resolve_gdi_name` -> `set_active_mode` -> /// `isolate_displays_ccd`) proceeds. Best-effort + idempotent: a no-op on a single-display (already /// sole/extended) box, so it is safe to call unconditionally. `rc == 0` is success. -pub unsafe fn force_extend_topology() { +pub fn force_extend_topology() { // A topology flag with no supplied path/mode arrays tells the OS to recompute + apply that preset // for the currently-connected displays (the same code path DisplaySwitch.exe drives). let rc = crate::input_desktop::retry_set_display_config(|| { @@ -125,7 +130,7 @@ pub unsafe fn force_extend_topology() { /// source not already driving another display — mode indices invalidated so `SDC_ALLOW_CHANGES` lets /// the OS pick modes for the new path. Returns `true` when the apply reports success; the caller /// still re-polls [`resolve_gdi_name`] to confirm the path actually committed. -pub unsafe fn activate_target_path(target_id: u32) -> bool { +pub fn activate_target_path(target_id: u32) -> bool { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -228,7 +233,7 @@ pub unsafe fn activate_target_path(target_id: u32) -> bool { /// Resolve the `\\.\DisplayN` GDI name for a virtual-display target id via the CCD API. Returns `None` /// until the OS activates the target into the desktop topology (needs a real WDDM GPU; on a /// GPU-less box this stays `None` even though ADD succeeded). -pub unsafe fn resolve_gdi_name(target_id: u32) -> Option { +pub fn resolve_gdi_name(target_id: u32) -> Option { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -278,13 +283,9 @@ pub unsafe fn resolve_gdi_name(target_id: u32) -> Option { /// ACTUAL mode and polls it to recreate the ring when it changes — a fullscreen game can change the /// virtual display's mode out from under the session-negotiated one (game-capture bug GB1). /// -/// # Safety -/// Calls the GDI/CCD APIs; safe to call from any thread. -pub unsafe fn active_resolution(target_id: u32) -> Option<(u32, u32)> { - // SAFETY: `resolve_gdi_name` is this module's own CCD helper: it takes `Copy` scalars / borrowed Rust - // data and returns an owned `String`, so every precondition is internal to it. This fn's own - // contract (call under the manager `state` lock) is what it needs. - let gdi = unsafe { resolve_gdi_name(target_id) }?; +/// Safe to call from any thread. +pub fn active_resolution(target_id: u32) -> Option<(u32, u32)> { + let gdi = resolve_gdi_name(target_id)?; let wname: Vec = gdi.encode_utf16().chain(std::iter::once(0)).collect(); let mut dm = DEVMODEW { dmSize: size_of::() as u16, @@ -310,20 +311,18 @@ pub unsafe fn active_resolution(target_id: u32) -> Option<(u32, u32)> { /// class) burns the ceiling and proceeds — behavior identical to the fixed sleep it replaces. /// Returns `true` when the state verified (typical: one or two 25 ms polls), `false` on ceiling. /// -/// # Safety -/// Runs the CCD/GDI query FFI; call under the manager `state` lock like the callers it serves. -pub unsafe fn wait_mode_settled(target_id: u32, mode: Mode, ceiling: std::time::Duration) -> bool { +/// Call under the manager `state` lock like the callers it serve — a *serialization* requirement, +/// not a soundness one: reading topology unlocked races the mutator and yields a stale answer. +pub fn wait_mode_settled(target_id: u32, mode: Mode, ceiling: std::time::Duration) -> bool { let deadline = std::time::Instant::now() + ceiling; loop { - // SAFETY: CCD/GDI FFI over a `Copy` target id, owned return — this fn's own contract - // (under the manager `state` lock) is what it needs. - if unsafe { resolve_gdi_name(target_id) }.is_some() { - // SAFETY: as above; `active_resolution` is this module's own helper over the same id. - // Nested rather than `&&`-joined so it stays SHORT-CIRCUIT: this polls every 25 ms, and - // the second CCD query must not run while the target has no active path at all. - if unsafe { active_resolution(target_id) } == Some((mode.width, mode.height)) { - return true; - } + // `&&` short-circuits, so the second CCD query still does not run while the target has no + // active path at all — this polls every 25 ms. (It was nested only so each call could carry + // its own `unsafe` proof; both are safe fns now.) + if resolve_gdi_name(target_id).is_some() + && active_resolution(target_id) == Some((mode.width, mode.height)) + { + return true; } if std::time::Instant::now() >= deadline { return false; @@ -338,12 +337,12 @@ pub unsafe fn wait_mode_settled(target_id: u32, mode: Mode, ceiling: std::time:: /// advertised mode never became settable; the isolate/layout paths already re-commit with this /// flag for the same "the OS won't re-evaluate unless told" class. Best-effort. /// -/// # Safety -/// Runs the CCD query/apply FFI; call under the manager `state` lock (sole topology mutator). -pub unsafe fn force_mode_reenumeration() -> bool { +/// Call under the manager `state` lock — it is the sole topology mutator, and concurrent applies +/// fight each other. A serialization requirement, not a soundness one. +pub fn force_mode_reenumeration() -> bool { // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let Some((paths, modes)) = (unsafe { query_active_config() }) else { + let Some((paths, modes)) = query_active_config() else { return false; }; // SAFETY: the CCD contract at the top of this file — the path/mode arrays go over as @@ -445,14 +444,12 @@ pub fn wait_mode_advertised(gdi_name: &str, mode: Mode, ceiling: std::time::Dura /// for that rare race, exactly as it was for a settle that expired. Returns `true` when departure /// was observed, `false` on ceiling. /// -/// # Safety -/// Runs the CCD query FFI; call under the manager `state` lock like the callers it serves. -pub unsafe fn wait_target_departed(target_id: u32, ceiling: std::time::Duration) -> bool { +/// Call under the manager `state` lock like the callers it serves (serialization, not soundness). +pub fn wait_target_departed(target_id: u32, ceiling: std::time::Duration) -> bool { let deadline = std::time::Instant::now() + ceiling; let mut absent_streak = 0u32; loop { - // SAFETY: CCD FFI over a `Copy` target id, owned return, under the caller's `state` lock. - if unsafe { resolve_gdi_name(target_id) }.is_none() { + if resolve_gdi_name(target_id).is_none() { absent_streak += 1; if absent_streak >= 2 { return true; @@ -471,7 +468,7 @@ pub unsafe fn wait_target_departed(target_id: u32, ceiling: std::time::Duration) /// secure (Winlogon) desktop makes it render SDR/composed so DXGI Desktop Duplication can capture it /// (the HDR fullscreen independent-flip otherwise storms `ACCESS_LOST` → black); re-enable on return so /// WGC keeps HDR on the normal desktop. Returns true on a successful `DisplayConfigSetDeviceInfo`. -pub unsafe fn set_advanced_color(target_id: u32, enable: bool) -> bool { +pub fn set_advanced_color(target_id: u32, enable: bool) -> bool { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -533,7 +530,7 @@ pub unsafe fn set_advanced_color(target_id: u32, enable: bool) -> bool { /// list (both happen transiently during a display-topology re-probe): the caller decides the fallback — /// the capture loop's poller keeps the last known value, since reading a blip as "HDR off" used to cost /// an HDR session TWO spurious ring recreates (false, then true again a poll later). -pub unsafe fn advanced_color_enabled(target_id: u32) -> Option { +pub fn advanced_color_enabled(target_id: u32) -> Option { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -587,9 +584,8 @@ pub unsafe fn advanced_color_enabled(target_id: u32) -> Option { /// slider default alone is ~2.5x). `None` = query failed / target not active (callers keep /// their last value or 1.0). /// -/// # Safety -/// Runs the read-only CCD query FFI over owned locals (same shape as [`advanced_color_enabled`]). -pub unsafe fn sdr_white_level_scale(target_id: u32) -> Option { +/// Read-only, over owned locals — same shape as [`advanced_color_enabled`]. +pub fn sdr_white_level_scale(target_id: u32) -> Option { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -904,7 +900,7 @@ const DISPLAYCONFIG_PATH_MODE_IDX_INVALID: u32 = 0xffff_ffff; /// Query the current ACTIVE display config (paths + modes), truncated to the real counts. `None` on /// API failure. Shared by [`isolate_displays_ccd`] (snapshot + per-attempt re-query) and /// [`count_other_active`]. -unsafe fn query_active_config() -> Option { +fn query_active_config() -> Option { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -939,10 +935,8 @@ unsafe fn query_active_config() -> Option { /// that would still be lit besides the managed virtual set. `None` on query failure. Used to VERIFY /// isolation actually took, and (in the `primary` topology) to detect a physical that is ALREADY /// active so we can skip a force-EXTEND that would reset its refresh. -pub unsafe fn count_other_active(keep_target_ids: &[u32]) -> Option { - // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned - // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let (paths, _) = unsafe { query_active_config() }?; +pub fn count_other_active(keep_target_ids: &[u32]) -> Option { + let (paths, _) = query_active_config()?; Some( paths .iter() @@ -1010,7 +1004,7 @@ fn utf16z_str(buf: &[u16]) -> String { /// matrix to unique targets) with its name, connector class and active state. Read-only CCD; can /// briefly serialize on the display-config lock during topology churn — callers must keep it OFF /// the capture thread (`display_events` runs it on its own listener thread and caches). -pub unsafe fn target_inventory() -> Vec { +pub fn target_inventory() -> Vec { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -1101,20 +1095,16 @@ pub unsafe fn target_inventory() -> Vec { /// later returns). Returns the original active config to restore on teardown. // pub so vdisplay::pf_vdisplay can reuse this backend-neutral CCD isolation helper // (it operates on real OS target ids — a pf-vdisplay monitor's target_id qualifies). -pub unsafe fn isolate_displays_ccd(keep_target_ids: &[u32]) -> Option { +pub fn isolate_displays_ccd(keep_target_ids: &[u32]) -> Option { // Snapshot the ORIGINAL active config ONCE for restore-on-teardown, before any changes. - // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned - // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let saved = unsafe { query_active_config() }?; + let saved = query_active_config()?; // Deactivate every non-keep display, then VERIFY and RETRY. A field-reported bug had a physical // monitor STAY ACTIVE in exclusive mode, so we don't trust a single SetDisplayConfig: re-query the // live topology each attempt and re-apply until ONLY the keep set is active. Secure-desktop // correctness depends on this — the lock screen must not land on a stray panel while we stream. for attempt in 1..=4u32 { - // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned - // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let (mut paths, mut modes) = unsafe { query_active_config() }?; + let (mut paths, mut modes) = query_active_config()?; let mut others = 0u32; for p in paths.iter_mut() { if keep_target_ids.contains(&p.targetInfo.id) { @@ -1141,9 +1131,7 @@ pub unsafe fn isolate_displays_ccd(keep_target_ids: &[u32]) -> Option 0 { - // SAFETY: this module's own helper over two borrowed local `Vec`s; it only reorders - // the source positions it is handed and calls no FFI of its own. - unsafe { anchor_kept_sources_at_origin(&paths, &mut modes) }; + anchor_kept_sources_at_origin(&paths, &mut modes); } // Commit the config. Even when nothing needed deactivating we re-commit: a legacy mode-set does // NOT drive the IddCx adapter's EVT_IDD_CX_ADAPTER_COMMIT_MODES, and without COMMIT_MODES the OS @@ -1161,9 +1149,7 @@ pub unsafe fn isolate_displays_ccd(keep_target_ids: &[u32]) -> Option Option Option = unsafe { target_inventory() } + let survivors: Vec = target_inventory() .iter() .filter(|t| t.active && !keep_target_ids.contains(&t.target_id)) .map(|t| format!("{} {} \"{}\"", t.target_id, t.tech, t.friendly)) @@ -1248,7 +1232,7 @@ pub unsafe fn isolate_displays_ccd(keep_target_ids: &[u32]) -> Option (Vec, Vec) { @@ -1311,7 +1295,7 @@ fn remap_mode_idx( /// (relative arrangement preserved) so the top-left-most lands exactly on the origin. A set that /// already covers `(0,0)` is left untouched, so a plain re-commit stays byte-identical and a /// user's negative-coordinate multi-monitor layout is never rearranged. -unsafe fn anchor_kept_sources_at_origin( +fn anchor_kept_sources_at_origin( paths: &[DISPLAYCONFIG_PATH_INFO], modes: &mut [DISPLAYCONFIG_MODE_INFO], ) { @@ -1365,10 +1349,10 @@ unsafe fn anchor_kept_sources_at_origin( /// Used by the IDD-push compose kick to dirty THE TARGET display: with parallel displays the /// cursor sits on ONE of them, and a cursor wiggle only dirties that one — a sibling display's /// kick must first know where to send the cursor (Stage W3 on-glass finding). -pub unsafe fn source_desktop_rect(target_id: u32) -> Option<(i32, i32, i32, i32)> { +pub fn source_desktop_rect(target_id: u32) -> Option<(i32, i32, i32, i32)> { // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let (paths, modes) = unsafe { query_active_config() }?; + let (paths, modes) = query_active_config()?; for p in &paths { if p.targetInfo.id != target_id || p.flags & DISPLAYCONFIG_PATH_ACTIVE == 0 { continue; @@ -1400,10 +1384,8 @@ pub unsafe fn source_desktop_rect(target_id: u32) -> Option<(i32, i32, i32, i32) /// or the query fails. Used to normalize desktop coordinates into the virtual HID pointer's /// absolute `0..=32767` axis space (win32k maps the device's logical extents onto the virtual /// screen). -pub unsafe fn desktop_bounds() -> Option<(i32, i32, i32, i32)> { - // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned - // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let (paths, modes) = unsafe { query_active_config() }?; +pub fn desktop_bounds() -> Option<(i32, i32, i32, i32)> { + let (paths, modes) = query_active_config()?; let mut acc: Option<(i32, i32, i32, i32)> = None; // (x0, y0, x1, y1) for p in &paths { if p.flags & DISPLAYCONFIG_PATH_ACTIVE == 0 { @@ -1435,13 +1417,11 @@ pub unsafe fn desktop_bounds() -> Option<(i32, i32, i32, i32)> { /// treats the source at `(0,0)` as primary, so auto-row's first member lands primary — the group's /// designated member. Paths not named stay where they are. Best-effort: a failure leaves the OS /// placement (mouse crossing may not match the layout table until the next apply). -pub unsafe fn apply_source_positions(positions: &[(u32, i32, i32)]) { +pub fn apply_source_positions(positions: &[(u32, i32, i32)]) { if positions.len() < 2 { return; // a single (or no) member sits at the origin — nothing to arrange } - // SAFETY: `query_active_config` is this module's own CCD helper: it takes nothing and returns owned - // `Vec`s built from a fresh `QueryDisplayConfig`, so it has no caller obligation at all. - let Some((paths, mut modes)) = (unsafe { query_active_config() }) else { + let Some((paths, mut modes)) = query_active_config() else { return; }; // Dedup source-mode indices (a cloned group shares one) — same discipline as @@ -1504,7 +1484,7 @@ pub unsafe fn apply_source_positions(positions: &[(u32, i32, i32)]) { /// atomic CCD `SetDisplayConfig` (NOT GDI `CDS_SET_PRIMARY`, which storms /// `DXGI_ERROR_MODE_CHANGE_IN_PROGRESS` when another display is live — see [`set_active_mode`]). /// Returns the original config to restore on teardown. -pub unsafe fn set_virtual_primary_ccd(keep_target_id: u32) -> Option { +pub fn set_virtual_primary_ccd(keep_target_id: u32) -> Option { let mut np = 0u32; let mut nm = 0u32; // SAFETY: the CCD contract at the top of this file — `&mut np`/`&mut nm` are live @@ -1612,7 +1592,7 @@ pub unsafe fn set_virtual_primary_ccd(keep_target_id: u32) -> Option