diff --git a/docs/DRM_CAPTURE_SECURITY.md b/docs/DRM_CAPTURE_SECURITY.md index c5806c34a..32ce72dea 100644 --- a/docs/DRM_CAPTURE_SECURITY.md +++ b/docs/DRM_CAPTURE_SECURITY.md @@ -180,8 +180,10 @@ presents) but reuses RustDesk's own hardened IPC. that is never coming; - it is rate limited to **one wake per 20 s process-wide** with exactly one concurrent winner (compare-exchange claim), so a reconnect storm cannot - become an input-injection storm, and it is useless as a way to keep a - screen lit; + become an input-injection storm. The limit is not what stops the wake being + used to hold a screen on -- 20 s is shorter than every idle period measured + below, so a shorter gap would make relighting easier, not harder. What + bounds that is the next point; - the wake is **one-shot: it resets the compositor's idle timer, it does not hold the display on**. If nothing else keeps the session awake, the connector idles off again one full idle period later -- measured 2026-07-31: 30.3 s at diff --git a/src/ipc.rs b/src/ipc.rs index fde59ce1a..581844eb3 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -75,6 +75,9 @@ use ipc_fs::{ check_pid, ensure_secure_ipc_parent_dir, scrub_secure_ipc_parent_dir, should_scrub_parent_entries_after_check_pid, write_pid, }; +// Gated with the module that uses it, so a `drm`-less build does not carry an unused import. +#[cfg(all(target_os = "linux", feature = "drm"))] +use ipc_fs::remove_ipc_entry_via_secure_parent_fd; use parity_tokio_ipc::{ Connection as Conn, ConnectionClient as ConnClient, Endpoint, Incoming, SecurityAttributes, }; diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index b2ece0c5f..b16450b1b 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -125,8 +125,15 @@ fn new_drm_listener() -> ResultType { // postfix reuses hbb_common's expected mode for that directory; it only creates/chmods the // directory (no pid/socket side effects) and is idempotent with the real `_service` listener. let _ = ensure_secure_ipc_parent_dir(&path, "_service")?; - // Clear any stale socket from a previous run before binding. - std::fs::remove_file(&path).ok(); + // Clear any stale entry from a previous run before binding. NOT `std::fs::remove_file`: that is + // `unlink(2)`, which returns EISDIR against a directory-typed squatter and leaves it in place, + // and `endpoint.incoming()` below then fails EADDRINUSE -- DRM capture would fall back to the + // portal for the whole boot over an entry we could have removed. The fd-based helper the + // `_service` listener already uses fstats the entry and picks `AT_REMOVEDIR` when it needs to, + // on the directory this function hardened one statement earlier. + if let Err(err) = remove_ipc_entry_via_secure_parent_fd(&path) { + log::warn!("drm: could not clear a stale entry at {}: {}", &path, err); + } let mut endpoint = Endpoint::new(path.clone()); endpoint.set_security_attributes(SecurityAttributes::allow_everyone_create()?); let incoming = endpoint.incoming()?; @@ -469,7 +476,10 @@ static DRM_LAST_WAKE: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU6 static DRM_WAKE_UNAVAILABLE: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); /// Minimum gap between two wakes. Long enough that a client reconnecting in a loop cannot flood the -/// compositor with synthetic activity, short enough to be useless as a way to keep a screen lit. +/// compositor with synthetic activity. It is NOT what stops the wake being used to hold a screen +/// on: 20 s is SHORTER than every idle period we have measured (30.3 s at a greeter, 70.3 s in a +/// session), so a shorter gap would make relighting easier, not harder. What bounds that is that +/// the wake is one-shot -- it resets the compositor's idle timer and does not hold the display up. /// Server-side config key: may the service wake a display the compositor has idle-DISABLED, by /// injecting one synthetic pointer round trip, so there is a scanout to capture? /// @@ -647,6 +657,10 @@ fn drm_wake_displays(reason: &str) -> bool { /// and the client would be handed whatever is still scanning out. libdrmtap does report the idle /// panel (`crtc=0 (inactive)`); it is our own active-CRTC filter that drops it, so the count of /// what was dropped is exactly the right signal. +/// +/// Everything above is the shared contract of both cfg arms. This last paragraph specialises the +/// one it sits on; the `drm-wake` arm follows below. +/// /// Wake-less build (`--features drm` without `drm-wake`): enumerate and answer. No wake code is /// compiled in at all, so the service cannot inject input even by accident; a host whose outputs /// are idle-disabled simply reports the displays that are still scanning out, which is the diff --git a/src/ipc/fs.rs b/src/ipc/fs.rs index e0157f3a9..d7a82fa04 100644 --- a/src/ipc/fs.rs +++ b/src/ipc/fs.rs @@ -164,9 +164,19 @@ fn scrub_preexisting_ipc_parent_entries( Ok(()) } -fn remove_ipc_socket_via_secure_parent_fd(postfix: &str) -> ResultType<()> { - let path = config::Config::ipc_path(postfix); - let parent_dir = Path::new(&path) +/// Remove one entry from the IPC parent directory through a no-follow fd on that directory. +/// +/// Prefer this over `std::fs::remove_file` for anything about to be bound: `remove_file` is +/// `unlink(2)`, which returns EISDIR against a directory-typed squatter and leaves it in place, +/// and the bind that follows then fails EADDRINUSE. `remove_parent_entry_via_fd` fstats the +/// entry first and picks `AT_REMOVEDIR` when it needs to. +pub(crate) fn remove_ipc_entry_via_secure_parent_fd(path: &str) -> ResultType<()> { + let entry_name = Path::new(path) + .file_name() + .and_then(|n| n.to_str()) + .ok_or_else(|| Error::new(ErrorKind::InvalidInput, format!("invalid ipc path: {path}")))? + .to_owned(); + let parent_dir = Path::new(path) .parent() .ok_or_else(|| Error::new(ErrorKind::InvalidInput, format!("invalid ipc path: {path}")))?; let parent_c = CString::new(parent_dir.as_os_str().as_bytes().to_vec())?; @@ -179,8 +189,8 @@ fn remove_ipc_socket_via_secure_parent_fd(postfix: &str) -> ResultType<()> { return Err(Error::new( open_err.kind(), format!( - "failed to open ipc parent dir for stale socket cleanup (no-follow): postfix={}, parent={}, err={}", - postfix, + "failed to open ipc parent dir for stale socket cleanup (no-follow): path={}, parent={}, err={}", + path, parent_dir.display(), open_err ), @@ -189,7 +199,11 @@ fn remove_ipc_socket_via_secure_parent_fd(postfix: &str) -> ResultType<()> { } }; let _fd_guard = FdGuard(fd); - remove_parent_entry_via_fd(fd, parent_dir, &format!("ipc{}", postfix)) + remove_parent_entry_via_fd(fd, parent_dir, &entry_name) +} + +fn remove_ipc_socket_via_secure_parent_fd(postfix: &str) -> ResultType<()> { + remove_ipc_entry_via_secure_parent_fd(&config::Config::ipc_path(postfix)) } // Purpose: diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 1ff05eb00..d5555c7dd 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -1648,7 +1648,11 @@ pub(super) fn get_display_infos() -> Option> { /// It asks `assign_wayland_outputs` rather than re-matching by name, and that is the point: whatever /// that function decides IS the geometry advertised to the client for each index, so deriving the /// primary from the same assignment makes the advertised primary and the advertised geometry agree -/// BY CONSTRUCTION. Matching by name here separately was the second, weaker copy of the same +/// BY CONSTRUCTION -- whenever the geometry augmentation runs it at all. `augment_with_wayland_geometry` +/// declines below two DRM connectors or two compositor outputs, and in that band this function still +/// runs the full assignment; it cannot answer worse than the documented fallback there, because a sole +/// output is matched to the lowest connector and the fallback branch returns 0 anyway. +/// Matching by name here separately was the second, weaker copy of the same /// question -- it had neither the unique-resolution step nor the layout-order fallback, so on any /// compositor whose output names do not normalize to the DRM connector names (or reports none at /// all, which the Wayland display code has its own "nameless compositor" path for) it silently