From e74a054bd314efece879addcef9e75cec6bff522 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Mon, 2 Nov 2026 22:13:39 -0500 Subject: [PATCH] fix(sandbox): harden managed-children registry and workload launcher Recover the managed-children registry from a poisoned lock in unregister, like its sibling accessors already do, so a PID whose waiter panicked while holding the lock cannot stay registered forever and be skipped by the orphan reaper after the PID is reused. Classify a missing /proc/self/task//children file instead of treating every NotFound as a vanished task. When no task in a scan exposes the file (kernel built without CONFIG_PROC_CHILDREN, or restricted procfs), log a warning and emit an OCSF detection finding once per process instead of silently leaving the orphan reaper inert while zombies accumulate. Contain a panicking job on the workload launcher thread, the way the network broker contains handler panics: the panic is reported, the thread leaves its job loop, and the health flag turns false so every later spawn fails fast instead of hanging on a launcher that still reports healthy. Closes #5186 Signed-off-by: Emilien Macchi --- .../src/linux/workload_launcher.rs | 97 ++++- .../openshell-sandbox/src/managed_children.rs | 333 +++++++++++++++++- 2 files changed, 411 insertions(+), 19 deletions(-) diff --git a/crates/openshell-sandbox/src/linux/workload_launcher.rs b/crates/openshell-sandbox/src/linux/workload_launcher.rs index 51b3af629e..8a8b6529e1 100644 --- a/crates/openshell-sandbox/src/linux/workload_launcher.rs +++ b/crates/openshell-sandbox/src/linux/workload_launcher.rs @@ -92,7 +92,16 @@ pub fn start() -> io::Result<(WorkloadLauncher, NotificationListener)> { } } while let Ok(job) = jobs_rx.recv() { - job(); + // Contain a job panic the way the network broker contains a + // handler panic. The launcher thread owns the inherited + // listener filter, and its state past a mid-job panic is not + // known-good, so stop accepting work: the health flag turns + // false and every later spawn fails fast instead of hanging + // on a launcher that reports healthy. + if let Err(panic) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(job)) { + tracing::error!("workload launcher job panicked: {}", panic_message(&*panic)); + break; + } } thread_alive.store(false, Ordering::Release); }) @@ -113,11 +122,22 @@ pub fn start() -> io::Result<(WorkloadLauncher, NotificationListener)> { )) } +/// Render a caught panic payload for a log message. +fn panic_message(panic: &(dyn std::any::Any + Send)) -> String { + panic + .downcast_ref::() + .map(String::as_str) + .or_else(|| panic.downcast_ref::<&'static str>().copied()) + .unwrap_or("non-string panic payload") + .to_string() +} + #[cfg(test)] #[allow(unsafe_code)] mod tests { use std::mem::size_of; use std::os::fd::AsRawFd as _; + use std::process::Command; use super::*; @@ -127,7 +147,7 @@ mod tests { let executable = std::env::current_exe().expect("test executable"); let mut child = launcher .execute(move || { - let mut command = std::process::Command::new(executable); + let mut command = Command::new(executable); command .arg("--exact") .arg("linux::workload_launcher::tests::inherited_listener_child") @@ -173,6 +193,79 @@ mod tests { assert!(listener.as_raw_fd() >= 0); } + #[test] + fn panicking_job_marks_the_launcher_unhealthy() { + const HELPER_ENV: &str = "OPENSHELL_LAUNCHER_PANIC_TEST_HELPER"; + if std::env::var_os(HELPER_ENV).is_none() { + // The panic hook's backtrace symbolization opens files on the + // launcher thread, whose user-notify filter traps opens. Nothing + // services that listener here, so run with backtraces off; the + // parent environment must not leak RUST_BACKTRACE=1 into the + // child. + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "linux::workload_launcher::tests::panicking_job_marks_the_launcher_unhealthy", + "--nocapture", + ]) + .env(HELPER_ENV, "1") + .env("RUST_BACKTRACE", "0") + .output() + .expect("start isolated launcher panic test"); + assert!( + output.status.success(), + "launcher panic test failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let (launcher, _listener) = start().expect("start launcher"); + assert!(launcher.is_alive()); + + let failure = launcher.execute(|| panic!("launcher job failed")); + assert!( + failure.is_err(), + "a panicking job must fail its caller, not hang it" + ); + + // The health flag flips when the launcher thread leaves its job loop, + // after the caller has already observed the failure. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while launcher.is_alive() { + assert!( + std::time::Instant::now() < deadline, + "the launcher never reported the job panic" + ); + thread::sleep(std::time::Duration::from_millis(10)); + } + + let failure = launcher + .execute(|| ()) + .expect_err("the launcher must refuse work after a job panic"); + assert_eq!(failure.kind(), io::ErrorKind::BrokenPipe); + assert!( + failure.to_string().contains("not running"), + "unexpected refusal error: {failure}" + ); + } + + #[test] + fn panic_message_renders_every_payload_shape() { + #[derive(Debug)] + struct CustomPayload; + + // A job that panics through a format argument carries a String payload. + let formatted = "job failed after 3 attempts".to_string(); + assert_eq!(panic_message(&formatted), "job failed after 3 attempts"); + + // `panic!` with a literal carries a &'static str payload. + assert_eq!(panic_message(&"literal panic"), "literal panic"); + + // Any other payload type falls back to the placeholder. + assert_eq!(panic_message(&CustomPayload), "non-string panic payload"); + } + #[test] fn inherited_listener_child() { if std::env::var_os("OPENSHELL_WORKLOAD_LAUNCHER_CHILD").is_none() { diff --git a/crates/openshell-sandbox/src/managed_children.rs b/crates/openshell-sandbox/src/managed_children.rs index adbfdb62de..4d0eae7633 100644 --- a/crates/openshell-sandbox/src/managed_children.rs +++ b/crates/openshell-sandbox/src/managed_children.rs @@ -14,7 +14,7 @@ use std::collections::{HashMap, HashSet}; use std::io; use std::sync::atomic::AtomicI32; -use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::{LazyLock, Mutex, MutexGuard}; use std::time::Duration; @@ -22,6 +22,7 @@ static MANAGED_CHILDREN: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); static NEXT_GENERATION: AtomicU64 = AtomicU64::new(1); static REAPER_EVENT_FD: AtomicI32 = AtomicI32::new(-1); +static PROC_CHILDREN_SUPPORT_REPORTED: AtomicBool = AtomicBool::new(false); const REAPER_RECOVERY_INTERVAL: Duration = Duration::from_secs(30); #[cfg(test)] static REAPER_SCAN_COUNT: AtomicU64 = AtomicU64::new(0); @@ -74,6 +75,14 @@ impl RegistryGuard { pub fn contains(&self, pid: i32) -> bool { self.0.contains_key(&pid) } + + /// Remove exactly this supervised-child registration. A newer + /// registration for a reused PID is preserved. + pub fn unregister(&mut self, child: ManagedChild) { + if self.0.get(&child.pid) == Some(&child.generation) { + self.0.remove(&child.pid); + } + } } /// Lock the registry for an atomic spawn-and-register or inspect-and-reap @@ -94,11 +103,7 @@ pub fn register(pid: u32) -> Option { /// Remove exactly this supervised-child registration. A newer registration /// for a reused PID is preserved. pub fn unregister(child: ManagedChild) { - if let Ok(mut children) = MANAGED_CHILDREN.lock() - && children.get(&child.pid) == Some(&child.generation) - { - children.remove(&child.pid); - } + lock().unregister(child); } /// Return `true` if `pid` is currently in the supervised-child set. @@ -227,23 +232,113 @@ fn reap_unmanaged_children_once() -> io::Result { } fn direct_child_pids() -> io::Result> { + scan_task_children( + std::path::Path::new("/proc/self/task"), + &PROC_CHILDREN_SUPPORT_REPORTED, + ) +} + +/// Collect the child PIDs listed by every task directory below `task_root`. +/// +/// Missing children support is reported through `reported` (see +/// [`report_missing_support_once`]). +fn scan_task_children( + task_root: &std::path::Path, + reported: &AtomicBool, +) -> io::Result> { let mut children = HashSet::new(); - for task in std::fs::read_dir("/proc/self/task")? { + let mut saw_children_file = false; + let mut saw_task_without_children = false; + for task in std::fs::read_dir(task_root)? { let task = task?; - let path = task.path().join("children"); - let contents = match std::fs::read_to_string(path) { - Ok(contents) => contents, - Err(error) if error.kind() == io::ErrorKind::NotFound => continue, - Err(error) => return Err(error), - }; - children.extend( + match read_task_children(&task.path())? { + TaskChildren::Pids(pids) => { + saw_children_file = true; + children.extend(pids); + } + TaskChildren::VanishedTask => {} + TaskChildren::UnsupportedKernel => saw_task_without_children = true, + } + } + // Corroborate before reporting: an environment without children-file + // support exposes no children file for any task, while a teardown or + // TID-reuse race on a supported kernel strands at most one task + // directory without its file while the rest still list theirs. + if saw_task_without_children && !saw_children_file { + report_missing_support_once(reported); + } + Ok(children) +} + +/// The `children` entries of one task directory. +#[derive(Debug, PartialEq, Eq)] +enum TaskChildren { + /// Positive child PIDs listed by the kernel. + Pids(Vec), + /// The task exited while the directory listing was being walked. + VanishedTask, + /// No `children` file is exposed for a task directory that still exists, + /// so children cannot be discovered from procfs. + UnsupportedKernel, +} + +fn read_task_children(task_dir: &std::path::Path) -> io::Result { + match std::fs::read_to_string(task_dir.join("children")) { + Ok(contents) => Ok(TaskChildren::Pids( contents .split_ascii_whitespace() .filter_map(|value| value.parse::().ok()) - .filter(|pid| *pid > 0), - ); + .filter(|pid| *pid > 0) + .collect(), + )), + Err(error) if error.kind() == io::ErrorKind::NotFound => { + if task_dir.try_exists()? { + Ok(TaskChildren::UnsupportedKernel) + } else { + Ok(TaskChildren::VanishedTask) + } + } + Err(error) => Err(error), } - Ok(children) +} + +/// Emit the missing-support warning and detection finding the first time +/// `reported` flips from false to true, and do nothing afterwards. +fn report_missing_support_once(reported: &AtomicBool) { + if reported + .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed) + .is_err() + { + return; + } + tracing::warn!( + "no /proc/self/task/*/children: the orphan reaper cannot discover \ + children (kernel built without CONFIG_PROC_CHILDREN, or restricted \ + procfs such as gVisor or a subset=pid mount)" + ); + openshell_ocsf::ocsf_emit!( + openshell_ocsf::DetectionFindingBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(openshell_ocsf::ActivityId::Open) + .severity(openshell_ocsf::SeverityId::Medium) + .confidence(openshell_ocsf::ConfidenceId::High) + .is_alert(true) + .finding_info( + openshell_ocsf::FindingInfo::new( + "proc-children-unavailable", + "Sandbox Orphan Reaper Unavailable", + ) + .with_desc( + "No task under /proc/self/task exposes a children file, so \ + the sandbox orphan reaper cannot discover unmanaged \ + children, and exited descendants accumulate as zombies \ + until the process limit. The likely causes are a kernel \ + built without CONFIG_PROC_CHILDREN or restricted procfs \ + (for example gVisor or a subset=pid mount).", + ) + ) + .message("orphan reaper unavailable: no /proc/self/task/*/children") + .build() + ); } #[cfg(test)] @@ -251,9 +346,34 @@ mod tests { use super::*; use nix::sys::wait::{Id, WaitPidFlag, WaitStatus, waitid, waitpid}; use nix::unistd::Pid; + use std::io::Write; + use std::os::unix::fs::PermissionsExt; use std::process::Command; + use std::sync::Arc; use std::sync::mpsc; use std::time::{Duration, Instant}; + use tracing_subscriber::layer::SubscriberExt; + + /// Sink for the plain `tracing` events emitted alongside OCSF events. + #[derive(Clone, Default)] + struct CapturedLogs(Arc>>); + + impl Write for CapturedLogs { + fn write(&mut self, bytes: &[u8]) -> io::Result { + self.0.lock().expect("log buffer").extend_from_slice(bytes); + Ok(bytes.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + impl CapturedLogs { + fn text(&self) -> String { + String::from_utf8_lossy(&self.0.lock().expect("log buffer")).into_owned() + } + } fn fork_exiting_child() -> i32 { // SAFETY: the child does not run Rust code after fork; it exits through @@ -440,6 +560,53 @@ mod tests { assert!(!is_managed(i32::try_from(pid).expect("test pid"))); } + #[test] + fn unregister_recovers_from_a_poisoned_registry() { + let pid = 1_000_003_u32; + let child = lock().register(pid).expect("register child"); + + // Poison the registry mutex exactly as a panicking holder would. The + // deliberate panic prints through the default hook; swapping the + // process-global hook here would swallow the diagnostics of a + // sibling test that fails at the same moment. + let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _registry = lock(); + panic!("poison the managed-children registry"); + })); + + unregister(child); + assert!(!is_managed(i32::try_from(pid).expect("test pid"))); + } + + #[test] + fn task_children_classification_distinguishes_missing_support() { + let dir = tempfile::tempdir().expect("create procfs stand-in"); + + // A task directory without a children file: the kernel was built + // without CONFIG_PROC_CHILDREN. + let unsupported = dir.path().join("1234"); + std::fs::create_dir_all(&unsupported).expect("create task directory"); + assert_eq!( + read_task_children(&unsupported).expect("read unsupported task"), + TaskChildren::UnsupportedKernel + ); + + // A live task directory with parseable and unparseable entries. + std::fs::write(unsupported.join("children"), "123 456 0 -7 not-a-pid\n") + .expect("write children file"); + assert_eq!( + read_task_children(&unsupported).expect("read children"), + TaskChildren::Pids(vec![123, 456]) + ); + + // A task directory that vanished mid-scan. + let vanished = dir.path().join("5678"); + assert_eq!( + read_task_children(&vanished).expect("read vanished task"), + TaskChildren::VanishedTask + ); + } + #[test] fn child_pid_parser_observes_a_live_child() { let mut child = Command::new("sleep") @@ -454,4 +621,136 @@ mod tests { child.kill().expect("kill child"); child.wait().expect("wait for child"); } + + #[test] + fn missing_proc_children_support_is_reported_once() { + let capture = openshell_ocsf::test_support::OcsfCapture::default(); + let logs = CapturedLogs::default(); + let writer = logs.clone(); + let _guard = tracing::subscriber::set_default( + tracing_subscriber::registry().with(capture.clone()).with( + tracing_subscriber::fmt::layer() + .with_writer(move || writer.clone()) + .with_ansi(false), + ), + ); + + // A private latch: the process-wide one is shared with every other + // test that scans the real /proc/self/task, so asserting on it would + // depend on parallel test ordering. + let reported = AtomicBool::new(false); + report_missing_support_once(&reported); + report_missing_support_once(&reported); + + let events = capture.json_events(); + assert_eq!( + events.len(), + 1, + "the kernel condition must be reported exactly once: {events:?}" + ); + let event = &events[0]; + assert_eq!(event["class_uid"], 2004); + assert_eq!(event["activity_id"], 1); + assert_eq!(event["severity_id"], 3); + assert_eq!(event["severity"], "Medium"); + assert_eq!(event["confidence_id"], 3); + assert_eq!(event["confidence"], "High"); + assert_eq!(event["is_alert"].as_bool(), Some(true)); + assert_eq!(event["finding_info"]["uid"], "proc-children-unavailable"); + assert_eq!( + event["finding_info"]["title"], + "Sandbox Orphan Reaper Unavailable" + ); + assert!( + event["finding_info"]["desc"] + .as_str() + .is_some_and(|desc| desc.contains("CONFIG_PROC_CHILDREN")), + "finding description: {event:?}" + ); + assert!( + event["message"] + .as_str() + .is_some_and(|message| message.contains("/proc/self/task/*/children")), + "finding message: {event:?}" + ); + + let warnings = logs.text().matches("CONFIG_PROC_CHILDREN").count(); + assert_eq!( + warnings, + 1, + "the warning must also be emitted once: {}", + logs.text() + ); + } + + #[test] + fn scan_reports_missing_support_only_when_no_task_lists_children() { + let capture = openshell_ocsf::test_support::OcsfCapture::default(); + let logs = CapturedLogs::default(); + let writer = logs.clone(); + let _guard = tracing::subscriber::set_default( + tracing_subscriber::registry().with(capture.clone()).with( + tracing_subscriber::fmt::layer() + .with_writer(move || writer.clone()) + .with_ansi(false), + ), + ); + + let dir = tempfile::tempdir().expect("create procfs stand-in"); + + // A task tree where no task exposes a children file: the whole + // environment lacks children-file support, so the scan reports it. + for tid in ["1234", "5678"] { + std::fs::create_dir(dir.path().join(tid)).expect("create task directory"); + } + let reported = AtomicBool::new(false); + let pids = scan_task_children(dir.path(), &reported).expect("scan unsupported tree"); + assert!(pids.is_empty(), "no children can be listed: {pids:?}"); + assert_eq!( + capture.json_events().len(), + 1, + "a fully unsupported task tree must be reported" + ); + + // A tree where some task still lists its children is a supported + // environment that lost a task mid-scan: no report. + std::fs::write(dir.path().join("1234").join("children"), "42\n") + .expect("write children file"); + let reported = AtomicBool::new(false); + let pids = scan_task_children(dir.path(), &reported).expect("scan mixed tree"); + assert_eq!(pids, HashSet::from([42])); + assert_eq!( + capture.json_events().len(), + 1, + "a mixed task tree must not be reported as unsupported" + ); + + let warnings = logs.text().matches("/proc/self/task/*/children").count(); + assert_eq!(warnings, 1, "exactly one warning: {}", logs.text()); + } + + #[test] + fn unreadable_task_directory_surfaces_its_error() { + // Root bypasses directory permissions, so the read cannot be made to + // fail. + if rustix::process::geteuid().is_root() { + return; + } + + let dir = tempfile::tempdir().expect("create procfs stand-in"); + let task = dir.path().join("9012"); + std::fs::create_dir(&task).expect("create task directory"); + std::fs::set_permissions(&task, std::fs::Permissions::from_mode(0o000)) + .expect("make task directory unreadable"); + + let outcome = read_task_children(&task); + // Restore search permission first so the stand-in directory can be + // removed even when the assertions below fail. + std::fs::set_permissions(&task, std::fs::Permissions::from_mode(0o700)) + .expect("restore task directory permissions"); + + let error = outcome + .expect_err("an unreadable task directory must surface its error, not classify it"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } }