From db65980979e21c86ff5bbadb98d06e4426ec6960 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 08:49:06 +0200 Subject: [PATCH] fix(pf-vdisplay): the ghost-monitor reap fed live devices to pnputil, and two unsafe fns had no unsafe in them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows half of the sweep — the reap bug, a panic that poisons two locks, and a round of unsafe reduction. * **The ghost reap selected the wrong devices.** It filtered `Status -ne 'OK'`, a HEALTH field: that matches devices that are PRESENT but in Error/Degraded/Unknown, not the ABSENT ones the reap is for — and it handed them to `pnputil /remove-device`, contradicting its own documented contract. It runs from `add_monitor`'s mid-session slot-exhaustion recovery, so the blast radius is a live session. Now filters on `-not $_.Present`. * **`ensure_pinger` still used the panicking `thread::spawn` while holding two locks**, poisoning both — the un-fixed twin of a fix that already landed for `ensure_exclusive_watch`. Same shape applied. Unsafe reduction, continuing the program that made pf-win-display's CCD helpers safe fns: * `resolve_target_gdi` and `reisolate_after_swap` were `unsafe fn`s containing zero unsafe operations, and the three call-site SAFETY proofs described FFI they no longer perform. Both are now safe fns and those blocks are gone. * `VdisplayDriver::open`'s `# Safety` section named no caller obligation — the same empty shape an earlier phase already removed from `open_device`. * `(*detail).DevicePath.as_ptr()` derived a pointer from a `[u16; 1]` field and handed it to `CreateFileW`, which reads the whole flexible-array path beyond it. Now taken with `&raw const` from the full struct, so the pointer carries the provenance of the bytes actually read — the same correction already made for `MONITORINFOEXW` in ddc.rs. Comment fixes, all verified against the code: three intra-doc links to a type this crate does not have; a doc-comment run merged so that `shrink_action` — the gate that keeps a `Primary` group's physical panels lit — read as undocumented while its rationale sat on an unrelated polling helper; and the backend module header, which documented itself against a `sudovda` module that does not exist and a fallback the crate says was removed. Adds the first tests for `knobs.rs`, `instance.rs` and `driver.rs` — including `is_privileged_sid`, the security-relevant predicate that decides whether an existing single-instance name is another host or a squat, which had no coverage on any platform. --- .../src/vdisplay/windows/manager.rs | 253 ++++++++++++------ .../src/vdisplay/windows/manager/driver.rs | 98 ++++++- .../src/vdisplay/windows/manager/instance.rs | 71 ++++- .../src/vdisplay/windows/manager/knobs.rs | 125 +++++++-- .../src/vdisplay/windows/pf_vdisplay.rs | 88 ++++-- 5 files changed, 487 insertions(+), 148 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index eaa84a17..dbf97864 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -1,15 +1,16 @@ //! Host-lifetime virtual-display **ownership model** (Goal-1 §2.5). One reference-counted monitor -//! lifecycle, shared by both Windows backends (SudoVDA + pf-vdisplay) instead of the two verbatim- -//! duplicated `MGR: Mutex` globals each backend used to carry. +//! lifecycle, born as the shared half of two Windows backends (SudoVDA + pf-vdisplay) so the two +//! verbatim-duplicated `MGR: Mutex` globals could go; the SudoVDA backend has since been +//! removed, so pf-vdisplay is the sole driver behind the seam. //! //! [`VirtualDisplayManager`] owns the earned Idle/Active/Lingering refcount machine + the linger timer + //! a **typed** [`OwnedHandle`] control device (no more raw `isize` smuggled across the pinger/linger -//! threads). The backend differences — the IOCTL protocol and the per-monitor REMOVE key — are the only +//! threads). The driver-specific part — the IOCTL protocol and the per-monitor REMOVE key — is the only //! thing behind the [`VdisplayDriver`] seam; the state machine, the render-adapter pin decision, the -//! GDI/CCD glue (`pf_win_display::win_display`), and the generation-stamped [`MonitorLease`] are backend-neutral. +//! GDI/CCD glue (`pf_win_display::win_display`), and the generation-stamped [`MonitorLease`] are driver-neutral. //! -//! It's a process-wide singleton ([`vdm`]) initialised once with the chosen backend's driver — the -//! host runs exactly one virtual-display backend per process. The session holds a [`MonitorLease`]; +//! It's a process-wide singleton ([`vdm`]) initialised once with the driver — the host runs exactly +//! one virtual-display backend per process. The session holds a [`MonitorLease`]; //! its `Drop` releases the refcount (a *stale* lease — its monitor was preempted + recreated under it — //! is a no-op, so it can never tear down the live monitor). @@ -86,6 +87,12 @@ struct Monitor { /// is why WUDFHost death is ALL-slot shared fate. wudf_pid: u32, gdi_name: Option, + /// The mode the OS actually COMMITTED for this monitor, not the one the client asked for — all + /// three paths that write it (create, re-arrival, in-place resize) read it back through + /// [`committed_mode_or`]. It is what `output_for` hands the capturer as `preferred_mode`, what + /// `/display/state` reports, and what the next mid-stream Reconfigure diffs against, so a + /// requested-but-never-committed refresh here mis-paces the encoder AND suppresses the resize + /// that would fix it. mode: Mode, /// The monitor id the driver actually resolved (the EDID serial / ConnectorIndex) — equals the /// slot key when the per-client preference was honored, or the auto-allocated id (diagnostics). @@ -165,7 +172,7 @@ struct GroupState { ccd_exclusive: bool, } -/// How a mid-stream re-arrival ([`ManagerInner::re_add`]) ended. +/// How a mid-stream re-arrival ([`VirtualDisplayManager::re_add`]) ended. /// /// Three-way on purpose. `re_add` REMOVEs the old driver monitor before it ADDs the new one, so /// once the ADD fails the old monitor is GONE — and the caller used to answer that by putting its @@ -188,7 +195,7 @@ enum ReAdd { /// What a NON-LAST-member teardown owes the group's topology. /// -/// Split out of [`ManagerInner::teardown_removed`] so the gate is testable without a driver, a CCD +/// Split out of [`VirtualDisplayManager::teardown_removed`] so the gate is testable without a driver, a CCD /// device or a desktop — the Windows half of this crate has no other way to pin a decision. #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum ShrinkAction { @@ -200,11 +207,7 @@ enum ShrinkAction { Nothing, } -/// `ccd_exclusive` is the discriminator, NOT `ccd_saved.is_some()`: `Topology::Primary` stores a -/// 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 +/// One stage of [`VirtualDisplayManager::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. /// @@ -257,6 +260,15 @@ fn isolate_displays_ccd_seam(keep_target_ids: &[u32]) -> Option { isolate_displays_ccd(keep_target_ids) } +/// Decide the [`ShrinkAction`] a NON-LAST-member teardown owes the group. +/// +/// `ccd_exclusive` is the discriminator, NOT `has_saved`: `Topology::Primary` stores a `ccd_saved` +/// 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. (This paragraph had been +/// concatenated onto `poll_gdi_name`'s doc with no blank line between them, so the only written +/// record of the Phase-3.3 gate documented an unrelated polling helper and this fn read as +/// undocumented — a maintainer's invitation to "simplify" it back to the broken predicate.) fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction { if ccd_exclusive { ShrinkAction::Reisolate @@ -267,6 +279,53 @@ fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction { } } +/// The mode `target_id` is ACTUALLY running, for a caller about to RECORD it, with `requested` as +/// the fallback whenever the read-back cannot be trusted. +/// +/// Every path that stores a `Monitor.mode` owes this call: `set_active_mode` deliberately commits +/// the highest advertised refresh <= the requested one rather than lose the client's resolution, and +/// the `wait_mode_settled` that precedes every store verifies the RESOLUTION only — so a `true` +/// settle is no evidence at all about the refresh. `active_mode`'s own doc states the contract: +/// "Callers that RECORD a mode must record this, or they claim a refresh the display is not +/// running." +/// +/// Deliberately narrowed to the REFRESH. A read-back that FAILS, or that reports a different +/// RESOLUTION, keeps `requested`: the create path proceeds even when its settle timed out, so the +/// OS may still be sitting on its own default there, and recording that would hand the capturer + +/// the client a size nobody negotiated. The capturer already re-resolves the live size on its own +/// (`active_resolution` poll, game-capture GB1); the refresh is the field only this read-back can +/// answer. +fn committed_mode_or(target_id: u32, requested: Mode) -> Mode { + let Some((width, height, refresh_hz)) = pf_win_display::win_display::active_mode(target_id) + else { + return requested; + }; + if (width, height) != (requested.width, requested.height) { + tracing::warn!( + target_id, + requested = format!("{}x{}", requested.width, requested.height), + active = format!("{width}x{height}"), + "the OS is not running the requested resolution after the settle — recording the \ + requested mode (the capturer re-resolves the live size itself)" + ); + return requested; + } + if refresh_hz != requested.refresh_hz { + tracing::info!( + target_id, + requested_hz = requested.refresh_hz, + committed_hz = refresh_hz, + "the OS committed a different refresh than requested (the driver does not advertise \ + it) — recording what the display actually runs" + ); + } + Mode { + width, + height, + refresh_hz, + } +} + /// The manager's guarded state: the slot map + the (single) group record. One lock for both — every /// group mutation happens on a slot transition, so splitting them would only invite lock-order bugs. #[derive(Default)] @@ -528,11 +587,11 @@ impl VirtualDisplayManager { } let reap = !slot.opened_once; claim_instance()?; - // SAFETY: `VdisplayDriver::open` is `unsafe` only because it issues SetupAPI + `DeviceIoControl` - // FFI in the caller's apartment; the `device` mutex (held here) serializes it, so there is no - // concurrent open. `open` has no handle precondition to uphold, and the `OwnedHandle` it - // returns is the sole owner of the device. - let (handle, watchdog_s, driver_proto) = unsafe { self.driver.open(reap)? }; + // `open` is a SAFE fn: it discharges every FFI precondition inside its own body (it opens the + // handle it then IOCTLs) and returns an `OwnedHandle` that is the sole owner of the device. + // The `device` mutex held here serializes racing opens — a *serialization* requirement, not + // a soundness one, which is exactly why it is not expressed as `unsafe`. + let (handle, watchdog_s, driver_proto) = self.driver.open(reap)?; slot.opened_once = true; self.watchdog_s.store(watchdog_s, Ordering::Relaxed); self.driver_proto.store(driver_proto, Ordering::Relaxed); @@ -909,36 +968,57 @@ impl VirtualDisplayManager { let interval = Duration::from_millis(self.watchdog_s.load(Ordering::Relaxed) as u64 * 1000 / 3); let stop_t = stop.clone(); - let thread = thread::spawn(move || { - let mut warned = false; - while !stop_t.load(Ordering::Relaxed) { - if let Some(h) = vdm().device_handle() { - // SAFETY: `ping` requires `dev` to be a valid control handle. The `h` Arc from - // `device_handle()` is held across this call, so the handle stays open even if - // it is retired concurrently — at worst the IOCTL fails (the retire drops only - // the manager's reference; see `DeviceSlot`). The pinger thread only spins - // while the `&'static` manager singleton lives. - match unsafe { vdm().driver.ping(dev_raw(&h)) } { - Ok(()) => warned = false, - Err(e) if is_device_gone(&e) => { - // The device itself is gone (driver upgrade / WUDFHost restart) — pings - // can only keep failing on this handle. Retire it so the next session's - // `ensure_device` reopens; the monitors are already dead driver-side. - vdm().invalidate_device(&e); - } - Err(e) => { - if !warned { - tracing::warn!( - "virtual-display keepalive PING failed (control handle lost?): {e:#}" - ); - warned = true; + let thread = thread::Builder::new() + .name("vdisplay-pinger".into()) + .spawn(move || { + let mut warned = false; + while !stop_t.load(Ordering::Relaxed) { + if let Some(h) = vdm().device_handle() { + // SAFETY: `ping` requires `dev` to be a valid control handle. The `h` Arc + // from `device_handle()` is held across this call, so the handle stays open + // even if it is retired concurrently — at worst the IOCTL fails (the retire + // drops only the manager's reference; see `DeviceSlot`). The pinger thread + // only spins while the `&'static` manager singleton lives. + match unsafe { vdm().driver.ping(dev_raw(&h)) } { + Ok(()) => warned = false, + Err(e) if is_device_gone(&e) => { + // The device itself is gone (driver upgrade / WUDFHost restart) — + // pings can only keep failing on this handle. Retire it so the next + // session's `ensure_device` reopens; the monitors are already dead + // driver-side. + vdm().invalidate_device(&e); + } + Err(e) => { + if !warned { + tracing::warn!( + "virtual-display keepalive PING failed (control handle lost?): {e:#}" + ); + warned = true; + } } } } + thread::sleep(interval); } - thread::sleep(interval); + }); + // NOT `thread::spawn` (which PANICS when the OS refuses the thread), for the same reason + // `ensure_exclusive_watch` was moved off it: this runs holding `pinger` and — via + // `create_monitor` ← `acquire` — the manager `state` guard, so an unwind here poisons the + // two locks the whole manager runs on, and every later `acquire`/`release`/`snapshot` + // `.lock().unwrap()` panics for the rest of the process. A missing pinger degrades to the + // driver's watchdog tearing the displays down (recoverable, and loud); a poisoned manager is + // neither. It also gains the thread name its two siblings already have. + let thread = match thread { + Ok(t) => t, + Err(e) => { + tracing::error!( + error = %e, + "could not spawn the virtual-display keepalive pinger — the driver's host-gone \ + watchdog will tear this monitor down when it expires" + ); + return; } - }); + }; *guard = Some(Pinger { stop, thread }); } @@ -1167,9 +1247,14 @@ impl VirtualDisplayManager { /// commits the target's path directly (supplied-config apply, the same thing display Settings /// does), which doesn't consult the lid policy at all. /// - /// # Safety - /// Runs the CCD (QueryDisplayConfig / SetDisplayConfig) FFI; call under the `state` lock. - unsafe fn resolve_target_gdi(&self, target_id: u32) -> Option { + /// Call under the `state` lock: this mutates the LIVE CCD topology (force-EXTEND, explicit path + /// activation), and the manager's sole-topology-mutator contract is what keeps two acquires from + /// interleaving path commits. A *serialization* requirement, not a soundness one — every CCD + /// helper it calls is a safe fn in `pf_win_display::win_display`, so this function performs no + /// unsafe operation at all. It was an `unsafe fn` back when the FFI was inline here, and stayed + /// one after the FFI moved out: three call sites then carried `unsafe {}` blocks whose SAFETY + /// proofs asserted things about FFI that is no longer in the body. + fn resolve_target_gdi(&self, target_id: u32) -> Option { // 50 ms sampling (latency plan P0.5): the SAME 3 s per-stage ceilings — the 3-stage ladder // 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 @@ -1194,12 +1279,15 @@ impl VirtualDisplayManager { /// (first member isolates and captures the restore; a later member re-issues the isolate with /// the grown managed set — a sibling slot is never deactivated). /// + /// The returned `Monitor.mode` is what the OS COMMITTED, which need not be `mode` — see the + /// read-back after the settle. + /// /// # Safety /// `dev` must be the live control handle. unsafe fn create_monitor( &'static self, dev: HANDLE, - mode: Mode, + mut mode: Mode, slot: u32, client_hdr: Option, hw_cursor: bool, @@ -1230,9 +1318,8 @@ impl VirtualDisplayManager { // Resolve the capture target — wait for Windows to auto-activate the freshly-ADDed IDD into its // OWN display path, with the integrated-screen clone fallback (shared by the re-arrival path). - // SAFETY: `resolve_target_gdi` runs the CCD FFI (a `Copy` `u32` target by value, owned return), - // under the `state` lock. - let gdi_name = unsafe { self.resolve_target_gdi(added.target_id) }; + // Its `state`-lock discipline is satisfied: `acquire` holds the lock across this whole call. + let gdi_name = self.resolve_target_gdi(added.target_id); match &gdi_name { Some(n) => { tracing::info!( @@ -1368,6 +1455,17 @@ impl VirtualDisplayManager { verified = settled, "topology settle (verified-state wait)" ); + // Record what actually COMMITTED, not what was asked for — the same read-back + // `resize_in_place` does, for the same reason. `set_active_mode` deliberately falls + // back to the highest advertised refresh <= requested rather than lose the client's + // resolution, and `wait_mode_settled` verifies the RESOLUTION only, so `settled` + // says nothing about the refresh. Storing the request would make `mon.mode` claim a + // rate the display is not running: `output_for` hands the capturer that as + // `preferred_mode` (the encoder then paces to a rate the output never reaches), + // `/display/state` reports it, and the next Reconfigure diffs against it — a client + // re-requesting the rate it actually has would pay a needless resize, while one + // re-requesting the phantom rate takes the plain JOIN branch and never tries again. + mode = committed_mode_or(added.target_id, mode); // EXPERIMENTAL `pnp_disable_monitors`, second selector (ANY topology): monitors // that are connected but NOT part of the desktop — the standby TV/monitor the @@ -1507,29 +1605,10 @@ impl VirtualDisplayManager { "in-place mode set did not commit within 1.5s (advertised after {advertised_ms} ms)" ); } - // Record what actually COMMITTED, not what was asked for. `set_active_mode` deliberately - // falls back to the highest advertised refresh <= requested rather than lose the client's - // resolution, so `mon.mode = mode` claimed a rate the display might not be running — and - // `mon.mode` is what the next resize diffs against and what `/display/state` reports. - let committed = pf_win_display::win_display::active_mode(mon.target_id); - let landed = match committed { - Some((w, h, hz)) => Mode { - width: w, - height: h, - refresh_hz: hz, - }, - // The settle above already verified the resolution; if the read-back races we still - // know the size took, so trust the request rather than leaving `mon.mode` stale. - None => mode, - }; - if landed.refresh_hz != mode.refresh_hz { - tracing::info!( - requested_hz = mode.refresh_hz, - committed_hz = landed.refresh_hz, - "in-place resize: the OS committed a different refresh than requested (the driver \ - does not advertise it) — recording what it actually runs" - ); - } + // Record what actually COMMITTED, not what was asked for — see [`committed_mode_or`], which + // the fresh-create and re-arrival paths share with this one so all three store the same + // truth: `mon.mode` is what the next resize diffs against and what `/display/state` reports. + let landed = committed_mode_or(mon.target_id, mode); tracing::info!( advertised_ms, settle_ms = settle_start.elapsed().as_millis() as u64, @@ -1607,7 +1686,7 @@ impl VirtualDisplayManager { // values passed by value — no borrow crosses the call. // SAFETY (both ADDs): `dev` is the live control handle; `render_pin`/`client_hdr` are owned // `Copy`/`Option` values passed by value — no borrow crosses the call. - let (added, mode, rollback_err) = match unsafe { + let (added, mut mode, rollback_err) = match unsafe { self.driver .add_monitor(dev, mode, render_pin, slot, client_hdr, old.hw_cursor) } { @@ -1646,9 +1725,9 @@ impl VirtualDisplayManager { } }; self.ensure_pinger(); - // 3. Resolve the NEW target's GDI name (target_id changes across a re-arrival). - // SAFETY: CCD FFI over a `Copy` target id, under the `state` lock. - let gdi_name = unsafe { self.resolve_target_gdi(added.target_id) }; + // 3. Resolve the NEW target's GDI name (target_id changes across a re-arrival). Under the + // `state` lock, as its topology-mutator discipline requires. + let gdi_name = self.resolve_target_gdi(added.target_id); match &gdi_name { Some(n) => { tracing::info!( @@ -1659,9 +1738,9 @@ impl VirtualDisplayManager { // ADD only advertises the mode; force it active so DXGI/IDD captures the new size. set_active_mode(n, mode); // 4. Re-isolate the composited set with the NEW target replacing the old — preserving - // the group's first-member restore snapshot. - // SAFETY: CCD FFI over borrowed Copy target ids, under the `state` lock. - unsafe { self.reisolate_after_swap(inner, added.target_id) }; + // the group's first-member restore snapshot. Under the `state` lock (the caller + // holds it and lent us `inner`), as its topology-mutator discipline requires. + self.reisolate_after_swap(inner, added.target_id); // 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(); @@ -1671,6 +1750,12 @@ impl VirtualDisplayManager { verified = settled, "re-arrival topology settle (verified-state wait)" ); + // Store what COMMITTED, not what was asked for — the settle above verifies the + // resolution only, so it is no evidence about the refresh (see + // [`committed_mode_or`]). Doing this here rather than at the `Monitor` construction + // below keeps it on the arm where a path actually exists: with no GDI name there is + // no committed mode to read, and the request stands. + mode = committed_mode_or(added.target_id, mode); } None => tracing::warn!( "re-arrival target {} not yet an active display path (auto-activate, EXTEND preset \ @@ -1708,9 +1793,11 @@ impl VirtualDisplayManager { /// old slot has already been removed from the map by the caller, so `inner.target_ids()` is the /// surviving siblings; the new target joins them. /// - /// # Safety - /// Drives the CCD topology FFI; call under the `state` lock. - unsafe fn reisolate_after_swap(&self, inner: &mut MgrInner, new_target: u32) { + /// Call under the `state` lock — it commits a new CCD topology, so it must not interleave with + /// another slot transition's commit. A *serialization* requirement, not a soundness one: every + /// helper it reaches (`isolate_displays_ccd_seam`, `set_virtual_primary_ccd`) is a safe fn, so + /// this body performs no unsafe operation. (`&mut MgrInner` already proves the lock is held.) + fn reisolate_after_swap(&self, inner: &mut MgrInner, new_target: u32) { use crate::policy::Topology; match topology_action() { Topology::Exclusive => { diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager/driver.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager/driver.rs index c5d4e085..510ad56d 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager/driver.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager/driver.rs @@ -1,12 +1,20 @@ -//! The backend-specific virtual-display **seam** (SudoVDA vs pf-vdisplay), carved out of the manager -//! (plan §W3): the REMOVE-key type, the `add_monitor` reply, and the IOCTL trait. This is the ONLY -//! thing that differs between the two Windows backends — the refcount machine, linger, pinger, and -//! CCD/GDI glue are all backend-neutral in [`super::VirtualDisplayManager`]. +//! The virtual-display driver **seam**, carved out of the manager (plan §W3): the REMOVE-key type, +//! the `add_monitor` reply, and the IOCTL trait. It isolates the DRIVER's wire protocol from the +//! lifecycle — the refcount machine, linger, pinger and CCD/GDI glue are all driver-neutral in +//! [`super::VirtualDisplayManager`]. It was born as a two-backend seam (SudoVDA vs pf-vdisplay) and +//! has exactly one implementor since SudoVDA was removed: `crate::driver::PfVdisplayDriver` (the +//! flattened module name of `vdisplay/windows/pf_vdisplay.rs`). Kept as a trait because it is also +//! the only place the IOCTL surface can be faked, not because a second backend is expected. use super::*; -/// The per-backend REMOVE key the driver stamps on ADD and consumes on REMOVE. SudoVDA keys monitors by -/// a fresh `GUID`; pf-vdisplay keys them by a monotonic `u64` session id. +/// The per-driver REMOVE key stamped on ADD and consumed on REMOVE. pf-vdisplay keys monitors by a +/// monotonic `u64` session id. +/// +/// `Guid` is a RETAINED, UNUSED variant: it keyed SudoVDA's monitors (a fresh `GUID` per monitor) and +/// nothing constructs it since that backend was removed — the `else` arms in `pf_vdisplay`'s +/// `update_modes`/`remove_monitor` that reject it are therefore dead today. Left in place so the +/// enum still documents that the key is a per-driver choice rather than a `u64` by nature. #[derive(Clone, Copy)] pub(crate) enum MonitorKey { Guid(windows::core::GUID), @@ -29,10 +37,10 @@ pub(crate) struct AddedMonitor { pub cursor_excluded: bool, } -/// The backend-specific IOCTL surface — the *only* thing that differs between SudoVDA and pf-vdisplay. -/// Everything else (the refcount machine, the linger, the pinger, the CCD/GDI glue) is shared in -/// [`VirtualDisplayManager`]. `Send + Sync` because the manager (and so the boxed driver) is a -/// `&'static` singleton reached from the pinger + linger threads. +/// The driver's IOCTL surface — everything else (the refcount machine, the linger, the pinger, the +/// CCD/GDI glue) is driver-neutral and shared in [`VirtualDisplayManager`]. `Send + Sync` because the +/// manager (and so the boxed driver) is a `&'static` singleton reached from the pinger + linger +/// threads. pub(crate) trait VdisplayDriver: Send + Sync { fn name(&self) -> &'static str; /// Find + open the control device, validate it (version handshake), and read the watchdog @@ -42,9 +50,14 @@ pub(crate) trait VdisplayDriver: Send + Sync { /// owned handle + watchdog seconds + the driver's reported protocol version (the in-place /// resize gates on it). /// - /// # Safety - /// Issues setup-API + `DeviceIoControl` calls; runs in the caller's apartment. - unsafe fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)>; + /// SAFE, and owning — unlike every other method here, which takes the raw `dev` handle. It has + /// no caller obligation: it takes only a `bool`, opens the handle it then IOCTLs, and hands back + /// an `OwnedHandle` that closes on drop. It used to be an `unsafe fn` whose `# Safety` section + /// ("issues setup-API + `DeviceIoControl` calls; runs in the caller's apartment") restated what + /// the body does rather than naming anything a caller could uphold — an un-checkable proof + /// obligation at the one call site, which trains a reviewer to wave through the neighbouring + /// blocks where the `dev` precondition is real. + fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)>; /// ADD a virtual monitor at `mode`, pinning the IDD render GPU to `render_luid` first if `Some`, and /// requesting `preferred_monitor_id` (the host's per-client stable id; `0` = auto). `client_hdr` /// is the CLIENT display's HDR volume for the monitor's EDID CTA HDR block (`None` = the @@ -85,3 +98,62 @@ pub(crate) trait VdisplayDriver: Send + Sync { /// `dev` must be the live control handle. unsafe fn ping(&self, dev: HANDLE) -> Result<()>; } + +#[cfg(test)] +mod tests { + use super::*; + + /// A driver that implements nothing but the required methods — so the DEFAULTED `update_modes` + /// is what gets called. + struct FakeDriver; + + impl VdisplayDriver for FakeDriver { + fn name(&self) -> &'static str { + "fake" + } + fn open(&self, _reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)> { + anyhow::bail!("fake driver has no control device") + } + unsafe fn add_monitor( + &self, + _dev: HANDLE, + _mode: Mode, + _render_luid: Option, + _preferred_monitor_id: u32, + _client_hdr: Option, + _hw_cursor: bool, + ) -> Result { + anyhow::bail!("fake driver adds no monitors") + } + unsafe fn remove_monitor(&self, _dev: HANDLE, _key: &MonitorKey) -> Result<()> { + Ok(()) + } + unsafe fn ping(&self, _dev: HANDLE) -> Result<()> { + Ok(()) + } + } + + /// The `update_modes` default must ERR, not silently succeed: `resize_in_place` treats `Ok(())` + /// as "the driver refreshed the monitor's advertised mode list" and goes straight on to the CCD + /// force-set + settle — so a default that returned `Ok` would burn the full 1.5 s settle against + /// a mode list nobody updated, on every mid-stream resize, before falling back to the + /// re-arrival it should have taken immediately. + #[test] + fn the_defaulted_update_modes_reports_not_supported() { + let d = FakeDriver; + let mode = Mode { + width: 1920, + height: 1080, + refresh_hz: 60, + }; + // SAFETY: the defaulted `update_modes` discharges its `dev` obligation by never using it — + // the body discards all three arguments and errs — so the null handle is never touched. + let err = unsafe { d.update_modes(HANDLE::default(), &MonitorKey::Session(1), mode) } + .expect_err("the default must not report success"); + assert!( + err.to_string() + .contains("does not support in-place mode updates"), + "unexpected error text: {err:#}" + ); + } +} diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs index b41bc49e..da1332c5 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs @@ -64,16 +64,27 @@ fn acquire_single_instance() -> Result { unsafe { let h = match CreateMutexW(Some(&sa), false, w!("Global\\punktfunk-vdisplay-manager")) { Ok(h) => h, - // The name exists but its creator's DACL denies this token the implicit OPEN (the SCM - // service creates it as SYSTEM; a second elevated-admin host lands here instead of in - // the ALREADY_EXISTS branch — validated on-glass). Legitimately that means an instance - // is live; it is ALSO exactly what a squat looks like, so say both. + // ACCESS_DENIED has THREE causes here and the handle alone cannot tell them apart, so + // name all three rather than assert one. (1) The name exists but its creator's DACL + // denies this token the implicit OPEN — the SCM service creates it as SYSTEM, so a + // second elevated-admin host lands here instead of in the ALREADY_EXISTS branch + // (validated on-glass); that is a live instance. (2) The same shape is exactly what a + // SQUAT looks like. (3) `CreateMutexW` also fails ACCESS_DENIED when the caller holds no + // SeCreateGlobalPrivilege at all — granted by default to Administrators, SYSTEM and the + // SERVICE groups but NOT to an ordinary interactive user, so an un-elevated + // `punktfunk-host serve` reaches this arm with no such object existing anywhere. Naming + // only (1)+(2) sent that operator hunting a process that does not exist and a + // `handle.exe` that finds nothing — the same misdiagnosis family as 2026-08-05 L-16, + // which this block exists to remove. Err(e) if e.code().0 == 0x8007_0005u32 as i32 => anyhow::bail!( - "{IN_USE}\n\nIf no other punktfunk-host is running, the name \ - `Global\\punktfunk-vdisplay-manager` has been SQUATTED by another process — any \ - account with SeCreateGlobalPrivilege can create it first and deny us access, \ - which disables virtual-display streaming until that process exits. Find the \ - holder with Sysinternals `handle.exe -a punktfunk-vdisplay-manager`." + "{IN_USE}\n\nIf no other punktfunk-host is running, either this process cannot \ + create a `Global\\` kernel object at all (it needs SeCreateGlobalPrivilege — run \ + the host ELEVATED or as the installed service account; an ordinary interactive \ + user does not hold it), or the name `Global\\punktfunk-vdisplay-manager` has been \ + SQUATTED by another process — any account with that privilege can create it first \ + and deny us access, which disables virtual-display streaming until that process \ + exits. Sysinternals `handle.exe -a punktfunk-vdisplay-manager` tells the two \ + apart: a holder means a squat, NOTHING means the privilege." ), Err(e) => { return Err(e).context("CreateMutexW(punktfunk-vdisplay single-instance guard)"); @@ -190,6 +201,48 @@ fn object_owner_sid(h: HANDLE) -> Option { /// SYSTEM, BUILTIN\Administrators, or a member of the Administrators-owned set — the principals a /// legitimate pf-vdisplay manager runs as. +/// +/// Deliberately NARROW, and the narrowness is the security property: this predicate is what decides +/// whether an existing single-instance name is reported as "another punktfunk-host" (benign, wait it +/// out) or as a SQUAT (an attack on virtual-display availability). Widening it — `S-1-5-32-` as a +/// prefix, or any `S-1-5-21-…` domain account — silently reclassifies a non-administrative squatter +/// as one of ours and restores the exact misdiagnosis the 2026-08-05 L-16 fix removed. LocalService +/// (`S-1-5-19`) and NetworkService (`S-1-5-20`) are excluded ON PURPOSE: the plugin runner is forced +/// to LocalService, so a name owned by it is a plugin, not a host. fn is_privileged_sid(sid: &str) -> bool { matches!(sid, "S-1-5-18" | "S-1-5-32-544") || sid.starts_with("S-1-5-80-") // service SIDs } + +#[cfg(test)] +mod tests { + use super::is_privileged_sid; + + /// Pins the classification above — the only pure decision in this module, and the one whose + /// widening is silent (nothing fails; a squat merely starts reading as a sibling host). + #[test] + fn is_privileged_sid_accepts_system_admins_and_service_sids_only() { + assert!(is_privileged_sid("S-1-5-18"), "SYSTEM"); + assert!(is_privileged_sid("S-1-5-32-544"), "BUILTIN\\Administrators"); + assert!( + is_privileged_sid("S-1-5-80-3139157870-2983391045-3678747466-658725712-1809340420"), + "an NT SERVICE\\… per-service SID" + ); + + assert!(!is_privileged_sid("S-1-5-32-545"), "BUILTIN\\Users"); + assert!( + !is_privileged_sid("S-1-5-21-1004336348-1177238915-682003330-1001"), + "a local/domain user account" + ); + // LocalService / NetworkService: the plugin runner's accounts, deliberately NOT ours. + assert!(!is_privileged_sid("S-1-5-19"), "LocalService"); + assert!(!is_privileged_sid("S-1-5-20"), "NetworkService"); + assert!( + !is_privileged_sid(""), + "an unreadable owner is never 'fine'" + ); + // Prefix discipline: `S-1-5-80` without the trailing dash is a different SID string, and + // `S-1-5-8` (Proxy) must not slip in under a loosened prefix. + assert!(!is_privileged_sid("S-1-5-8"), "Proxy"); + assert!(!is_privileged_sid("S-1-5-800-1"), "not a service SID"); + } +} diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager/knobs.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager/knobs.rs index 1c0f0241..71bd1833 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager/knobs.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager/knobs.rs @@ -2,25 +2,44 @@ //! carved out of the manager (plan §W3): the linger window, the keep-alive-forever pin, and the //! per-monitor topology action. Pure readers of [`crate::policy`] + env — no manager state. +/// The historical Windows linger window, and the fallback for every rung that cannot answer. +const DEFAULT_LINGER_MS: u64 = 10_000; + /// Linger window before a session-less monitor is torn down. The console display-management policy /// wins when configured (`keep_alive`); otherwise the legacy `PUNKTFUNK_MONITOR_LINGER_MS` env knob, /// else the 10 s default. pub(super) fn linger_ms() -> u64 { - use crate::policy::{prefs, Linger}; - if let Some(eff) = prefs().configured_effective() { - return match eff.keep_alive.linger() { - Linger::Immediate => 0, - Linger::For(d) => d.as_millis() as u64, - // `forever` is handled BEFORE this by `keep_alive_forever()` in `release` (→ `Pinned`), so - // this arm is only reached defensively (e.g. a caller that resolves ms without the pin - // check) — fall back to the default rather than a huge linger. - Linger::Forever => 10_000, - }; + resolve_linger_ms( + crate::policy::prefs() + .configured_effective() + .map(|eff| eff.keep_alive.linger()), + std::env::var("PUNKTFUNK_MONITOR_LINGER_MS") + .ok() + .and_then(|s| s.parse().ok()), + ) +} + +/// The precedence itself, lifted out of the readers so it is pinnable without a settings file, an +/// environment or a manager (this module's decisions are the ONLY ones on the Windows lifecycle path +/// that need neither a driver nor a desktop, and they had no tests at all). +/// +/// `configured` is the console policy's resolved [`Linger`](crate::policy::Linger) (`None` = the +/// host was never configured), `env_ms` the parsed legacy knob. The configured policy outranks the +/// env knob entirely — an operator who set the console must not have it silently overridden by a +/// leftover variable. +fn resolve_linger_ms(configured: Option, env_ms: Option) -> u64 { + use crate::policy::Linger; + match configured { + Some(Linger::Immediate) => 0, + Some(Linger::For(d)) => d.as_millis() as u64, + // `forever` is handled BEFORE this by `keep_alive_forever()` in `release` (→ `Pinned`), so + // this arm is only reached defensively (e.g. a caller that resolves ms without the pin + // check) — fall back to the default rather than a huge linger. + Some(Linger::Forever) => DEFAULT_LINGER_MS, + // Unconfigured: the legacy env knob, else the historical default. An unparseable value + // arrives here as `None` (the caller's `parse().ok()`), i.e. it reads as unset. + None => env_ms.unwrap_or(DEFAULT_LINGER_MS), } - std::env::var("PUNKTFUNK_MONITOR_LINGER_MS") - .ok() - .and_then(|s| s.parse().ok()) - .unwrap_or(10_000) } /// Whether the configured console policy's `keep_alive` resolves to **forever** (`Pinned`) — the @@ -50,13 +69,79 @@ pub(super) fn exclusive_reassert_ms() -> u64 { /// extended; `Primary` makes it primary while keeping the physical(s) active; `Exclusive` disables the /// physical(s) so the IDD is the sole composited desktop. pub(super) fn topology_action() -> crate::policy::Topology { + let configured = crate::policy::prefs() + .configured_effective() + .map(|_| crate::effective_topology()); + resolve_topology_action(configured, std::env::var("PUNKTFUNK_NO_ISOLATE").is_ok()) +} + +/// The precedence for [`topology_action`], lifted out for the same reason as [`resolve_linger_ms`]. +/// `configured` is [`crate::effective_topology`]'s answer when the console configured anything at +/// all (that fn is the rung responsible for never returning `Auto`); `no_isolate_env` is the legacy +/// `PUNKTFUNK_NO_ISOLATE` opt-out, which an unconfigured host still honors. +fn resolve_topology_action( + configured: Option, + no_isolate_env: bool, +) -> crate::policy::Topology { use crate::policy::Topology; - if crate::policy::prefs().configured_effective().is_some() { - return crate::effective_topology(); + match configured { + Some(t) => t, + None if no_isolate_env => Topology::Extend, + None => Topology::Exclusive, } - if std::env::var("PUNKTFUNK_NO_ISOLATE").is_ok() { - Topology::Extend - } else { - Topology::Exclusive +} + +#[cfg(test)] +mod tests { + use super::{resolve_linger_ms, resolve_topology_action, DEFAULT_LINGER_MS}; + use crate::policy::{Linger, Topology}; + use std::time::Duration; + + /// The console policy is the top rung: a host that configured `keep_alive` must not have it + /// silently overridden by a leftover `PUNKTFUNK_MONITOR_LINGER_MS`. + #[test] + fn configured_policy_beats_the_legacy_env_knob() { + assert_eq!( + resolve_linger_ms(Some(Linger::For(Duration::from_secs(3))), Some(60_000)), + 3_000 + ); + assert_eq!(resolve_linger_ms(Some(Linger::Immediate), Some(60_000)), 0); + } + + /// Unconfigured hosts keep the historical behavior: the env knob, else the 10 s default. An + /// unparseable value reaches this fn as `None` (the reader's `parse().ok()`), so it reads as + /// unset rather than as zero — a `linger_ms = 0` would tear the monitor down on every + /// disconnect. + #[test] + fn an_unconfigured_host_honours_the_env_knob_then_the_default() { + assert_eq!(resolve_linger_ms(None, Some(250)), 250); + assert_eq!(resolve_linger_ms(None, None), DEFAULT_LINGER_MS); + } + + /// `Forever` is the `Pinned` lifecycle, resolved by `keep_alive_forever()` before any ms are + /// asked for; reaching this fn with it means a caller skipped the pin check, and the answer is + /// the default window — NOT an effectively infinite linger that would keep the physical panels + /// dark with nothing to release them. + #[test] + fn forever_resolves_to_the_default_not_a_huge_linger() { + assert_eq!( + resolve_linger_ms(Some(Linger::Forever), None), + DEFAULT_LINGER_MS + ); + } + + /// The unconfigured rungs are `Exclusive` by default, `Extend` under the legacy opt-out — and + /// neither is `Auto`, which the manager's `match` would treat as plain extend without ever + /// saying so. + #[test] + fn the_unconfigured_topology_rungs_never_yield_auto() { + assert_eq!(resolve_topology_action(None, false), Topology::Exclusive); + assert_eq!(resolve_topology_action(None, true), Topology::Extend); + // A configured host's answer is whatever `effective_topology()` resolved — passed through + // verbatim, env knob or not. + assert_eq!( + resolve_topology_action(Some(Topology::Primary), true), + Topology::Primary + ); } } diff --git a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs index e456a3b7..355d3ee6 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs @@ -8,11 +8,13 @@ //! the wire contract OWNED by [`pf_driver_proto::control`] (versioned + `#[repr(C)] Pod` structs, //! NOT the SudoVDA ABI). No DLL, no named pipe. See `design/windows-host-rewrite.md`. //! -//! This is a faithful clone of [`super::sudovda`] (the shipping fallback) repointed at the new driver: -//! same reference-counted/lingering monitor lifecycle, same CCD isolation + active-mode forcing — those -//! backend-NEUTRAL helpers are REUSED from `sudovda` (a pf-vdisplay monitor's `target_id` is a real OS -//! target id, so the CCD/DXGI code works unchanged). Only the driver-specific bits (GUID, IOCTL codes, -//! request/reply structs, the version handshake) differ, per `pf_driver_proto`. +//! punktfunk's IddCx driver is the SOLE Windows backend — the legacy SudoVDA fallback was removed and +//! its driver is no longer shipped (`lib.rs`), so nothing here is a "clone of the fallback" any more. +//! The backend-NEUTRAL half — the reference-counted/lingering monitor lifecycle, the CCD isolation and +//! the active-mode forcing — lives in [`super::manager`] and `pf_win_display::win_display` (a +//! pf-vdisplay monitor's `target_id` is a real OS target id, so that CCD/DXGI code applies unchanged). +//! Only the driver-specific bits (GUID, IOCTL codes, request/reply structs, the version handshake) are +//! here, per `pf_driver_proto`. // Every `unsafe` block in this file carries a `// SAFETY:` proof; enforce it (unsafe-proof program). #![deny(clippy::undocumented_unsafe_blocks)] @@ -97,9 +99,10 @@ unsafe fn ioctl(h: HANDLE, code: u32, input: &[u8], output: &mut [u8]) -> Result /// pinning an OS VidPN target against the IddCx adapter's fixed monitor-slot budget; once ~16 accumulate, /// `IOCTL_ADD` wedges at 0x80070490 (`ERROR_NOT_FOUND`) and every session black-screens until a manual /// reset/reboot. Removing the not-present PDOs frees the slots — the in-process equivalent of -/// `reset-pf-vdisplay.ps1` step 2 (proven on-box). Best-effort + idempotent: only NOT-present nodes -/// (`Status != OK`) are removed, so the LIVE session's monitor (`Status OK`) is never touched; any -/// failure is logged and swallowed. Returns the number removed. +/// `reset-pf-vdisplay.ps1` step 2 (proven on-box). Best-effort + idempotent: only ABSENT nodes +/// (`Present` false AND `Status` `Unknown`) are removed, so a LIVE session's monitor is never +/// touched — not even while it is in a transient problem state; any failure is logged and +/// swallowed. Returns the number removed. /// /// The outcome is logged UNCONDITIONALLY, as found + removed: the old script counted only removals /// and the host spoke only when that count was positive, so a reap whose pnputil never launched and @@ -108,8 +111,17 @@ unsafe fn ioctl(h: HANDLE, code: u32, input: &[u8], output: &mut [u8]) -> Result /// wedge with every sleep cycle. fn reap_ghost_monitors() -> u32 { // Mirrors reset-pf-vdisplay.ps1 step 2. powershell is always present for the SYSTEM service; the - // matched tokens ('OK', 'punktfunk', the InstanceId) are locale-invariant, so this is safe on a - // non-English box (unlike a .ps1 *file* read in the machine codepage). + // matched tokens ('Unknown', 'punktfunk', the InstanceId) are locale-invariant, so this is safe + // on a non-English box (unlike a .ps1 *file* read in the machine codepage). + // + // The selector asks about PRESENCE, not health — the exact complement of the liveness predicate + // the adapter reload below uses (`$_.Present -or $_.Status -ne 'Unknown'`). It used to read + // `Status -ne 'OK'`, which is a HEALTH field: `Error`, `Degraded` and `Unknown` all satisfy it, + // so a PRESENT virtual monitor in a transient problem state was handed to `pnputil + // /remove-device` — and this runs mid-session from `add_monitor`'s 0x80070490 recovery, i.e. + // while sibling sessions are live, so it could rip out a live client's monitor. `Present` is the + // authoritative bit; the `Status -eq 'Unknown'` conjunct is the guard for `Present` reading null + // (`-not $null` is TRUE, which alone would select every device on the box). // // pnputil is resolved by full path and `$LASTEXITCODE` pre-seeded to failure before every // launch, exactly like the reload path below: a LocalSystem service's PATH need not include @@ -117,7 +129,7 @@ fn reap_ghost_monitors() -> u32 { // elevated), and the old bare-name call failed INVISIBLY there — `SilentlyContinue` swallowed // the miss, no exit code was written, and the ghosts stayed to wedge `IOCTL_ADD` at 0x80070490. const REAP_PS: &str = "$ErrorActionPreference='SilentlyContinue'; \ - $g = @(Get-PnpDevice -Class Monitor | Where-Object { $_.Status -ne 'OK' -and $_.FriendlyName -match 'punktfunk' }); \ + $g = @(Get-PnpDevice -Class Monitor | Where-Object { -not $_.Present -and $_.Status -eq 'Unknown' -and $_.FriendlyName -match 'punktfunk' }); \ $pnp = ($env:SystemRoot + '\\System32\\pnputil.exe'); \ $n = 0; foreach ($d in $g) { $LASTEXITCODE = 1; if (Test-Path $pnp) { & $pnp /remove-device $d.InstanceId *> $null }; if ($LASTEXITCODE -eq 0) { $n++ } }; \ Write-Output ($g.Count.ToString() + ' ' + $n)"; @@ -593,14 +605,20 @@ fn probe_device() -> Probe { // 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. + // before `buf` is dropped. That path pointer is taken as a RAW place projection off + // `detail`, so it keeps the whole `buf` allocation's provenance: `DevicePath` is declared + // `[u16; 1]` (a flexible-array-member stub), so `.as_ptr()` would auto-ref it and hand + // `CreateFileW` a pointer tagged for TWO bytes while the API reads the full NUL-terminated + // path (100+ bytes) — everything past `DevicePath[0]` out of bounds for that tag, and a + // compiler entitled to fold the zero-init back in and pass an EMPTY device name. Same + // defect class (and same fix) as the `MONITORINFOEXW` retag in `vdisplay/ddc.rs`. let opened = unsafe { (*detail).cbSize = size_of::() as u32; SetupDiGetDeviceInterfaceDetailW(hdev.0, &idata, Some(detail), required, None, None) .context("SetupDiGetDeviceInterfaceDetailW(pf-vdisplay)") .and_then(|()| { CreateFileW( - PCWSTR((*detail).DevicePath.as_ptr()), + PCWSTR((&raw const (*detail).DevicePath).cast::()), 0xC000_0000, // GENERIC_READ | GENERIC_WRITE FILE_SHARE_READ | FILE_SHARE_WRITE, None, @@ -635,7 +653,7 @@ impl VdisplayDriver for PfVdisplayDriver { "pf-vdisplay" } - unsafe fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)> { + fn open(&self, reap_orphans: bool) -> Result<(OwnedHandle, u32, u32)> { // A short re-probe, and deliberately NO adapter reload — this replaces the second, impatient // copy of the recovery that used to live here. Session bring-up already ran the full // `ensure_available` before constructing the backend, so anything left for this open to @@ -686,18 +704,42 @@ impl VdisplayDriver for PfVdisplayDriver { ); } let watchdog_s = info.watchdog_timeout_s.max(1); - if info.protocol_version < pf_driver_proto::PROTOCOL_VERSION { + // UNCONDITIONAL: this line is the only place the negotiated watchdog is reported, and the + // pinger's cadence (`watchdog/3`) is derived from it — yet it used to sit in the `else` of + // the version warning, so exactly the hosts where the number is worth having (anything but + // an exact-version pair) logged nothing at all. + tracing::info!( + "pf-vdisplay protocol {} (host drives {}..={}, watchdog timeout {}s)", + info.protocol_version, + pf_driver_proto::MIN_DRIVER_PROTOCOL_VERSION, + pf_driver_proto::PROTOCOL_VERSION, + watchdog_s + ); + // Version-SPECIFIC capability gaps, reported independently. Every bump since v3 is ADDITIVE, + // so the old blanket `< PROTOCOL_VERSION` test named the WRONG gap: it told a v4 or v5 + // driver it "lacks the in-place resize" — added IN v4 — purely because it was not v6. Each + // rung below names the capability the host actually gates on that version. + if info.protocol_version < 4 { tracing::warn!( - "pf-vdisplay protocol {} (host supports {}): driver lacks the in-place resize — \ - mid-stream resizes use the monitor re-arrival path until the driver is updated", - info.protocol_version, - pf_driver_proto::PROTOCOL_VERSION + "pf-vdisplay protocol {}: driver lacks the in-place mid-stream resize \ + (IOCTL_UPDATE_MODES, added in v4) — every mid-stream resize costs a monitor \ + re-arrival (one hotplug per switch) until the driver is updated", + info.protocol_version ); - } else { + } + if info.protocol_version < 5 { + tracing::warn!( + "pf-vdisplay protocol {}: driver lacks the IddCx hardware-cursor channel (added in \ + v5) — the pointer stays composited into the captured frame", + info.protocol_version + ); + } + if info.protocol_version < 6 { tracing::info!( - "pf-vdisplay protocol {} (watchdog timeout {}s)", - info.protocol_version, - watchdog_s + "pf-vdisplay protocol {}: driver lacks the mid-stream cursor-forward flip \ + (IOCTL_SET_CURSOR_FORWARD, added in v6) — the cursor model declared at monitor ADD \ + stands for the whole session", + info.protocol_version ); } // Reap monitors orphaned by a crashed previous host — a FIRST-CLASS op (driver returns