diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 5817fc4..b9e8d68 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -225,7 +225,9 @@ pub use output::{ErrorPayload, Response}; pub use permission_report::PermissionReport; pub use permission_state::PermissionState; pub use point::Point; -pub use private_file_ops::{PrivateFileOps, install_private_file_ops}; +pub use private_file_ops::{ + PrivateFileOps, bounded_read, install_private_file_ops, temporary_file_name, +}; pub use process_id::ProcessId; pub use process_identity::ProcessIdentity; pub use recovery_hint::RecoveryHint; diff --git a/crates/core/src/private_file.rs b/crates/core/src/private_file.rs index 08f8d8f..cf6efc0 100644 --- a/crates/core/src/private_file.rs +++ b/crates/core/src/private_file.rs @@ -1,11 +1,7 @@ use std::fs::File; use std::fs::OpenOptions; -use std::hash::{BuildHasher, RandomState}; -use std::io::{Read, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; - -static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); pub(crate) fn open_private_lock(path: &Path, create: bool) -> std::io::Result { crate::private_file_ops::with_active_ops(|ops| ops.open_private_lock(path, create)) @@ -49,7 +45,7 @@ pub(crate) fn read_private_bounded_portable( max_bytes: u64, ) -> std::io::Result> { let file = open_private_read(path)?; - read_bounded(file, max_bytes) + crate::private_file_ops::bounded_read(file, max_bytes) } pub(crate) fn read_regular_bounded(path: &Path, max_bytes: u64) -> std::io::Result> { @@ -61,7 +57,7 @@ pub(crate) fn read_regular_bounded(path: &Path, max_bytes: u64) -> std::io::Resu }; validate_regular(&file)?; validate_local_filesystem(&file)?; - read_bounded(file, max_bytes) + crate::private_file_ops::bounded_read(file, max_bytes) } pub(crate) fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { @@ -177,33 +173,15 @@ fn open_private_read(path: &Path) -> std::io::Result { Ok(file) } -fn read_bounded(file: File, max_bytes: u64) -> std::io::Result> { - let metadata = file.metadata()?; - if metadata.len() > max_bytes { - return Err(invalid_input("file exceeds its read limit")); - } - let capacity = usize::try_from(metadata.len().min(max_bytes)).unwrap_or(usize::MAX); - let mut bytes = Vec::with_capacity(capacity); - file.take(max_bytes.saturating_add(1)) - .read_to_end(&mut bytes)?; - if bytes.len() as u64 > max_bytes { - return Err(invalid_input("file grew beyond its read limit")); - } - Ok(bytes) -} - fn create_temporary(path: &Path) -> std::io::Result<(PathBuf, File)> { let file_name = path .file_name() .and_then(|name| name.to_str()) .ok_or_else(|| invalid_input("private file path has an invalid filename"))?; for _ in 0..32 { - let nonce = RandomState::new().hash_one(( - std::process::id(), - TEMP_COUNTER.fetch_add(1, Ordering::Relaxed), - std::time::SystemTime::now(), + let temporary = path.with_file_name(crate::private_file_ops::temporary_file_name( + std::ffi::OsStr::new(file_name), )); - let temporary = path.with_file_name(format!(".{file_name}.{nonce:016x}.tmp")); let mut options = OpenOptions::new(); options.write(true).create_new(true); configure_unix(&mut options, 0o600); diff --git a/crates/core/src/private_file_ops.rs b/crates/core/src/private_file_ops.rs index bfe7b00..cf532ad 100644 --- a/crates/core/src/private_file_ops.rs +++ b/crates/core/src/private_file_ops.rs @@ -1,6 +1,53 @@ +use std::ffi::{OsStr, OsString}; use std::fs::File; +use std::hash::{BuildHasher, RandomState}; +use std::io::Read; use std::path::Path; use std::sync::OnceLock; +use std::sync::atomic::{AtomicU64, Ordering}; + +static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); + +/// Reads `file` fully on behalf of a `PrivateFileOps` implementation, +/// rejecting files larger than `max_bytes` before allocating and detecting +/// growth past the limit during the read. +pub fn bounded_read(file: File, max_bytes: u64) -> std::io::Result> { + let metadata = file.metadata()?; + if metadata.len() > max_bytes { + return Err(read_limit_error("file exceeds its read limit")); + } + let capacity = usize::try_from(metadata.len().min(max_bytes)).unwrap_or(usize::MAX); + let mut bytes = Vec::with_capacity(capacity); + file.take(max_bytes.saturating_add(1)) + .read_to_end(&mut bytes)?; + if bytes.len() as u64 > max_bytes { + return Err(read_limit_error("file grew beyond its read limit")); + } + Ok(bytes) +} + +fn read_limit_error(message: &'static str) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::InvalidData, message) +} + +/// Produces one `.{name}.{nonce:016x}.tmp` candidate for a temporary that +/// will be promoted over a destination whose leaf name is `name`. +/// +/// The hashed nonce keeps the name unpredictable to a same-privilege racer; +/// callers loop over fresh candidates when creation collides. Every +/// `PrivateFileOps` implementation names its temporaries through this one +/// scheme. +pub fn temporary_file_name(name: &OsStr) -> OsString { + let nonce = RandomState::new().hash_one(( + std::process::id(), + TEMP_COUNTER.fetch_add(1, Ordering::Relaxed), + std::time::SystemTime::now(), + )); + let mut temporary = OsString::from("."); + temporary.push(name); + temporary.push(format!(".{nonce:016x}.tmp")); + temporary +} /// Platform seam for the five private-file primitives. /// diff --git a/crates/ffi/tests/c_abi_session_liveness.rs b/crates/ffi/tests/c_abi_session_liveness.rs index d5d03bd..d3a3553 100644 --- a/crates/ffi/tests/c_abi_session_liveness.rs +++ b/crates/ffi/tests/c_abi_session_liveness.rs @@ -7,14 +7,13 @@ use common::{ad_adapter_create_with_session, ad_adapter_destroy, with_isolated_h use std::ffi::CString; use std::time::Duration; -/// Runs alone in its own process because private-file install state is -/// process-global: on Windows the first adapter construction installs -/// `WindowsPrivateFile`, whose atomic writes hold a process-lifetime temp -/// lease inside the written file's parent. If another adapter-creating test -/// ran first in this process, the session writes below would plant that -/// lease inside the session directory and the same-process gc removal would -/// fail by design. In the product, session writes and `session gc` never -/// share a process, so the isolation here models the real topology. +/// Runs alone in its own process for env-var hygiene: the isolated HOME +/// swap is process-wide, so a dedicated process keeps it from interleaving +/// with adapter state other suites establish. The historical gc hazard is +/// gone — on Windows the installed `WindowsPrivateFile` now scopes its temp +/// lease to each atomic write, so no process-lifetime directory handle +/// lingers inside the session directory and same-process gc removal (as +/// exercised below) succeeds against everything this process wrote. #[test] fn session_scoped_adapter_holds_liveness_until_destroyed() { with_isolated_home(|| { diff --git a/crates/ffi/tests/c_abi_session_trace.rs b/crates/ffi/tests/c_abi_session_trace.rs index 29539df..628d2b0 100644 --- a/crates/ffi/tests/c_abi_session_trace.rs +++ b/crates/ffi/tests/c_abi_session_trace.rs @@ -5,50 +5,10 @@ use agent_desktop_core::session::{ }; use common::{ AdResult, ad_adapter_create_with_session, ad_adapter_destroy, ad_check_permissions, - ad_free_string, ad_status, + ad_free_string, ad_status, with_isolated_home, }; use std::ffi::CString; use std::fs; -use std::sync::Mutex; - -static HOME_LOCK: Mutex<()> = Mutex::new(()); - -struct TestHome { - _lock: std::sync::MutexGuard<'static, ()>, - dir: std::path::PathBuf, - previous: Option, -} - -impl TestHome { - fn new() -> Self { - let lock = HOME_LOCK.lock().unwrap(); - let dir = std::env::temp_dir().join(format!( - "agent-desktop-ffi-session-trace-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - fs::create_dir_all(&dir).unwrap(); - let previous = std::env::var_os("HOME"); - unsafe { std::env::set_var("HOME", &dir) }; - Self { - _lock: lock, - dir, - previous, - } - } -} - -impl Drop for TestHome { - fn drop(&mut self) { - match self.previous.as_ref() { - Some(previous) => unsafe { std::env::set_var("HOME", previous) }, - None => unsafe { std::env::remove_var("HOME") }, - } - let _ = fs::remove_dir_all(&self.dir); - } -} fn trace_segments(session_id: &str) -> Vec { fs::read_dir(trace_dir(session_id).unwrap()) @@ -78,53 +38,55 @@ unsafe fn call_status(session_id: &str) { #[test] fn traced_ffi_commands_reuse_one_process_segment_and_emit_ordered_boundaries() { - let _home = TestHome::new(); - let manifest = start_session(StartSessionOptions { - name: None, - trace: SessionTraceMode::On, - ..Default::default() - }) - .unwrap(); + with_isolated_home(|| { + let manifest = start_session(StartSessionOptions { + name: None, + trace: SessionTraceMode::On, + ..Default::default() + }) + .unwrap(); - unsafe { - call_status(&manifest.id); - call_status(&manifest.id); - } + unsafe { + call_status(&manifest.id); + call_status(&manifest.id); + } - let segments = trace_segments(&manifest.id); - assert_eq!(segments.len(), 1); - let events: Vec = fs::read_to_string(&segments[0]) - .unwrap() - .lines() - .map(|line| serde_json::from_str(line).unwrap()) - .filter(|event: &serde_json::Value| event["command"].as_str() == Some("status")) - .collect(); - let boundaries: Vec<_> = events - .iter() - .filter_map(|event| event["event"].as_str()) - .filter(|event| matches!(*event, "command.start" | "command.end")) - .collect(); - assert_eq!( - boundaries, - [ - "command.start", - "command.end", - "command.start", - "command.end" - ] - ); + let segments = trace_segments(&manifest.id); + assert_eq!(segments.len(), 1); + let events: Vec = fs::read_to_string(&segments[0]) + .unwrap() + .lines() + .map(|line| serde_json::from_str(line).unwrap()) + .filter(|event: &serde_json::Value| event["command"].as_str() == Some("status")) + .collect(); + let boundaries: Vec<_> = events + .iter() + .filter_map(|event| event["event"].as_str()) + .filter(|event| matches!(*event, "command.start" | "command.end")) + .collect(); + assert_eq!( + boundaries, + [ + "command.start", + "command.end", + "command.start", + "command.end" + ] + ); + }); } #[test] fn manifestless_session_does_not_create_trace_files() { - let _home = TestHome::new(); - let session_id = "plain-session"; - unsafe { - let session = CString::new(session_id).unwrap(); - let adapter = ad_adapter_create_with_session(session.as_ptr()); - assert!(!adapter.is_null()); - let _ = ad_check_permissions(adapter); - ad_adapter_destroy(adapter); - } - assert!(!trace_dir(session_id).unwrap().exists()); + with_isolated_home(|| { + let session_id = "plain-session"; + unsafe { + let session = CString::new(session_id).unwrap(); + let adapter = ad_adapter_create_with_session(session.as_ptr()); + assert!(!adapter.is_null()); + let _ = ad_check_permissions(adapter); + ad_adapter_destroy(adapter); + } + assert!(!trace_dir(session_id).unwrap().exists()); + }); } diff --git a/crates/windows/src/system/com_runtime.rs b/crates/windows/src/system/com_runtime.rs index d725f54..56fc600 100644 --- a/crates/windows/src/system/com_runtime.rs +++ b/crates/windows/src/system/com_runtime.rs @@ -18,7 +18,7 @@ pub(crate) enum ComApartment { } impl ComApartment { - #[cfg(test)] + #[cfg(any(test, target_os = "windows"))] pub(crate) fn permits_co_uninitialize(self) -> bool { matches!(self, ComApartment::OwnedMta) } @@ -118,7 +118,7 @@ pub(crate) fn apartment_probe_reports_mta(hresult: i32, apartment_type: i32) -> fn com_bootstrap_failure(message: &str, hresult: i32) -> AdapterError { AdapterError::new(ErrorCode::Internal, message) - .with_platform_detail(format!("COM HRESULT 0x{:08X}", hresult as u32)) + .with_platform_detail(crate::system::permissions::com_hresult_detail(hresult)) .with_suggestion( "Verify the host process allows COM initialization, then rerun the command", ) diff --git a/crates/windows/src/system/permissions.rs b/crates/windows/src/system/permissions.rs index 85e1b79..4297bc4 100644 --- a/crates/windows/src/system/permissions.rs +++ b/crates/windows/src/system/permissions.rs @@ -7,48 +7,33 @@ const E_ACCESSDENIED: i32 = 0x8007_0005_u32 as i32; #[cfg(target_os = "windows")] mod imp { - use windows_sys::Win32::Foundation::{S_FALSE, S_OK}; use windows_sys::Win32::System::Com::{ CLSCTX_INPROC_SERVER, COINIT_MULTITHREADED, CoCreateInstance, CoInitializeEx, CoUninitialize, }; - use windows_sys::core::GUID; + use windows_sys::core::{GUID, IID_IUnknown, IUnknown_Vtbl}; + + use crate::system::com_runtime::classify_co_initialize_hresult; const CLSID_CUIAUTOMATION: GUID = GUID::from_u128(0xff48dba4_60ef_4201_aa87_54103eef594e); - const IID_IUNKNOWN: GUID = GUID::from_u128(0x00000000_0000_0000_c000_000000000046); - const RPC_E_CHANGED_MODE: i32 = 0x8001_0106_u32 as i32; - - #[repr(C)] - struct ComObject { - vtable: *const ComVtable, - } - - #[repr(C)] - struct ComVtable { - query_interface: usize, - add_ref: usize, - release: unsafe extern "system" fn(this: *mut core::ffi::c_void) -> u32, - } - - const _: () = assert!(size_of::() == 3 * size_of::()); pub(super) fn probe_uia_access() -> i32 { unsafe { let init_status = CoInitializeEx(core::ptr::null(), COINIT_MULTITHREADED as u32); - if init_status < 0 && init_status != RPC_E_CHANGED_MODE { - return init_status; - } - let balance_apartment = init_status == S_OK || init_status == S_FALSE; + let apartment = match classify_co_initialize_hresult(init_status) { + Ok(apartment) => apartment, + Err(failure) => return failure, + }; let mut instance: *mut core::ffi::c_void = core::ptr::null_mut(); let create_status = CoCreateInstance( &CLSID_CUIAUTOMATION, core::ptr::null_mut(), CLSCTX_INPROC_SERVER, - &IID_IUNKNOWN, + &IID_IUnknown, &mut instance, ); release_instance(instance); - if balance_apartment { + if apartment.permits_co_uninitialize() { CoUninitialize(); } create_status @@ -59,9 +44,9 @@ mod imp { if instance.is_null() { return; } - let object = instance.cast::(); + let vtable = unsafe { *instance.cast::<*const IUnknown_Vtbl>() }; unsafe { - ((*(*object).vtable).release)(instance); + ((*vtable).Release)(instance); } } @@ -85,23 +70,17 @@ mod imp { pub(crate) fn report(deadline: Deadline) -> Result { ensure_budget(deadline)?; - let report = PermissionReport { - accessibility: accessibility_report_state(), - screen_recording: screen_recording_report_state(), - automation: automation_report_state(), - }; - ensure_budget(deadline)?; - Ok(report) + report_from_probed_uia(deadline, imp::probe_uia_access()) } pub(crate) fn request_report(deadline: Deadline) -> Result { - request_report_with(deadline, imp::probe_uia_access, report) + request_report_with(deadline, imp::probe_uia_access, report_from_probed_uia) } fn request_report_with( deadline: Deadline, probe: impl FnOnce() -> i32, - report: impl FnOnce(Deadline) -> Result, + report: impl FnOnce(Deadline, i32) -> Result, ) -> Result { ensure_budget(deadline)?; let hresult = probe(); @@ -109,7 +88,20 @@ fn request_report_with( if matches!(map_uia_access(hresult), PermissionState::Denied { .. }) { return Err(uia_access_denied_error(hresult)); } - report(deadline) + report(deadline, hresult) +} + +fn report_from_probed_uia( + deadline: Deadline, + uia_hresult: i32, +) -> Result { + let report = PermissionReport { + accessibility: map_uia_access(uia_hresult), + screen_recording: screen_recording_report_state(), + automation: automation_report_state(), + }; + ensure_budget(deadline)?; + Ok(report) } pub(crate) fn map_uia_access(hresult: i32) -> PermissionState { @@ -146,10 +138,6 @@ pub(crate) fn com_hresult_detail(hresult: i32) -> String { } } -fn accessibility_report_state() -> PermissionState { - map_uia_access(imp::probe_uia_access()) -} - fn screen_recording_report_state() -> PermissionState { map_capture_availability(imp::probe_capture_availability()) } @@ -158,7 +146,7 @@ fn automation_report_state() -> PermissionState { PermissionState::NotRequired } -fn ensure_budget(deadline: Deadline) -> Result<(), AdapterError> { +pub(crate) fn ensure_budget(deadline: Deadline) -> Result<(), AdapterError> { if deadline.is_expired() { Err(deadline.timeout_error()) } else { diff --git a/crates/windows/src/system/permissions_tests.rs b/crates/windows/src/system/permissions_tests.rs index 151e8c1..2943be1 100644 --- a/crates/windows/src/system/permissions_tests.rs +++ b/crates/windows/src/system/permissions_tests.rs @@ -73,7 +73,7 @@ fn request_on_a_denied_probe_is_a_structured_error_not_a_prompt() { let error = request_report_with( Deadline::after(1_000).unwrap(), || 0x8007_0005_u32 as i32, - |_| panic!("a denied probe must not fall through to the report"), + |_, _| panic!("a denied probe must not fall through to the report"), ) .unwrap_err(); @@ -82,9 +82,15 @@ fn request_on_a_denied_probe_is_a_structured_error_not_a_prompt() { } #[test] -fn request_on_a_granted_probe_returns_the_probe_report() { - let report = request_report_with(Deadline::after(1_000).unwrap(), || 0, report).unwrap(); +fn request_on_a_granted_probe_reports_accessibility_from_that_single_probe() { + let report = request_report_with( + Deadline::after(1_000).unwrap(), + || 0, + report_from_probed_uia, + ) + .unwrap(); + assert_eq!(report.accessibility, PermissionState::Granted); assert_eq!(report.automation, PermissionState::NotRequired); } diff --git a/crates/windows/src/system/private_file/mod.rs b/crates/windows/src/system/private_file/mod.rs index c8792ce..213279d 100644 --- a/crates/windows/src/system/private_file/mod.rs +++ b/crates/windows/src/system/private_file/mod.rs @@ -2,7 +2,7 @@ //! //! Four measured behaviors drive four modules: per-component reparse-point //! rejection (`path`), `ReplaceFileW`-based atomic promotion with a -//! per-process temp lease (`replace`), `TokenOwner` foreign-principal +//! write-scoped temp lease (`replace`), `TokenOwner` foreign-principal //! detection (`owner`), and control-call-disciplined storage locality //! (`locality`). Each override mirrors the portable default's observable //! semantics — parent handling, create/append/lock open modes, the hashed @@ -30,10 +30,10 @@ mod path; mod replace; use std::fs::{File, OpenOptions}; -use std::io::{ErrorKind, Read, Write}; +use std::io::{ErrorKind, Write}; use std::path::Path; -use agent_desktop_core::PrivateFileOps; +use agent_desktop_core::{PrivateFileOps, bounded_read}; /// Windows implementation of core's private-file seam, installed once per /// process by the binary and FFI entry points. @@ -58,16 +58,11 @@ impl PrivateFileOps for WindowsPrivateFile { .ok_or_else(|| invalid_input("private file path has an invalid filename"))?; path::ensure_private_directory_chain(parent)?; validate_destination_if_present(path)?; - let lease = replace::lease_temp_directory(parent)?; + let lease = replace::acquire_write_lease(parent)?; let (temporary, file) = replace::create_private_temp_file(&lease, destination_name)?; - let result = write_all_and_sync(file, bytes).and_then(|()| { - replace::promote_temp_to_destination(path, &temporary)?; - validate_written_destination(path) - }); - if result.is_err() { - let _ = std::fs::remove_file(&temporary); - } - result + write_all_and_sync(file, bytes)?; + replace::promote_temp_to_destination(path, &temporary)?; + validate_written_destination(path) } fn open_private_append(&self, path: &Path) -> std::io::Result { @@ -100,7 +95,7 @@ impl PrivateFileOps for WindowsPrivateFile { options.read(true); let file = path::open_leaf_regular_no_follow(path, &mut options, "private file")?; owner::require_owned_by_token_owner(&file, "private file")?; - read_bounded(file, max_bytes) + bounded_read(file, max_bytes) } fn ensure_private(&self, path: &Path) -> std::io::Result<()> { @@ -126,21 +121,6 @@ fn write_all_and_sync(mut file: File, bytes: &[u8]) -> std::io::Result<()> { file.sync_all() } -fn read_bounded(file: File, max_bytes: u64) -> std::io::Result> { - let metadata = file.metadata()?; - if metadata.len() > max_bytes { - return Err(invalid_input("file exceeds its read limit")); - } - let capacity = usize::try_from(metadata.len().min(max_bytes)).unwrap_or(usize::MAX); - let mut bytes = Vec::with_capacity(capacity); - file.take(max_bytes.saturating_add(1)) - .read_to_end(&mut bytes)?; - if bytes.len() as u64 > max_bytes { - return Err(invalid_input("file grew beyond its read limit")); - } - Ok(bytes) -} - fn invalid_input(message: &'static str) -> std::io::Error { std::io::Error::new(ErrorKind::InvalidData, message) } diff --git a/crates/windows/src/system/private_file/replace.rs b/crates/windows/src/system/private_file/replace.rs index 1170f45..e588e47 100644 --- a/crates/windows/src/system/private_file/replace.rs +++ b/crates/windows/src/system/private_file/replace.rs @@ -14,28 +14,30 @@ //! destination; an absent destination means no reader holds it, so that //! branch falls back to the `MoveFileExW`-backed `std::fs::rename`. //! -//! Temp files live in a per-process lease directory inside the destination's -//! parent, which inherits the same profile ACL. The lease handle is held for -//! the life of the process with a share mode that deliberately omits -//! `FILE_SHARE_DELETE`, making the directory undeletable while its owner -//! lives; a sweeper probes stale lease directories with `DELETE` access and -//! reclaims only those whose owning process is gone. That narrowed share -//! mode applies exclusively to this internal lease handle — artifact opens -//! keep Rust's default wide `FILE_SHARE_READ|WRITE|DELETE` mask, because any -//! hardened open that narrows it re-introduces the measured sharing-failure -//! cluster. The sweep runs lazily, once per parent per process on first -//! write, never per write. Temp names reuse core's hashed-nonce scheme so -//! they stay unpredictable to a same-privilege racer. +//! Temp files live in a write-scoped lease directory inside the +//! destination's parent, which inherits the same profile ACL. Each atomic +//! write creates its own lease directory — named with the pid plus a +//! per-write nonce, so concurrent same-parent writes in one process never +//! collide — and holds its handle for the duration of the write with a share +//! mode that deliberately omits `FILE_SHARE_DELETE`. That held handle is the +//! live-writer guard: a sweep before each write probes lease directories +//! with `DELETE` access and reclaims only those no live writer holds. The +//! lease handle is dropped and the directory removed on every exit path, so +//! a long-lived process retains no directory handle that would defeat +//! same-process snapshot pruning. The narrowed share mode applies +//! exclusively to this internal lease handle — artifact opens keep Rust's +//! default wide `FILE_SHARE_READ|WRITE|DELETE` mask, because any hardened +//! open that narrows it re-introduces the measured sharing-failure cluster. +//! Temp names reuse core's hashed-nonce scheme so they stay unpredictable to +//! a same-privilege racer. -use std::collections::HashMap; +use std::ffi::OsStr; use std::fs::{File, OpenOptions}; use std::hash::{BuildHasher, RandomState}; use std::io::ErrorKind; use std::os::windows::ffi::OsStrExt; use std::os::windows::fs::OpenOptionsExt; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::{Arc, Mutex, OnceLock}; use windows_sys::Win32::Foundation::{ ERROR_ACCESS_DENIED, ERROR_FILE_NOT_FOUND, ERROR_SHARING_VIOLATION, @@ -48,14 +50,13 @@ use windows_sys::Win32::Storage::FileSystem::{ use super::{invalid_input, locality, owner, path}; const TEMP_LEASE_PREFIX: &str = ".agent-desktop-tmp-p"; +const LEASE_CREATE_ATTEMPTS: usize = 32; const TEMP_CREATE_ATTEMPTS: usize = 32; const MEASURED_REPLACE_FLAGS: u32 = 0; -static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); - pub(super) struct TempDirLease { directory: PathBuf, - _liveness_handle: File, + liveness_handle: Option, } impl TempDirLease { @@ -64,64 +65,69 @@ impl TempDirLease { } } -pub(super) fn lease_temp_directory(parent: &Path) -> std::io::Result> { - static ACTIVE_LEASES: OnceLock>>> = OnceLock::new(); - let registry = ACTIVE_LEASES.get_or_init(|| Mutex::new(HashMap::new())); - let mut leases = registry - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - if let Some(lease) = leases.get(parent) { - return Ok(lease.clone()); +impl Drop for TempDirLease { + fn drop(&mut self) { + drop(self.liveness_handle.take()); + let _ = std::fs::remove_dir_all(&self.directory); } - sweep_stale_lease_directories(parent); - let lease = Arc::new(establish_lease(parent)?); - leases.insert(parent.to_path_buf(), lease.clone()); - Ok(lease) } -fn establish_lease(parent: &Path) -> std::io::Result { - let directory = parent.join(own_lease_name()); - match std::fs::create_dir(&directory) { - Ok(()) => {} - Err(error) if error.kind() == ErrorKind::AlreadyExists => { - let _ = std::fs::remove_dir_all(&directory); - match std::fs::create_dir(&directory) { - Ok(()) => {} - Err(retry) if retry.kind() == ErrorKind::AlreadyExists => {} - Err(retry) => return Err(retry), +pub(super) fn acquire_write_lease(parent: &Path) -> std::io::Result { + sweep_stale_lease_directories(parent); + for _ in 0..LEASE_CREATE_ATTEMPTS { + let directory = parent.join(fresh_lease_name()); + match std::fs::create_dir(&directory) { + Ok(()) => {} + Err(error) if error.kind() == ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error), + } + match open_verified_liveness_handle(&directory) { + Ok(handle) => { + return Ok(TempDirLease { + directory, + liveness_handle: Some(handle), + }); + } + Err(error) if error.kind() == ErrorKind::NotFound => continue, + Err(error) => { + let _ = std::fs::remove_dir_all(&directory); + return Err(error); } } - Err(error) => return Err(error), } - let liveness_handle = OpenOptions::new() + Err(std::io::Error::new( + ErrorKind::AlreadyExists, + "could not allocate a private temp lease directory", + )) +} + +fn open_verified_liveness_handle(directory: &Path) -> std::io::Result { + let handle = OpenOptions::new() .read(true) .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) .custom_flags(FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT) - .open(&directory)?; - path::require_verified_lease_directory(&liveness_handle)?; - owner::require_owned_by_token_owner(&liveness_handle, "the private temp directory")?; - locality::require_local_for_private_write(&liveness_handle, "the private temp directory")?; - Ok(TempDirLease { - directory, - _liveness_handle: liveness_handle, - }) + .open(directory)?; + path::require_verified_lease_directory(&handle)?; + owner::require_owned_by_token_owner(&handle, "the private temp directory")?; + locality::require_local_for_private_write(&handle, "the private temp directory")?; + Ok(handle) } -fn own_lease_name() -> String { - format!("{TEMP_LEASE_PREFIX}{}", std::process::id()) +fn fresh_lease_name() -> String { + let nonce = RandomState::new().hash_one(std::time::SystemTime::now()); + format!("{TEMP_LEASE_PREFIX}{}-{nonce:016x}", std::process::id()) } fn sweep_stale_lease_directories(parent: &Path) { let Ok(entries) = std::fs::read_dir(parent) else { return; }; - let own_name = own_lease_name(); for entry in entries.flatten() { let name = entry.file_name(); let Some(name_text) = name.to_str() else { continue; }; - if !name_text.starts_with(TEMP_LEASE_PREFIX) || name_text == own_name { + if !name_text.starts_with(TEMP_LEASE_PREFIX) { continue; } let candidate = entry.path(); @@ -146,14 +152,11 @@ pub(super) fn create_private_temp_file( destination_name: &str, ) -> std::io::Result<(PathBuf, File)> { for _ in 0..TEMP_CREATE_ATTEMPTS { - let nonce = RandomState::new().hash_one(( - std::process::id(), - TEMP_COUNTER.fetch_add(1, Ordering::Relaxed), - std::time::SystemTime::now(), - )); let temporary = lease .directory() - .join(format!(".{destination_name}.{nonce:016x}.tmp")); + .join(agent_desktop_core::temporary_file_name(OsStr::new( + destination_name, + ))); match OpenOptions::new() .write(true) .create_new(true) diff --git a/crates/windows/src/system/private_file/replace_tests.rs b/crates/windows/src/system/private_file/replace_tests.rs index a28a91e..7772dfe 100644 --- a/crates/windows/src/system/private_file/replace_tests.rs +++ b/crates/windows/src/system/private_file/replace_tests.rs @@ -186,8 +186,17 @@ fn write_atomic_replaces_a_destination_held_open_by_a_wide_share_reader() { ); } +fn temp_lease_entries(parent: &Path) -> Vec { + std::fs::read_dir(parent) + .unwrap() + .flatten() + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".agent-desktop-tmp-")) + .collect() +} + #[test] -fn the_first_write_into_a_parent_reclaims_a_dead_processes_lease_directory() { +fn a_write_reclaims_an_orphan_lease_directory_no_live_writer_holds() { let scratch = Scratch::new("sweep-stale"); let stale = scratch.path().join(".agent-desktop-tmp-p4294967295"); std::fs::create_dir(&stale).unwrap(); @@ -199,16 +208,12 @@ fn the_first_write_into_a_parent_reclaims_a_dead_processes_lease_directory() { assert!( !stale.exists(), - "a lease directory with no live owner must be reclaimed" + "a lease directory with no live holder must be reclaimed" ); - let own_lease = scratch - .path() - .join(format!(".agent-desktop-tmp-p{}", std::process::id())); - assert!(own_lease.is_dir(), "this process's lease must exist"); } #[test] -fn a_lease_directory_whose_owner_still_lives_survives_the_sweep() { +fn a_lease_directory_held_without_share_delete_survives_a_concurrent_writes_sweep() { let scratch = Scratch::new("sweep-live"); let foreign = scratch.path().join(".agent-desktop-tmp-p1"); std::fs::create_dir(&foreign).unwrap(); @@ -231,27 +236,46 @@ fn a_lease_directory_whose_owner_still_lives_survives_the_sweep() { } #[test] -fn temporaries_are_confined_to_the_lease_directory_and_consumed_on_success() { +fn a_successful_write_consumes_its_temporary_and_its_lease_directory() { let scratch = Scratch::new("temp-confinement"); let ops = WindowsPrivateFile::new(); ops.write_atomic(&scratch.path().join("artifact.json"), b"payload") .unwrap(); - let own_lease_name = format!(".agent-desktop-tmp-p{}", std::process::id()); - for entry in std::fs::read_dir(scratch.path()).unwrap().flatten() { - let name = entry.file_name().to_string_lossy().into_owned(); - assert!( - name == "artifact.json" || name == own_lease_name, - "unexpected sibling {name}: temporaries must live inside the lease directory" - ); - } - let leftovers: Vec<_> = std::fs::read_dir(scratch.path().join(&own_lease_name)) + let siblings: Vec = std::fs::read_dir(scratch.path()) .unwrap() .flatten() + .map(|entry| entry.file_name().to_string_lossy().into_owned()) .collect(); - assert!( - leftovers.is_empty(), - "a successful write must consume its temporary" + assert_eq!( + siblings, + vec!["artifact.json"], + "temporaries must live inside the lease directory and both must be consumed" ); } + +#[test] +fn write_atomic_leaves_no_temp_lease_residue_on_success_or_failure() { + let scratch = Scratch::new("no-residue"); + let destination = scratch.path().join("artifact.json"); + let ops = WindowsPrivateFile::new(); + + ops.write_atomic(&destination, b"first").unwrap(); + assert_eq!( + temp_lease_entries(scratch.path()), + Vec::::new(), + "a successful write must leave no .agent-desktop-tmp-* residue" + ); + + let held = open_reader_with_share(&destination, FILE_SHARE_READ); + ops.write_atomic(&destination, b"second") + .expect_err("promotion over a no-share-delete holder must fail"); + drop(held); + assert_eq!( + temp_lease_entries(scratch.path()), + Vec::::new(), + "a failed write must leave no .agent-desktop-tmp-* residue" + ); + assert_eq!(std::fs::read(&destination).unwrap(), b"first"); +} diff --git a/crates/windows/src/system/session.rs b/crates/windows/src/system/session.rs index bac081a..7cd1a42 100644 --- a/crates/windows/src/system/session.rs +++ b/crates/windows/src/system/session.rs @@ -8,7 +8,7 @@ use agent_desktop_core::{AdapterError, AdapterSession, Deadline, ErrorCode}; use crate::system::com_runtime::classify_mta_usage_hresult; -use crate::system::permissions::com_hresult_detail; +use crate::system::permissions::{com_hresult_detail, ensure_budget}; type MtaUsageRelease = Box i32 + Send + Sync>; @@ -105,14 +105,6 @@ fn mta_usage_release_failure(hresult: i32) -> AdapterError { ) } -fn ensure_budget(deadline: Deadline) -> Result<(), AdapterError> { - if deadline.is_expired() { - Err(deadline.timeout_error()) - } else { - Ok(()) - } -} - #[cfg(target_os = "windows")] mod imp { use windows_sys::Win32::System::Com::{