From 08d311d60eabe95797bfade29a61b64c3979fc7d Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Sat, 1 Aug 2026 19:28:34 -0300 Subject: [PATCH] drm: clear the stale _drm entry by fd, and fix three comments that argue backwards new_drm_listener cleared the stale socket with std::fs::remove_file, which is unlink(2). Against a directory-typed squatter that returns EISDIR and leaves the entry in place, and endpoint.incoming() then fails EADDRINUSE, so DRM capture falls back to the portal for the rest of the boot over an entry we could have removed. The _service listener has never had that hole: it removes entries through a no-follow fd on the parent directory, fstatting the entry first and choosing AT_REMOVEDIR when it needs to. That helper now takes a path instead of a postfix, so the _drm listener - which deliberately stays outside hbb_common's postfix machinery - can use the same one on the directory it just hardened. The precondition is narrow (an unprivileged process has to win the creation race before the root service first hardens the dir on a fresh boot), which is why the failure is a warn and not a bail. Three comments stated their reason backwards or more strongly than the code supports. None of them changes behaviour; all three would send the next reader to verify the wrong thing. The wake's 20 s rate limit was justified as being short enough to be useless as a way to keep a screen lit. That is inverted: a shorter gap would make relighting easier, not harder, and 20 s is below every idle period we have measured (30.3 s at a greeter, 70.3 s in a session). What actually bounds it is that the wake is one-shot, which the next sentence of the same doc already says. Fixed at both sites, the constant and the security doc. The doc block above drm_enumerate_settled reads as one paragraph but spans a cfg split, so its shared contract and the wake-less specialisation looked like one statement about the arm below it. Marked explicitly. And get_primary_index claimed its answer agrees with the advertised geometry by construction, which is true only where augment_with_wayland_geometry runs the same assignment - it declines below two connectors or two outputs, and in that band the two functions run different code. The answer is still never worse than the documented fallback there, and now the comment says which. --- docs/DRM_CAPTURE_SECURITY.md | 6 ++++-- src/ipc.rs | 3 +++ src/ipc/drm.rs | 20 +++++++++++++++++--- src/ipc/fs.rs | 26 ++++++++++++++++++++------ src/server/drm_capturer.rs | 6 +++++- 5 files changed, 49 insertions(+), 12 deletions(-) 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