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.
This commit is contained in:
Mariano Abad
2026-08-01 19:28:34 -03:00
parent 2648ad0a2e
commit 08d311d60e
5 changed files with 49 additions and 12 deletions

View File

@@ -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

View File

@@ -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,
};

View File

@@ -125,8 +125,15 @@ fn new_drm_listener() -> ResultType<Incoming> {
// 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

View File

@@ -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:

View File

@@ -1648,7 +1648,11 @@ pub(super) fn get_display_infos() -> Option<Vec<DisplayInfo>> {
/// 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