fix(pf-vdisplay): the ghost-monitor reap fed live devices to pnputil, and two unsafe fns had no unsafe in them
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.
This commit is contained in:
@@ -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<Mgr>` 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<Mgr>` 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<String>,
|
||||
/// 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<SavedConfig> {
|
||||
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<String> {
|
||||
/// 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<String> {
|
||||
// 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<punktfunk_core::quic::HdrMeta>,
|
||||
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 => {
|
||||
|
||||
@@ -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<LUID>,
|
||||
_preferred_monitor_id: u32,
|
||||
_client_hdr: Option<punktfunk_core::quic::HdrMeta>,
|
||||
_hw_cursor: bool,
|
||||
) -> Result<AddedMonitor> {
|
||||
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:#}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -64,16 +64,27 @@ fn acquire_single_instance() -> Result<OwnedHandle> {
|
||||
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<String> {
|
||||
|
||||
/// 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");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<crate::policy::Linger>, env_ms: Option<u64>) -> 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<crate::policy::Topology>,
|
||||
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
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>() 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::<u16>()),
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user