From 5fdfa4384cbb7a30a846d0b0034174bbeb5020cd Mon Sep 17 00:00:00 2001 From: vasilito Date: Tue, 28 Jul 2026 07:19:51 +0900 Subject: [PATCH] sessiond: reap inhibitors when caller FD closes (logind contract) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 5-lane review flagged that sessiond inhibitors were reaped only on bus-owner disappearance (via NameOwnerChanged polling). The logind contract requires the inhibitor to be released when the returned FD is closed — regardless of whether the bus connection survives. This commit closes that gap. - Add InhibitorEntry.inhibitor_fd: Option tracking the caller-side FD the daemon sent back. - Add manager::remove_inhibitor_for_fd(fd) that scans inhibitors for a matching caller FD and removes the matching entry, closing the daemon-side FD copy. - In Inhibit(), spawn a tokio task that polls the caller-side FD for POLLHUP via nix::poll::poll. When the caller closes their end, the task calls remove_inhibitor_for_fd(fd) to drop the entry. Uses tokio (already a dependency). - nix is already an existing dependency via libredox-transitive; use only poll() and PollFd which are stdlib-adjacent. If nix is not available, fall back to a no-op (the NameOwnerChanged reaper remains the fallback for lost-bus-owner cleanup). Verified by host cargo test: 63/63 tests pass, including 3 new tests: - closing_caller_fd_removes_inhibitor - multiple_inhibitors_same_sender_independent_fd_close - inhibitor_fd_closure_does_not_affect_other_sender --- .../redbear-sessiond/source/src/manager.rs | 206 ++++++++++++++++-- .../source/src/runtime_state.rs | 4 + 2 files changed, 192 insertions(+), 18 deletions(-) diff --git a/local/recipes/system/redbear-sessiond/source/src/manager.rs b/local/recipes/system/redbear-sessiond/source/src/manager.rs index 98a4a7b655..8d1ff24db5 100644 --- a/local/recipes/system/redbear-sessiond/source/src/manager.rs +++ b/local/recipes/system/redbear-sessiond/source/src/manager.rs @@ -1,12 +1,12 @@ use std::{ - collections::HashSet, + collections::{HashMap, HashSet}, fs, io::Write, - os::fd::OwnedFd as StdOwnedFd, + os::fd::{AsRawFd, OwnedFd as StdOwnedFd}, os::unix::net::UnixStream, sync::{ Arc, Mutex, - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicU64, Ordering}, }, time::Duration, }; @@ -25,15 +25,14 @@ use tokio::spawn as tokio_spawn; use crate::runtime_state::{InhibitorEntry, SharedRuntime}; -type InhibitorFdSlot = (Option, StdOwnedFd); - #[derive(Clone, Debug)] pub struct LoginManager { runtime: SharedRuntime, session_path: OwnedObjectPath, seat_path: OwnedObjectPath, user_path: OwnedObjectPath, - inhibitor_fds: Arc>>, + inhibitor_fds: Arc>>, + next_inhibitor_id: Arc, connection: Arc>>, seat_announced: Arc, dead_senders: Arc>>, @@ -51,7 +50,8 @@ impl LoginManager { session_path, seat_path, user_path, - inhibitor_fds: Arc::new(Mutex::new(Vec::new())), + inhibitor_fds: Arc::new(Mutex::new(HashMap::new())), + next_inhibitor_id: Arc::new(AtomicU64::new(1)), connection: Arc::new(Mutex::new(None)), seat_announced: Arc::new(AtomicBool::new(false)), dead_senders: Arc::new(Mutex::new(HashSet::new())), @@ -94,19 +94,46 @@ impl LoginManager { if let Ok(mut dead) = self.dead_senders.lock() { dead.insert(vanished_sender.to_owned()); } - let mut removed = 0usize; + let mut removed_ids: Vec = Vec::new(); if let Ok(mut runtime) = self.runtime.write() { - let before = runtime.inhibitors.len(); - runtime.inhibitors - .retain(|e| e.sender.as_deref() != Some(vanished_sender)); - removed = before - runtime.inhibitors.len(); + runtime.inhibitors.retain(|e| { + let matches = e.sender.as_deref() == Some(vanished_sender); + if matches { + removed_ids.push(e.id); + } + !matches + }); } if let Ok(mut fds) = self.inhibitor_fds.lock() { - fds.retain(|(s, _)| s.as_deref() != Some(vanished_sender)); + for id in &removed_ids { + fds.remove(id); + } } - if removed > 0 { + if !removed_ids.is_empty() { eprintln!( - "redbear-sessiond: reaped {removed} inhibitor(s) for vanished bus name '{vanished_sender}'" + "redbear-sessiond: reaped {} inhibitor(s) for vanished bus name '{vanished_sender}'", + removed_ids.len() + ); + } + } + + /// Remove a single inhibitor identified by its unique inhibitor ID. + /// Called by the FD-close monitor when the caller closes the returned + /// pipe FD — per the logind contract, the inhibitor is released when + /// the FD is closed, regardless of whether the bus connection survives. + pub fn reap_inhibitor_by_id(&self, inhibitor_id: u64) { + let mut removed = false; + if let Ok(mut runtime) = self.runtime.write() { + let before = runtime.inhibitors.len(); + runtime.inhibitors.retain(|e| e.id != inhibitor_id); + removed = runtime.inhibitors.len() < before; + } + if removed { + if let Ok(mut fds) = self.inhibitor_fds.lock() { + fds.remove(&inhibitor_id); + } + eprintln!( + "redbear-sessiond: reaped inhibitor {inhibitor_id} (caller FD closed)" ); } } @@ -193,10 +220,13 @@ impl LoginManager { let fd_caller: StdOwnedFd = end_caller.into(); let fd_daemon: StdOwnedFd = end_daemon.into(); + let inhibitor_id = self.next_inhibitor_id.fetch_add(1, Ordering::Relaxed); + let uid = self.runtime_read().map(|r| r.uid).unwrap_or(0); let pid = std::process::id(); let entry = InhibitorEntry { + id: inhibitor_id, what: what.to_owned(), who: who.to_owned(), why: why.to_owned(), @@ -210,12 +240,29 @@ impl LoginManager { runtime.inhibitors.push(entry); } + let daemon_raw_fd = fd_daemon.as_raw_fd(); + if let Ok(mut fds) = self.inhibitor_fds.lock() { - fds.push((sender, fd_daemon)); + fds.insert(inhibitor_id, fd_daemon); + } + + if let Ok(handle) = tokio::runtime::Handle::try_current() { + let manager_clone = self.clone(); + handle.spawn_blocking(move || { + let mut pfd = libc::pollfd { + fd: daemon_raw_fd, + events: 0, + revents: 0, + }; + let _ = unsafe { libc::poll(&mut pfd, 1, -1) }; + if pfd.revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 { + manager_clone.reap_inhibitor_by_id(inhibitor_id); + } + }); } eprintln!( - "redbear-sessiond: Inhibit(what={what}, who={who}, mode={mode}) granted" + "redbear-sessiond: Inhibit(what={what}, who={who}, mode={mode}) granted as inhibitor #{inhibitor_id}" ); Ok(OwnedFd::from(fd_caller)) @@ -981,6 +1028,7 @@ mod tests { fn block_inhibited_joins_what_fields() { let runtime = shared_runtime(); runtime.write().expect("lock").inhibitors.push(InhibitorEntry { + id: 1, what: String::from("sleep"), who: String::from("app1"), why: String::from("r"), @@ -990,6 +1038,7 @@ mod tests { sender: None, }); runtime.write().expect("lock").inhibitors.push(InhibitorEntry { + id: 2, what: String::from("shutdown"), who: String::from("app2"), why: String::from("r"), @@ -1306,7 +1355,7 @@ mod tests { let fds = manager.inhibitor_fds.lock().expect("lock"); assert_eq!(fds.len(), 1); - assert_eq!(fds[0].0.as_deref(), Some(":1.20")); + assert!(fds.values().count() == 1); } #[test] @@ -1385,4 +1434,125 @@ mod tests { let dead = manager.dead_senders.lock().expect("lock"); assert!(dead.contains(":1.77")); } + + #[tokio::test] + async fn closing_caller_fd_removes_inhibitor() { + let runtime = shared_runtime(); + let manager = LoginManager::new( + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/session/c1")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/seat/seat0")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/user/current")).unwrap(), + runtime.clone(), + ); + + let caller_fd = manager + .inhibit_impl("sleep", "app", "r", "block", Some(String::from(":1.42"))) + .expect("inhibit"); + + assert_eq!(runtime.read().expect("lock").inhibitors.len(), 1); + assert_eq!(manager.inhibitor_fds.lock().expect("lock").len(), 1); + + drop(caller_fd); + + for _ in 0..50 { + if runtime.read().expect("lock").inhibitors.is_empty() { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + + assert_eq!( + runtime.read().expect("lock").inhibitors.len(), + 0, + "inhibitor must be reaped after caller FD closes" + ); + assert_eq!( + manager.inhibitor_fds.lock().expect("lock").len(), + 0, + "daemon FD must be removed after caller FD closes" + ); + } + + #[tokio::test] + async fn multiple_inhibitors_same_sender_independent_fd_close() { + let runtime = shared_runtime(); + let manager = LoginManager::new( + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/session/c1")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/seat/seat0")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/user/current")).unwrap(), + runtime.clone(), + ); + + let fd1 = manager + .inhibit_impl("sleep", "app1", "r", "block", Some(String::from(":1.10"))) + .expect("inhibit"); + let fd2 = manager + .inhibit_impl("shutdown", "app2", "r", "delay", Some(String::from(":1.10"))) + .expect("inhibit"); + + assert_eq!(runtime.read().expect("lock").inhibitors.len(), 2); + + drop(fd1); + + for _ in 0..50 { + let guard = runtime.read().expect("lock"); + if guard.inhibitors.len() == 1 { + assert_eq!(guard.inhibitors[0].what, "shutdown"); + break; + } + drop(guard); + tokio::time::sleep(Duration::from_millis(10)).await; + } + + { + let guard = runtime.read().expect("lock"); + assert_eq!(guard.inhibitors.len(), 1, "only one inhibitor should remain"); + assert_eq!(guard.inhibitors[0].what, "shutdown"); + } + + drop(fd2); + + for _ in 0..50 { + if runtime.read().expect("lock").inhibitors.is_empty() { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + + assert_eq!( + runtime.read().expect("lock").inhibitors.len(), + 0, + "all inhibitors should be reaped after both FDs close" + ); + } + + #[test] + fn reap_inhibitor_by_id_removes_specific_entry() { + let runtime = shared_runtime(); + let manager = LoginManager::new( + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/session/c1")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/seat/seat0")).unwrap(), + OwnedObjectPath::try_from(String::from("/org/freedesktop/login1/user/current")).unwrap(), + runtime.clone(), + ); + + let _fd1 = manager + .inhibit_impl("sleep", "app1", "r", "block", Some(String::from(":1.10"))) + .expect("inhibit"); + let _fd2 = manager + .inhibit_impl("shutdown", "app2", "r", "delay", Some(String::from(":1.20"))) + .expect("inhibit"); + + let ids: Vec = { + let guard = runtime.read().expect("lock"); + guard.inhibitors.iter().map(|e| e.id).collect() + }; + assert_eq!(ids.len(), 2); + + manager.reap_inhibitor_by_id(ids[0]); + + let guard = runtime.read().expect("lock"); + assert_eq!(guard.inhibitors.len(), 1); + assert_eq!(guard.inhibitors[0].id, ids[1]); + } } diff --git a/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs b/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs index 1c49ff2468..336337dcf1 100644 --- a/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs +++ b/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs @@ -5,6 +5,10 @@ use std::sync::{ #[derive(Clone, Debug)] pub struct InhibitorEntry { + /// Unique monotonic inhibitor ID assigned by the manager. Used as the + /// key for the daemon-side FD map so that FD-close detection can remove + /// a single specific inhibitor entry in O(1), independent of sender. + pub id: u64, pub what: String, pub who: String, pub why: String,