From 58e6413bcc6bd4d83819c9463e916dd726253b17 Mon Sep 17 00:00:00 2001 From: Olabode Olaoke Date: Fri, 4 Sep 2026 08:17:59 -0600 Subject: [PATCH 1/5] SEC-007: Harden backend process recovery Prevent stale identifiers and mutable record paths from authorizing process signals or cleanup of the wrong filesystem object. Co-authored-by: Fuzzy <644e8093c651dbf16ecec80095552ed2e9180ec9e3de2f5f4f0d86ecfbe9d5f5@buzz.block.builderlab.xyz> Signed-off-by: Olabode Olaoke --- src-tauri/Cargo.lock | 1 + src-tauri/Cargo.toml | 7 + src-tauri/src/lib.rs | 4 +- src-tauri/src/services/acp/goose_serve.rs | 1298 ++++++++--- src-tauri/src/services/acp/mod.rs | 1 + .../src/services/acp/process_record_store.rs | 2019 +++++++++++++++++ src-tauri/src/services/process.rs | 74 +- 7 files changed, 3070 insertions(+), 334 deletions(-) create mode 100644 src-tauri/src/services/acp/process_record_store.rs diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index a8a7b3e12..638426ab6 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -17,6 +17,7 @@ dependencies = [ "dirs", "doctor", "dunce", + "errno", "etcetera 0.11.0", "fern", "flate2", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 69f2915fb..1749e84a3 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -46,6 +46,7 @@ ignore = "0.4.25" fern = "0.7" infer = "0.19.0" libc = "0.2" +errno = "0.3" log = "0.4.29" mime_guess = "2" futures-util = "0.3" @@ -88,10 +89,16 @@ zip = { version = "2", default-features = false, features = ["deflate"] } [target.'cfg(windows)'.dependencies] keyring = { version = "3.6.3", default-features = false, features = ["windows-native"] } windows-sys = { version = "0.59", features = [ + "Wdk_Foundation", + "Wdk_Storage_FileSystem", "Win32_Foundation", "Win32_Globalization", + "Win32_Security", + "Win32_Security_Authorization", "Win32_Storage_FileSystem", "Win32_System_Com", + "Win32_System_IO", + "Win32_System_Memory", "Win32_System_Threading", "Win32_UI_Shell", ] } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index ddbda54f4..46851d5c0 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -755,7 +755,9 @@ pub fn run() { .stop_for_app_exit(); app.state::() .kill_all_tunnels(); - services::acp::goose_serve::GooseServeProcess::kill_singleton(); + tauri::async_runtime::block_on( + services::acp::goose_serve::GooseServeProcess::kill_singleton(), + ); } #[cfg(target_os = "macos")] RunEvent::Reopen { .. } => { diff --git a/src-tauri/src/services/acp/goose_serve.rs b/src-tauri/src/services/acp/goose_serve.rs index 69f145e17..fe86cadf9 100644 --- a/src-tauri/src/services/acp/goose_serve.rs +++ b/src-tauri/src/services/acp/goose_serve.rs @@ -5,12 +5,12 @@ use crate::commands::runtime_config::{ local_byo_key_providers_enabled, RuntimeConfig, RuntimeConfigState, }; use std::collections::HashMap; -use std::io::Write; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; +use super::process_record_store::ProcessRecordStore; use crate::services::diagnostic_log::{ self, DiagnosticCategory, DiagnosticFieldValue, DiagnosticLevel, }; @@ -22,11 +22,7 @@ use crate::services::log_redaction::redact_log_line; use crate::services::managed_acp_tools; use crate::services::path_env; #[cfg(unix)] -use crate::services::process::ProcessId; -#[cfg(unix)] -use crate::services::process::{kill_process, terminate_process}; -use crate::services::process::{pid_t_from_u32, process_is_alive}; -#[cfg(windows)] +use crate::services::process::{kill_process, pid_t_from_u32, terminate_process}; use crate::services::process::{IdentityProbe, ProcessIdentity}; use tokio::io::{AsyncBufReadExt, AsyncRead, BufReader}; @@ -56,8 +52,10 @@ const GOOSE_FAST_MODEL_ENV: &str = "GOOSE_FAST_MODEL"; pub struct GooseServeProcess { port: u16, secret_key: String, - process_record_dir: PathBuf, - _child: Child, + process_record_store: ProcessRecordStore, + process_record_path: PathBuf, + process_record: super::process_record_store::VerifiedRecord, + child: tokio::sync::Mutex, } /// Global singleton — initialised once at app startup. @@ -87,60 +85,31 @@ impl GooseServeProcess { .await } - /// Kill the child process. Called from the app exit handler to ensure - /// the child doesn't outlive the Tauri process. - pub fn kill(&self) { - #[cfg(unix)] - if let Some(child_pid) = self._child.id() { - match pid_t_from_u32(child_pid) { - Some(pid) => { - log::info!("Killing goose serve child (pid {child_pid})"); - terminate_process(pid); - } - None => { - log::warn!( - "Skipping goose serve child kill because pid {child_pid} is outside pid_t range" - ); - } - } - } - - #[cfg(windows)] - let remove_process_record = if let Some(handle) = self._child.raw_handle() { - log::info!("Killing goose serve child through its retained process handle"); - // SAFETY: Tokio owns this process handle for the lifetime of `_child`. - match unsafe { - crate::services::process::terminate_process_handle(handle, Duration::from_secs(5)) - } { - Ok(()) => true, - Err(error) => { - log::warn!( - "Failed to stop goose serve child: {error}; keeping process record for recovery" - ); - false + /// Terminate and reap the exact retained child, then remove its exact + /// recovery record. If exit cannot be confirmed, keep the record for the + /// next startup recovery pass. + pub async fn kill(&self) { + let mut child = self.child.lock().await; + match stop_child_and_reap(&mut child).await { + Ok(()) => { + if let Err(error) = self + .process_record_store + .remove_verified(&self.process_record_path, &self.process_record) + { + log::warn!("Failed to remove confirmed-exit goose serve record: {error}"); } } - } else { - log::warn!( - "Cannot stop goose serve child through its retained handle; keeping process record for recovery" - ); - false - }; - - #[cfg(unix)] - let remove_process_record = true; - - // Keep recovery evidence until child exit has been confirmed on Windows. - if remove_process_record { - let _ = std::fs::remove_file(process_record_path(&self.process_record_dir)); + Err(error) => log::warn!( + "Failed to stop and reap goose serve child: {error}; keeping process record for recovery" + ), } } /// Kill the singleton goose serve process if it exists. Called from the /// app exit handler. - pub fn kill_singleton() { + pub async fn kill_singleton() { if let Some(process) = GOOSE_SERVE.get() { - process.kill(); + process.kill().await; } } @@ -149,10 +118,20 @@ impl GooseServeProcess { // Kill any orphaned goose serve process left by a previous run // (e.g. tauri dev hot-reload). - let process_record_dir = + let process_record_dir = if let Some(dir) = crate::services::e2e_mode::E2eMode::process_record_dir_for(&app_handle) - .unwrap_or_else(|| std::env::temp_dir().join(PROCESS_RECORD_DIR_NAME)); - kill_stale_serve_process(&process_record_dir).await; + { + dir + } else { + app_handle + .path() + .app_data_dir() + .map_err(|error| format!("Failed to resolve app data directory: {error}"))? + .join("processes") + .join(PROCESS_RECORD_DIR_NAME) + }; + let process_record_store = ProcessRecordStore::open(process_record_dir)?; + kill_stale_serve_process(&process_record_store).await; let port = reserve_free_port()?; let secret_key = format!("berd-{}", uuid::Uuid::new_v4().simple()); @@ -282,6 +261,8 @@ impl GooseServeProcess { ) })?; let pid = child.id(); + let process_record_path = + process_record_store.new_record_path(std::process::id(), current_executable_hash()); diagnostic_log::record_event( DiagnosticLevel::Info, DiagnosticCategory::GooseServe, @@ -290,27 +271,29 @@ impl GooseServeProcess { diagnostic_log::fields([("pid", optional_u32_value(pid)), ("port", port.into())]), ); + #[cfg(unix)] + { + let publication = write_pid_file(&process_record_store, &process_record_path, &child); + if publication.is_err() { + log::warn!("Failed to publish goose serve recovery record; stopping child and failing startup"); + } + require_published_record(&mut child, publication).await?; + } + #[cfg(windows)] - if let Err(error) = write_process_record(&process_record_dir, &child) { - log::warn!( - "Failed to publish goose serve recovery record: {error}; stopping child and failing startup" - ); - if let Some(handle) = child.raw_handle() { - // SAFETY: Tokio owns this process handle for the lifetime of `child`. - if let Err(stop_error) = unsafe { - crate::services::process::terminate_process_handle( - handle, - Duration::from_secs(5), - ) - } { - log::warn!("Failed to stop recordless goose serve child: {stop_error}"); - } + { + let publication = + write_process_record(&process_record_store, &process_record_path, &child); + if publication.is_err() { + log::warn!("Failed to publish goose serve recovery record; stopping child and failing startup"); } - return Err(format!( - "Failed to publish goose serve recovery record: {error}" - )); + require_published_record(&mut child, publication).await?; } + let process_record = + retain_published_record(&process_record_store, &process_record_path, &mut child) + .await?; + spawn_log_reader(child.stdout.take(), "stdout"); spawn_log_reader(child.stderr.take(), "stderr"); @@ -340,22 +323,27 @@ impl GooseServeProcess { ("port", port.into()), ]), ); + teardown_published_child( + &process_record_store, + &process_record_path, + Some(&process_record), + &mut child, + &error, + ) + .await; return Err(error); } } log::info!("Goose serve is ready on port {port}"); - #[cfg(unix)] - if let Some(pid) = pid { - write_pid_file(&process_record_dir, pid); - } - Ok(GooseServeProcess { port, secret_key, - process_record_dir, - _child: child, + process_record_store, + process_record_path, + process_record, + child: tokio::sync::Mutex::new(child), }) } } @@ -416,21 +404,173 @@ const PROCESS_RECORD_EXTENSION: &str = "json"; struct ServeProcessRecord { owner_pid: u32, serve_pid: u32, - #[cfg(windows)] #[serde(default)] owner_identity: Option, - #[cfg(windows)] #[serde(default)] serve_identity: Option, } -fn process_record_path(dir: &Path) -> PathBuf { - let exe = std::env::current_exe().unwrap_or_default(); - let exe_hash = fnv1a(exe.to_string_lossy().as_bytes()); - dir.join(format!( - "{}-{exe_hash:016x}.{PROCESS_RECORD_EXTENSION}", - std::process::id() - )) +fn current_executable_hash() -> u64 { + let executable = std::env::current_exe().unwrap_or_default(); + fnv1a(executable.to_string_lossy().as_bytes()) +} + +const CHILD_TEARDOWN_TIMEOUT: Duration = Duration::from_secs(5); +#[cfg(unix)] +const CHILD_GRACEFUL_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(2); + +async fn bounded_child_teardown(timeout: Duration, teardown: F) -> Result<(), String> +where + F: std::future::Future>, +{ + tokio::time::timeout(timeout, teardown) + .await + .map_err(|_| format!("child teardown exceeded {} seconds", timeout.as_secs()))? +} + +async fn stop_child_and_reap(child: &mut Child) -> Result<(), String> { + bounded_child_teardown(CHILD_TEARDOWN_TIMEOUT, async { + #[cfg(unix)] + { + let pid = child.id().and_then(pid_t_from_u32).ok_or_else(|| { + "child has no valid process id for graceful termination".to_string() + })?; + if !terminate_process(pid) { + if child + .try_wait() + .map_err(|error| { + format!("failed to inspect child after SIGTERM error: {error}") + })? + .is_some() + { + return Ok(()); + } + } else { + match tokio::time::timeout(CHILD_GRACEFUL_SHUTDOWN_TIMEOUT, child.wait()).await { + Ok(Ok(_)) => return Ok(()), + Ok(Err(error)) => { + return Err(format!("failed to reap child after SIGTERM: {error}")); + } + Err(_) => {} + } + } + } + + let kill_error = child.kill().await.err(); + child.wait().await.map_err(|wait_error| match kill_error { + Some(kill_error) => { + format!("failed to kill child ({kill_error}) and reap it ({wait_error})") + } + None => format!("failed to reap child after forced termination: {wait_error}"), + })?; + Ok(()) + }) + .await +} + +async fn teardown_published_child_with( + store: &ProcessRecordStore, + path: &Path, + retained: Option<&super::process_record_store::VerifiedRecord>, + reason: &str, + teardown: F, +) where + F: std::future::Future>, +{ + finish_published_child_teardown(store, path, retained, reason, teardown.await); +} + +async fn teardown_published_child( + store: &ProcessRecordStore, + path: &Path, + retained: Option<&super::process_record_store::VerifiedRecord>, + child: &mut Child, + reason: &str, +) { + teardown_published_child_with(store, path, retained, reason, stop_child_and_reap(child)).await; +} + +fn finish_published_child_teardown( + store: &ProcessRecordStore, + path: &Path, + retained: Option<&super::process_record_store::VerifiedRecord>, + reason: &str, + teardown: Result<(), String>, +) { + match teardown { + Ok(()) => { + let Some(verified) = retained else { + log::warn!( + "{reason}; child exited, but the published recovery record was never retained, so exact cleanup is impossible; keeping recovery evidence" + ); + return; + }; + if let Err(error) = store.remove_verified(path, verified) { + log::warn!("{reason}; child exited, but exact record cleanup failed: {error}"); + } + } + Err(error) => log::warn!( + "{reason}; child teardown was not confirmed ({error}); keeping recovery record" + ), + } +} + +async fn retain_published_record_with( + store: &ProcessRecordStore, + path: &Path, + child: &mut Child, + retain: F, +) -> Result +where + F: FnOnce( + &ProcessRecordStore, + &Path, + ) -> Result< + super::process_record_store::VerifiedRecord, + super::process_record_store::VerifiedReadError, + >, +{ + match retain(store, path) { + Ok(record) => Ok(record), + Err(error) => { + let startup_error = format!( + "Failed to retain published goose serve recovery record {}: {}", + path.display(), + error.message + ); + teardown_published_child(store, path, error.verified.as_ref(), child, &startup_error) + .await; + Err(startup_error) + } + } +} + +async fn retain_published_record( + store: &ProcessRecordStore, + path: &Path, + child: &mut Child, +) -> Result { + retain_published_record_with(store, path, child, |store, path| { + store.read_verified_for_cleanup(path) + }) + .await +} + +async fn require_published_record( + child: &mut Child, + publication: Result<(), String>, +) -> Result<(), String> { + if let Err(error) = publication { + stop_child_and_reap(child).await.map_err(|teardown_error| { + format!( + "Failed to publish goose serve recovery record: {error}; child teardown also failed: {teardown_error}" + ) + })?; + return Err(format!( + "Failed to publish goose serve recovery record: {error}" + )); + } + Ok(()) } /// Legacy single-slot PID file used before per-owner process records. It is @@ -455,46 +595,42 @@ fn fnv1a(bytes: &[u8]) -> u64 { } #[cfg(unix)] -fn write_pid_file(dir: &Path, serve_pid: u32) { - if let Err(error) = std::fs::create_dir_all(dir) { - log::warn!( - "Failed to create goose serve process record dir {}: {error}", - dir.display() - ); - return; - } - - let path = process_record_path(dir); +fn write_pid_file(store: &ProcessRecordStore, path: &Path, child: &Child) -> Result<(), String> { + let serve_pid = child.id().ok_or_else(|| "child has no pid".to_string())?; + let owner_identity = crate::services::process::capture_process_identity(std::process::id()); + let serve_identity = crate::services::process::capture_process_identity(serve_pid); + #[cfg(not(target_os = "macos"))] + let (owner_identity, serve_identity) = ( + Some(owner_identity.map_err(|error| format!("failed to identify owner: {error}"))?), + Some(serve_identity.map_err(|error| format!("failed to identify child: {error}"))?), + ); + #[cfg(target_os = "macos")] + let (owner_identity, serve_identity) = match (owner_identity, serve_identity) { + (Ok(owner), Ok(serve)) => (Some(owner), Some(serve)), + _ => { + // macOS currently cannot bind executable vnode identity to a PID + // without a pathname race. Publish a deletion-only record so normal + // startup works, but stale recovery can never authorize signaling. + (None, None) + } + }; let record = ServeProcessRecord { owner_pid: std::process::id(), serve_pid, + owner_identity, + serve_identity, }; - match std::fs::File::create(&path) { - Ok(mut file) => { - if let Err(error) = serde_json::to_writer(&mut file, &record) { - log::warn!( - "Failed to write goose serve process record {}: {error}", - path.display() - ); - } - if let Err(error) = file.write_all(b"\n") { - log::warn!( - "Failed to finish goose serve process record {}: {error}", - path.display() - ); - } - } - Err(error) => { - log::warn!( - "Failed to create goose serve process record {}: {error}", - path.display() - ); - } - } + let serialized = serde_json::to_vec(&record) + .map_err(|error| format!("failed to serialize process record: {error}"))?; + store.publish(path, &serialized) } #[cfg(windows)] -fn write_process_record(dir: &Path, child: &Child) -> Result<(), String> { +fn write_process_record( + store: &ProcessRecordStore, + path: &Path, + child: &Child, +) -> Result<(), String> { let handle = child .raw_handle() .ok_or_else(|| "child has no process handle".to_string())?; @@ -503,13 +639,6 @@ fn write_process_record(dir: &Path, child: &Child) -> Result<(), String> { // SAFETY: Tokio owns this process handle for the lifetime of `child`. let serve_identity = unsafe { crate::services::process::process_identity_from_handle(handle) } .map_err(|error| format!("failed to identify child: {error}"))?; - std::fs::create_dir_all(dir).map_err(|error| { - format!( - "failed to create process record dir {}: {error}", - dir.display() - ) - })?; - let path = process_record_path(dir); let record = ServeProcessRecord { owner_pid: owner_identity.pid, serve_pid: serve_identity.pid, @@ -522,70 +651,28 @@ fn write_process_record(dir: &Path, child: &Child) -> Result<(), String> { path.display() ) })?; - let temp_path = path.with_extension(format!("{PROCESS_RECORD_EXTENSION}.tmp")); - let _ = std::fs::remove_file(&temp_path); - let write_result = (|| { - let mut file = std::fs::File::create(&temp_path).map_err(|error| { - format!( - "failed to create temporary process record {}: {error}", - temp_path.display() - ) - })?; - file.write_all(&serialized).map_err(|error| { - format!( - "failed to write temporary process record {}: {error}", - temp_path.display() - ) - })?; - file.write_all(b"\n").map_err(|error| { - format!( - "failed to finish temporary process record {}: {error}", - temp_path.display() - ) - })?; - file.sync_all().map_err(|error| { - format!( - "failed to sync temporary process record {}: {error}", - temp_path.display() - ) - })?; - std::fs::rename(&temp_path, &path).map_err(|error| { - format!( - "failed to publish process record {}: {error}", - path.display() - ) - }) - })(); - if write_result.is_err() { - let _ = std::fs::remove_file(&temp_path); - } - write_result + store.publish(path, &serialized) } /// Scan records left by previous runs and kill only true orphans: backend /// processes whose owning Tauri process is no longer alive. All errors are /// logged and swallowed so startup is never blocked. -async fn kill_stale_serve_process(dir: &Path) { +async fn kill_stale_serve_process(store: &ProcessRecordStore) { remove_legacy_pid_file(); - let entries = match std::fs::read_dir(dir) { + let entries = match store.entries() { Ok(entries) => entries, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return, Err(error) => { - log::warn!( - "Failed to read goose serve process record dir {}: {error}", - dir.display() - ); + log::warn!("Failed to enumerate goose serve process records: {error}"); return; } }; - for entry in entries.flatten() { - let path = entry.path(); + for path in entries { if !is_process_record_path(&path) { continue; } - cleanup_process_record(&path).await; + cleanup_process_record(store, &path).await; } } @@ -608,123 +695,125 @@ fn is_process_record_path(path: &Path) -> bool { .is_some_and(|extension| extension == PROCESS_RECORD_EXTENSION) } -async fn cleanup_process_record(path: &Path) { - let record = match read_process_record(path) { +async fn cleanup_process_record(store: &ProcessRecordStore, path: &Path) { + let (record, verified) = match read_process_record(store, path) { Ok(record) => record, Err(error) => { log::warn!( - "Failed to read goose serve process record {}: {error}; removing", - path.display() + "Failed to read goose serve process record {}: {}; removing only if it remains the exact validated object", + path.display(), + error.message ); - let _ = std::fs::remove_file(path); + if let Some(verified) = error.verified { + if let Err(remove_error) = store.remove_verified(path, &verified) { + log::warn!( + "Failed exact cleanup of invalid process record {}: {remove_error}", + path.display() + ); + } + } return; } }; - #[cfg(windows)] - if let Some(owner_identity) = &record.owner_identity { - match crate::services::process::probe_process_identity(owner_identity) { - IdentityProbe::Matches => { - log::debug!( - "Goose serve process record {} is still owned by live process {}; leaving it alone", - path.display(), - record.owner_pid - ); - return; - } - IdentityProbe::Unverifiable => { - log::warn!( - "Cannot verify owner of goose serve process record {}; keeping it", - path.display() - ); - return; - } - IdentityProbe::Gone | IdentityProbe::Mismatch => { - cleanup_orphaned_serve_process(path, &record).await; - return; - } - } - } - - let Some(owner_pid) = pid_t_from_u32(record.owner_pid) else { + let Some(owner_identity) = &record.owner_identity else { log::warn!( - "Goose serve process record {} has invalid owner pid {}; removing", - path.display(), - record.owner_pid + "Process record {} lacks stable owner identity; keeping recovery evidence without signaling", + path.display() ); - let _ = std::fs::remove_file(path); return; }; - - if process_is_alive(owner_pid) { - log::debug!( - "Goose serve process record {} is still owned by live process {}; leaving it alone", - path.display(), - record.owner_pid - ); - return; + match crate::services::process::probe_process_identity(owner_identity) { + IdentityProbe::Matches => { + log::debug!( + "Process record {} is still owned by live exact process {}; leaving it alone", + path.display(), + record.owner_pid + ); + return; + } + IdentityProbe::Unverifiable => { + log::warn!( + "Cannot verify owner identity for {}; keeping recovery evidence", + path.display() + ); + return; + } + IdentityProbe::Gone | IdentityProbe::Mismatch => {} } - #[cfg(unix)] - let Some(serve_pid) = pid_t_from_u32(record.serve_pid) else { - log::warn!( - "Goose serve process record {} has invalid serve pid {}; removing", - path.display(), - record.serve_pid - ); - let _ = std::fs::remove_file(path); - return; - }; + cleanup_orphaned_serve_process(store, path, &verified, &record).await; +} - #[cfg(unix)] - cleanup_orphaned_serve_process(path, serve_pid).await; - #[cfg(windows)] - cleanup_orphaned_serve_process(path, &record).await; +fn read_process_record( + store: &ProcessRecordStore, + path: &Path, +) -> Result< + ( + ServeProcessRecord, + super::process_record_store::VerifiedRecord, + ), + super::process_record_store::VerifiedReadError, +> { + let verified = store.read_verified_for_cleanup(path)?; + match serde_json::from_slice(&verified.bytes) { + Ok(parsed) => Ok((parsed, verified)), + Err(error) => Err(super::process_record_store::VerifiedReadError { + message: error.to_string(), + verified: Some(verified), + }), + } } -fn read_process_record(path: &Path) -> Result { - let contents = std::fs::read_to_string(path).map_err(|error| error.to_string())?; - serde_json::from_str(&contents).map_err(|error| error.to_string()) +fn remove_verified_or_warn( + store: &ProcessRecordStore, + path: &Path, + verified: &super::process_record_store::VerifiedRecord, + reason: &str, +) { + if let Err(error) = store.remove_verified(path, verified) { + log::warn!( + "Failed exact cleanup of process record {} after {reason}: {error}; keeping it", + path.display() + ); + } } #[cfg(windows)] -async fn cleanup_orphaned_serve_process(path: &Path, record: &ServeProcessRecord) { +async fn cleanup_orphaned_serve_process( + store: &ProcessRecordStore, + path: &Path, + verified: &super::process_record_store::VerifiedRecord, + record: &ServeProcessRecord, +) { let Some(identity) = &record.serve_identity else { log::warn!( - "Goose serve process record {} has no Windows process identity; removing without killing PID {}", + "Process record {} has no Windows process identity; removing without signaling PID {}", path.display(), record.serve_pid ); - let _ = std::fs::remove_file(path); + remove_verified_or_warn( + store, + path, + verified, + "missing stable Windows serve identity", + ); return; }; - let identity = identity.clone(); - - log::info!( - "Killing orphaned goose serve process (pid {})", - identity.pid - ); - diagnostic_log::record_event( - DiagnosticLevel::Warn, - DiagnosticCategory::GooseServe, - "stale_process_kill", - None, - diagnostic_log::fields([("pid", (identity.pid as i64).into())]), - ); match crate::services::process::kill_process_if_identity_matches( - &identity, + identity, Duration::from_secs(5), ) { Ok(outcome) if outcome.exit_confirmed() => { - let _ = std::fs::remove_file(path); + remove_verified_or_warn(store, path, verified, "confirmed Windows process exit"); } Ok(_) => log::warn!( - "Goose serve process {} did not confirm exit; keeping process record {}", + "Goose serve {} did not confirm exit; keeping {}", identity.pid, path.display() ), Err(error) => log::warn!( - "Failed to kill orphaned goose serve process {}: {error}; keeping process record {}", + "Failed to stop goose serve {}: {error}; keeping {}", identity.pid, path.display() ), @@ -732,86 +821,155 @@ async fn cleanup_orphaned_serve_process(path: &Path, record: &ServeProcessRecord } #[cfg(unix)] -async fn cleanup_orphaned_serve_process(path: &Path, pid: ProcessId) { - if !process_is_alive(pid) { - log::info!( - "Previous goose serve (pid {pid}) is no longer running, removing process record {}", - path.display() +async fn cleanup_orphaned_serve_process( + store: &ProcessRecordStore, + path: &Path, + verified: &super::process_record_store::VerifiedRecord, + record: &ServeProcessRecord, +) { + cleanup_orphaned_serve_process_with_ops( + store, + path, + verified, + record, + crate::services::process::probe_process_identity, + terminate_process, + kill_process, + Duration::from_millis(200), + Duration::from_millis(50), + ) + .await; +} + +#[cfg(unix)] +#[allow(clippy::too_many_arguments)] +async fn cleanup_orphaned_serve_process_with_ops( + store: &ProcessRecordStore, + path: &Path, + verified: &super::process_record_store::VerifiedRecord, + record: &ServeProcessRecord, + mut probe: FProbe, + mut terminate: FTerm, + mut kill: FKill, + term_delay: Duration, + kill_delay: Duration, +) where + FProbe: FnMut(&ProcessIdentity) -> IdentityProbe, + FTerm: FnMut(crate::services::process::ProcessId) -> bool, + FKill: FnMut(crate::services::process::ProcessId) -> bool, +{ + let Some(identity) = &record.serve_identity else { + log::warn!( + "Process record {} lacks stable serve identity; removing without signaling PID {}", + path.display(), + record.serve_pid ); - let _ = std::fs::remove_file(path); + remove_verified_or_warn(store, path, verified, "missing stable Unix serve identity"); return; + }; + match probe(identity) { + IdentityProbe::Gone | IdentityProbe::Mismatch => { + remove_verified_or_warn(store, path, verified, "initial probe confirmed no match"); + return; + } + IdentityProbe::Unverifiable => { + log::warn!( + "Cannot verify stale serve identity {}; keeping {}", + identity.pid, + path.display() + ); + return; + } + IdentityProbe::Matches => {} } - // Guard against PID recycling: verify the process is actually a goose binary. - if !is_goose_process(pid) { + diagnostic_log::record_event( + DiagnosticLevel::Warn, + DiagnosticCategory::GooseServe, + "stale_process_kill", + None, + diagnostic_log::fields([("pid", (identity.pid as i64).into())]), + ); + let Some(pid) = pid_t_from_u32(identity.pid) else { log::warn!( - "PID {pid} is alive but is not a goose process (PID was likely recycled), removing process record {}", + "Invalid stale serve PID {}; keeping {}", + identity.pid, path.display() ); - let _ = std::fs::remove_file(path); + return; + }; + if !terminate(pid) { + match probe(identity) { + IdentityProbe::Gone | IdentityProbe::Mismatch => { + remove_verified_or_warn( + store, + path, + verified, + "failed SIGTERM followed by no identity match", + ); + } + IdentityProbe::Matches | IdentityProbe::Unverifiable => log::warn!( + "SIGTERM failed for exact goose serve {}; exit remains unconfirmed; keeping {}", + identity.pid, + path.display() + ), + } return; } - - log::info!("Killing orphaned goose serve process (pid {pid})"); + tokio::time::sleep(term_delay).await; + match probe(identity) { + IdentityProbe::Gone | IdentityProbe::Mismatch => { + remove_verified_or_warn(store, path, verified, "post-SIGTERM probe confirmed exit"); + return; + } + IdentityProbe::Unverifiable => { + log::warn!( + "Cannot reverify goose serve {} after SIGTERM; keeping {}", + identity.pid, + path.display() + ); + return; + } + IdentityProbe::Matches => {} + } diagnostic_log::record_event( DiagnosticLevel::Warn, DiagnosticCategory::GooseServe, - "stale_process_kill", + "stale_process_kill_forced", None, - diagnostic_log::fields([("pid", (pid as i64).into())]), + diagnostic_log::fields([("pid", (identity.pid as i64).into())]), ); - terminate_process(pid); - - // Give it a moment to exit, then force-kill if still alive. - tokio::time::sleep(Duration::from_millis(200)).await; - if process_is_alive(pid) { - log::warn!("Orphaned goose serve (pid {pid}) did not exit after SIGTERM, sending SIGKILL"); - diagnostic_log::record_event( - DiagnosticLevel::Warn, - DiagnosticCategory::GooseServe, - "stale_process_kill_forced", - None, - diagnostic_log::fields([("pid", (pid as i64).into())]), - ); - kill_process(pid); + if !kill(pid) { + match probe(identity) { + IdentityProbe::Gone | IdentityProbe::Mismatch => { + remove_verified_or_warn( + store, + path, + verified, + "failed SIGKILL followed by no identity match", + ); + } + IdentityProbe::Matches | IdentityProbe::Unverifiable => log::warn!( + "SIGKILL failed for exact goose serve {}; exit remains unconfirmed; keeping {}", + identity.pid, + path.display() + ), + } + return; } - - let _ = std::fs::remove_file(path); -} - -#[cfg(unix)] -/// Check whether the given PID belongs to a goose binary. Uses -/// `proc_pidpath` on macOS and `/proc/{pid}/exe` on Linux. -fn is_goose_process(pid: ProcessId) -> bool { - if let Some(name) = process_executable_name(pid) { - name.contains("goose") - } else { - // If we can't determine the process name, err on the side of caution - // and assume it is NOT a goose process to avoid killing an unrelated PID. - false + tokio::time::sleep(kill_delay).await; + match probe(identity) { + IdentityProbe::Gone | IdentityProbe::Mismatch => { + remove_verified_or_warn(store, path, verified, "post-SIGKILL probe confirmed exit"); + } + IdentityProbe::Matches | IdentityProbe::Unverifiable => log::warn!( + "Goose serve {} exit remains unconfirmed; keeping {}", + identity.pid, + path.display() + ), } } -#[cfg(target_os = "macos")] -fn process_executable_name(pid: ProcessId) -> Option { - let mut buf = vec![0u8; libc::PROC_PIDPATHINFO_MAXSIZE as usize]; - // SAFETY: buf is large enough for the maximum path length. - let len = - unsafe { libc::proc_pidpath(pid, buf.as_mut_ptr() as *mut libc::c_void, buf.len() as u32) }; - if len <= 0 { - return None; - } - let path = std::str::from_utf8(&buf[..len as usize]).ok()?; - path.rsplit('/').next().map(String::from) -} - -#[cfg(all(unix, not(target_os = "macos")))] -fn process_executable_name(pid: ProcessId) -> Option { - let exe_link = format!("/proc/{pid}/exe"); - let path = std::fs::read_link(exe_link).ok()?; - path.file_name()?.to_str().map(String::from) -} - /// Paths resolved by `resolve_berdctl_spawn_paths`, consumed by /// `apply_berdctl_env` after PATH assembly. #[cfg(feature = "berdctl")] @@ -1219,15 +1377,26 @@ pub(crate) fn reserve_free_port() -> Result { #[cfg(test)] mod tests { + #[cfg(all(unix, not(target_os = "macos")))] + use super::cleanup_process_record; + #[cfg(unix)] + use super::ServeProcessRecord; use super::{ acp_websocket_url, add_release_webview_origin_arg, apply_goose_search_paths_env, apply_runtime_goose_provider_env, apply_shell_env_with_extended_path, - apply_shell_env_with_extended_path_inner, DATABRICKS_HOST_ENV, TAURI_WEBVIEW_ORIGIN, + apply_shell_env_with_extended_path_inner, require_published_record, stop_child_and_reap, + DATABRICKS_HOST_ENV, TAURI_WEBVIEW_ORIGIN, }; use crate::commands::runtime_config::default_runtime_config; + #[cfg(unix)] + use crate::services::acp::process_record_store::ProcessRecordStore; + #[cfg(unix)] + use crate::services::process::IdentityProbe; use std::collections::HashMap; use std::ffi::OsString; use std::path::{Path, PathBuf}; + #[cfg(unix)] + use std::time::Duration; use tokio::process::Command; fn env_value(command: &Command, key: &str) -> Option { @@ -1240,6 +1409,493 @@ mod tests { }) } + #[tokio::test] + async fn process_record_publication_failure_kills_and_reaps_the_child() { + #[cfg(unix)] + let mut command = { + let mut command = Command::new("sh"); + command.args(["-c", "sleep 30"]); + command + }; + #[cfg(windows)] + let mut command = { + let mut command = Command::new("cmd.exe"); + command.args(["/d", "/c", "ping -n 31 127.0.0.1 >nul"]); + command + }; + let mut child = command.spawn().expect("spawn long-lived child"); + #[cfg(unix)] + let pid = child.id().expect("child pid"); + + let error = require_published_record(&mut child, Err("forced failure".to_string())) + .await + .expect_err("publication failure must abort startup"); + + assert!(error.contains("forced failure")); + assert!(child.id().is_none(), "wait must reap the child"); + #[cfg(unix)] + assert_eq!( + unsafe { libc::kill(pid as i32, 0) }, + -1, + "child must no longer exist" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn child_teardown_attempts_sigterm_before_forced_kill() { + let temp = tempfile::tempdir().expect("temp dir"); + let marker = temp.path().join("term-received"); + let script = format!( + "trap 'printf term > {} ; exit 0' TERM; while :; do sleep 1; done", + marker.display() + ); + let mut child = Command::new("sh") + .args(["-c", &script]) + .spawn() + .expect("spawn child"); + tokio::time::sleep(Duration::from_millis(100)).await; + + stop_child_and_reap(&mut child) + .await + .expect("graceful teardown"); + + assert!(child.id().is_none(), "wait must reap the child"); + assert_eq!( + std::fs::read_to_string(marker).expect("SIGTERM handler marker"), + "term" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn readiness_failure_reaps_child_and_exact_deletes_retained_record() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + let mut child = Command::new("sh") + .args(["-c", "sleep 30"]) + .spawn() + .expect("spawn child"); + let pid = child.id().expect("child pid"); + + super::teardown_published_child( + &store, + &path, + Some(&verified), + &mut child, + "forced readiness failure", + ) + .await; + + assert!(child.id().is_none(), "wait must reap the child"); + assert_eq!( + unsafe { libc::kill(pid as i32, 0) }, + -1, + "child must no longer exist" + ); + assert!(!path.exists(), "confirmed exit permits exact cleanup"); + } + + #[cfg(unix)] + #[tokio::test] + async fn post_publication_retention_failure_reaps_child_but_keeps_record() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let mut child = Command::new("sh") + .args(["-c", "sleep 30"]) + .spawn() + .expect("spawn child"); + let pid = child.id().expect("child pid"); + + let error = super::retain_published_record_with(&store, &path, &mut child, |_, _| { + Err(super::super::process_record_store::VerifiedReadError { + message: "forced retention failure".to_string(), + verified: None, + }) + }) + .await + .expect_err("retention failure must abort startup"); + + assert!(error.contains("forced retention failure")); + assert!(child.id().is_none(), "wait must reap the child"); + assert_eq!( + unsafe { libc::kill(pid as i32, 0) }, + -1, + "child must no longer exist" + ); + assert!( + path.exists(), + "without the originally retained object, exact cleanup must not reopen by path" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn published_child_teardown_timeout_keeps_exact_record() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + + super::teardown_published_child_with( + &store, + &path, + Some(&verified), + "forced timeout", + super::bounded_child_teardown(Duration::ZERO, async { + std::future::pending::>().await + }), + ) + .await; + + assert!(path.exists(), "timed-out teardown must retain evidence"); + } + + #[cfg(unix)] + fn test_process_record( + identity: &crate::services::process::ProcessIdentity, + ) -> ServeProcessRecord { + ServeProcessRecord { + owner_pid: identity.pid, + serve_pid: identity.pid, + owner_identity: Some(identity.clone()), + serve_identity: Some(identity.clone()), + } + } + + #[cfg(unix)] + #[tokio::test] + async fn failed_sigterm_removes_record_when_follow_up_probe_confirms_gone() { + use std::collections::VecDeque; + + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + let identity = crate::services::process::ProcessIdentity { + pid: std::process::id(), + created_at: 1, + exe: "test".to_string(), + }; + let record = test_process_record(&identity); + let mut probes = VecDeque::from([IdentityProbe::Matches, IdentityProbe::Gone]); + + super::cleanup_orphaned_serve_process_with_ops( + &store, + &path, + &verified, + &record, + |_| probes.pop_front().expect("scripted probe"), + |_| false, + |_| panic!("SIGKILL must not run after failed SIGTERM"), + Duration::ZERO, + Duration::ZERO, + ) + .await; + + assert!(!path.exists(), "confirmed exit permits exact cleanup"); + } + + #[cfg(unix)] + #[tokio::test] + async fn failed_sigkill_removes_record_when_follow_up_probe_confirms_mismatch() { + use std::collections::VecDeque; + + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + let identity = crate::services::process::ProcessIdentity { + pid: std::process::id(), + created_at: 1, + exe: "test".to_string(), + }; + let record = test_process_record(&identity); + let mut probes = VecDeque::from([ + IdentityProbe::Matches, + IdentityProbe::Matches, + IdentityProbe::Mismatch, + ]); + + super::cleanup_orphaned_serve_process_with_ops( + &store, + &path, + &verified, + &record, + |_| probes.pop_front().expect("scripted probe"), + |_| true, + |_| false, + Duration::ZERO, + Duration::ZERO, + ) + .await; + + assert!(!path.exists(), "confirmed mismatch permits exact cleanup"); + } + + #[cfg(unix)] + #[tokio::test] + async fn identity_change_between_term_and_kill_never_sends_kill() { + use std::cell::Cell; + use std::collections::VecDeque; + use std::rc::Rc; + + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + let identity = crate::services::process::ProcessIdentity { + pid: std::process::id(), + created_at: 1, + exe: "test".to_string(), + }; + let record = ServeProcessRecord { + owner_pid: identity.pid, + serve_pid: identity.pid, + owner_identity: Some(identity.clone()), + serve_identity: Some(identity), + }; + let mut probes = VecDeque::from([IdentityProbe::Matches, IdentityProbe::Mismatch]); + let killed = Rc::new(Cell::new(false)); + let killed_for_closure = Rc::clone(&killed); + + super::cleanup_orphaned_serve_process_with_ops( + &store, + &path, + &verified, + &record, + |_| probes.pop_front().expect("scripted probe"), + |_| true, + move |_| { + killed_for_closure.set(true); + true + }, + Duration::ZERO, + Duration::ZERO, + ) + .await; + + assert!(!killed.get(), "identity mismatch must suppress SIGKILL"); + assert!(!path.exists(), "mismatched identity is confirmed gone"); + } + + #[cfg(unix)] + #[tokio::test] + async fn unconfirmed_exit_after_kill_retains_record() { + use std::collections::VecDeque; + + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"record").expect("publish"); + let verified = store.read_verified_for_cleanup(&path).expect("retain"); + let identity = crate::services::process::ProcessIdentity { + pid: std::process::id(), + created_at: 1, + exe: "test".to_string(), + }; + let record = ServeProcessRecord { + owner_pid: identity.pid, + serve_pid: identity.pid, + owner_identity: Some(identity.clone()), + serve_identity: Some(identity), + }; + let mut probes = VecDeque::from([ + IdentityProbe::Matches, + IdentityProbe::Matches, + IdentityProbe::Unverifiable, + ]); + + super::cleanup_orphaned_serve_process_with_ops( + &store, + &path, + &verified, + &record, + |_| probes.pop_front().expect("scripted probe"), + |_| true, + |_| true, + Duration::ZERO, + Duration::ZERO, + ) + .await; + + assert!(path.exists(), "unconfirmed exit must retain evidence"); + } + + #[cfg(target_os = "macos")] + #[tokio::test] + async fn macos_publishes_deletion_only_record_when_identity_is_unavailable() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + let mut child = Command::new("sh") + .args(["-c", "sleep 30"]) + .spawn() + .expect("spawn child"); + + super::write_pid_file(&store, &path, &child).expect("publish deletion-only record"); + let bytes = store + .read_verified_for_cleanup(&path) + .expect("read record") + .bytes; + let record: ServeProcessRecord = serde_json::from_slice(&bytes).expect("parse record"); + assert!(record.owner_identity.is_none()); + assert!(record.serve_identity.is_none()); + + child.kill().await.expect("kill child"); + child.wait().await.expect("reap child"); + } + + #[cfg(target_os = "macos")] + #[tokio::test] + async fn macos_identityless_stale_record_is_retained_without_signaling() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + let record = ServeProcessRecord { + owner_pid: std::process::id(), + serve_pid: std::process::id(), + owner_identity: None, + serve_identity: None, + }; + store + .publish( + &path, + &serde_json::to_vec(&record).expect("serialize record"), + ) + .expect("publish identityless record"); + + super::cleanup_process_record(&store, &path).await; + + assert!( + path.exists(), + "identityless recovery evidence must be retained" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn clean_shutdown_does_not_delete_successor_record() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + store.publish(&path, b"published").expect("publish record"); + let verified = store + .read_verified_for_cleanup(&path) + .expect("retain record"); + let displaced = root.join("displaced.json"); + std::fs::rename(&path, &displaced).expect("displace published record"); + std::fs::write(&path, b"successor\n").expect("write successor"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("secure successor"); + let child = Command::new("sh") + .args(["-c", "sleep 30"]) + .spawn() + .expect("spawn child"); + let process = super::GooseServeProcess { + port: 0, + secret_key: String::new(), + process_record_store: store, + process_record_path: path.clone(), + process_record: verified, + child: tokio::sync::Mutex::new(child), + }; + + process.kill().await; + + assert_eq!(std::fs::read(&path).unwrap(), b"successor\n"); + assert_eq!(std::fs::read(&displaced).unwrap(), b"published\n"); + } + + #[cfg(unix)] + #[tokio::test] + async fn malformed_record_cleanup_rejects_successor_substitution() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).expect("open store"); + let path = root.join("malformed.json"); + std::fs::write(&path, b"not-json").expect("write malformed record"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("secure malformed record"); + + let error = super::read_process_record(&store, &path).expect_err("reject malformed"); + let verified = error.verified.expect("retain validated malformed object"); + let displaced = root.join("malformed-displaced.json"); + std::fs::rename(&path, &displaced).expect("displace malformed record"); + std::fs::write(&path, b"successor\n").expect("write successor"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("secure successor"); + + assert!(store.remove_verified(&path, &verified).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), b"successor\n"); + assert_eq!(std::fs::read(&displaced).unwrap(), b"not-json"); + } + + #[cfg(all(unix, not(target_os = "macos")))] + #[tokio::test] + async fn stale_owner_pid_reuse_does_not_kill_an_unrelated_process() { + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let path = store.new_record_path(std::process::id(), 1); + let mut unrelated = Command::new("sh"); + unrelated.args(["-c", "sleep 30"]); + let mut unrelated = unrelated.spawn().expect("spawn unrelated child"); + let unrelated_pid = unrelated.id().expect("unrelated pid"); + let mut stale_owner = Command::new("sh") + .arg("-c") + .arg("exit 0") + .spawn() + .expect("spawn stale owner"); + let stale_owner_pid = stale_owner.id().expect("stale owner pid"); + let stale_owner_identity = + crate::services::process::capture_process_identity(stale_owner_pid) + .expect("capture stale owner identity"); + stale_owner.wait().await.expect("reap stale owner"); + let mut unrelated_identity = + crate::services::process::capture_process_identity(unrelated_pid) + .expect("capture unrelated identity"); + unrelated_identity.created_at = unrelated_identity.created_at.wrapping_add(1); + let record = ServeProcessRecord { + owner_pid: stale_owner_pid, + serve_pid: unrelated_pid, + owner_identity: Some(stale_owner_identity), + serve_identity: Some(unrelated_identity), + }; + store + .publish( + &path, + &serde_json::to_vec(&record).expect("serialize record"), + ) + .expect("publish stale record"); + + cleanup_process_record(&store, &path).await; + + assert!(!path.exists(), "recycled-PID record should be removed"); + assert!( + unrelated + .try_wait() + .expect("probe unrelated child") + .is_none(), + "a non-goose process at the recorded PID must not be killed" + ); + unrelated.kill().await.expect("kill unrelated child"); + unrelated.wait().await.expect("reap unrelated child"); + } + #[test] fn acp_websocket_url_includes_secret_key_token() { assert_eq!( diff --git a/src-tauri/src/services/acp/mod.rs b/src-tauri/src/services/acp/mod.rs index 51ea427e8..7da4b6d6d 100644 --- a/src-tauri/src/services/acp/mod.rs +++ b/src-tauri/src/services/acp/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod goose_serve; +mod process_record_store; mod security_env; pub(crate) use goose_serve::GooseServeProcess; diff --git a/src-tauri/src/services/acp/process_record_store.rs b/src-tauri/src/services/acp/process_record_store.rs new file mode 100644 index 000000000..6d7fe0dac --- /dev/null +++ b/src-tauri/src/services/acp/process_record_store.rs @@ -0,0 +1,2019 @@ +use std::fs::{self, File}; +use std::io::{Read, Write}; +use std::path::{Path, PathBuf}; + +const MAX_RECORD_BYTES: u64 = 4 * 1024; +#[cfg(unix)] +const RECORD_DIR_MODE: u32 = 0o700; +#[cfg(unix)] +const RECORD_FILE_MODE: u32 = 0o600; + +#[derive(Debug)] +pub(super) struct VerifiedRecord { + pub(super) bytes: Vec, + identity: platform::FileIdentity, + #[cfg(windows)] + file: File, +} + +#[derive(Debug)] +pub(super) struct VerifiedReadError { + pub(super) message: String, + /// Present only after the object passed type/ownership/ACL validation. + pub(super) verified: Option, +} + +#[derive(Debug)] +pub(super) struct ProcessRecordStore { + root: PathBuf, + handle: platform::RootHandle, +} + +impl ProcessRecordStore { + pub(super) fn open(root: PathBuf) -> Result { + let handle = platform::open_root(&root)?; + Ok(Self { root, handle }) + } + + pub(super) fn new_record_path(&self, owner_pid: u32, executable_hash: u64) -> PathBuf { + self.root.join(format!( + "{owner_pid}-{executable_hash:016x}-{}.json", + uuid::Uuid::new_v4().simple() + )) + } + + pub(super) fn publish(&self, destination: &Path, bytes: &[u8]) -> Result<(), String> { + let stored_len = bytes + .len() + .checked_add(1) + .ok_or_else(|| format!("process record exceeds {MAX_RECORD_BYTES} bytes"))?; + if stored_len as u64 > MAX_RECORD_BYTES { + return Err(format!("process record exceeds {MAX_RECORD_BYTES} bytes")); + } + ensure_direct_child(&self.root, destination)?; + let temp = self.root.join(format!( + ".process-record-{}.tmp", + uuid::Uuid::new_v4().simple() + )); + let mut temp_identity = None; + let result = (|| { + let mut file = platform::create( + &self.root, + &self.handle, + temp.file_name().expect("temp has a name"), + )?; + temp_identity = Some(platform::file_identity(&file)?); + file.write_all(bytes) + .and_then(|_| file.write_all(b"\n")) + .and_then(|_| file.sync_all()) + .map_err(|error| format!("failed to write {}: {error}", temp.display()))?; + platform::rename( + &self.handle, + &file, + temp.file_name().expect("temp has a name"), + destination.file_name().expect("destination has a name"), + )?; + platform::sync(&self.handle)?; + Ok(()) + })(); + if result.is_err() { + if let Some(identity) = temp_identity.as_ref() { + let _ = platform::remove(&self.root, &self.handle, &temp, Some(identity)); + } + } + result + } + + pub(super) fn read_verified_for_cleanup( + &self, + path: &Path, + ) -> Result { + let read = (|| -> Result { + ensure_direct_child(&self.root, path).map_err(|message| VerifiedReadError { + message, + verified: None, + })?; + let file = platform::open(&self.root, &self.handle, path).map_err(|message| { + VerifiedReadError { + message, + verified: None, + } + })?; + let metadata = file.metadata().map_err(|error| VerifiedReadError { + message: format!("failed to inspect {}: {error}", path.display()), + verified: None, + })?; + platform::validate_metadata(path, &metadata).map_err(|message| VerifiedReadError { + message, + verified: None, + })?; + let identity = platform::file_identity(&file).map_err(|message| VerifiedReadError { + message, + verified: None, + })?; + if metadata.len() > MAX_RECORD_BYTES { + #[cfg(unix)] + let record = VerifiedRecord { + bytes: Vec::new(), + identity, + }; + #[cfg(windows)] + let record = VerifiedRecord { + bytes: Vec::new(), + identity, + file, + }; + return Err(VerifiedReadError { + message: format!( + "process record {} exceeds {MAX_RECORD_BYTES} bytes", + path.display() + ), + verified: Some(record), + }); + } + let mut bytes = Vec::with_capacity(metadata.len() as usize); + (&file) + .take(MAX_RECORD_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|error| VerifiedReadError { + message: format!("failed to read {}: {error}", path.display()), + verified: None, + })?; + if bytes.len() as u64 > MAX_RECORD_BYTES { + #[cfg(unix)] + let record = VerifiedRecord { bytes, identity }; + #[cfg(windows)] + let record = VerifiedRecord { + bytes, + identity, + file, + }; + return Err(VerifiedReadError { + message: format!( + "process record {} exceeds {MAX_RECORD_BYTES} bytes", + path.display() + ), + verified: Some(record), + }); + } + #[cfg(unix)] + return Ok(VerifiedRecord { bytes, identity }); + #[cfg(windows)] + Ok(VerifiedRecord { + bytes, + identity, + file, + }) + })(); + read + } + + #[cfg(test)] + pub(super) fn read_verified(&self, path: &Path) -> Result { + self.read_verified_for_cleanup(path) + .map_err(|error| error.message) + } + + #[cfg(test)] + pub(super) fn read(&self, path: &Path) -> Result, String> { + self.read_verified(path).map(|record| record.bytes) + } + + pub(super) fn entries(&self) -> Result, String> { + platform::entries(&self.root, &self.handle) + } + + pub(super) fn remove_verified( + &self, + path: &Path, + verified: &VerifiedRecord, + ) -> Result<(), String> { + platform::remove_verified(&self.root, &self.handle, path, verified) + } + + #[cfg(test)] + pub(super) fn remove(&self, path: &Path) -> Result<(), String> { + platform::remove(&self.root, &self.handle, path, None) + } +} + +fn ensure_direct_child(root: &Path, path: &Path) -> Result<(), String> { + if path.parent() != Some(root) || path.file_name().is_none() { + return Err(format!("process record escapes {}", root.display())); + } + Ok(()) +} + +#[cfg(unix)] +mod platform { + use super::*; + use std::ffi::{CString, OsStr, OsString}; + use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; + use std::os::unix::ffi::{OsStrExt, OsStringExt}; + use std::os::unix::fs::{DirBuilderExt, MetadataExt}; + + fn device_id(device: T) -> u64 + where + T: TryInto, + T::Error: std::fmt::Debug, + { + device + .try_into() + .expect("filesystem device identifier must fit in u64") + } + + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + pub(super) struct FileIdentity { + device: u64, + inode: u64, + } + + pub(super) fn file_identity(file: &File) -> Result { + let metadata = file + .metadata() + .map_err(|error| format!("failed to identify process record: {error}"))?; + Ok(FileIdentity { + device: device_id(metadata.dev()), + inode: metadata.ino(), + }) + } + + #[derive(Debug)] + pub(super) struct RootHandle(OwnedFd); + + fn c_name(name: &OsStr) -> Result { + CString::new(name.as_bytes()).map_err(|_| "process record name contains NUL".to_string()) + } + + fn name_for(root: &Path, path: &Path) -> Result { + super::ensure_direct_child(root, path)?; + c_name(path.file_name().expect("direct child has a name")) + } + + pub(super) fn open_root(path: &Path) -> Result { + if !path.exists() { + let mut builder = fs::DirBuilder::new(); + builder.recursive(true).mode(RECORD_DIR_MODE); + builder + .create(path) + .map_err(|error| format!("failed to create {}: {error}", path.display()))?; + } + let c_path = CString::new(path.as_os_str().as_bytes()) + .map_err(|_| format!("{} contains NUL", path.display()))?; + // SAFETY: c_path is NUL terminated and flags require an actual directory. + let fd = unsafe { + libc::open( + c_path.as_ptr(), + libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC | libc::O_NOFOLLOW, + ) + }; + if fd < 0 { + return Err(format!( + "failed to safely open {}: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + // SAFETY: open returned a new owned descriptor. + let fd = unsafe { OwnedFd::from_raw_fd(fd) }; + let mut stat = std::mem::MaybeUninit::::uninit(); + // SAFETY: stat points to writable storage and fd is valid. + if unsafe { libc::fstat(fd.as_raw_fd(), stat.as_mut_ptr()) } != 0 { + return Err(format!( + "failed to inspect {}: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + // SAFETY: successful fstat initialized stat. + let stat = unsafe { stat.assume_init() }; + if stat.st_uid != unsafe { libc::geteuid() } { + return Err(format!( + "process record root {} has the wrong owner", + path.display() + )); + } + if stat.st_mode & 0o777 != RECORD_DIR_MODE as libc::mode_t { + // SAFETY: fd is our validated directory handle. + if unsafe { libc::fchmod(fd.as_raw_fd(), RECORD_DIR_MODE as libc::mode_t) } != 0 { + return Err(format!( + "failed to make {} owner-private: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + } + let mut checked = std::mem::MaybeUninit::::uninit(); + // SAFETY: checked points to writable storage and fd is valid. + if unsafe { libc::fstat(fd.as_raw_fd(), checked.as_mut_ptr()) } != 0 { + return Err(format!( + "failed to re-inspect {}: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + // SAFETY: successful fstat initialized checked. + if unsafe { checked.assume_init() }.st_mode & 0o777 != RECORD_DIR_MODE as libc::mode_t { + return Err(format!( + "process record root {} is not owner-private", + path.display() + )); + } + Ok(RootHandle(fd)) + } + + pub(super) fn create( + _root_path: &Path, + root: &RootHandle, + name: &OsStr, + ) -> Result { + let name = c_name(name)?; + // SAFETY: root is retained, name is a single NUL-terminated component. + let fd = unsafe { + libc::openat( + root.0.as_raw_fd(), + name.as_ptr(), + libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_CLOEXEC | libc::O_NOFOLLOW, + RECORD_FILE_MODE as libc::c_uint, + ) + }; + if fd < 0 { + return Err(format!( + "failed to create process record: {}", + std::io::Error::last_os_error() + )); + } + // SAFETY: openat returned a new owned descriptor. + let file = unsafe { File::from_raw_fd(fd) }; + let metadata = file + .metadata() + .map_err(|e| format!("failed to inspect new process record: {e}"))?; + validate_metadata(Path::new(name.to_str().unwrap_or("")), &metadata)?; + Ok(file) + } + + fn open_file(root: &RootHandle, name: &CString) -> std::io::Result { + // SAFETY: root is retained and name is a direct child. + let fd = unsafe { + libc::openat( + root.0.as_raw_fd(), + name.as_ptr(), + libc::O_RDONLY | libc::O_NONBLOCK | libc::O_CLOEXEC | libc::O_NOFOLLOW, + ) + }; + if fd < 0 { + return Err(std::io::Error::last_os_error()); + } + // SAFETY: openat returned a new owned descriptor. + Ok(unsafe { File::from_raw_fd(fd) }) + } + + pub(super) fn open(root_path: &Path, root: &RootHandle, path: &Path) -> Result { + let name = name_for(root_path, path)?; + open_file(root, &name) + .map_err(|error| format!("failed to safely open {}: {error}", path.display())) + } + + fn unlink_name(root: &RootHandle, name: &CString) -> std::io::Result<()> { + // SAFETY: root is retained and name is a root-relative direct child. + if unsafe { libc::unlinkat(root.0.as_raw_fd(), name.as_ptr(), 0) } != 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + } + + fn rename_with_unlink( + root: &RootHandle, + source: &File, + from: &OsStr, + to: &OsStr, + mut unlink: F, + ) -> Result<(), String> + where + F: FnMut(&RootHandle, &CString) -> std::io::Result<()>, + { + let from = c_name(from)?; + let to = c_name(to)?; + let source_identity = file_identity(source)?; + let mut current = std::mem::MaybeUninit::::uninit(); + // SAFETY: current is writable and from is a root-relative name. + if unsafe { + libc::fstatat( + root.0.as_raw_fd(), + from.as_ptr(), + current.as_mut_ptr(), + libc::AT_SYMLINK_NOFOLLOW, + ) + } != 0 + { + return Err(format!( + "failed to bind process record publication: {}", + std::io::Error::last_os_error() + )); + } + // SAFETY: successful fstatat initialized current. + let current = unsafe { current.assume_init() }; + if source_identity + != (FileIdentity { + device: device_id(current.st_dev), + inode: current.st_ino, + }) + { + return Err("temporary process record changed before publication".to_string()); + } + // linkat is an atomic no-replace publication primitive: it fails if + // the destination exists, then unlinkat removes the temporary name. + // Both operations are anchored to the retained same-directory fd, and + // the source name was just verified against the retained source handle. + if unsafe { + libc::linkat( + root.0.as_raw_fd(), + from.as_ptr(), + root.0.as_raw_fd(), + to.as_ptr(), + 0, + ) + } != 0 + { + return Err(format!( + "failed to publish process record without replacement: {}", + std::io::Error::last_os_error() + )); + } + if let Err(temp_error) = unlink(root, &from) { + // Publication created `to` as a second name for the retained source. + // Roll it back before the caller cleans the identity-bound temp name, + // otherwise both names retain nlink == 2 and fail metadata validation. + let rollback_error = unlink(root, &to).err(); + return Err(match rollback_error { + Some(rollback_error) => format!( + "published process record but failed to remove temporary name: {temp_error}; \ + failed to roll back destination: {rollback_error}" + ), + None => format!( + "published process record but failed to remove temporary name: {temp_error}; \ + rolled back destination" + ), + }); + } + Ok(()) + } + + pub(super) fn rename( + root: &RootHandle, + source: &File, + from: &OsStr, + to: &OsStr, + ) -> Result<(), String> { + rename_with_unlink(root, source, from, to, unlink_name) + } + + #[cfg(test)] + pub(super) fn unlink_name_for_test(root: &RootHandle, name: &CString) -> std::io::Result<()> { + unlink_name(root, name) + } + + #[cfg(test)] + pub(super) fn rename_with_unlink_for_test( + root: &RootHandle, + source: &File, + from: &OsStr, + to: &OsStr, + unlink: F, + ) -> Result<(), String> + where + F: FnMut(&RootHandle, &CString) -> std::io::Result<()>, + { + rename_with_unlink(root, source, from, to, unlink) + } + + fn duplicate_root(root: &RootHandle) -> std::io::Result { + // SAFETY: F_DUPFD_CLOEXEC returns an independent close-on-exec descriptor. + let duplicate = unsafe { libc::fcntl(root.0.as_raw_fd(), libc::F_DUPFD_CLOEXEC, 0) }; + if duplicate < 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(duplicate) + } + + #[cfg(test)] + pub(super) fn duplicate_root_for_test(root: &RootHandle) -> std::io::Result { + duplicate_root(root) + } + + pub(super) fn entries(root_path: &Path, root: &RootHandle) -> Result, String> { + let duplicate = duplicate_root(root) + .map_err(|error| format!("failed to duplicate process record directory: {error}"))?; + // SAFETY: fdopendir takes ownership of duplicate. + let directory = unsafe { libc::fdopendir(duplicate) }; + if directory.is_null() { + unsafe { libc::close(duplicate) }; + return Err(format!( + "failed to enumerate process records: {}", + std::io::Error::last_os_error() + )); + } + let mut paths = Vec::new(); + let result = loop { + errno::set_errno(errno::Errno(0)); + // SAFETY: directory remains valid until closed below. + let entry = unsafe { libc::readdir(directory) }; + if entry.is_null() { + let read_error = errno::errno(); + if read_error.0 != 0 { + break Err(format!( + "failed while enumerating process records: {}", + std::io::Error::from_raw_os_error(read_error.0) + )); + } + break Ok(paths); + } + // SAFETY: d_name is NUL-terminated for a valid dirent. + let bytes = unsafe { std::ffi::CStr::from_ptr((*entry).d_name.as_ptr()) }.to_bytes(); + if bytes == b"." || bytes == b".." { + continue; + } + let name = OsString::from_vec(bytes.to_vec()); + let c = match c_name(&name) { + Ok(c) => c, + Err(error) => break Err(error), + }; + let mut stat = std::mem::MaybeUninit::::uninit(); + if unsafe { + libc::fstatat( + root.0.as_raw_fd(), + c.as_ptr(), + stat.as_mut_ptr(), + libc::AT_SYMLINK_NOFOLLOW, + ) + } == 0 + { + // SAFETY: successful fstatat initialized stat. + let stat = unsafe { stat.assume_init() }; + if stat.st_mode & libc::S_IFMT == libc::S_IFREG { + paths.push(root_path.join(name)); + } + } + }; + // SAFETY: directory was returned by fdopendir and closes duplicate. + unsafe { libc::closedir(directory) }; + result + } + + pub(super) fn remove( + root_path: &Path, + root: &RootHandle, + path: &Path, + expected: Option<&FileIdentity>, + ) -> Result<(), String> { + let name = name_for(root_path, path)?; + let file = match open_file(root, &name) { + Ok(file) => file, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(format!("failed to safely open {}: {error}", path.display())); + } + }; + validate_metadata( + path, + &file + .metadata() + .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?, + )?; + let opened_identity = file_identity(&file)?; + if expected.is_some_and(|expected| *expected != opened_identity) { + return Err("process record changed since it was read".to_string()); + } + let mut current = std::mem::MaybeUninit::::uninit(); + // SAFETY: current is writable and name is root-relative. + if unsafe { + libc::fstatat( + root.0.as_raw_fd(), + name.as_ptr(), + current.as_mut_ptr(), + libc::AT_SYMLINK_NOFOLLOW, + ) + } != 0 + { + return Err(format!( + "failed to bind process record deletion: {}", + std::io::Error::last_os_error() + )); + } + // SAFETY: successful fstatat initialized current. + let current = unsafe { current.assume_init() }; + if opened_identity + != (FileIdentity { + device: device_id(current.st_dev), + inode: current.st_ino, + }) + { + return Err("process record changed before deletion".to_string()); + } + // SAFETY: root is retained and name identifies the validated object. + if unsafe { libc::unlinkat(root.0.as_raw_fd(), name.as_ptr(), 0) } != 0 { + let error = std::io::Error::last_os_error(); + if error.kind() != std::io::ErrorKind::NotFound { + return Err(format!("failed to remove {}: {error}", path.display())); + } + } + Ok(()) + } + + pub(super) fn remove_verified( + root_path: &Path, + root: &RootHandle, + path: &Path, + verified: &VerifiedRecord, + ) -> Result<(), String> { + remove(root_path, root, path, Some(&verified.identity)) + } + + pub(super) fn sync(root: &RootHandle) -> Result<(), String> { + // SAFETY: root is a valid retained directory descriptor. + if unsafe { libc::fsync(root.0.as_raw_fd()) } != 0 { + return Err(format!( + "failed to sync process record directory: {}", + std::io::Error::last_os_error() + )); + } + Ok(()) + } + + pub(super) fn validate_metadata(path: &Path, metadata: &fs::Metadata) -> Result<(), String> { + if !metadata.is_file() + || metadata.uid() != unsafe { libc::geteuid() } + || metadata.mode() & 0o077 != 0 + || metadata.nlink() != 1 + { + return Err(format!( + "process record {} is not an owner-private regular file", + path.display() + )); + } + Ok(()) + } +} + +#[cfg(windows)] +mod platform { + use super::*; + use std::ffi::OsStr; + use std::fs::OpenOptions; + use std::mem::{offset_of, size_of, zeroed}; + use std::os::windows::ffi::OsStrExt; + use std::os::windows::fs::{MetadataExt, OpenOptionsExt}; + use std::os::windows::io::{AsRawHandle, FromRawHandle}; + use std::ptr::{null, null_mut}; + use windows_sys::Wdk::Foundation::OBJECT_ATTRIBUTES; + use windows_sys::Wdk::Storage::FileSystem::{ + FileIdBothDirectoryInformation, NtCreateFile, NtQueryDirectoryFile, FILE_CREATE, + FILE_ID_BOTH_DIR_INFORMATION, FILE_NON_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT, + FILE_SYNCHRONOUS_IO_NONALERT, + }; + use windows_sys::Win32::Foundation::{ + CloseHandle, LocalFree, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS, HANDLE, + STATUS_NO_MORE_FILES, STATUS_OBJECT_NAME_NOT_FOUND, UNICODE_STRING, + }; + use windows_sys::Win32::Security::Authorization::{ + GetExplicitEntriesFromAclW, GetSecurityInfo, SetEntriesInAclW, SetSecurityInfo, + EXPLICIT_ACCESS_W, GRANT_ACCESS, SE_FILE_OBJECT, TRUSTEE_IS_SID, TRUSTEE_IS_USER, + TRUSTEE_W, + }; + #[cfg(test)] + use windows_sys::Win32::Security::{CreateWellKnownSid, WinWorldSid, SECURITY_MAX_SID_SIZE}; + use windows_sys::Win32::Security::{ + EqualSid, GetSecurityDescriptorControl, GetTokenInformation, TokenUser, ACL, + DACL_SECURITY_INFORMATION, NO_INHERITANCE, OWNER_SECURITY_INFORMATION, + PROTECTED_DACL_SECURITY_INFORMATION, PSID, SE_DACL_PROTECTED, TOKEN_QUERY, TOKEN_USER, + }; + use windows_sys::Win32::Storage::FileSystem::{ + FileDispositionInfo, FileRenameInfo, GetFileInformationByHandle, + SetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, DELETE, FILE_ALL_ACCESS, + FILE_APPEND_DATA, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, + FILE_ATTRIBUTE_REPARSE_POINT, FILE_DISPOSITION_INFO, FILE_FLAG_BACKUP_SEMANTICS, + FILE_FLAG_OPEN_REPARSE_POINT, FILE_READ_ATTRIBUTES, FILE_READ_DATA, FILE_RENAME_INFO, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_WRITE_ATTRIBUTES, + FILE_WRITE_DATA, READ_CONTROL, SYNCHRONIZE, WRITE_DAC, + }; + use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; + + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + pub(super) struct FileIdentity(u32, u64); + + pub(super) fn file_identity(file: &File) -> Result { + let mut info = std::mem::MaybeUninit::::uninit(); + // SAFETY: `file` owns a valid handle and `info` points to writable storage. + if unsafe { GetFileInformationByHandle(file.as_raw_handle() as HANDLE, info.as_mut_ptr()) } + == 0 + { + return Err(format!( + "failed to identify process record: {}", + std::io::Error::last_os_error() + )); + } + // SAFETY: a successful call initialized the complete structure. + let info = unsafe { info.assume_init() }; + let index = (u64::from(info.nFileIndexHigh) << 32) | u64::from(info.nFileIndexLow); + Ok(FileIdentity(info.dwVolumeSerialNumber, index)) + } + + #[derive(Debug)] + pub(super) struct RootHandle { + directory: File, + } + + struct Handle(HANDLE); + impl Drop for Handle { + fn drop(&mut self) { + if !self.0.is_null() { + // SAFETY: this wrapper uniquely owns the handle. + unsafe { CloseHandle(self.0) }; + } + } + } + + fn nt_success(status: i32) -> bool { + status >= 0 + } + + fn current_user_sid() -> Result<(Vec, PSID), String> { + let mut token = null_mut(); + // SAFETY: token points to writable handle storage. + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) } == 0 { + return Err(format!( + "failed to open process token: {}", + std::io::Error::last_os_error() + )); + } + let token = Handle(token); + let mut length = 0; + // SAFETY: probing required size with a null buffer is documented. + unsafe { GetTokenInformation(token.0, TokenUser, null_mut(), 0, &mut length) }; + if std::io::Error::last_os_error().raw_os_error() != Some(ERROR_INSUFFICIENT_BUFFER as i32) + { + return Err(format!( + "failed to size token user: {}", + std::io::Error::last_os_error() + )); + } + let mut buffer = vec![0u8; length as usize]; + // SAFETY: buffer has the size returned by the preceding probe. + if unsafe { + GetTokenInformation( + token.0, + TokenUser, + buffer.as_mut_ptr().cast(), + length, + &mut length, + ) + } == 0 + { + return Err(format!( + "failed to read token user: {}", + std::io::Error::last_os_error() + )); + } + // SAFETY: successful TokenUser query initialized TOKEN_USER in buffer. + let sid = unsafe { (*(buffer.as_ptr().cast::())).User.Sid }; + Ok((buffer, sid)) + } + + fn secure_for_current_user(file: &File) -> Result<(), String> { + let (_sid_buffer, sid) = current_user_sid()?; + let mut acl: *mut ACL = null_mut(); + let access = EXPLICIT_ACCESS_W { + grfAccessPermissions: FILE_ALL_ACCESS, + grfAccessMode: GRANT_ACCESS, + grfInheritance: NO_INHERITANCE, + Trustee: TRUSTEE_W { + pMultipleTrustee: null_mut(), + MultipleTrusteeOperation: 0, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_USER, + ptstrName: sid.cast(), + }, + }; + // SAFETY: access and acl are valid; Windows allocates acl. + let status = unsafe { SetEntriesInAclW(1, &access, null(), &mut acl) }; + if status != ERROR_SUCCESS { + return Err(format!("failed to create owner-only ACL: {status}")); + } + // SAFETY: the retained file handle and ACL remain valid for this call. + let status = unsafe { + SetSecurityInfo( + file.as_raw_handle() as HANDLE, + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + acl, + null(), + ) + }; + // SAFETY: SetEntriesInAclW allocated acl with LocalAlloc. + unsafe { LocalFree(acl.cast()) }; + if status != ERROR_SUCCESS { + return Err(format!("failed to set owner-only ACL by handle: {status}")); + } + validate_owner_and_acl(file) + } + + fn validate_owner_and_acl(file: &File) -> Result<(), String> { + let (_sid_buffer, current_sid) = current_user_sid()?; + let mut owner: PSID = null_mut(); + let mut dacl: *mut ACL = null_mut(); + let mut descriptor = null_mut(); + // SAFETY: output pointers and retained handle are valid. + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle() as HANDLE, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + &mut owner, + null_mut(), + &mut dacl, + null_mut(), + &mut descriptor, + ) + }; + if status != ERROR_SUCCESS { + return Err(format!("failed to inspect ACL by handle: {status}")); + } + let mut entry_count = 0; + let mut entries: *mut EXPLICIT_ACCESS_W = null_mut(); + let mut control = 0; + let mut revision = 0; + let protected = !descriptor.is_null() + && unsafe { GetSecurityDescriptorControl(descriptor, &mut control, &mut revision) } + != 0 + && control & SE_DACL_PROTECTED != 0; + let entries_status = if dacl.is_null() { + 1 + } else { + // SAFETY: dacl is owned by descriptor; Windows allocates entries. + unsafe { GetExplicitEntriesFromAclW(dacl, &mut entry_count, &mut entries) } + }; + let one_current_user_grant = entries_status == ERROR_SUCCESS + && entry_count == 1 + && !entries.is_null() + && unsafe { + (*entries).grfAccessMode == GRANT_ACCESS + && (*entries).grfAccessPermissions & FILE_ALL_ACCESS == FILE_ALL_ACCESS + && (*entries).Trustee.TrusteeForm == TRUSTEE_IS_SID + && !(*entries).Trustee.ptstrName.is_null() + && EqualSid((*entries).Trustee.ptstrName.cast(), current_sid) != 0 + }; + if !entries.is_null() { + // SAFETY: GetExplicitEntriesFromAclW allocated entries with LocalAlloc. + unsafe { LocalFree(entries.cast()) }; + } + let valid = protected + && !owner.is_null() + && unsafe { EqualSid(owner, current_sid) } != 0 + && one_current_user_grant; + // SAFETY: GetSecurityInfo allocated descriptor with LocalAlloc. + unsafe { LocalFree(descriptor) }; + if !valid { + return Err( + "process record object is not owned solely by the current user".to_string(), + ); + } + Ok(()) + } + + #[cfg(test)] + pub(super) fn assert_everyone_full_control(path: &Path) -> Result<(), String> { + let file = OpenOptions::new() + .read(true) + .access_mode(READ_CONTROL) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS) + .open(path) + .map_err(|error| format!("failed to open mutated ACL object: {error}"))?; + let mut dacl: *mut ACL = null_mut(); + let mut descriptor = null_mut(); + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle() as HANDLE, + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + &mut dacl, + null_mut(), + &mut descriptor, + ) + }; + if status != ERROR_SUCCESS { + return Err(format!("failed to inspect mutated ACL: {status}")); + } + let mut world = vec![0u8; SECURITY_MAX_SID_SIZE as usize]; + let mut world_len = world.len() as u32; + if unsafe { + CreateWellKnownSid( + WinWorldSid, + null_mut(), + world.as_mut_ptr().cast(), + &mut world_len, + ) + } == 0 + { + unsafe { LocalFree(descriptor) }; + return Err(format!( + "failed to construct Everyone SID: {}", + std::io::Error::last_os_error() + )); + } + let mut entry_count = 0; + let mut entries: *mut EXPLICIT_ACCESS_W = null_mut(); + let entries_status = if dacl.is_null() { + 1 + } else { + unsafe { GetExplicitEntriesFromAclW(dacl, &mut entry_count, &mut entries) } + }; + let hostile = entries_status == ERROR_SUCCESS + && !entries.is_null() + && (0..entry_count as usize).any(|index| unsafe { + let entry = &*entries.add(index); + entry.grfAccessMode == GRANT_ACCESS + && entry.grfAccessPermissions & FILE_ALL_ACCESS == FILE_ALL_ACCESS + && entry.Trustee.TrusteeForm == TRUSTEE_IS_SID + && !entry.Trustee.ptstrName.is_null() + && EqualSid(entry.Trustee.ptstrName.cast(), world.as_mut_ptr().cast()) != 0 + }); + if !entries.is_null() { + unsafe { LocalFree(entries.cast()) }; + } + unsafe { LocalFree(descriptor) }; + if !hostile { + return Err("mutated ACL lacks an explicit Everyone full-control grant".to_string()); + } + Ok(()) + } + + fn open_directory(path: &Path) -> Result { + OpenOptions::new() + .read(true) + .access_mode( + FILE_READ_DATA | FILE_READ_ATTRIBUTES | READ_CONTROL | WRITE_DAC | SYNCHRONIZE, + ) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS) + .open(path) + .map_err(|e| format!("failed to safely open {}: {e}", path.display())) + } + + #[derive(Debug)] + enum RelativeFileError { + Invalid(String), + NtStatus(i32), + } + + impl std::fmt::Display for RelativeFileError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Invalid(message) => formatter.write_str(message), + Self::NtStatus(status) => write!( + formatter, + "failed to open process record by retained root: NTSTATUS {status:#x}" + ), + } + } + } + + fn relative_file( + root: &RootHandle, + name: &OsStr, + desired_access: u32, + disposition: u32, + ) -> Result { + let mut name_w: Vec = name.encode_wide().collect(); + if name_w.is_empty() + || name_w + .iter() + .any(|unit| *unit == 0 || *unit == b'\\' as u16 || *unit == b'/' as u16) + { + return Err(RelativeFileError::Invalid( + "process record name is not a single Windows path component".to_string(), + )); + } + let bytes = name_w + .len() + .checked_mul(size_of::()) + .and_then(|value| u16::try_from(value).ok()) + .ok_or_else(|| { + RelativeFileError::Invalid("process record name is too long".to_string()) + })?; + let unicode = UNICODE_STRING { + Length: bytes, + MaximumLength: bytes, + Buffer: name_w.as_mut_ptr(), + }; + let attributes = OBJECT_ATTRIBUTES { + Length: size_of::() as u32, + RootDirectory: root.directory.as_raw_handle() as HANDLE, + ObjectName: &unicode, + Attributes: 0x40, // OBJ_CASE_INSENSITIVE + SecurityDescriptor: null(), + SecurityQualityOfService: null(), + }; + let mut io_status: IO_STATUS_BLOCK = unsafe { zeroed() }; + let mut handle = null_mut(); + // SAFETY: all structures live for the call; the name is root-relative. + let status = unsafe { + NtCreateFile( + &mut handle, + desired_access, + &attributes, + &mut io_status, + null(), + FILE_ATTRIBUTE_NORMAL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + disposition, + FILE_NON_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_REPARSE_POINT, + null(), + 0, + ) + }; + if !nt_success(status) { + return Err(RelativeFileError::NtStatus(status)); + } + // SAFETY: successful NtCreateFile returned one newly owned handle. + Ok(unsafe { File::from_raw_handle(handle.cast()) }) + } + + pub(super) fn open_root(path: &Path) -> Result { + fs::create_dir_all(path) + .map_err(|e| format!("failed to create {}: {e}", path.display()))?; + let directory = open_directory(path)?; + let metadata = directory + .metadata() + .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?; + if !metadata.is_dir() || metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return Err(format!( + "process record root {} is not a real directory", + path.display() + )); + } + secure_for_current_user(&directory)?; + Ok(RootHandle { directory }) + } + + pub(super) fn create( + _root_path: &Path, + root: &RootHandle, + name: &OsStr, + ) -> Result { + validate_owner_and_acl(&root.directory)?; + let file = relative_file( + root, + name, + FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_READ_ATTRIBUTES + | FILE_WRITE_ATTRIBUTES + | READ_CONTROL + | WRITE_DAC + | SYNCHRONIZE + | DELETE, + FILE_CREATE, + ) + .map_err(|error| error.to_string())?; + secure_for_current_user(&file)?; + validate_metadata( + Path::new(name), + &file + .metadata() + .map_err(|e| format!("failed to inspect new process record: {e}"))?, + )?; + Ok(file) + } + + pub(super) fn open(root_path: &Path, root: &RootHandle, path: &Path) -> Result { + super::ensure_direct_child(root_path, path)?; + validate_owner_and_acl(&root.directory)?; + let file = relative_file( + root, + path.file_name().expect("direct child has a name"), + FILE_READ_DATA | FILE_READ_ATTRIBUTES | READ_CONTROL | SYNCHRONIZE | DELETE, + FILE_OPEN, + ) + .map_err(|error| error.to_string())?; + validate_owner_and_acl(&file)?; + Ok(file) + } + + pub(super) fn rename( + root: &RootHandle, + source: &File, + _from: &OsStr, + to: &OsStr, + ) -> Result<(), String> { + validate_owner_and_acl(&root.directory)?; + validate_owner_and_acl(source)?; + let name: Vec = to.encode_wide().collect(); + if name.is_empty() + || name + .iter() + .any(|unit| *unit == 0 || *unit == b'\\' as u16 || *unit == b'/' as u16) + { + return Err("process record destination is not a single component".to_string()); + } + let name_bytes = name + .len() + .checked_mul(size_of::()) + .ok_or_else(|| "destination name is too long".to_string())?; + let total = offset_of!(FILE_RENAME_INFO, FileName) + .checked_add(name_bytes) + .ok_or_else(|| "rename buffer is too large".to_string())?; + let words = total.div_ceil(size_of::()); + let mut storage = vec![0usize; words]; + let info = storage.as_mut_ptr().cast::(); + // SAFETY: storage is aligned and sized for header plus complete UTF-16 name. + unsafe { + (*info).Anonymous.ReplaceIfExists = 0; + (*info).RootDirectory = root.directory.as_raw_handle() as HANDLE; + (*info).FileNameLength = u32::try_from(name_bytes) + .map_err(|_| "destination name is too long".to_string())?; + std::ptr::copy_nonoverlapping( + name.as_ptr(), + std::ptr::addr_of_mut!((*info).FileName).cast(), + name.len(), + ); + } + // SAFETY: source is the exact temp handle; rename target is relative to retained root; replacement is disabled. + if unsafe { + SetFileInformationByHandle( + source.as_raw_handle() as HANDLE, + FileRenameInfo, + storage.as_ptr().cast(), + u32::try_from(total).map_err(|_| "rename buffer is too large".to_string())?, + ) + } == 0 + { + return Err(format!( + "failed to publish process record by handle: {}", + std::io::Error::last_os_error() + )); + } + Ok(()) + } + + fn read_directory_u32(bytes: &[u8], offset: usize) -> Result { + let end = offset + .checked_add(4) + .ok_or_else(|| "directory offset overflow".to_string())?; + let value = bytes + .get(offset..end) + .ok_or_else(|| "truncated directory entry".to_string())?; + Ok(u32::from_le_bytes( + value.try_into().expect("four-byte slice"), + )) + } + + fn parse_directory_entries(bytes: &[u8]) -> Result, String> { + let name_offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileName); + let attributes_offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileAttributes); + let name_length_offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileNameLength); + let mut entries = Vec::new(); + let mut offset = 0usize; + while offset < bytes.len() { + let remaining = &bytes[offset..]; + if remaining.len() < name_offset { + return Err("truncated directory entry header".to_string()); + } + let next = read_directory_u32(remaining, 0)? as usize; + let attributes = read_directory_u32(remaining, attributes_offset)?; + let name_len = read_directory_u32(remaining, name_length_offset)? as usize; + if !name_len.is_multiple_of(2) { + return Err("misaligned UTF-16 directory name".to_string()); + } + let record_end = name_offset + .checked_add(name_len) + .ok_or_else(|| "directory entry overflow".to_string())?; + let name_bytes = remaining + .get(name_offset..record_end) + .ok_or_else(|| "truncated directory name".to_string())?; + let units: Vec = name_bytes + .chunks_exact(2) + .map(|pair| u16::from_le_bytes([pair[0], pair[1]])) + .collect(); + let name = String::from_utf16(&units) + .map_err(|_| "invalid UTF-16 directory name".to_string())?; + entries.push((name, attributes)); + if next == 0 { + if record_end != remaining.len() { + return Err("unexplained trailing directory bytes".to_string()); + } + offset = bytes.len(); + } else { + let minimum_next = record_end + .checked_add(7) + .map(|value| value & !7) + .ok_or_else(|| "directory entry overflow".to_string())?; + if next < minimum_next || !next.is_multiple_of(8) || next > remaining.len() { + return Err("overlapping or misaligned directory entry".to_string()); + } + offset = offset + .checked_add(next) + .ok_or_else(|| "directory offset overflow".to_string())?; + } + } + Ok(entries) + } + + fn is_enumerable_record(name: &str, attributes: u32) -> bool { + name != "." + && name != ".." + && attributes & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DIRECTORY) == 0 + } + + pub(super) fn entries(root_path: &Path, root: &RootHandle) -> Result, String> { + validate_owner_and_acl(&root.directory)?; + let mut paths = Vec::new(); + let mut restart = 1; + loop { + let mut storage = vec![0usize; 64 * 1024 / size_of::()]; + let mut io_status: IO_STATUS_BLOCK = unsafe { zeroed() }; + // SAFETY: buffer and IO status are writable; synchronous retained directory handle remains valid. + let status = unsafe { + NtQueryDirectoryFile( + root.directory.as_raw_handle() as HANDLE, + null_mut(), + None, + null(), + &mut io_status, + storage.as_mut_ptr().cast(), + (storage.len() * size_of::()) as u32, + FileIdBothDirectoryInformation, + 0, + null(), + restart, + ) + }; + restart = 0; + if status == STATUS_NO_MORE_FILES { + break; + } + if !nt_success(status) { + return Err(format!( + "failed to enumerate retained process record root: NTSTATUS {status:#x}" + )); + } + let capacity = storage.len() * size_of::(); + let used = io_status.Information; + if used == 0 || used > capacity { + return Err("invalid process record directory byte count".to_string()); + } + // SAFETY: storage is live and used is bounded by capacity. + let bytes = unsafe { std::slice::from_raw_parts(storage.as_ptr().cast::(), used) }; + for (name, attributes) in parse_directory_entries(bytes)? { + if is_enumerable_record(&name, attributes) { + paths.push(root_path.join(name)); + } + } + } + Ok(paths) + } + + pub(super) fn remove_verified( + root_path: &Path, + _root: &RootHandle, + path: &Path, + verified: &VerifiedRecord, + ) -> Result<(), String> { + super::ensure_direct_child(root_path, path)?; + validate_metadata( + path, + &verified + .file + .metadata() + .map_err(|e| format!("failed to inspect retained {}: {e}", path.display()))?, + )?; + if file_identity(&verified.file)? != verified.identity { + return Err("retained process record identity changed".to_string()); + } + delete_handle(path, &verified.file) + } + + fn delete_handle(path: &Path, file: &File) -> Result<(), String> { + let disposition = FILE_DISPOSITION_INFO { DeleteFile: 1 }; + // SAFETY: disposition applies to the exact retained and validated handle. + if unsafe { + SetFileInformationByHandle( + file.as_raw_handle() as HANDLE, + FileDispositionInfo, + (&disposition as *const FILE_DISPOSITION_INFO).cast(), + size_of::() as u32, + ) + } == 0 + { + return Err(format!( + "failed to remove {} by retained handle: {}", + path.display(), + std::io::Error::last_os_error() + )); + } + Ok(()) + } + + pub(super) fn remove( + root_path: &Path, + root: &RootHandle, + path: &Path, + expected: Option<&FileIdentity>, + ) -> Result<(), String> { + super::ensure_direct_child(root_path, path)?; + let file = match relative_file( + root, + path.file_name().expect("direct child has a name"), + FILE_READ_DATA | FILE_READ_ATTRIBUTES | READ_CONTROL | SYNCHRONIZE | DELETE, + FILE_OPEN, + ) { + Ok(file) => file, + Err(RelativeFileError::NtStatus(STATUS_OBJECT_NAME_NOT_FOUND)) => return Ok(()), + Err(error) => return Err(error.to_string()), + }; + validate_metadata( + path, + &file + .metadata() + .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?, + )?; + let opened_identity = file_identity(&file)?; + if expected.is_some_and(|expected| *expected != opened_identity) { + return Err("process record changed since it was read".to_string()); + } + delete_handle(path, &file) + } + + pub(super) fn sync(_root: &RootHandle) -> Result<(), String> { + // The exact temporary file is flushed before handle-relative rename. + // Windows does not provide a portable directory-fsync equivalent. + Ok(()) + } + + #[cfg(test)] + mod parser_tests { + use super::*; + + fn entry(name: &[u16]) -> Vec { + let name_offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileName); + let length_offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileNameLength); + let mut bytes = vec![0u8; name_offset + name.len() * 2]; + bytes[length_offset..length_offset + 4] + .copy_from_slice(&((name.len() * 2) as u32).to_le_bytes()); + for (pair, unit) in bytes[name_offset..].chunks_exact_mut(2).zip(name) { + pair.copy_from_slice(&unit.to_le_bytes()); + } + bytes + } + + #[test] + fn rejects_truncated_fixed_header() { + assert!(parse_directory_entries(&[0; 4]).is_err()); + } + + #[test] + fn rejects_odd_utf16_length() { + let mut bytes = entry(&[b'a' as u16]); + let offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileNameLength); + bytes[offset..offset + 4].copy_from_slice(&1u32.to_le_bytes()); + assert!(parse_directory_entries(&bytes).is_err()); + } + + #[test] + fn rejects_truncated_or_overflowing_record_end() { + let mut bytes = entry(&[b'a' as u16]); + let offset = offset_of!(FILE_ID_BOTH_DIR_INFORMATION, FileNameLength); + bytes[offset..offset + 4].copy_from_slice(&u32::MAX.to_le_bytes()); + assert!(parse_directory_entries(&bytes).is_err()); + } + + #[test] + fn rejects_overlapping_next_offset() { + let mut bytes = entry(&[b'a' as u16]); + bytes[0..4].copy_from_slice(&8u32.to_le_bytes()); + assert!(parse_directory_entries(&bytes).is_err()); + } + + #[test] + fn rejects_misaligned_next_offset() { + let mut bytes = entry(&[b'a' as u16]); + let next = bytes.len() + 1; + bytes[0..4].copy_from_slice(&(next as u32).to_le_bytes()); + bytes.resize(next, 0); + assert!(parse_directory_entries(&bytes).is_err()); + } + + #[test] + fn rejects_next_offset_beyond_used_bytes() { + let mut bytes = entry(&[b'a' as u16]); + let next = ((bytes.len() + 7) & !7) + 8; + bytes[0..4].copy_from_slice(&(next as u32).to_le_bytes()); + assert!(parse_directory_entries(&bytes).is_err()); + } + + #[test] + fn filters_directories_reparse_points_and_dot_entries() { + assert!(is_enumerable_record("record.json", FILE_ATTRIBUTE_NORMAL)); + assert!(!is_enumerable_record( + "directory.json", + FILE_ATTRIBUTE_DIRECTORY + )); + assert!(!is_enumerable_record( + "reparse.json", + FILE_ATTRIBUTE_REPARSE_POINT + )); + assert!(!is_enumerable_record(".", FILE_ATTRIBUTE_NORMAL)); + assert!(!is_enumerable_record("..", FILE_ATTRIBUTE_NORMAL)); + } + + #[test] + fn rejects_terminal_trailing_bytes_even_when_zero() { + let mut bytes = entry(&[b'a' as u16]); + bytes.push(0); + assert!(parse_directory_entries(&bytes).is_err()); + *bytes.last_mut().unwrap() = 1; + assert!(parse_directory_entries(&bytes).is_err()); + } + } + + pub(super) fn validate_metadata(path: &Path, metadata: &fs::Metadata) -> Result<(), String> { + if !metadata.is_file() || metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return Err(format!( + "process record {} is not a non-reparse regular file", + path.display() + )); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn rejects_oversize_records_before_creating_a_file() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let destination = store.new_record_path(1, 2); + assert!(store + .publish(&destination, &vec![b'x'; MAX_RECORD_BYTES as usize + 1]) + .is_err()); + assert!(!destination.exists()); + } + + #[test] + fn rejects_payload_that_only_exceeds_limit_after_newline() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let destination = store.new_record_path(1, 2); + assert!(store + .publish(&destination, &vec![b'x'; MAX_RECORD_BYTES as usize]) + .is_err()); + assert!(!destination.exists()); + } + + #[cfg(unix)] + #[test] + fn enumeration_duplicate_is_close_on_exec() { + use std::os::fd::RawFd; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let duplicate: RawFd = super::platform::duplicate_root_for_test(&store.handle).unwrap(); + // SAFETY: duplicate remains open until the explicit close below. + let flags = unsafe { libc::fcntl(duplicate, libc::F_GETFD) }; + assert!(flags >= 0); + assert_ne!(flags & libc::FD_CLOEXEC, 0); + // SAFETY: duplicate is owned by this test. + assert_eq!(unsafe { libc::close(duplicate) }, 0); + } + + #[cfg(windows)] + fn install_junction(link: &Path, target: &Path) { + let status = std::process::Command::new("cmd.exe") + .args([ + "/d", + "/c", + "mklink", + "/J", + &link.to_string_lossy(), + &target.to_string_lossy(), + ]) + .status() + .expect("run mklink"); + assert!(status.success(), "create test junction"); + } + + #[cfg(windows)] + #[test] + fn retained_handle_controls_create_enumerate_read_and_delete_after_root_swap() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let retained = temp.path().join("retained"); + let decoy = temp.path().join("decoy"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + fs::rename(&root, &retained).unwrap(); + fs::create_dir(&decoy).unwrap(); + install_junction(&root, &decoy); + + let apparent = store.new_record_path(1, 2); + store.publish(&apparent, b"secure").unwrap(); + let name = apparent.file_name().unwrap(); + assert_eq!(fs::read(retained.join(name)).unwrap(), b"secure\n"); + assert!(!decoy.join(name).exists()); + assert_eq!(store.entries().unwrap(), vec![apparent.clone()]); + assert_eq!(store.read(&apparent).unwrap(), b"secure\n"); + fs::write(decoy.join(name), b"decoy").unwrap(); + store.remove(&apparent).unwrap(); + assert!(!retained.join(name).exists()); + assert_eq!(fs::read(decoy.join(name)).unwrap(), b"decoy"); + } + + #[cfg(windows)] + #[test] + fn reparse_child_is_rejected_without_touching_target() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let target = root.join("target.txt"); + fs::write(&target, b"canary").unwrap(); + let link = root.join("link.json"); + let status = std::process::Command::new("cmd.exe") + .args([ + "/d", + "/c", + "mklink", + &link.to_string_lossy(), + &target.to_string_lossy(), + ]) + .status() + .expect("run mklink"); + assert!(status.success(), "create test symlink"); + assert!(store.read(&link).is_err()); + assert!(store.remove(&link).is_err()); + assert_eq!(fs::read(target).unwrap(), b"canary"); + } + + #[cfg(windows)] + fn replace_dacl_with_everyone_full_control(path: &Path) { + let status = std::process::Command::new("icacls.exe") + .arg(path) + .args(["/inheritance:r", "/grant:r", "*S-1-1-0:(F)"]) + .status() + .expect("run icacls"); + assert!(status.success(), "mutate test DACL"); + super::platform::assert_everyone_full_control(path) + .expect("hostile Everyone full-control ACE must be present"); + } + + #[cfg(windows)] + #[test] + fn destination_substitution_cannot_replace_existing_record() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let destination = root.join("destination.json"); + store.publish(&destination, b"attacker").unwrap(); + let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); + let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); + source.write_all(b"original\n").unwrap(); + source.sync_all().unwrap(); + + assert!(super::platform::rename( + &store.handle, + &source, + temp_name, + destination.file_name().unwrap(), + ) + .is_err()); + assert_eq!(fs::read(&destination).unwrap(), b"attacker\n"); + assert_eq!(fs::read(root.join(temp_name)).unwrap(), b"original\n"); + } + + #[cfg(windows)] + #[test] + fn temp_name_substitution_publishes_exact_retained_handle() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); + let temp_path = root.join(temp_name); + let displaced = root.join("displaced.tmp"); + let destination = root.join("destination.json"); + let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); + source.write_all(b"original\n").unwrap(); + source.sync_all().unwrap(); + fs::rename(&temp_path, &displaced).unwrap(); + fs::write(&temp_path, b"substitute\n").unwrap(); + + super::platform::rename( + &store.handle, + &source, + temp_name, + destination.file_name().unwrap(), + ) + .unwrap(); + + assert_eq!(fs::read(&destination).unwrap(), b"original\n"); + assert_eq!(fs::read(&temp_path).unwrap(), b"substitute\n"); + assert!(!displaced.exists()); + } + + #[cfg(windows)] + #[test] + fn mutated_root_dacl_is_rejected_before_create() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + replace_dacl_with_everyone_full_control(&root); + let destination = store.new_record_path(1, 2); + + assert!(store.publish(&destination, b"record").is_err()); + assert!(!destination.exists()); + } + + #[cfg(windows)] + #[test] + fn mutated_record_dacl_is_rejected_before_read_or_path_delete() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let record = store.new_record_path(1, 2); + store.publish(&record, b"record").unwrap(); + replace_dacl_with_everyone_full_control(&record); + + assert!(store.read(&record).is_err()); + assert!(store.remove(&record).is_err()); + assert_eq!(fs::read(&record).unwrap(), b"record\n"); + } + + #[cfg(windows)] + #[test] + fn exact_handle_deletion_removes_displaced_record_not_successor() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let record = store.new_record_path(1, 2); + store.publish(&record, b"original").unwrap(); + let verified = store.read_verified(&record).unwrap(); + let displaced = root.join("displaced.json"); + fs::rename(&record, &displaced).unwrap(); + store.publish(&record, b"successor").unwrap(); + + store.remove_verified(&record, &verified).unwrap(); + + assert!(!displaced.exists()); + assert_eq!(fs::read(&record).unwrap(), b"successor\n"); + } + + #[test] + fn destination_collision_never_replaces_existing_record() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let destination = store.new_record_path(1, 2); + store.publish(&destination, b"first").unwrap(); + assert!(store.publish(&destination, b"second").is_err()); + assert_eq!(store.read(&destination).unwrap(), b"first\n"); + } + + #[test] + fn publish_read_remove_round_trip_is_complete_and_bounded() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let destination = store.new_record_path(1, 2); + store + .publish(&destination, br#"{"owner_pid":1,"serve_pid":2}"#) + .unwrap(); + assert_eq!( + store.read(&destination).unwrap(), + b"{\"owner_pid\":1,\"serve_pid\":2}\n" + ); + assert_eq!(store.entries().unwrap(), vec![destination.clone()]); + store.remove(&destination).unwrap(); + assert!(!destination.exists()); + } + + #[cfg(unix)] + #[test] + fn existing_owner_private_root_is_repaired_to_writable_mode() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + fs::create_dir(&root).unwrap(); + fs::set_permissions(&root, fs::Permissions::from_mode(0o500)).unwrap(); + + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let destination = store.new_record_path(1, 2); + store.publish(&destination, b"{}").unwrap(); + + assert_eq!( + fs::metadata(root).unwrap().permissions().mode() & 0o777, + 0o700 + ); + } + + #[cfg(unix)] + #[test] + fn removing_missing_record_uses_structured_not_found_result() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + + store.remove(&root.join("missing.json")).unwrap(); + } + + #[cfg(unix)] + #[test] + fn temp_name_substitution_is_rejected_before_publication() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); + let temp_path = root.join(temp_name); + let displaced = root.join("displaced.tmp"); + let destination = root.join("destination.json"); + let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); + source.write_all(b"original\n").unwrap(); + source.sync_all().unwrap(); + fs::rename(&temp_path, &displaced).unwrap(); + fs::write(&temp_path, b"substitute\n").unwrap(); + + assert!(super::platform::rename( + &store.handle, + &source, + temp_name, + destination.file_name().unwrap(), + ) + .is_err()); + + assert!(!destination.exists()); + assert_eq!(fs::read(&temp_path).unwrap(), b"substitute\n"); + assert_eq!(fs::read(&displaced).unwrap(), b"original\n"); + } + + #[cfg(unix)] + #[test] + fn failed_temp_unlink_rolls_back_published_destination() { + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::MetadataExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); + let temp_path = root.join(temp_name); + let destination = root.join("destination.json"); + let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); + source.write_all(b"original\n").unwrap(); + source.sync_all().unwrap(); + let identity = super::platform::file_identity(&source).unwrap(); + let from_name = std::ffi::CString::new(temp_name.as_bytes()).unwrap(); + let mut injected = false; + + let error = super::platform::rename_with_unlink_for_test( + &store.handle, + &source, + temp_name, + destination.file_name().unwrap(), + |root, name| { + if !injected && name == &from_name { + injected = true; + return Err(std::io::Error::from_raw_os_error(libc::EACCES)); + } + super::platform::unlink_name_for_test(root, name) + }, + ) + .unwrap_err(); + + assert!(error.contains("rolled back destination")); + assert!(!destination.exists()); + assert_eq!(fs::metadata(&temp_path).unwrap().nlink(), 1); + assert_eq!(fs::read(&temp_path).unwrap(), b"original\n"); + super::platform::remove(&root, &store.handle, &temp_path, Some(&identity)).unwrap(); + assert!(!temp_path.exists()); + } + + #[cfg(unix)] + #[test] + fn temp_cleanup_rejects_successor_substitution() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); + let temp_path = root.join(temp_name); + let displaced = root.join("displaced.tmp"); + let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); + source.write_all(b"original\n").unwrap(); + source.sync_all().unwrap(); + let identity = super::platform::file_identity(&source).unwrap(); + fs::rename(&temp_path, &displaced).unwrap(); + fs::write(&temp_path, b"successor\n").unwrap(); + fs::set_permissions(&temp_path, fs::Permissions::from_mode(0o600)).unwrap(); + + assert!( + super::platform::remove(&root, &store.handle, &temp_path, Some(&identity),).is_err() + ); + + assert_eq!(fs::read(&temp_path).unwrap(), b"successor\n"); + assert_eq!(fs::read(&displaced).unwrap(), b"original\n"); + } + + #[cfg(unix)] + #[test] + fn verified_removal_rejects_successor_substitution() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let record = store.new_record_path(1, 2); + store.publish(&record, b"original").unwrap(); + let verified = store.read_verified(&record).unwrap(); + let displaced = root.join("displaced.json"); + fs::rename(&record, &displaced).unwrap(); + fs::write(&record, b"successor\n").unwrap(); + fs::set_permissions(&record, fs::Permissions::from_mode(0o600)).unwrap(); + + assert!(store.remove_verified(&record, &verified).is_err()); + assert_eq!(fs::read(&record).unwrap(), b"successor\n"); + assert_eq!(fs::read(&displaced).unwrap(), b"original\n"); + } + + #[cfg(unix)] + #[test] + fn planted_symlink_never_changes_its_target() { + use std::os::unix::fs::symlink; + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root).unwrap(); + let canary = temp.path().join("canary"); + fs::write(&canary, b"unchanged").unwrap(); + let destination = store.new_record_path(1, 2); + symlink(&canary, &destination).unwrap(); + assert!(store.publish(&destination, b"hostile").is_err()); + assert_eq!(fs::read(&canary).unwrap(), b"unchanged"); + assert!(store.read(&destination).is_err()); + assert!(store.remove(&destination).is_err()); + } + + #[cfg(unix)] + #[test] + fn rejects_fifo_hardlink_and_world_readable_records() { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + + let fifo = root.join("fifo.json"); + let fifo_c = CString::new(fifo.as_os_str().as_bytes()).unwrap(); + // SAFETY: fifo_c is a valid NUL-terminated path. + assert_eq!(unsafe { libc::mkfifo(fifo_c.as_ptr(), 0o600) }, 0); + assert!(store.read(&fifo).is_err()); + assert!(store.remove(&fifo).is_err()); + + let original = root.join("original.json"); + fs::write(&original, b"{}").unwrap(); + fs::set_permissions(&original, fs::Permissions::from_mode(0o600)).unwrap(); + let linked = root.join("linked.json"); + fs::hard_link(&original, &linked).unwrap(); + assert!(store.read(&linked).is_err()); + assert!(store.remove(&linked).is_err()); + + let permissive = root.join("permissive.json"); + fs::write(&permissive, b"{}").unwrap(); + fs::set_permissions(&permissive, fs::Permissions::from_mode(0o666)).unwrap(); + assert!(store.read(&permissive).is_err()); + assert!(store.remove(&permissive).is_err()); + } + + #[cfg(unix)] + #[test] + fn bounded_read_rejects_an_oversized_existing_record() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let record = root.join("oversized.json"); + fs::write(&record, vec![b'x'; MAX_RECORD_BYTES as usize + 1]).unwrap(); + fs::set_permissions(&record, fs::Permissions::from_mode(0o600)).unwrap(); + assert!(store.read(&record).is_err()); + } + + #[cfg(unix)] + #[test] + fn oversize_verified_cleanup_rejects_successor_substitution() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let record = root.join("oversized.json"); + fs::write(&record, vec![b'x'; MAX_RECORD_BYTES as usize + 1]).unwrap(); + fs::set_permissions(&record, fs::Permissions::from_mode(0o600)).unwrap(); + let error = store.read_verified_for_cleanup(&record).unwrap_err(); + let verified = error.verified.expect("validated oversize object retained"); + let displaced = root.join("oversized-displaced.json"); + fs::rename(&record, &displaced).unwrap(); + fs::write(&record, b"successor\n").unwrap(); + fs::set_permissions(&record, fs::Permissions::from_mode(0o600)).unwrap(); + + assert!(store.remove_verified(&record, &verified).is_err()); + assert_eq!(fs::read(&record).unwrap(), b"successor\n"); + assert_eq!( + fs::metadata(&displaced).unwrap().len(), + MAX_RECORD_BYTES + 1 + ); + } + + #[cfg(unix)] + #[test] + fn retained_directory_handle_defeats_root_path_swap() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let retained = temp.path().join("retained"); + let decoy = temp.path().join("decoy"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + fs::rename(&root, &retained).unwrap(); + fs::create_dir(&decoy).unwrap(); + symlink(&decoy, &root).unwrap(); + + let apparent = store.new_record_path(1, 2); + store.publish(&apparent, b"secure").unwrap(); + let name = apparent.file_name().unwrap(); + assert_eq!(fs::read(retained.join(name)).unwrap(), b"secure\n"); + assert!(!decoy.join(name).exists()); + assert_eq!(store.read(&apparent).unwrap(), b"secure\n"); + store.remove(&apparent).unwrap(); + assert!(!retained.join(name).exists()); + } + + #[cfg(unix)] + #[test] + fn concurrent_publications_are_complete_and_distinct() { + use std::sync::Arc; + + let temp = tempfile::tempdir().unwrap(); + let store = Arc::new(ProcessRecordStore::open(temp.path().join("records")).unwrap()); + let workers: Vec<_> = (0..16) + .map(|index| { + let store = Arc::clone(&store); + std::thread::spawn(move || { + let path = store.new_record_path(index, index as u64); + let payload = format!("record-{index}"); + store.publish(&path, payload.as_bytes()).unwrap(); + (path, payload) + }) + }) + .collect(); + let results: Vec<_> = workers + .into_iter() + .map(|worker| worker.join().unwrap()) + .collect(); + assert_eq!(store.entries().unwrap().len(), results.len()); + for (path, payload) in results { + assert_eq!( + store.read(&path).unwrap(), + format!("{payload}\n").as_bytes() + ); + } + } + + #[cfg(unix)] + #[test] + fn directory_and_records_are_owner_private_under_permissive_umask() { + use std::os::unix::fs::PermissionsExt; + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let destination = store.new_record_path(1, 2); + store.publish(&destination, b"{}").unwrap(); + assert_eq!( + fs::metadata(root).unwrap().permissions().mode() & 0o777, + 0o700 + ); + assert_eq!( + fs::metadata(destination).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } +} diff --git a/src-tauri/src/services/process.rs b/src-tauri/src/services/process.rs index 2b9f14bf8..2647f2b65 100644 --- a/src-tauri/src/services/process.rs +++ b/src-tauri/src/services/process.rs @@ -55,7 +55,7 @@ pub(crate) fn process_is_alive(pid: ProcessId) -> bool { #[cfg(windows)] mod windows_identity; -#[cfg(windows)] +#[cfg(any(unix, windows))] #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub(crate) struct ProcessIdentity { pub pid: u32, @@ -63,6 +63,13 @@ pub(crate) struct ProcessIdentity { pub exe: String, } +#[cfg(unix)] +impl ProcessIdentity { + fn matches(&self, other: &Self) -> bool { + self == other + } +} + #[cfg(windows)] impl ProcessIdentity { fn matches(&self, other: &Self) -> bool { @@ -72,7 +79,7 @@ impl ProcessIdentity { } } -#[cfg(windows)] +#[cfg(any(unix, windows))] #[derive(Debug, PartialEq, Eq)] pub(crate) enum IdentityProbe { Matches, @@ -125,6 +132,59 @@ pub(crate) fn kill_process(pid: ProcessId) -> bool { unsafe { libc::kill(pid, libc::SIGKILL) == 0 } } +#[cfg(target_os = "linux")] +pub(crate) fn capture_process_identity(pid: u32) -> std::io::Result { + use std::os::unix::fs::MetadataExt; + + let exe_link = std::path::PathBuf::from(format!("/proc/{pid}/exe")); + let metadata = std::fs::metadata(&exe_link)?; + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat"))?; + let close = stat + .rfind(')') + .ok_or_else(|| std::io::Error::other("malformed proc stat"))?; + let created_at = stat[close + 2..] + .split_whitespace() + .nth(19) + .ok_or_else(|| std::io::Error::other("missing proc start token"))? + .parse::() + .map_err(std::io::Error::other)?; + Ok(ProcessIdentity { + pid, + created_at, + exe: format!("{}:{}", metadata.dev(), metadata.ino()), + }) +} + +#[cfg(target_os = "macos")] +pub(crate) fn capture_process_identity(_pid: u32) -> std::io::Result { + // `proc_pidpath` plus pathname metadata is not process-bound and races + // executable replacement. Fail closed until a validated process-vnode API + // supplies the executable identity used for both capture and probing. + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "macOS process-bound executable identity is unavailable", + )) +} + +#[cfg(all(unix, not(any(target_os = "linux", target_os = "macos"))))] +pub(crate) fn capture_process_identity(_pid: u32) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "process-bound executable identity is unavailable on this Unix target", + )) +} + +#[cfg(unix)] +pub(crate) fn probe_process_identity(identity: &ProcessIdentity) -> IdentityProbe { + match capture_process_identity(identity.pid) { + Ok(current) if current.matches(identity) => IdentityProbe::Matches, + Ok(_) => IdentityProbe::Mismatch, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => IdentityProbe::Gone, + Err(error) if error.raw_os_error() == Some(libc::ESRCH) => IdentityProbe::Gone, + Err(_) => IdentityProbe::Unverifiable, + } +} + #[cfg(windows)] pub(crate) fn capture_process_identity(pid: u32) -> std::io::Result { windows_identity::capture(pid) @@ -153,16 +213,6 @@ pub(crate) unsafe fn process_identity_from_handle( unsafe { windows_identity::identity_from_handle(handle as _) } } -/// # Safety -/// `handle` must remain a valid process handle with terminate and synchronize access. -#[cfg(windows)] -pub(crate) unsafe fn terminate_process_handle( - handle: *mut std::ffi::c_void, - wait: std::time::Duration, -) -> std::io::Result<()> { - unsafe { windows_identity::terminate_handle(handle as _, wait) } -} - #[cfg(windows)] pub(crate) fn kill_process_if_identity_matches( identity: &ProcessIdentity, From d8f97b1b893075450152eba13cbcd41eaac9f6e5 Mon Sep 17 00:00:00 2001 From: Budzeg <78171487+budzeg@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:56:20 -0600 Subject: [PATCH 2/5] fix: bind backend recovery to retained objects Retain publication handles, use process-bound Linux signals, revalidate Windows deletion ACLs, and retain unreaped children across startup retries. Stream recovery within its time budget and run the focused native suites in the existing CI gates. Document the Unix evidence-retention fallback. Generated with Codex Signed-off-by: Olabode Olaoke --- docs/process-recovery.md | 21 + justfile | 6 + scripts/windows/CI-Windows.ps1 | 5 + src-tauri/src/services/acp/goose_serve.rs | 411 +++++++------ .../src/services/acp/process_record_store.rs | 567 +++++++++--------- src-tauri/src/services/process.rs | 162 ++++- 6 files changed, 689 insertions(+), 483 deletions(-) create mode 100644 docs/process-recovery.md diff --git a/docs/process-recovery.md b/docs/process-recovery.md new file mode 100644 index 000000000..f85ca1dde --- /dev/null +++ b/docs/process-recovery.md @@ -0,0 +1,21 @@ +# Backend process recovery + +Berd stores one compact, owner-private recovery record per backend under the app data directory's `processes/berd-serve` folder. Legacy shared-temp JSON records are never read or migrated. Upgrading can leave an older backend for manual cleanup or a reboot. + +Publication retains the file it created through startup and shutdown. A terminal newline commits a single-line payload; readers reject incomplete writes. Windows publishes and deletes through retained file handles and revalidates the record and directory ACLs before deletion. Unix creates the final name exclusively and never unlinks by pathname: there is no portable atomic operation that deletes only the retained file when another process can replace its name. Partial and completed records therefore remain as recovery evidence on Unix, including after a clean shutdown. + +Stale recovery requires an exact owner identity and an orphaned backend identity. Linux probes through a retained `/proc` directory descriptor and signals that same process with `pidfd_send_signal`; unsupported kernels retain evidence without falling back to a numeric PID signal. Windows checks and terminates through one process handle. macOS cannot yet establish the required executable identity, so it retains evidence without signaling stale records. Normal shutdown still terminates and reaps the owned child on all platforms. + +A failed startup retains any unreaped child in memory. Another startup must finish that teardown before it can spawn a replacement; the app exit path also retries it. A child already reaped by readiness checks counts as confirmed exited. + +Recovery streams directory entries off the async runtime through a buffer of at most 256 paths. Enumeration and process cleanup have a two-second startup budget. Scans continue beyond the first buffer while time remains. Exhausting the budget leaves unprocessed evidence for a later attempt; recovery does not guarantee a full scan of an arbitrarily large directory. Retained Unix records can consume disk space over time. + +## Validation + +`just tauri-test` and the Windows-native `just ci-windows` gate include these focused suites: + +- `services::acp::process_record_store`: publication substitution, partial writes, ACL changes, exact deletion, directory parsing, and scan buffering. +- `services::acp::goose_serve::recovery_tests`: failed startup, already-reaped children, retry ownership, signal escalation, and retained evidence. +- `services::process::`: process identity and the Linux exit-between-probe-and-signal regression. + +Run the native Linux and Windows CI jobs before merging; compilation on macOS cannot validate those operating-system operations. diff --git a/justfile b/justfile index 27c54a6b1..4845e0ff7 100644 --- a/justfile +++ b/justfile @@ -237,6 +237,9 @@ _tauri-test-skill-marketplace: [unix] _tauri-test-unix: + just _tauri-cargo-unix test --lib services::acp::process_record_store + just _tauri-cargo-unix test --lib services::acp::goose_serve::recovery_tests + just _tauri-cargo-unix test --lib services::process:: # rust-cache can restore Sherpa's generated cache directory without its native libraries. if [ "$(uname -s)" = "Linux" ]; then rm -rf src-tauri/target/sherpa-onnx-prebuilt; fi just _tauri-cargo-unix test -p tauri-plugin-berdctl --features server @@ -251,6 +254,9 @@ _tauri-test-skill-marketplace: [windows] _tauri-test-windows: + just _tauri-cargo-windows test --lib services::acp::process_record_store + just _tauri-cargo-windows test --lib services::acp::goose_serve::recovery_tests + just _tauri-cargo-windows test --lib services::process:: just _tauri-cargo-windows test -p tauri-plugin-berdctl --features server just _tauri-cargo-windows test -p berdctl just _tauri-cargo-windows test --lib telemetry diff --git a/scripts/windows/CI-Windows.ps1 b/scripts/windows/CI-Windows.ps1 index e83627ebd..7fc1e224f 100644 --- a/scripts/windows/CI-Windows.ps1 +++ b/scripts/windows/CI-Windows.ps1 @@ -58,6 +58,11 @@ Invoke-CargoCheck -ArgumentList @( "test", "--lib", "commands::system::tests::windows_chrome_launch_" ) -Label "cargo test Windows Chrome launch" +# SEC-007 recovery needs native handle, ACL and process-lifetime coverage. +foreach ($filter in @("services::acp::process_record_store", "services::acp::goose_serve::recovery_tests", "services::process::")) { + Invoke-CargoCheck -ArgumentList @("test", "--lib", $filter) -Label "cargo test $filter" +} + # Clippy compiles both configurations, so separate `cargo check` calls only # repeat the same compile coverage. Invoke-CargoCheck -ArgumentList @( diff --git a/src-tauri/src/services/acp/goose_serve.rs b/src-tauri/src/services/acp/goose_serve.rs index fe86cadf9..f9a0ed509 100644 --- a/src-tauri/src/services/acp/goose_serve.rs +++ b/src-tauri/src/services/acp/goose_serve.rs @@ -10,7 +10,7 @@ use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; -use super::process_record_store::ProcessRecordStore; +use super::process_record_store::{ProcessRecordStore, VerifiedRecord, RECOVERY_TIMEOUT}; use crate::services::diagnostic_log::{ self, DiagnosticCategory, DiagnosticFieldValue, DiagnosticLevel, }; @@ -22,7 +22,7 @@ use crate::services::log_redaction::redact_log_line; use crate::services::managed_acp_tools; use crate::services::path_env; #[cfg(unix)] -use crate::services::process::{kill_process, pid_t_from_u32, terminate_process}; +use crate::services::process::{pid_t_from_u32, terminate_process}; use crate::services::process::{IdentityProbe, ProcessIdentity}; use tokio::io::{AsyncBufReadExt, AsyncRead, BufReader}; @@ -61,6 +61,36 @@ pub struct GooseServeProcess { /// Global singleton — initialised once at app startup. static GOOSE_SERVE: OnceCell = OnceCell::const_new(); +struct FailedStartup { + child: Child, + store: ProcessRecordStore, + path: PathBuf, + record: Option, +} + +static FAILED_STARTUP: tokio::sync::Mutex> = + tokio::sync::Mutex::const_new(None); + +async fn retry_failed_startup(pending: &mut Option) -> Result<(), String> { + if let Some(failed) = pending.as_mut() { + stop_child_and_reap(&mut failed.child) + .await + .map_err(|error| { + format!("Previous goose serve startup has not been reaped: {error}") + })?; + if let Some(record) = &failed.record { + remove_verified_or_warn( + &failed.store, + &failed.path, + record, + "failed startup retry confirmed exit", + ); + } + *pending = None; + } + Ok(()) +} + impl GooseServeProcess { /// Return the WebSocket URL for connecting to this server. pub fn ws_url(&self) -> String { @@ -108,6 +138,9 @@ impl GooseServeProcess { /// Kill the singleton goose serve process if it exists. Called from the /// app exit handler. pub async fn kill_singleton() { + if let Err(error) = retry_failed_startup(&mut *FAILED_STARTUP.lock().await).await { + log::warn!("{error}"); + } if let Some(process) = GOOSE_SERVE.get() { process.kill().await; } @@ -115,6 +148,7 @@ impl GooseServeProcess { async fn spawn(app_handle: tauri::AppHandle) -> Result { let process_started_at = Instant::now(); + retry_failed_startup(&mut *FAILED_STARTUP.lock().await).await?; // Kill any orphaned goose serve process left by a previous run // (e.g. tauri dev hot-reload). @@ -272,27 +306,23 @@ impl GooseServeProcess { ); #[cfg(unix)] - { - let publication = write_pid_file(&process_record_store, &process_record_path, &child); - if publication.is_err() { - log::warn!("Failed to publish goose serve recovery record; stopping child and failing startup"); - } - require_published_record(&mut child, publication).await?; - } - + let publication = write_pid_file(&process_record_store, &process_record_path, &child); #[cfg(windows)] - { - let publication = - write_process_record(&process_record_store, &process_record_path, &child); - if publication.is_err() { - log::warn!("Failed to publish goose serve recovery record; stopping child and failing startup"); + let publication = write_process_record(&process_record_store, &process_record_path, &child); + let process_record = match require_published_record(&mut child, publication).await { + Ok(record) => record, + Err(error) => { + if child.id().is_some() { + *FAILED_STARTUP.lock().await = Some(FailedStartup { + child, + store: process_record_store, + path: process_record_path, + record: None, + }); + } + return Err(error); } - require_published_record(&mut child, publication).await?; - } - - let process_record = - retain_published_record(&process_record_store, &process_record_path, &mut child) - .await?; + }; spawn_log_reader(child.stdout.take(), "stdout"); spawn_log_reader(child.stderr.take(), "stderr"); @@ -331,6 +361,14 @@ impl GooseServeProcess { &error, ) .await; + if child.id().is_some() { + *FAILED_STARTUP.lock().await = Some(FailedStartup { + child, + store: process_record_store, + path: process_record_path, + record: Some(process_record), + }); + } return Err(error); } } @@ -430,6 +468,14 @@ where async fn stop_child_and_reap(child: &mut Child) -> Result<(), String> { bounded_child_teardown(CHILD_TEARDOWN_TIMEOUT, async { + // try_wait caches exit status; readiness may already have reaped this child. + if child + .try_wait() + .map_err(|error| format!("failed to inspect child: {error}"))? + .is_some() + { + return Ok(()); + } #[cfg(unix)] { let pid = child.id().and_then(pid_t_from_u32).ok_or_else(|| { @@ -515,64 +561,23 @@ fn finish_published_child_teardown( } } -async fn retain_published_record_with( - store: &ProcessRecordStore, - path: &Path, +async fn require_published_record( child: &mut Child, - retain: F, -) -> Result -where - F: FnOnce( - &ProcessRecordStore, - &Path, - ) -> Result< - super::process_record_store::VerifiedRecord, - super::process_record_store::VerifiedReadError, - >, -{ - match retain(store, path) { + publication: Result, +) -> Result { + match publication { Ok(record) => Ok(record), Err(error) => { - let startup_error = format!( - "Failed to retain published goose serve recovery record {}: {}", - path.display(), - error.message - ); - teardown_published_child(store, path, error.verified.as_ref(), child, &startup_error) - .await; - Err(startup_error) + stop_child_and_reap(child).await.map_err(|teardown_error| { + format!("Failed to publish goose serve recovery record: {error}; child teardown also failed: {teardown_error}") + })?; + Err(format!( + "Failed to publish goose serve recovery record: {error}" + )) } } } -async fn retain_published_record( - store: &ProcessRecordStore, - path: &Path, - child: &mut Child, -) -> Result { - retain_published_record_with(store, path, child, |store, path| { - store.read_verified_for_cleanup(path) - }) - .await -} - -async fn require_published_record( - child: &mut Child, - publication: Result<(), String>, -) -> Result<(), String> { - if let Err(error) = publication { - stop_child_and_reap(child).await.map_err(|teardown_error| { - format!( - "Failed to publish goose serve recovery record: {error}; child teardown also failed: {teardown_error}" - ) - })?; - return Err(format!( - "Failed to publish goose serve recovery record: {error}" - )); - } - Ok(()) -} - /// Legacy single-slot PID file used before per-owner process records. It is /// unsafe when multiple dev worktrees share the same Tauri executable path, so /// new launches remove it without killing the recorded process. @@ -595,7 +600,11 @@ fn fnv1a(bytes: &[u8]) -> u64 { } #[cfg(unix)] -fn write_pid_file(store: &ProcessRecordStore, path: &Path, child: &Child) -> Result<(), String> { +fn write_pid_file( + store: &ProcessRecordStore, + path: &Path, + child: &Child, +) -> Result { let serve_pid = child.id().ok_or_else(|| "child has no pid".to_string())?; let owner_identity = crate::services::process::capture_process_identity(std::process::id()); let serve_identity = crate::services::process::capture_process_identity(serve_pid); @@ -609,7 +618,7 @@ fn write_pid_file(store: &ProcessRecordStore, path: &Path, child: &Child) -> Res (Ok(owner), Ok(serve)) => (Some(owner), Some(serve)), _ => { // macOS currently cannot bind executable vnode identity to a PID - // without a pathname race. Publish a deletion-only record so normal + // without a pathname race. Publish an evidence-only record so normal // startup works, but stale recovery can never authorize signaling. (None, None) } @@ -630,7 +639,7 @@ fn write_process_record( store: &ProcessRecordStore, path: &Path, child: &Child, -) -> Result<(), String> { +) -> Result { let handle = child .raw_handle() .ok_or_else(|| "child has no process handle".to_string())?; @@ -660,7 +669,8 @@ fn write_process_record( async fn kill_stale_serve_process(store: &ProcessRecordStore) { remove_legacy_pid_file(); - let entries = match store.entries() { + let deadline = tokio::time::Instant::now() + RECOVERY_TIMEOUT; + let mut entries = match store.scan() { Ok(entries) => entries, Err(error) => { log::warn!("Failed to enumerate goose serve process records: {error}"); @@ -668,11 +678,17 @@ async fn kill_stale_serve_process(store: &ProcessRecordStore) { } }; - for path in entries { + while let Ok(Some(path)) = tokio::time::timeout_at(deadline, entries.recv()).await { if !is_process_record_path(&path) { continue; } - cleanup_process_record(store, &path).await; + if tokio::time::timeout_at(deadline, cleanup_process_record(store, &path)) + .await + .is_err() + { + log::warn!("Goose serve recovery budget exhausted; retaining remaining records"); + break; + } } } @@ -800,10 +816,23 @@ async fn cleanup_orphaned_serve_process( ); return; }; - match crate::services::process::kill_process_if_identity_matches( - identity, - Duration::from_secs(5), - ) { + let identity = identity.clone(); + let target = identity.clone(); + let outcome = tokio::task::spawn_blocking(move || { + crate::services::process::kill_process_if_identity_matches( + &target, + Duration::from_millis(250), + ) + }) + .await; + let outcome = match outcome { + Ok(outcome) => outcome, + Err(error) => { + log::warn!("Windows recovery worker failed: {error}"); + return; + } + }; + match outcome { Ok(outcome) if outcome.exit_confirmed() => { remove_verified_or_warn(store, path, verified, "confirmed Windows process exit"); } @@ -820,28 +849,54 @@ async fn cleanup_orphaned_serve_process( } } -#[cfg(unix)] +#[cfg(target_os = "linux")] async fn cleanup_orphaned_serve_process( store: &ProcessRecordStore, path: &Path, - verified: &super::process_record_store::VerifiedRecord, + verified: &VerifiedRecord, record: &ServeProcessRecord, ) { + let Some(identity) = &record.serve_identity else { + return; + }; + let target = match crate::services::process::RetainedProcess::open(identity.pid) { + Ok(target) => target, + Err(error) => { + log::warn!( + "Cannot retain stale process {}: {error}; keeping recovery evidence", + identity.pid + ); + return; + } + }; cleanup_orphaned_serve_process_with_ops( store, path, verified, record, - crate::services::process::probe_process_identity, - terminate_process, - kill_process, + |identity| target.probe(identity), + || target.signal(libc::SIGTERM).is_ok(), + || target.signal(libc::SIGKILL).is_ok(), Duration::from_millis(200), Duration::from_millis(50), ) .await; } -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "linux")))] +async fn cleanup_orphaned_serve_process( + _store: &ProcessRecordStore, + path: &Path, + _verified: &VerifiedRecord, + _record: &ServeProcessRecord, +) { + log::debug!( + "Process-bound recovery is unavailable; retaining {}", + path.display() + ); +} + +#[cfg(any(target_os = "linux", all(unix, test)))] #[allow(clippy::too_many_arguments)] async fn cleanup_orphaned_serve_process_with_ops( store: &ProcessRecordStore, @@ -855,8 +910,8 @@ async fn cleanup_orphaned_serve_process_with_ops( kill_delay: Duration, ) where FProbe: FnMut(&ProcessIdentity) -> IdentityProbe, - FTerm: FnMut(crate::services::process::ProcessId) -> bool, - FKill: FnMut(crate::services::process::ProcessId) -> bool, + FTerm: FnMut() -> bool, + FKill: FnMut() -> bool, { let Some(identity) = &record.serve_identity else { log::warn!( @@ -890,15 +945,7 @@ async fn cleanup_orphaned_serve_process_with_ops( None, diagnostic_log::fields([("pid", (identity.pid as i64).into())]), ); - let Some(pid) = pid_t_from_u32(identity.pid) else { - log::warn!( - "Invalid stale serve PID {}; keeping {}", - identity.pid, - path.display() - ); - return; - }; - if !terminate(pid) { + if !terminate() { match probe(identity) { IdentityProbe::Gone | IdentityProbe::Mismatch => { remove_verified_or_warn( @@ -939,7 +986,7 @@ async fn cleanup_orphaned_serve_process_with_ops( None, diagnostic_log::fields([("pid", (identity.pid as i64).into())]), ); - if !kill(pid) { + if !kill() { match probe(identity) { IdentityProbe::Gone | IdentityProbe::Mismatch => { remove_verified_or_warn( @@ -1376,38 +1423,8 @@ pub(crate) fn reserve_free_port() -> Result { } #[cfg(test)] -mod tests { - #[cfg(all(unix, not(target_os = "macos")))] - use super::cleanup_process_record; - #[cfg(unix)] - use super::ServeProcessRecord; - use super::{ - acp_websocket_url, add_release_webview_origin_arg, apply_goose_search_paths_env, - apply_runtime_goose_provider_env, apply_shell_env_with_extended_path, - apply_shell_env_with_extended_path_inner, require_published_record, stop_child_and_reap, - DATABRICKS_HOST_ENV, TAURI_WEBVIEW_ORIGIN, - }; - use crate::commands::runtime_config::default_runtime_config; - #[cfg(unix)] - use crate::services::acp::process_record_store::ProcessRecordStore; - #[cfg(unix)] - use crate::services::process::IdentityProbe; - use std::collections::HashMap; - use std::ffi::OsString; - use std::path::{Path, PathBuf}; - #[cfg(unix)] - use std::time::Duration; - use tokio::process::Command; - - fn env_value(command: &Command, key: &str) -> Option { - command.as_std().get_envs().find_map(|(k, v)| { - if k == key { - v.map(|value| value.to_os_string()) - } else { - None - } - }) - } +mod recovery_tests { + use super::*; #[tokio::test] async fn process_record_publication_failure_kills_and_reaps_the_child() { @@ -1469,7 +1486,7 @@ mod tests { #[cfg(unix)] #[tokio::test] - async fn readiness_failure_reaps_child_and_exact_deletes_retained_record() { + async fn readiness_failure_reaps_child_and_retains_unix_evidence() { let temp = tempfile::tempdir().expect("temp dir"); let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); let path = store.new_record_path(std::process::id(), 1); @@ -1496,41 +1513,47 @@ mod tests { -1, "child must no longer exist" ); - assert!(!path.exists(), "confirmed exit permits exact cleanup"); + assert!( + path.exists(), + "Unix retains evidence without pathname deletion" + ); } #[cfg(unix)] #[tokio::test] - async fn post_publication_retention_failure_reaps_child_but_keeps_record() { - let temp = tempfile::tempdir().expect("temp dir"); - let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + async fn already_reaped_child_teardown_succeeds() { + let mut child = Command::new("sh").args(["-c", "exit 0"]).spawn().unwrap(); + child.wait().await.unwrap(); + stop_child_and_reap(&mut child) + .await + .expect("already-reaped child is confirmed exited"); + } + + #[cfg(unix)] + #[tokio::test] + async fn next_startup_reaps_retained_failed_child_even_with_live_owner() { + let temp = tempfile::tempdir().unwrap(); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); let path = store.new_record_path(std::process::id(), 1); - store.publish(&path, b"record").expect("publish"); - let mut child = Command::new("sh") - .args(["-c", "sleep 30"]) + let record = store.publish(&path, b"{}").unwrap(); + let child = Command::new("sleep") + .arg("30") + .kill_on_drop(true) .spawn() - .expect("spawn child"); - let pid = child.id().expect("child pid"); - - let error = super::retain_published_record_with(&store, &path, &mut child, |_, _| { - Err(super::super::process_record_store::VerifiedReadError { - message: "forced retention failure".to_string(), - verified: None, - }) - }) - .await - .expect_err("retention failure must abort startup"); - - assert!(error.contains("forced retention failure")); - assert!(child.id().is_none(), "wait must reap the child"); - assert_eq!( - unsafe { libc::kill(pid as i32, 0) }, - -1, - "child must no longer exist" - ); + .unwrap(); + let pid = child.id().unwrap(); + let mut pending = Some(super::FailedStartup { + child, + store, + path: path.clone(), + record: Some(record), + }); + super::retry_failed_startup(&mut pending).await.unwrap(); + assert!(pending.is_none()); + assert_eq!(unsafe { libc::kill(pid as i32, 0) }, -1); assert!( path.exists(), - "without the originally retained object, exact cleanup must not reopen by path" + "Unix retains evidence without pathname deletion" ); } @@ -1571,7 +1594,7 @@ mod tests { #[cfg(unix)] #[tokio::test] - async fn failed_sigterm_removes_record_when_follow_up_probe_confirms_gone() { + async fn failed_sigterm_confirms_gone_without_unsafe_unix_deletion() { use std::collections::VecDeque; let temp = tempfile::tempdir().expect("temp dir"); @@ -1593,19 +1616,22 @@ mod tests { &verified, &record, |_| probes.pop_front().expect("scripted probe"), - |_| false, - |_| panic!("SIGKILL must not run after failed SIGTERM"), + || false, + || panic!("SIGKILL must not run after failed SIGTERM"), Duration::ZERO, Duration::ZERO, ) .await; - assert!(!path.exists(), "confirmed exit permits exact cleanup"); + assert!( + path.exists(), + "Unix retains evidence without pathname deletion" + ); } #[cfg(unix)] #[tokio::test] - async fn failed_sigkill_removes_record_when_follow_up_probe_confirms_mismatch() { + async fn failed_sigkill_confirms_mismatch_without_unsafe_unix_deletion() { use std::collections::VecDeque; let temp = tempfile::tempdir().expect("temp dir"); @@ -1631,14 +1657,17 @@ mod tests { &verified, &record, |_| probes.pop_front().expect("scripted probe"), - |_| true, - |_| false, + || true, + || false, Duration::ZERO, Duration::ZERO, ) .await; - assert!(!path.exists(), "confirmed mismatch permits exact cleanup"); + assert!( + path.exists(), + "Unix retains evidence without pathname deletion" + ); } #[cfg(unix)] @@ -1674,8 +1703,8 @@ mod tests { &verified, &record, |_| probes.pop_front().expect("scripted probe"), - |_| true, - move |_| { + || true, + move || { killed_for_closure.set(true); true }, @@ -1685,7 +1714,10 @@ mod tests { .await; assert!(!killed.get(), "identity mismatch must suppress SIGKILL"); - assert!(!path.exists(), "mismatched identity is confirmed gone"); + assert!( + path.exists(), + "Unix retains evidence without pathname deletion" + ); } #[cfg(unix)] @@ -1721,8 +1753,8 @@ mod tests { &verified, &record, |_| probes.pop_front().expect("scripted probe"), - |_| true, - |_| true, + || true, + || true, Duration::ZERO, Duration::ZERO, ) @@ -1733,7 +1765,7 @@ mod tests { #[cfg(target_os = "macos")] #[tokio::test] - async fn macos_publishes_deletion_only_record_when_identity_is_unavailable() { + async fn macos_publishes_evidence_only_record_when_identity_is_unavailable() { let temp = tempfile::tempdir().expect("temp dir"); let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); let path = store.new_record_path(std::process::id(), 1); @@ -1742,7 +1774,7 @@ mod tests { .spawn() .expect("spawn child"); - super::write_pid_file(&store, &path, &child).expect("publish deletion-only record"); + super::write_pid_file(&store, &path, &child).expect("publish evidence-only record"); let bytes = store .read_verified_for_cleanup(&path) .expect("read record") @@ -1884,7 +1916,10 @@ mod tests { cleanup_process_record(&store, &path).await; - assert!(!path.exists(), "recycled-PID record should be removed"); + assert!( + path.exists(), + "Unix retains evidence without pathname deletion" + ); assert!( unrelated .try_wait() @@ -1895,6 +1930,30 @@ mod tests { unrelated.kill().await.expect("kill unrelated child"); unrelated.wait().await.expect("reap unrelated child"); } +} + +#[cfg(test)] +mod tests { + use super::{ + acp_websocket_url, add_release_webview_origin_arg, apply_goose_search_paths_env, + apply_runtime_goose_provider_env, apply_shell_env_with_extended_path, + apply_shell_env_with_extended_path_inner, DATABRICKS_HOST_ENV, TAURI_WEBVIEW_ORIGIN, + }; + use crate::commands::runtime_config::default_runtime_config; + use std::collections::HashMap; + use std::ffi::OsString; + use std::path::{Path, PathBuf}; + use tokio::process::Command; + + fn env_value(command: &Command, key: &str) -> Option { + command.as_std().get_envs().find_map(|(k, v)| { + if k == key { + v.map(|value| value.to_os_string()) + } else { + None + } + }) + } #[test] fn acp_websocket_url_includes_secret_key_token() { diff --git a/src-tauri/src/services/acp/process_record_store.rs b/src-tauri/src/services/acp/process_record_store.rs index 6d7fe0dac..a99d07baf 100644 --- a/src-tauri/src/services/acp/process_record_store.rs +++ b/src-tauri/src/services/acp/process_record_store.rs @@ -3,6 +3,8 @@ use std::io::{Read, Write}; use std::path::{Path, PathBuf}; const MAX_RECORD_BYTES: u64 = 4 * 1024; +const SCAN_BUFFER_ENTRIES: usize = 256; +pub(super) const RECOVERY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(2); #[cfg(unix)] const RECORD_DIR_MODE: u32 = 0o700; #[cfg(unix)] @@ -12,7 +14,6 @@ const RECORD_FILE_MODE: u32 = 0o600; pub(super) struct VerifiedRecord { pub(super) bytes: Vec, identity: platform::FileIdentity, - #[cfg(windows)] file: File, } @@ -42,46 +43,73 @@ impl ProcessRecordStore { )) } - pub(super) fn publish(&self, destination: &Path, bytes: &[u8]) -> Result<(), String> { - let stored_len = bytes - .len() - .checked_add(1) - .ok_or_else(|| format!("process record exceeds {MAX_RECORD_BYTES} bytes"))?; - if stored_len as u64 > MAX_RECORD_BYTES { - return Err(format!("process record exceeds {MAX_RECORD_BYTES} bytes")); + /// The returned handle is the object we created, never a reopened pathname. + pub(super) fn publish( + &self, + destination: &Path, + bytes: &[u8], + ) -> Result { + self.publish_with(destination, bytes, |_| Ok(())) + } + + fn publish_with( + &self, + destination: &Path, + bytes: &[u8], + after_create: F, + ) -> Result + where + F: FnOnce(&File) -> Result<(), String>, + { + // A single terminal newline commits the compact, single-line payload. + // Readers reject an interrupted write, including an otherwise valid JSON prefix. + if bytes.len() as u64 >= MAX_RECORD_BYTES || bytes.contains(&b'\n') { + return Err(format!( + "process record must be one line and fit within {MAX_RECORD_BYTES} bytes" + )); } ensure_direct_child(&self.root, destination)?; - let temp = self.root.join(format!( + #[cfg(unix)] + let staging = destination.to_path_buf(); + #[cfg(windows)] + let staging = self.root.join(format!( ".process-record-{}.tmp", uuid::Uuid::new_v4().simple() )); - let mut temp_identity = None; + // Unix has no portable handle-bound rename/unlink. Exclusive creation + // avoids both the source-name race and a destructive rollback by name. + let mut file = + platform::create(&self.root, &self.handle, staging.file_name().expect("name"))?; + let identity = platform::file_identity(&file)?; let result = (|| { - let mut file = platform::create( - &self.root, - &self.handle, - temp.file_name().expect("temp has a name"), - )?; - temp_identity = Some(platform::file_identity(&file)?); + after_create(&file)?; file.write_all(bytes) .and_then(|_| file.write_all(b"\n")) .and_then(|_| file.sync_all()) - .map_err(|error| format!("failed to write {}: {error}", temp.display()))?; + .map_err(|error| format!("failed to write {}: {error}", staging.display()))?; + #[cfg(windows)] platform::rename( &self.handle, &file, - temp.file_name().expect("temp has a name"), - destination.file_name().expect("destination has a name"), + staging.file_name().expect("name"), + destination.file_name().expect("name"), )?; - platform::sync(&self.handle)?; - Ok(()) + platform::sync(&self.handle) })(); - if result.is_err() { - if let Some(identity) = temp_identity.as_ref() { - let _ = platform::remove(&self.root, &self.handle, &temp, Some(identity)); - } + let mut stored = bytes.to_vec(); + stored.push(b'\n'); + let verified = VerifiedRecord { + bytes: stored, + identity, + file, + }; + if let Err(error) = result { + // Windows removes only the retained object. Unix deliberately keeps + // partial evidence: unlinkat cannot atomically match an open file. + let _ = self.remove_verified(&staging, &verified); + return Err(error); } - result + Ok(verified) } pub(super) fn read_verified_for_cleanup( @@ -112,12 +140,6 @@ impl ProcessRecordStore { verified: None, })?; if metadata.len() > MAX_RECORD_BYTES { - #[cfg(unix)] - let record = VerifiedRecord { - bytes: Vec::new(), - identity, - }; - #[cfg(windows)] let record = VerifiedRecord { bytes: Vec::new(), identity, @@ -140,9 +162,6 @@ impl ProcessRecordStore { verified: None, })?; if bytes.len() as u64 > MAX_RECORD_BYTES { - #[cfg(unix)] - let record = VerifiedRecord { bytes, identity }; - #[cfg(windows)] let record = VerifiedRecord { bytes, identity, @@ -156,14 +175,20 @@ impl ProcessRecordStore { verified: Some(record), }); } - #[cfg(unix)] - return Ok(VerifiedRecord { bytes, identity }); - #[cfg(windows)] - Ok(VerifiedRecord { + let record = VerifiedRecord { bytes, identity, file, - }) + }; + if !record.bytes.ends_with(b"\n") + || record.bytes[..record.bytes.len() - 1].contains(&b'\n') + { + return Err(VerifiedReadError { + message: "incomplete or multiline process record".to_string(), + verified: Some(record), + }); + } + Ok(record) })(); read } @@ -179,8 +204,30 @@ impl ProcessRecordStore { self.read_verified(path).map(|record| record.bytes) } + /// Enumerate off the async runtime with bounded memory. Dropping the receiver + /// at the recovery deadline also releases a producer waiting on a full buffer. + pub(super) fn scan(&self) -> Result, String> { + let root = self.root.clone(); + let handle = self.handle.try_clone().map_err(|error| error.to_string())?; + let (sender, receiver) = tokio::sync::mpsc::channel(SCAN_BUFFER_ENTRIES); + tokio::task::spawn_blocking(move || { + if let Err(error) = + platform::entries(&root, &handle, |path| sender.blocking_send(path).is_ok()) + { + log::warn!("Failed to enumerate recovery records: {error}"); + } + }); + Ok(receiver) + } + + #[cfg(test)] pub(super) fn entries(&self) -> Result, String> { - platform::entries(&self.root, &self.handle) + let mut paths = Vec::new(); + platform::entries(&self.root, &self.handle, |path| { + paths.push(path); + true + })?; + Ok(paths) } pub(super) fn remove_verified( @@ -241,6 +288,12 @@ mod platform { #[derive(Debug)] pub(super) struct RootHandle(OwnedFd); + impl RootHandle { + pub(super) fn try_clone(&self) -> std::io::Result { + self.0.try_clone().map(Self) + } + } + fn c_name(name: &OsStr) -> Result { CString::new(name.as_bytes()).map_err(|_| "process record name contains NUL".to_string()) } @@ -374,122 +427,16 @@ mod platform { .map_err(|error| format!("failed to safely open {}: {error}", path.display())) } - fn unlink_name(root: &RootHandle, name: &CString) -> std::io::Result<()> { - // SAFETY: root is retained and name is a root-relative direct child. - if unsafe { libc::unlinkat(root.0.as_raw_fd(), name.as_ptr(), 0) } != 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) - } - - fn rename_with_unlink( - root: &RootHandle, - source: &File, - from: &OsStr, - to: &OsStr, - mut unlink: F, - ) -> Result<(), String> - where - F: FnMut(&RootHandle, &CString) -> std::io::Result<()>, - { - let from = c_name(from)?; - let to = c_name(to)?; - let source_identity = file_identity(source)?; - let mut current = std::mem::MaybeUninit::::uninit(); - // SAFETY: current is writable and from is a root-relative name. - if unsafe { - libc::fstatat( - root.0.as_raw_fd(), - from.as_ptr(), - current.as_mut_ptr(), - libc::AT_SYMLINK_NOFOLLOW, - ) - } != 0 - { - return Err(format!( - "failed to bind process record publication: {}", - std::io::Error::last_os_error() - )); - } - // SAFETY: successful fstatat initialized current. - let current = unsafe { current.assume_init() }; - if source_identity - != (FileIdentity { - device: device_id(current.st_dev), - inode: current.st_ino, - }) - { - return Err("temporary process record changed before publication".to_string()); - } - // linkat is an atomic no-replace publication primitive: it fails if - // the destination exists, then unlinkat removes the temporary name. - // Both operations are anchored to the retained same-directory fd, and - // the source name was just verified against the retained source handle. - if unsafe { - libc::linkat( - root.0.as_raw_fd(), - from.as_ptr(), + fn duplicate_root(root: &RootHandle) -> std::io::Result { + // A fresh open file description gives each scan its own directory offset. + // SAFETY: root is retained and the literal is a NUL-terminated directory name. + let duplicate = unsafe { + libc::openat( root.0.as_raw_fd(), - to.as_ptr(), - 0, + c".".as_ptr(), + libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC, ) - } != 0 - { - return Err(format!( - "failed to publish process record without replacement: {}", - std::io::Error::last_os_error() - )); - } - if let Err(temp_error) = unlink(root, &from) { - // Publication created `to` as a second name for the retained source. - // Roll it back before the caller cleans the identity-bound temp name, - // otherwise both names retain nlink == 2 and fail metadata validation. - let rollback_error = unlink(root, &to).err(); - return Err(match rollback_error { - Some(rollback_error) => format!( - "published process record but failed to remove temporary name: {temp_error}; \ - failed to roll back destination: {rollback_error}" - ), - None => format!( - "published process record but failed to remove temporary name: {temp_error}; \ - rolled back destination" - ), - }); - } - Ok(()) - } - - pub(super) fn rename( - root: &RootHandle, - source: &File, - from: &OsStr, - to: &OsStr, - ) -> Result<(), String> { - rename_with_unlink(root, source, from, to, unlink_name) - } - - #[cfg(test)] - pub(super) fn unlink_name_for_test(root: &RootHandle, name: &CString) -> std::io::Result<()> { - unlink_name(root, name) - } - - #[cfg(test)] - pub(super) fn rename_with_unlink_for_test( - root: &RootHandle, - source: &File, - from: &OsStr, - to: &OsStr, - unlink: F, - ) -> Result<(), String> - where - F: FnMut(&RootHandle, &CString) -> std::io::Result<()>, - { - rename_with_unlink(root, source, from, to, unlink) - } - - fn duplicate_root(root: &RootHandle) -> std::io::Result { - // SAFETY: F_DUPFD_CLOEXEC returns an independent close-on-exec descriptor. - let duplicate = unsafe { libc::fcntl(root.0.as_raw_fd(), libc::F_DUPFD_CLOEXEC, 0) }; + }; if duplicate < 0 { return Err(std::io::Error::last_os_error()); } @@ -501,7 +448,11 @@ mod platform { duplicate_root(root) } - pub(super) fn entries(root_path: &Path, root: &RootHandle) -> Result, String> { + pub(super) fn entries( + root_path: &Path, + root: &RootHandle, + mut visit: impl FnMut(PathBuf) -> bool, + ) -> Result<(), String> { let duplicate = duplicate_root(root) .map_err(|error| format!("failed to duplicate process record directory: {error}"))?; // SAFETY: fdopendir takes ownership of duplicate. @@ -513,8 +464,11 @@ mod platform { std::io::Error::last_os_error() )); } - let mut paths = Vec::new(); + let started = std::time::Instant::now(); let result = loop { + if started.elapsed() >= RECOVERY_TIMEOUT { + break Ok(()); + } errno::set_errno(errno::Errno(0)); // SAFETY: directory remains valid until closed below. let entry = unsafe { libc::readdir(directory) }; @@ -526,7 +480,7 @@ mod platform { std::io::Error::from_raw_os_error(read_error.0) )); } - break Ok(paths); + break Ok(()); } // SAFETY: d_name is NUL-terminated for a valid dirent. let bytes = unsafe { std::ffi::CStr::from_ptr((*entry).d_name.as_ptr()) }.to_bytes(); @@ -550,8 +504,8 @@ mod platform { { // SAFETY: successful fstatat initialized stat. let stat = unsafe { stat.assume_init() }; - if stat.st_mode & libc::S_IFMT == libc::S_IFREG { - paths.push(root_path.join(name)); + if stat.st_mode & libc::S_IFMT == libc::S_IFREG && !visit(root_path.join(name)) { + break Ok(()); } } }; @@ -560,73 +514,36 @@ mod platform { result } + #[cfg(test)] pub(super) fn remove( root_path: &Path, root: &RootHandle, path: &Path, - expected: Option<&FileIdentity>, + _expected: Option<&FileIdentity>, ) -> Result<(), String> { let name = name_for(root_path, path)?; - let file = match open_file(root, &name) { - Ok(file) => file, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(error) => { - return Err(format!("failed to safely open {}: {error}", path.display())); - } - }; - validate_metadata( - path, - &file - .metadata() - .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?, - )?; - let opened_identity = file_identity(&file)?; - if expected.is_some_and(|expected| *expected != opened_identity) { - return Err("process record changed since it was read".to_string()); + match open_file(root, &name) { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + _ => Err( + "handle-bound record deletion is unavailable on Unix; retaining evidence" + .to_string(), + ), } - let mut current = std::mem::MaybeUninit::::uninit(); - // SAFETY: current is writable and name is root-relative. - if unsafe { - libc::fstatat( - root.0.as_raw_fd(), - name.as_ptr(), - current.as_mut_ptr(), - libc::AT_SYMLINK_NOFOLLOW, - ) - } != 0 - { - return Err(format!( - "failed to bind process record deletion: {}", - std::io::Error::last_os_error() - )); - } - // SAFETY: successful fstatat initialized current. - let current = unsafe { current.assume_init() }; - if opened_identity - != (FileIdentity { - device: device_id(current.st_dev), - inode: current.st_ino, - }) - { - return Err("process record changed before deletion".to_string()); - } - // SAFETY: root is retained and name identifies the validated object. - if unsafe { libc::unlinkat(root.0.as_raw_fd(), name.as_ptr(), 0) } != 0 { - let error = std::io::Error::last_os_error(); - if error.kind() != std::io::ErrorKind::NotFound { - return Err(format!("failed to remove {}: {error}", path.display())); - } - } - Ok(()) } pub(super) fn remove_verified( root_path: &Path, - root: &RootHandle, + _root: &RootHandle, path: &Path, verified: &VerifiedRecord, ) -> Result<(), String> { - remove(root_path, root, path, Some(&verified.identity)) + ensure_direct_child(root_path, path)?; + if file_identity(&verified.file)? != verified.identity { + return Err("retained process record identity changed".to_string()); + } + // A last-moment fstatat still leaves a race before unlinkat. Never + // delete a pathname on the strength of an earlier identity check. + Err("handle-bound record deletion is unavailable on Unix; retaining evidence".to_string()) } pub(super) fn sync(root: &RootHandle) -> Result<(), String> { @@ -724,6 +641,14 @@ mod platform { directory: File, } + impl RootHandle { + pub(super) fn try_clone(&self) -> std::io::Result { + Ok(Self { + directory: self.directory.try_clone()?, + }) + } + } + struct Handle(HANDLE); impl Drop for Handle { fn drop(&mut self) { @@ -1213,7 +1138,10 @@ mod platform { .checked_add(7) .map(|value| value & !7) .ok_or_else(|| "directory entry overflow".to_string())?; - if next < minimum_next || !next.is_multiple_of(8) || next > remaining.len() { + if next < minimum_next + || !next.is_multiple_of(8) + || next > remaining.len().saturating_sub(name_offset) + { return Err("overlapping or misaligned directory entry".to_string()); } offset = offset @@ -1230,11 +1158,15 @@ mod platform { && attributes & (FILE_ATTRIBUTE_REPARSE_POINT | FILE_ATTRIBUTE_DIRECTORY) == 0 } - pub(super) fn entries(root_path: &Path, root: &RootHandle) -> Result, String> { + pub(super) fn entries( + root_path: &Path, + root: &RootHandle, + mut visit: impl FnMut(PathBuf) -> bool, + ) -> Result<(), String> { validate_owner_and_acl(&root.directory)?; - let mut paths = Vec::new(); let mut restart = 1; - loop { + let started = std::time::Instant::now(); + while started.elapsed() < RECOVERY_TIMEOUT { let mut storage = vec![0usize; 64 * 1024 / size_of::()]; let mut io_status: IO_STATUS_BLOCK = unsafe { zeroed() }; // SAFETY: buffer and IO status are writable; synchronous retained directory handle remains valid. @@ -1270,21 +1202,23 @@ mod platform { // SAFETY: storage is live and used is bounded by capacity. let bytes = unsafe { std::slice::from_raw_parts(storage.as_ptr().cast::(), used) }; for (name, attributes) in parse_directory_entries(bytes)? { - if is_enumerable_record(&name, attributes) { - paths.push(root_path.join(name)); + if is_enumerable_record(&name, attributes) && !visit(root_path.join(name)) { + return Ok(()); } } } - Ok(paths) + Ok(()) } pub(super) fn remove_verified( root_path: &Path, - _root: &RootHandle, + root: &RootHandle, path: &Path, verified: &VerifiedRecord, ) -> Result<(), String> { super::ensure_direct_child(root_path, path)?; + validate_owner_and_acl(&root.directory)?; + validate_owner_and_acl(&verified.file)?; validate_metadata( path, &verified @@ -1319,6 +1253,7 @@ mod platform { Ok(()) } + #[cfg(test)] pub(super) fn remove( root_path: &Path, root: &RootHandle, @@ -1326,6 +1261,7 @@ mod platform { expected: Option<&FileIdentity>, ) -> Result<(), String> { super::ensure_direct_child(root_path, path)?; + validate_owner_and_acl(&root.directory)?; let file = match relative_file( root, path.file_name().expect("direct child has a name"), @@ -1342,6 +1278,7 @@ mod platform { .metadata() .map_err(|e| format!("failed to inspect {}: {e}", path.display()))?, )?; + validate_owner_and_acl(&file)?; let opened_identity = file_identity(&file)?; if expected.is_some_and(|expected| *expected != opened_identity) { return Err("process record changed since it was read".to_string()); @@ -1416,6 +1353,15 @@ mod platform { assert!(parse_directory_entries(&bytes).is_err()); } + #[test] + fn rejects_nonterminal_offset_at_end_of_buffer() { + let mut bytes = entry(&[b'a' as u16]); + let next = (bytes.len() + 7) & !7; + bytes.resize(next, 0); + bytes[0..4].copy_from_slice(&(next as u32).to_le_bytes()); + assert!(parse_directory_entries(&bytes).is_err()); + } + #[test] fn filters_directories_reparse_points_and_dot_entries() { assert!(is_enumerable_record("record.json", FILE_ATTRIBUTE_NORMAL)); @@ -1656,6 +1602,47 @@ mod tests { assert_eq!(fs::read(&record).unwrap(), b"record\n"); } + #[cfg(windows)] + #[test] + fn retained_deletion_revalidates_record_and_root_acls() { + for change_root in [false, true] { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("records"); + let store = ProcessRecordStore::open(root.clone()).unwrap(); + let path = store.new_record_path(1, 2); + let retained = store.publish(&path, b"original").unwrap(); + replace_dacl_with_everyone_full_control(if change_root { &root } else { &path }); + assert!(store.remove_verified(&path, &retained).is_err()); + drop(retained); + assert_eq!(fs::read(&path).unwrap(), b"original\n"); + } + } + + #[cfg(windows)] + #[test] + fn failed_publication_deletes_retained_object_not_replacement() { + let temp = tempfile::tempdir().unwrap(); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); + let path = store.new_record_path(1, 2); + let displaced = store.root.join("displaced.json"); + let replacement = std::cell::RefCell::new(None); + assert!(store + .publish_with(&path, b"original", |_| { + let staging = store.entries().unwrap().pop().unwrap(); + fs::rename(&staging, &displaced).unwrap(); + store.publish(&staging, b"successor").unwrap(); + *replacement.borrow_mut() = Some(staging); + Err("injected write failure".to_string()) + }) + .is_err()); + assert!(!displaced.exists()); + assert_eq!( + fs::read(replacement.into_inner().unwrap()).unwrap(), + b"successor\n" + ); + assert!(!path.exists()); + } + #[cfg(windows)] #[test] fn exact_handle_deletion_removes_displaced_record_not_successor() { @@ -1670,6 +1657,7 @@ mod tests { store.publish(&record, b"successor").unwrap(); store.remove_verified(&record, &verified).unwrap(); + drop(verified); // Windows completes disposition when the retained handle closes. assert!(!displaced.exists()); assert_eq!(fs::read(&record).unwrap(), b"successor\n"); @@ -1700,8 +1688,16 @@ mod tests { b"{\"owner_pid\":1,\"serve_pid\":2}\n" ); assert_eq!(store.entries().unwrap(), vec![destination.clone()]); - store.remove(&destination).unwrap(); - assert!(!destination.exists()); + #[cfg(windows)] + { + store.remove(&destination).unwrap(); + assert!(!destination.exists()); + } + #[cfg(unix)] + { + assert!(store.remove(&destination).is_err()); + assert!(destination.exists()); + } } #[cfg(unix)] @@ -1736,73 +1732,76 @@ mod tests { #[cfg(unix)] #[test] - fn temp_name_substitution_is_rejected_before_publication() { + fn publication_retains_created_object_across_name_substitution() { let temp = tempfile::tempdir().unwrap(); - let root = temp.path().join("records"); - let store = ProcessRecordStore::open(root.clone()).unwrap(); - let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); - let temp_path = root.join(temp_name); - let displaced = root.join("displaced.tmp"); - let destination = root.join("destination.json"); - let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); - source.write_all(b"original\n").unwrap(); - source.sync_all().unwrap(); - fs::rename(&temp_path, &displaced).unwrap(); - fs::write(&temp_path, b"substitute\n").unwrap(); - - assert!(super::platform::rename( - &store.handle, - &source, - temp_name, - destination.file_name().unwrap(), - ) - .is_err()); - - assert!(!destination.exists()); - assert_eq!(fs::read(&temp_path).unwrap(), b"substitute\n"); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); + let path = store.new_record_path(1, 2); + let displaced = store.root.join("displaced.json"); + let record = store + .publish_with(&path, b"original", |_| { + fs::rename(&path, &displaced).unwrap(); + store.publish(&path, b"successor").unwrap(); + Ok(()) + }) + .unwrap(); + assert_eq!( + record.identity, + platform::file_identity(&File::open(&displaced).unwrap()).unwrap() + ); assert_eq!(fs::read(&displaced).unwrap(), b"original\n"); + assert_eq!(fs::read(&path).unwrap(), b"successor\n"); + assert!(store.remove_verified(&path, &record).is_err()); + assert!(path.exists() && displaced.exists()); } #[cfg(unix)] #[test] - fn failed_temp_unlink_rolls_back_published_destination() { - use std::os::unix::ffi::OsStrExt; - use std::os::unix::fs::MetadataExt; - + fn failed_publication_does_not_unlink_a_successor() { let temp = tempfile::tempdir().unwrap(); - let root = temp.path().join("records"); - let store = ProcessRecordStore::open(root.clone()).unwrap(); - let temp_name = std::ffi::OsStr::new("controlled-temp.tmp"); - let temp_path = root.join(temp_name); - let destination = root.join("destination.json"); - let mut source = super::platform::create(&root, &store.handle, temp_name).unwrap(); - source.write_all(b"original\n").unwrap(); - source.sync_all().unwrap(); - let identity = super::platform::file_identity(&source).unwrap(); - let from_name = std::ffi::CString::new(temp_name.as_bytes()).unwrap(); - let mut injected = false; - - let error = super::platform::rename_with_unlink_for_test( - &store.handle, - &source, - temp_name, - destination.file_name().unwrap(), - |root, name| { - if !injected && name == &from_name { - injected = true; - return Err(std::io::Error::from_raw_os_error(libc::EACCES)); - } - super::platform::unlink_name_for_test(root, name) - }, - ) - .unwrap_err(); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); + let path = store.new_record_path(1, 2); + let displaced = store.root.join("partial.json"); + assert!(store + .publish_with(&path, b"original", |_| { + fs::rename(&path, &displaced).unwrap(); + store.publish(&path, b"successor").unwrap(); + Err("injected write failure".to_string()) + }) + .is_err()); + assert_eq!(fs::read(&path).unwrap(), b"successor\n"); + assert!(store.read(&displaced).is_err()); + } - assert!(error.contains("rolled back destination")); - assert!(!destination.exists()); - assert_eq!(fs::metadata(&temp_path).unwrap().nlink(), 1); - assert_eq!(fs::read(&temp_path).unwrap(), b"original\n"); - super::platform::remove(&root, &store.handle, &temp_path, Some(&identity)).unwrap(); - assert!(!temp_path.exists()); + #[test] + fn incomplete_payload_is_never_a_published_record() { + let temp = tempfile::tempdir().unwrap(); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); + let path = store.new_record_path(1, 2); + let mut file = + platform::create(&store.root, &store.handle, path.file_name().unwrap()).unwrap(); + file.write_all(b"{}").unwrap(); + assert!(store.read(&path).is_err()); + file.write_all(b"\n").unwrap(); + assert_eq!(store.read(&path).unwrap(), b"{}\n"); + } + + #[tokio::test] + async fn bounded_scan_continues_past_its_buffer_capacity() { + let temp = tempfile::tempdir().unwrap(); + let store = ProcessRecordStore::open(temp.path().join("records")).unwrap(); + for i in 0..SCAN_BUFFER_ENTRIES + 5 { + store + .publish(&store.new_record_path(1, i as u64), b"{}") + .unwrap(); + } + for _ in 0..2 { + let mut entries = store.scan().unwrap(); + let mut count = 0; + while entries.recv().await.is_some() { + count += 1; + } + assert_eq!(count, SCAN_BUFFER_ENTRIES + 5); + } } #[cfg(unix)] @@ -1963,8 +1962,8 @@ mod tests { assert_eq!(fs::read(retained.join(name)).unwrap(), b"secure\n"); assert!(!decoy.join(name).exists()); assert_eq!(store.read(&apparent).unwrap(), b"secure\n"); - store.remove(&apparent).unwrap(); - assert!(!retained.join(name).exists()); + assert!(store.remove(&apparent).is_err()); + assert!(retained.join(name).exists()); } #[cfg(unix)] diff --git a/src-tauri/src/services/process.rs b/src-tauri/src/services/process.rs index 2647f2b65..2414a759f 100644 --- a/src-tauri/src/services/process.rs +++ b/src-tauri/src/services/process.rs @@ -126,33 +126,101 @@ pub(crate) fn terminate_process(pid: ProcessId) -> bool { unsafe { libc::kill(pid, libc::SIGTERM) == 0 } } -#[cfg(unix)] -pub(crate) fn kill_process(pid: ProcessId) -> bool { - // SAFETY: sending SIGKILL as a last resort to a process id we previously recorded. - unsafe { libc::kill(pid, libc::SIGKILL) == 0 } +/// A proc directory descriptor pins one Linux process even after PID reuse. +/// pidfd_send_signal accepts this descriptor; there is deliberately no kill(pid) fallback. +#[cfg(target_os = "linux")] +pub(crate) struct RetainedProcess { + directory: std::fs::File, + pid: u32, +} + +#[cfg(target_os = "linux")] +impl RetainedProcess { + pub(crate) fn open(pid: u32) -> std::io::Result { + use std::os::unix::fs::OpenOptionsExt; + let directory = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY | libc::O_CLOEXEC | libc::O_NOFOLLOW) + .open(format!("/proc/{pid}"))?; + Ok(Self { directory, pid }) + } + + fn identity(&self) -> std::io::Result { + use std::io::Read; + use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::unix::fs::MetadataExt; + let open = |name: &std::ffi::CStr, flags| { + // SAFETY: the process directory is retained, and names are constant direct children. + let fd = unsafe { + libc::openat( + self.directory.as_raw_fd(), + name.as_ptr(), + flags | libc::O_CLOEXEC, + ) + }; + if fd < 0 { + return Err(std::io::Error::last_os_error()); + } + // SAFETY: openat returned a new owned descriptor. + Ok(unsafe { std::fs::File::from_raw_fd(fd) }) + }; + let metadata = open(c"exe", libc::O_PATH)?.metadata()?; + let mut stat = String::new(); + open(c"stat", libc::O_RDONLY | libc::O_NOFOLLOW)? + .take(4096) + .read_to_string(&mut stat)?; + let fields = stat + .rsplit_once(") ") + .ok_or_else(|| std::io::Error::other("malformed proc stat"))? + .1; + let created_at = fields + .split_whitespace() + .nth(19) + .ok_or_else(|| std::io::Error::other("missing proc start token"))? + .parse::() + .map_err(std::io::Error::other)?; + Ok(ProcessIdentity { + pid: self.pid, + created_at, + exe: format!("{}:{}", metadata.dev(), metadata.ino()), + }) + } + + pub(crate) fn probe(&self, expected: &ProcessIdentity) -> IdentityProbe { + match self.identity() { + Ok(current) if current.matches(expected) => IdentityProbe::Matches, + Ok(_) => IdentityProbe::Mismatch, + Err(error) if matches!(error.raw_os_error(), Some(libc::ENOENT | libc::ESRCH)) => { + IdentityProbe::Gone + } + Err(_) => IdentityProbe::Unverifiable, + } + } + + pub(crate) fn signal(&self, signal: libc::c_int) -> std::io::Result<()> { + use std::os::fd::AsRawFd; + // SAFETY: the syscall targets the retained process, never its numeric PID. + // Unsupported kernels return an error and recovery retains its evidence. + let result = unsafe { + libc::syscall( + libc::SYS_pidfd_send_signal, + self.directory.as_raw_fd(), + signal, + std::ptr::null::(), + 0u32, + ) + }; + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } + } } #[cfg(target_os = "linux")] pub(crate) fn capture_process_identity(pid: u32) -> std::io::Result { - use std::os::unix::fs::MetadataExt; - - let exe_link = std::path::PathBuf::from(format!("/proc/{pid}/exe")); - let metadata = std::fs::metadata(&exe_link)?; - let stat = std::fs::read_to_string(format!("/proc/{pid}/stat"))?; - let close = stat - .rfind(')') - .ok_or_else(|| std::io::Error::other("malformed proc stat"))?; - let created_at = stat[close + 2..] - .split_whitespace() - .nth(19) - .ok_or_else(|| std::io::Error::other("missing proc start token"))? - .parse::() - .map_err(std::io::Error::other)?; - Ok(ProcessIdentity { - pid, - created_at, - exe: format!("{}:{}", metadata.dev(), metadata.ino()), - }) + RetainedProcess::open(pid)?.identity() } #[cfg(target_os = "macos")] @@ -273,6 +341,54 @@ mod tests { ); } + #[cfg(target_os = "linux")] + #[test] + fn retained_process_signals_cannot_follow_a_reused_pid() { + for signal in [libc::SIGTERM, libc::SIGKILL] { + let mut child = std::process::Command::new("sleep") + .arg("30") + .spawn() + .unwrap(); + let retained = RetainedProcess::open(child.id()).unwrap(); + let identity = capture_process_identity(child.id()).unwrap(); + assert_eq!(retained.probe(&identity), IdentityProbe::Matches); + // Force exit in the exact window between the successful probe and + // signal. A later process must not be reachable through this handle. + child.kill().unwrap(); + child.wait().unwrap(); + let mut successor = std::process::Command::new("sleep") + .arg("30") + .spawn() + .unwrap(); + let result = retained.signal(signal); + let successor_alive = successor.try_wait().unwrap().is_none(); + successor.kill().unwrap(); + successor.wait().unwrap(); + assert_eq!(result.unwrap_err().raw_os_error(), Some(libc::ESRCH)); + assert!(successor_alive); + assert_eq!(retained.probe(&identity), IdentityProbe::Gone); + } + } + + #[cfg(target_os = "linux")] + #[test] + fn retained_process_signal_targets_the_verified_child() { + let mut child = std::process::Command::new("sleep") + .arg("30") + .spawn() + .unwrap(); + let retained = RetainedProcess::open(child.id()).unwrap(); + let identity = capture_process_identity(child.id()).unwrap(); + assert_eq!(retained.probe(&identity), IdentityProbe::Matches); + let result = retained.signal(libc::SIGKILL); + if result.is_err() { + child.kill().unwrap(); + } + child.wait().unwrap(); + result.expect("Linux recovery requires pidfd_send_signal"); + assert_eq!(retained.probe(&identity), IdentityProbe::Gone); + } + #[cfg(unix)] #[test] fn pid_t_from_u32_accepts_pid_t_boundary() { From 4c6bfb37827f36e71ee03de0ead55522bbd43868 Mon Sep 17 00:00:00 2001 From: Olabode Olaoke Date: Fri, 11 Sep 2026 18:05:57 -0600 Subject: [PATCH 3/5] fix: initialize Windows recovery record ownership Set the current user as owner when securing objects created with the token default owner. Elevated Windows runners can otherwise create Administrators-owned objects that immediately fail strict validation. Reject unrelated owners before repair and retain owner-only ACL validation. Generated with Codex Signed-off-by: Olabode Olaoke --- .../src/services/acp/process_record_store.rs | 76 ++++++++++++++++--- 1 file changed, 64 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/services/acp/process_record_store.rs b/src-tauri/src/services/acp/process_record_store.rs index a99d07baf..24ed73615 100644 --- a/src-tauri/src/services/acp/process_record_store.rs +++ b/src-tauri/src/services/acp/process_record_store.rs @@ -588,9 +588,11 @@ mod platform { FILE_ID_BOTH_DIR_INFORMATION, FILE_NON_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT, FILE_SYNCHRONOUS_IO_NONALERT, }; + #[cfg(test)] + use windows_sys::Win32::Foundation::STATUS_OBJECT_NAME_NOT_FOUND; use windows_sys::Win32::Foundation::{ CloseHandle, LocalFree, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS, HANDLE, - STATUS_NO_MORE_FILES, STATUS_OBJECT_NAME_NOT_FOUND, UNICODE_STRING, + STATUS_NO_MORE_FILES, UNICODE_STRING, }; use windows_sys::Win32::Security::Authorization::{ GetExplicitEntriesFromAclW, GetSecurityInfo, SetEntriesInAclW, SetSecurityInfo, @@ -600,9 +602,10 @@ mod platform { #[cfg(test)] use windows_sys::Win32::Security::{CreateWellKnownSid, WinWorldSid, SECURITY_MAX_SID_SIZE}; use windows_sys::Win32::Security::{ - EqualSid, GetSecurityDescriptorControl, GetTokenInformation, TokenUser, ACL, + EqualSid, GetSecurityDescriptorControl, GetTokenInformation, TokenOwner, TokenUser, ACL, DACL_SECURITY_INFORMATION, NO_INHERITANCE, OWNER_SECURITY_INFORMATION, - PROTECTED_DACL_SECURITY_INFORMATION, PSID, SE_DACL_PROTECTED, TOKEN_QUERY, TOKEN_USER, + PROTECTED_DACL_SECURITY_INFORMATION, PSID, SE_DACL_PROTECTED, TOKEN_INFORMATION_CLASS, + TOKEN_OWNER, TOKEN_QUERY, TOKEN_USER, }; use windows_sys::Win32::Storage::FileSystem::{ FileDispositionInfo, FileRenameInfo, GetFileInformationByHandle, @@ -611,7 +614,7 @@ mod platform { FILE_ATTRIBUTE_REPARSE_POINT, FILE_DISPOSITION_INFO, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, FILE_READ_ATTRIBUTES, FILE_READ_DATA, FILE_RENAME_INFO, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_WRITE_ATTRIBUTES, - FILE_WRITE_DATA, READ_CONTROL, SYNCHRONIZE, WRITE_DAC, + FILE_WRITE_DATA, READ_CONTROL, SYNCHRONIZE, WRITE_DAC, WRITE_OWNER, }; use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; @@ -664,6 +667,10 @@ mod platform { } fn current_user_sid() -> Result<(Vec, PSID), String> { + token_sid(TokenUser) + } + + fn token_sid(kind: TOKEN_INFORMATION_CLASS) -> Result<(Vec, PSID), String> { let mut token = null_mut(); // SAFETY: token points to writable handle storage. if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) } == 0 { @@ -675,7 +682,7 @@ mod platform { let token = Handle(token); let mut length = 0; // SAFETY: probing required size with a null buffer is documented. - unsafe { GetTokenInformation(token.0, TokenUser, null_mut(), 0, &mut length) }; + unsafe { GetTokenInformation(token.0, kind, null_mut(), 0, &mut length) }; if std::io::Error::last_os_error().raw_os_error() != Some(ERROR_INSUFFICIENT_BUFFER as i32) { return Err(format!( @@ -688,7 +695,7 @@ mod platform { if unsafe { GetTokenInformation( token.0, - TokenUser, + kind, buffer.as_mut_ptr().cast(), length, &mut length, @@ -700,13 +707,48 @@ mod platform { std::io::Error::last_os_error() )); } - // SAFETY: successful TokenUser query initialized TOKEN_USER in buffer. - let sid = unsafe { (*(buffer.as_ptr().cast::())).User.Sid }; + // SAFETY: each call uses the structure matching the queried token class. + let sid = unsafe { + if kind == TokenUser { + (*(buffer.as_ptr().cast::())).User.Sid + } else { + (*(buffer.as_ptr().cast::())).Owner + } + }; Ok((buffer, sid)) } fn secure_for_current_user(file: &File) -> Result<(), String> { let (_sid_buffer, sid) = current_user_sid()?; + let (_owner_buffer, default_owner) = token_sid(TokenOwner)?; + let mut owner = null_mut(); + let mut descriptor = null_mut(); + // Only repair our own objects, including those created with the token's + // default owner (which may be Administrators for an elevated process). + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle() as HANDLE, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + &mut owner, + null_mut(), + null_mut(), + null_mut(), + &mut descriptor, + ) + }; + if status != ERROR_SUCCESS { + return Err(format!( + "failed to inspect owner before initialization: {status}" + )); + } + let owned_by_token = !owner.is_null() + && unsafe { EqualSid(owner, sid) != 0 || EqualSid(owner, default_owner) != 0 }; + // SAFETY: GetSecurityInfo allocated descriptor with LocalAlloc. + unsafe { LocalFree(descriptor) }; + if !owned_by_token { + return Err("process record object belongs to another owner".to_string()); + } let mut acl: *mut ACL = null_mut(); let access = EXPLICIT_ACCESS_W { grfAccessPermissions: FILE_ALL_ACCESS, @@ -730,8 +772,10 @@ mod platform { SetSecurityInfo( file.as_raw_handle() as HANDLE, SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, - null_mut(), + OWNER_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | PROTECTED_DACL_SECURITY_INFORMATION, + sid, null_mut(), acl, null(), @@ -740,7 +784,9 @@ mod platform { // SAFETY: SetEntriesInAclW allocated acl with LocalAlloc. unsafe { LocalFree(acl.cast()) }; if status != ERROR_SUCCESS { - return Err(format!("failed to set owner-only ACL by handle: {status}")); + return Err(format!( + "failed to set current owner and owner-only ACL by handle: {status}" + )); } validate_owner_and_acl(file) } @@ -882,7 +928,12 @@ mod platform { OpenOptions::new() .read(true) .access_mode( - FILE_READ_DATA | FILE_READ_ATTRIBUTES | READ_CONTROL | WRITE_DAC | SYNCHRONIZE, + FILE_READ_DATA + | FILE_READ_ATTRIBUTES + | READ_CONTROL + | WRITE_DAC + | WRITE_OWNER + | SYNCHRONIZE, ) .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS) @@ -1001,6 +1052,7 @@ mod platform { | FILE_WRITE_ATTRIBUTES | READ_CONTROL | WRITE_DAC + | WRITE_OWNER | SYNCHRONIZE | DELETE, FILE_CREATE, From e6d40c0602c4124cbb4fdc9b1d4f4aba238ae334 Mon Sep 17 00:00:00 2001 From: Olabode Olaoke Date: Fri, 11 Sep 2026 18:13:15 -0600 Subject: [PATCH 4/5] fix: use native root-relative Windows record rename Publish through NtSetInformationFile with the retained source and directory handles. Use the native rename structure and documented buffer size, preserving no-replacement semantics. Generated with Codex Signed-off-by: Olabode Olaoke --- .../src/services/acp/process_record_store.rs | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src-tauri/src/services/acp/process_record_store.rs b/src-tauri/src/services/acp/process_record_store.rs index 24ed73615..cab4edc45 100644 --- a/src-tauri/src/services/acp/process_record_store.rs +++ b/src-tauri/src/services/acp/process_record_store.rs @@ -584,9 +584,9 @@ mod platform { use std::ptr::{null, null_mut}; use windows_sys::Wdk::Foundation::OBJECT_ATTRIBUTES; use windows_sys::Wdk::Storage::FileSystem::{ - FileIdBothDirectoryInformation, NtCreateFile, NtQueryDirectoryFile, FILE_CREATE, - FILE_ID_BOTH_DIR_INFORMATION, FILE_NON_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT, - FILE_SYNCHRONOUS_IO_NONALERT, + FileIdBothDirectoryInformation, FileRenameInformation, NtCreateFile, NtQueryDirectoryFile, + NtSetInformationFile, FILE_CREATE, FILE_ID_BOTH_DIR_INFORMATION, FILE_NON_DIRECTORY_FILE, + FILE_OPEN, FILE_OPEN_REPARSE_POINT, FILE_RENAME_INFORMATION, FILE_SYNCHRONOUS_IO_NONALERT, }; #[cfg(test)] use windows_sys::Win32::Foundation::STATUS_OBJECT_NAME_NOT_FOUND; @@ -608,13 +608,12 @@ mod platform { TOKEN_OWNER, TOKEN_QUERY, TOKEN_USER, }; use windows_sys::Win32::Storage::FileSystem::{ - FileDispositionInfo, FileRenameInfo, GetFileInformationByHandle, - SetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, DELETE, FILE_ALL_ACCESS, - FILE_APPEND_DATA, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, - FILE_ATTRIBUTE_REPARSE_POINT, FILE_DISPOSITION_INFO, FILE_FLAG_BACKUP_SEMANTICS, - FILE_FLAG_OPEN_REPARSE_POINT, FILE_READ_ATTRIBUTES, FILE_READ_DATA, FILE_RENAME_INFO, - FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_WRITE_ATTRIBUTES, - FILE_WRITE_DATA, READ_CONTROL, SYNCHRONIZE, WRITE_DAC, WRITE_OWNER, + FileDispositionInfo, GetFileInformationByHandle, SetFileInformationByHandle, + BY_HANDLE_FILE_INFORMATION, DELETE, FILE_ALL_ACCESS, FILE_APPEND_DATA, + FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, FILE_ATTRIBUTE_REPARSE_POINT, + FILE_DISPOSITION_INFO, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, + FILE_READ_ATTRIBUTES, FILE_READ_DATA, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, + FILE_WRITE_ATTRIBUTES, FILE_WRITE_DATA, READ_CONTROL, SYNCHRONIZE, WRITE_DAC, WRITE_OWNER, }; use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; @@ -1102,12 +1101,12 @@ mod platform { .len() .checked_mul(size_of::()) .ok_or_else(|| "destination name is too long".to_string())?; - let total = offset_of!(FILE_RENAME_INFO, FileName) + let total = size_of::() .checked_add(name_bytes) .ok_or_else(|| "rename buffer is too large".to_string())?; let words = total.div_ceil(size_of::()); let mut storage = vec![0usize; words]; - let info = storage.as_mut_ptr().cast::(); + let info = storage.as_mut_ptr().cast::(); // SAFETY: storage is aligned and sized for header plus complete UTF-16 name. unsafe { (*info).Anonymous.ReplaceIfExists = 0; @@ -1120,19 +1119,23 @@ mod platform { name.len(), ); } - // SAFETY: source is the exact temp handle; rename target is relative to retained root; replacement is disabled. - if unsafe { - SetFileInformationByHandle( + let mut io_status: IO_STATUS_BLOCK = unsafe { zeroed() }; + // Use the native API's explicit root-relative rename contract. The Win32 + // wrapper rejects this relative target on supported Windows runners. + // SAFETY: both handles and the aligned buffer live for this synchronous + // call; the source has DELETE access and replacement is disabled. + let status = unsafe { + NtSetInformationFile( source.as_raw_handle() as HANDLE, - FileRenameInfo, + &mut io_status, storage.as_ptr().cast(), u32::try_from(total).map_err(|_| "rename buffer is too large".to_string())?, + FileRenameInformation, ) - } == 0 - { + }; + if !nt_success(status) { return Err(format!( - "failed to publish process record by handle: {}", - std::io::Error::last_os_error() + "failed to publish process record by handle: NTSTATUS {status:#x}" )); } Ok(()) From 71d85c2059003e4eb25f0c7f3d344c58e49adc6f Mon Sep 17 00:00:00 2001 From: Olabode Olaoke Date: Fri, 11 Sep 2026 21:54:45 -0600 Subject: [PATCH 5/5] test: verify Linux stale backend recovery Exercise the real recovery scan with an exited owner and a backend that reports SIGTERM but stays alive. Require a SIGKILL exit while an unrelated child survives, without mocked probes or signals. Generated with Codex Signed-off-by: Olabode Olaoke --- src-tauri/src/services/acp/goose_serve.rs | 98 +++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/src-tauri/src/services/acp/goose_serve.rs b/src-tauri/src/services/acp/goose_serve.rs index f9a0ed509..09ae3740f 100644 --- a/src-tauri/src/services/acp/goose_serve.rs +++ b/src-tauri/src/services/acp/goose_serve.rs @@ -1877,6 +1877,104 @@ mod recovery_tests { assert_eq!(std::fs::read(&displaced).unwrap(), b"not-json"); } + #[cfg(target_os = "linux")] + #[tokio::test] + async fn stale_record_recovery_escalates_and_only_kills_the_recorded_backend() { + use std::os::unix::process::ExitStatusExt; + use std::process::Stdio; + use tokio::io::{AsyncBufReadExt, BufReader}; + + let temp = tempfile::tempdir().expect("temp dir"); + let store = ProcessRecordStore::open(temp.path().join("records")).expect("open store"); + let mut owner = Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .expect("spawn owner"); + let owner_identity = + crate::services::process::capture_process_identity(owner.id().expect("owner pid")) + .expect("capture owner identity"); + let mut backend = Command::new("sh") + // Block in a shell builtin, with no descendant to leak. Readiness + // confirms the handler is installed before recovery can signal it. + // Report SIGTERM receipt but stay alive until forced termination. + .args([ + "-c", + "trap 'printf \"term\\n\"' TERM; printf 'ready\\n'; while :; do read -r stop || :; done", + ]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .kill_on_drop(true) + .spawn() + .expect("spawn backend"); + let mut output = BufReader::new(backend.stdout.take().expect("backend stdout")); + let mut ready = String::new(); + tokio::time::timeout(Duration::from_secs(5), output.read_line(&mut ready)) + .await + .expect("backend readiness deadline") + .expect("backend readiness"); + assert_eq!(ready, "ready\n"); + let serve_identity = + crate::services::process::capture_process_identity(backend.id().expect("backend pid")) + .expect("capture backend identity"); + let mut unrelated = Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .expect("spawn unrelated child"); + let record = ServeProcessRecord { + owner_pid: owner_identity.pid, + serve_pid: serve_identity.pid, + owner_identity: Some(owner_identity), + serve_identity: Some(serve_identity), + }; + let path = store.new_record_path(record.owner_pid, 1); + store + .publish( + &path, + &serde_json::to_vec(&record).expect("serialize record"), + ) + .expect("publish record"); + owner.kill().await.expect("stop and reap owner"); + + // Exercise enumeration, record parsing, orphan detection, identity + // verification, and real SIGTERM/SIGKILL without injected operations. + let recovery = + tokio::time::timeout(Duration::from_secs(5), kill_stale_serve_process(&store)).await; + let mut term = String::new(); + let term_received = + tokio::time::timeout(Duration::from_secs(2), output.read_line(&mut term)).await; + let exited = tokio::time::timeout(Duration::from_secs(2), backend.wait()).await; + let unrelated_alive = unrelated + .try_wait() + .expect("probe unrelated child") + .is_none(); + + // Cleanup cannot turn a failed recovery into a passing exit assertion. + if exited.is_err() { + backend.kill().await.expect("cleanup backend"); + } + unrelated.kill().await.expect("cleanup unrelated child"); + recovery.expect("recovery deadline"); + term_received + .expect("SIGTERM receipt deadline") + .expect("read SIGTERM receipt"); + assert_eq!(term, "term\n", "recovery must attempt SIGTERM first"); + let status = exited + .expect("backend exit deadline") + .expect("reap backend"); + assert_eq!( + status.signal(), + Some(libc::SIGKILL), + "recovery must escalate" + ); + assert!( + unrelated_alive, + "recovery must leave the unrelated child alive" + ); + assert!(path.exists(), "Unix retains the recovery evidence"); + } + #[cfg(all(unix, not(target_os = "macos")))] #[tokio::test] async fn stale_owner_pid_reuse_does_not_kill_an_unrelated_process() {