From fdf9551a46587fb69dea0fec06207cc164a35572 Mon Sep 17 00:00:00 2001 From: Dom Giandinoto Date: Wed, 14 Oct 2026 09:31:16 -0700 Subject: [PATCH 1/5] global stop-event for interactive consoles --- .../isolation_session/common/console_mode.rs | 120 ++++++++++++++---- .../isolation_session/common/manager.rs | 59 ++++++--- 2 files changed, 137 insertions(+), 42 deletions(-) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs index 703f408c1..e266235d4 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs @@ -10,7 +10,9 @@ //! wxc-exec's local console, used to seed the agent's ConPTY size. use std::sync::atomic::{AtomicIsize, Ordering}; +use std::sync::OnceLock; +use crate::mxc_common::process_util::OwnedHandle; use windows::Win32::Foundation::HANDLE; use windows::Win32::System::Console::{ GetConsoleMode, GetConsoleScreenBufferInfo, GetStdHandle, SetConsoleCtrlHandler, @@ -19,8 +21,8 @@ use windows::Win32::System::Console::{ ENABLE_VIRTUAL_TERMINAL_INPUT, ENABLE_VIRTUAL_TERMINAL_PROCESSING, ENABLE_WINDOW_INPUT, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, }; -use windows::Win32::System::Threading::SetEvent; -use windows_core::BOOL; +use windows::Win32::System::Threading::{CreateEventW, ResetEvent, SetEvent}; +use windows_core::{BOOL, PCWSTR}; // ── Local console raw-VT-mode RAII guard ────────────────────────────────── // @@ -144,12 +146,12 @@ impl Drop for ConsoleModeRestorer { // ── Console Ctrl handler RAII guard ────────────────────────────────────── // -// When wxc-exec runs interactively, the local console owns the keyboard. A -// Ctrl-C from the user — or the user clicking the cmd.exe X button — fires -// `CTRL_C_EVENT` / `CTRL_CLOSE_EVENT` on wxc-exec. The OS default for -// `CTRL_CLOSE_EVENT` is to call `ExitProcess` on wxc-exec a short time -// later, which kills the relay threads mid-write and drops the agent's -// final output. +// When the executor or in-process/attached SDK path runs interactively, the +// local console owns the keyboard. A Ctrl-C from the user — or the user clicking +// the cmd.exe X button — fires `CTRL_C_EVENT` / `CTRL_CLOSE_EVENT` on the +// hosting process. The OS default for `CTRL_CLOSE_EVENT` is to call +// `ExitProcess` a short time later, which kills the relay threads mid-write and +// drops the agent's final output. // // `CtrlHandlerGuard` installs a process-wide handler for the relay // lifetime that signals the stdin relay's stop event on either event, @@ -173,6 +175,51 @@ impl Drop for ConsoleModeRestorer { /// integer payload. static STDIN_STOP_EVENT_RAW: AtomicIsize = AtomicIsize::new(0); +struct ProcessStopEvent(OwnedHandle); + +impl ProcessStopEvent { + fn get(&self) -> HANDLE { + self.0.get() + } +} + +// SAFETY: the owned HANDLE is an opaque kernel handle value. It is never +// dereferenced by Rust, and Windows event handles may be signaled/reset from +// any thread in the owning process. +unsafe impl Send for ProcessStopEvent {} +// SAFETY: shared access exposes only the duplicated raw HANDLE value; all +// mutation is performed by thread-safe Win32 event operations. +unsafe impl Sync for ProcessStopEvent {} + +/// Process-lifetime stdin stop event used by the console Ctrl handler. +/// +/// The handler caches the raw value of this exact handle in +/// `STDIN_STOP_EVENT_RAW`. Keeping one process-global event, rather than +/// leaking one event per interactive exec, bounds handle use in long-lived +/// in-process attached hosts while ensuring a late handler can only signal +/// the same live idle event, never a closed or recycled handle. +static STDIN_STOP_EVENT: OnceLock = OnceLock::new(); + +pub(super) fn prepare_ctrl_handler_stop_event() -> windows_core::Result { + if STDIN_STOP_EVENT.get().is_none() { + let handle = unsafe { CreateEventW(None, true, false, PCWSTR::null())? }; + if STDIN_STOP_EVENT + .set(ProcessStopEvent(OwnedHandle::new(handle))) + .is_err() + { + // Another thread won initialization; the losing OwnedHandle is + // dropped here, leaving the process-global event intact. + } + } + let event = STDIN_STOP_EVENT + .get() + .expect("process stop event is initialized before use"); + unsafe { + ResetEvent(event.get())?; + } + Ok(event.get()) +} + /// Process-wide console-ctrl handler. Win32 callback signature; cannot /// capture environment, so reads its target from `STDIN_STOP_EVENT_RAW`. /// @@ -199,19 +246,20 @@ unsafe extern "system" fn ctrl_handler(ctrl_type: u32) -> BOOL { /// `CTRL_CLOSE_EVENT`, allowing the stdin relay to exit gracefully rather /// than being killed by the OS default `ExitProcess`. /// -/// Only one guard is alive at a time in wxc-exec's lifecycle (calls into -/// the manager are sequential per invocation). The static target handle -/// is set at install and cleared at drop. +/// Only one guard is alive at a time across the executor relay path and the +/// in-process/attached SDK relay path. The static target handle is set at +/// install and cleared at drop. pub(super) struct CtrlHandlerGuard { installed: bool, } impl CtrlHandlerGuard { - /// Install the handler. `stop_event` must outlive the guard — the - /// caller (`manager::create_process`) holds it in an `OwnedHandle` - /// that drops after this guard, satisfying the constraint by Drop - /// ordering. A failure to install is silently swallowed; the guard's - /// `Drop` becomes a no-op in that case. + /// Install the handler. `stop_event` must be the process-lifetime event + /// returned by [`prepare_ctrl_handler_stop_event`]; the executor and the + /// in-process/attached SDK caller both use that same live target so an + /// already-running OS ctrl-handler thread cannot race with per-exec handle + /// closure. A failure to install is silently swallowed; the guard's `Drop` + /// becomes a no-op in that case. pub(super) fn install(stop_event: HANDLE) -> Self { STDIN_STOP_EVENT_RAW.store(stop_event.0 as isize, Ordering::SeqCst); let installed = unsafe { SetConsoleCtrlHandler(Some(ctrl_handler), true) }.is_ok(); @@ -259,12 +307,11 @@ mod tests { use super::*; use std::sync::Mutex; - use windows::Win32::Foundation::{CloseHandle, WAIT_OBJECT_0}; + use windows::Win32::Foundation::{HANDLE, WAIT_OBJECT_0}; use windows::Win32::System::Console::{ CTRL_BREAK_EVENT, CTRL_LOGOFF_EVENT, CTRL_SHUTDOWN_EVENT, }; - use windows::Win32::System::Threading::{CreateEventW, ResetEvent, WaitForSingleObject}; - use windows_core::PCWSTR; + use windows::Win32::System::Threading::{ResetEvent, SetEvent, WaitForSingleObject}; /// All `CtrlHandlerGuard` / `ctrl_handler` tests must serialize against /// each other: they share `STDIN_STOP_EVENT_RAW` and the process-wide @@ -302,7 +349,7 @@ mod tests { // a console attached) and that the static target is cleared on drop. let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - let event = unsafe { CreateEventW(None, true, false, PCWSTR::null()).unwrap() }; + let event = prepare_ctrl_handler_stop_event().unwrap(); { let _guard = CtrlHandlerGuard::install(event); @@ -313,10 +360,6 @@ mod tests { // After drop, the static must be cleared. assert_eq!(STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), 0); - - unsafe { - let _ = CloseHandle(event); - } } #[test] @@ -327,7 +370,7 @@ mod tests { // its default behavior (which would `ExitProcess` on close). let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - let event = unsafe { CreateEventW(None, true, false, PCWSTR::null()).unwrap() }; + let event = prepare_ctrl_handler_stop_event().unwrap(); let _guard = CtrlHandlerGuard::install(event); for signal in [CTRL_C_EVENT, CTRL_CLOSE_EVENT] { @@ -355,11 +398,34 @@ mod tests { signal ); } - drop(_guard); + } + + #[test] + fn test_ctrl_handler_target_survives_guard_teardown() { + // Simulates the race shape where the OS ctrl-handler thread has already + // read the raw handle while the main thread drops the guard. The handle + // is process-lifetime, so a late SetEvent still targets the same valid + // idle event rather than a closed/recycled per-exec handle. + let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); + + let event = prepare_ctrl_handler_stop_event().unwrap(); + let guard = CtrlHandlerGuard::install(event); + let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); + assert_ne!(raw, 0); + + drop(guard); + assert_eq!(STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), 0); + unsafe { - let _ = CloseHandle(event); + ResetEvent(event).unwrap(); + SetEvent(HANDLE(raw as *mut core::ffi::c_void)).unwrap(); } + let post = unsafe { WaitForSingleObject(event, 100) }; + assert_eq!( + post, WAIT_OBJECT_0, + "late signal should still reach the process-lifetime stop event" + ); } #[test] diff --git a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs index 7ec4c8b79..04b07092d 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs @@ -27,7 +27,9 @@ use windows::Win32::System::Threading::{CreateEventW, SetEvent, WaitForSingleObj use windows::UI::WindowId; use windows_core::{HSTRING, PCWSTR}; -use super::console_mode::{get_local_console_size, ConsoleModeRestorer, CtrlHandlerGuard}; +use super::console_mode::{ + get_local_console_size, prepare_ctrl_handler_stop_event, ConsoleModeRestorer, CtrlHandlerGuard, +}; use super::console_relay::{create_console_relay_thread, ConsoleRelayParams}; use super::error::{ activation_error, check_result, format_iso_error, lifecycle_err, op, transport_err, @@ -418,16 +420,41 @@ impl IsolationSessionManager { let wxc_stdin = unsafe { GetStdHandle(STD_INPUT_HANDLE) } .map_err(|e| lifecycle_err(format!("GetStdHandle(stdin) failed: {}", e)))?; + enum StdinStopEvent { + ProcessGlobal(HANDLE), + PerExec(OwnedHandle), + } + + impl StdinStopEvent { + fn get(&self) -> HANDLE { + match self { + Self::ProcessGlobal(handle) => *handle, + Self::PerExec(handle) => handle.get(), + } + } + } + // Manual-reset stop event for the stdin relay. Effective for a waitable // `h_read` (console = TTY mode); for pipe handles it has no effect on a // blocked `ReadFile`, so that relay misses its join and ends only when - // the calling process exits. Only `wxc-exec` reaches the non-TTY case. - let stdin_stop_event = unsafe { - CreateEventW(None, true, false, PCWSTR::null()) - .map_err(|e| lifecycle_err(format!("CreateEventW(stdin stop): {}", e)))? + // the calling process exits. Interactive relays also install the Ctrl + // handler, so they use the process-lifetime event prepared and reset by + // `prepare_ctrl_handler_stop_event`; otherwise a handler thread that + // already read the raw handle could signal its event after it was closed. + // Non-interactive relays do not install the handler and have a + // per-exec owned event. + let stdin_stop_event = if options.interactive { + StdinStopEvent::ProcessGlobal( + prepare_ctrl_handler_stop_event() + .map_err(|e| lifecycle_err(format!("prepare stdin stop event: {}", e)))?, + ) + } else { + let handle = unsafe { + CreateEventW(None, true, false, PCWSTR::null()) + .map_err(|e| lifecycle_err(format!("CreateEventW(stdin stop): {}", e)))? + }; + StdinStopEvent::PerExec(OwnedHandle::new(handle)) }; - // Declared before `scope`, which holds a raw copy: it must drop last. - let stdin_stop_owned = OwnedHandle::new(stdin_stop_event); // Relay threads bridge the calling process's stdio to // `IsoSessionProcess`'s pipes: console-handle inheritance cannot cross @@ -440,8 +467,9 @@ impl IsolationSessionManager { // read their own duplicates, so `Close()` does not disturb them. // - The calling process's std handles (`GetStdHandle`): OS-owned. // We do NOT close them. - // - Stop event for stdin relay (`CreateEventW`): RAII-closed via - // `OwnedHandle`. + // - Stop event for stdin relay: process-lifetime when interactive + // because the Ctrl handler caches its raw value; otherwise + // per-exec and RAII-closed via `OwnedHandle`. // - Relay thread handles: RAII-closed via `OwnedHandle` after we // `WaitForSingleObject` on each. // @@ -454,7 +482,7 @@ impl IsolationSessionManager { let stderr_handle_val = started.stderr; let stdin_handle_val = started.stdin; - let mut scope = RelayScope::new(process, stdin_stop_owned.get()); + let mut scope = RelayScope::new(process, stdin_stop_event.get()); // In interactive mode, switch the calling process's console to raw VT // so only the agent's ConPTY echoes and renders; otherwise both do. @@ -465,16 +493,17 @@ impl IsolationSessionManager { None }; - // Install a console Ctrl handler that signals `stdin_stop_owned` + // Install a console Ctrl handler that signals `stdin_stop_event` // on Ctrl-C or terminal-close events, so the relay loops drain // cleanly instead of being terminated by the OS default // `ExitProcess`. Interactive mode only — non-interactive mode // wants the default behavior so the parent can terminate // wxc-exec via Ctrl-C. Installed after the workload starts: the - // event is manual-reset and never cleared, so an earlier Ctrl-C - // would stop the stdin relay for the session's lifetime. + // process-global manual-reset event was reset at the start of this + // interactive exec, so a signal from a previous exec cannot leak + // forward into this relay. let _ctrl_guard = if options.interactive { - Some(CtrlHandlerGuard::install(stdin_stop_owned.get())) + Some(CtrlHandlerGuard::install(stdin_stop_event.get())) } else { None }; @@ -536,7 +565,7 @@ impl IsolationSessionManager { duplicate_handle(HANDLE(stdin_handle_val as *mut core::ffi::c_void)) .map_err(|e| lifecycle_err(format!("duplicate stdin handle: {}", e)))?; // Shares the event object, so `signal_stop` still reaches the relay. - let stdin_h_stop = duplicate_handle(stdin_stop_owned.get()) + let stdin_h_stop = duplicate_handle(stdin_stop_event.get()) .map_err(|e| lifecycle_err(format!("duplicate stdin stop event: {}", e)))?; if options.interactive { // Clone the WinRT process handle so the relay thread holds From 79cce444cb04a3e1fad8b35c6cdc3b87a833fa6c Mon Sep 17 00:00:00 2001 From: Dom Giandinoto Date: Wed, 14 Oct 2026 10:07:24 -0700 Subject: [PATCH 2/5] fmt fix --- src/mxc-sdk/src/backends/isolation_session/common/manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs index 04b07092d..05308b72d 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs @@ -440,7 +440,7 @@ impl IsolationSessionManager { // the calling process exits. Interactive relays also install the Ctrl // handler, so they use the process-lifetime event prepared and reset by // `prepare_ctrl_handler_stop_event`; otherwise a handler thread that - // already read the raw handle could signal its event after it was closed. + // already read the raw handle could signal its event after it was closed. // Non-interactive relays do not install the handler and have a // per-exec owned event. let stdin_stop_event = if options.interactive { From 7112fd2f3aecf1e9d1a7a116436d2068582f7470 Mon Sep 17 00:00:00 2001 From: Dom Giandinoto Date: Thu, 15 Oct 2026 10:09:17 -0700 Subject: [PATCH 3/5] mutex approach --- .../isolation_session/common/console_mode.rs | 205 ++++++++++++------ .../isolation_session/common/manager.rs | 56 ++--- 2 files changed, 147 insertions(+), 114 deletions(-) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs index e266235d4..de4f89ca3 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs @@ -10,9 +10,8 @@ //! wxc-exec's local console, used to seed the agent's ConPTY size. use std::sync::atomic::{AtomicIsize, Ordering}; -use std::sync::OnceLock; +use std::sync::Mutex; -use crate::mxc_common::process_util::OwnedHandle; use windows::Win32::Foundation::HANDLE; use windows::Win32::System::Console::{ GetConsoleMode, GetConsoleScreenBufferInfo, GetStdHandle, SetConsoleCtrlHandler, @@ -21,8 +20,8 @@ use windows::Win32::System::Console::{ ENABLE_VIRTUAL_TERMINAL_INPUT, ENABLE_VIRTUAL_TERMINAL_PROCESSING, ENABLE_WINDOW_INPUT, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, }; -use windows::Win32::System::Threading::{CreateEventW, ResetEvent, SetEvent}; -use windows_core::{BOOL, PCWSTR}; +use windows::Win32::System::Threading::SetEvent; +use windows_core::BOOL; // ── Local console raw-VT-mode RAII guard ────────────────────────────────── // @@ -175,49 +174,15 @@ impl Drop for ConsoleModeRestorer { /// integer payload. static STDIN_STOP_EVENT_RAW: AtomicIsize = AtomicIsize::new(0); -struct ProcessStopEvent(OwnedHandle); +/// Serializes the console Ctrl handler against `CtrlHandlerGuard` +/// install/teardown so it does not use STDIN_STOP_EVENT_RAW after it has +/// been closed (and the handle potentially recycled by the OS). +static CTRL_HANDLER_LOCK: Mutex<()> = Mutex::new(()); -impl ProcessStopEvent { - fn get(&self) -> HANDLE { - self.0.get() - } -} - -// SAFETY: the owned HANDLE is an opaque kernel handle value. It is never -// dereferenced by Rust, and Windows event handles may be signaled/reset from -// any thread in the owning process. -unsafe impl Send for ProcessStopEvent {} -// SAFETY: shared access exposes only the duplicated raw HANDLE value; all -// mutation is performed by thread-safe Win32 event operations. -unsafe impl Sync for ProcessStopEvent {} - -/// Process-lifetime stdin stop event used by the console Ctrl handler. -/// -/// The handler caches the raw value of this exact handle in -/// `STDIN_STOP_EVENT_RAW`. Keeping one process-global event, rather than -/// leaking one event per interactive exec, bounds handle use in long-lived -/// in-process attached hosts while ensuring a late handler can only signal -/// the same live idle event, never a closed or recycled handle. -static STDIN_STOP_EVENT: OnceLock = OnceLock::new(); - -pub(super) fn prepare_ctrl_handler_stop_event() -> windows_core::Result { - if STDIN_STOP_EVENT.get().is_none() { - let handle = unsafe { CreateEventW(None, true, false, PCWSTR::null())? }; - if STDIN_STOP_EVENT - .set(ProcessStopEvent(OwnedHandle::new(handle))) - .is_err() - { - // Another thread won initialization; the losing OwnedHandle is - // dropped here, leaving the process-global event intact. - } - } - let event = STDIN_STOP_EVENT - .get() - .expect("process stop event is initialized before use"); - unsafe { - ResetEvent(event.get())?; - } - Ok(event.get()) +fn lock_ctrl_handler() -> std::sync::MutexGuard<'static, ()> { + CTRL_HANDLER_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) } /// Process-wide console-ctrl handler. Win32 callback signature; cannot @@ -229,6 +194,9 @@ pub(super) fn prepare_ctrl_handler_stop_event() -> windows_core::Result /// returns; no other state is touched. unsafe extern "system" fn ctrl_handler(ctrl_type: u32) -> BOOL { if ctrl_type == CTRL_C_EVENT || ctrl_type == CTRL_CLOSE_EVENT { + // Guard against use-after-free of STDIN_STOP_EVENT_RAW. + let _lock = lock_ctrl_handler(); + let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); if raw != 0 { let h = HANDLE(raw as *mut core::ffi::c_void); @@ -248,19 +216,19 @@ unsafe extern "system" fn ctrl_handler(ctrl_type: u32) -> BOOL { /// /// Only one guard is alive at a time across the executor relay path and the /// in-process/attached SDK relay path. The static target handle is set at -/// install and cleared at drop. +/// install and cleared at drop, both under [`CTRL_HANDLER_LOCK`]. pub(super) struct CtrlHandlerGuard { installed: bool, } impl CtrlHandlerGuard { - /// Install the handler. `stop_event` must be the process-lifetime event - /// returned by [`prepare_ctrl_handler_stop_event`]; the executor and the - /// in-process/attached SDK caller both use that same live target so an - /// already-running OS ctrl-handler thread cannot race with per-exec handle - /// closure. A failure to install is silently swallowed; the guard's `Drop` - /// becomes a no-op in that case. + /// Install the handler. `stop_event` must outlive the guard — the + /// caller (`manager::create_process`) holds it in an `OwnedHandle` + /// that drops after this guard, satisfying the constraint by Drop + /// ordering. A failure to install is silently swallowed; the guard's + /// `Drop` becomes a no-op in that case. pub(super) fn install(stop_event: HANDLE) -> Self { + let _lock = lock_ctrl_handler(); STDIN_STOP_EVENT_RAW.store(stop_event.0 as isize, Ordering::SeqCst); let installed = unsafe { SetConsoleCtrlHandler(Some(ctrl_handler), true) }.is_ok(); if !installed { @@ -272,6 +240,12 @@ impl CtrlHandlerGuard { impl Drop for CtrlHandlerGuard { fn drop(&mut self) { + // Unregister and clear the target under the lock. A handler blocked on + // the lock here will, on resume, read the zeroed raw and do nothing; a + // handler already past the lock finishes against the still-open handle + // because this guard drops before `create_process`'s owning + // `OwnedHandle` closes it. + let _lock = lock_ctrl_handler(); if self.installed { unsafe { let _ = SetConsoleCtrlHandler(Some(ctrl_handler), false); @@ -305,13 +279,16 @@ pub(super) fn get_local_console_size() -> Option<(u16, u16)> { #[cfg(test)] mod tests { use super::*; - use std::sync::Mutex; + use crate::mxc_common::process_util::OwnedHandle; use windows::Win32::Foundation::{HANDLE, WAIT_OBJECT_0}; use windows::Win32::System::Console::{ CTRL_BREAK_EVENT, CTRL_LOGOFF_EVENT, CTRL_SHUTDOWN_EVENT, }; - use windows::Win32::System::Threading::{ResetEvent, SetEvent, WaitForSingleObject}; + use windows::Win32::System::Threading::{ + CreateEventW, ResetEvent, SetEvent, WaitForSingleObject, + }; + use windows_core::PCWSTR; /// All `CtrlHandlerGuard` / `ctrl_handler` tests must serialize against /// each other: they share `STDIN_STOP_EVENT_RAW` and the process-wide @@ -319,6 +296,14 @@ mod tests { /// not touch these, so a module-local mutex is sufficient. static CTRL_HANDLER_TEST_LOCK: Mutex<()> = Mutex::new(()); + /// Create a per-execution manual-reset stop event, mirroring the one + /// `create_process` owns via `OwnedHandle`. + fn make_stop_event() -> OwnedHandle { + let handle = unsafe { CreateEventW(None, true, false, PCWSTR::null()) } + .expect("create manual-reset test event"); + OwnedHandle::new(handle) + } + #[test] fn test_get_local_console_size_does_not_panic() { // `cargo test` runs with stdio as pipes (`None`) on CI; locally a @@ -349,7 +334,8 @@ mod tests { // a console attached) and that the static target is cleared on drop. let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - let event = prepare_ctrl_handler_stop_event().unwrap(); + let owned = make_stop_event(); + let event = owned.get(); { let _guard = CtrlHandlerGuard::install(event); @@ -370,7 +356,8 @@ mod tests { // its default behavior (which would `ExitProcess` on close). let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - let event = prepare_ctrl_handler_stop_event().unwrap(); + let owned = make_stop_event(); + let event = owned.get(); let _guard = CtrlHandlerGuard::install(event); for signal in [CTRL_C_EVENT, CTRL_CLOSE_EVENT] { @@ -402,29 +389,105 @@ mod tests { } #[test] - fn test_ctrl_handler_target_survives_guard_teardown() { - // Simulates the race shape where the OS ctrl-handler thread has already - // read the raw handle while the main thread drops the guard. The handle - // is process-lifetime, so a late SetEvent still targets the same valid - // idle event rather than a closed/recycled per-exec handle. + fn test_ctrl_handler_is_inert_after_guard_teardown() { + // After the guard drops it zeros the cached raw under the lock, so a + // handler that fires post-teardown signals nothing. With per-exec + // events this is what keeps a late handler from reaching into a + // closed/recycled handle: there is simply no live target to touch. let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - let event = prepare_ctrl_handler_stop_event().unwrap(); - let guard = CtrlHandlerGuard::install(event); - let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); - assert_ne!(raw, 0); + let owned = make_stop_event(); + let event = owned.get(); - drop(guard); + { + let _guard = CtrlHandlerGuard::install(event); + assert_ne!(STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), 0); + } assert_eq!(STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), 0); unsafe { ResetEvent(event).unwrap(); - SetEvent(HANDLE(raw as *mut core::ffi::c_void)).unwrap(); } - let post = unsafe { WaitForSingleObject(event, 100) }; - assert_eq!( + let result = unsafe { ctrl_handler(CTRL_C_EVENT) }; + assert!( + result.as_bool(), + "handler still suppresses the OS default after teardown" + ); + let post = unsafe { WaitForSingleObject(event, 0) }; + assert_ne!( post, WAIT_OBJECT_0, - "late signal should still reach the process-lifetime stop event" + "a handler after teardown must not signal the retired event" + ); + } + + #[test] + fn test_stale_generation_signal_does_not_disturb_next_execution() { + // Regression for reviewer finding (1): a late callback aimed at a prior + // execution's stop event must not stop the next execution's stdin + // relay. The next execution is prepared (its own event installed) + // before the stale callback is allowed to fire; because each execution + // owns a distinct event, signaling execution A's handle leaves + // execution B's event untouched. + let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); + + // Execution A installs and tears down, capturing its (still test-owned) + // raw handle value. + let owned_a = make_stop_event(); + let event_a = owned_a.get(); + let raw_a = { + let guard_a = CtrlHandlerGuard::install(event_a); + let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); + drop(guard_a); + raw + }; + assert_ne!(raw_a, 0); + + // Execution B begins with its own event, prepared before A's late + // callback is allowed to fire. + let owned_b = make_stop_event(); + let event_b = owned_b.get(); + let _guard_b = CtrlHandlerGuard::install(event_b); + unsafe { + ResetEvent(event_b).unwrap(); + } + + // A's delayed callback now signals A's own handle. + unsafe { + SetEvent(HANDLE(raw_a as *mut core::ffi::c_void)).unwrap(); + } + + // B's relay keys off event_b, which must remain unsignaled. + let b_state = unsafe { WaitForSingleObject(event_b, 0) }; + assert_ne!( + b_state, WAIT_OBJECT_0, + "a stale signal to execution A must not stop execution B's relay" + ); + } + + #[test] + fn test_per_execution_stop_events_are_independent() { + // Regression for reviewer finding (2): a stdin relay that outlived its + // bounded join watches its own execution's event. Signaling one + // execution's stop event must neither set nor clear another's, so the + // surviving relay sees only its own stop and never consumes the next + // execution's input. + let owned_a = make_stop_event(); + let owned_b = make_stop_event(); + + unsafe { + // Execution A's teardown signals A's event only. + SetEvent(owned_a.get()).unwrap(); + } + + let a_state = unsafe { WaitForSingleObject(owned_a.get(), 0) }; + let b_state = unsafe { WaitForSingleObject(owned_b.get(), 0) }; + assert_eq!( + a_state, WAIT_OBJECT_0, + "A's own event is signaled for A's relay" + ); + assert_ne!( + b_state, WAIT_OBJECT_0, + "B's event is independent and stays unsignaled" ); } diff --git a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs index 05308b72d..2fff5c711 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs @@ -27,9 +27,7 @@ use windows::Win32::System::Threading::{CreateEventW, SetEvent, WaitForSingleObj use windows::UI::WindowId; use windows_core::{HSTRING, PCWSTR}; -use super::console_mode::{ - get_local_console_size, prepare_ctrl_handler_stop_event, ConsoleModeRestorer, CtrlHandlerGuard, -}; +use super::console_mode::{get_local_console_size, ConsoleModeRestorer, CtrlHandlerGuard}; use super::console_relay::{create_console_relay_thread, ConsoleRelayParams}; use super::error::{ activation_error, check_result, format_iso_error, lifecycle_err, op, transport_err, @@ -420,41 +418,15 @@ impl IsolationSessionManager { let wxc_stdin = unsafe { GetStdHandle(STD_INPUT_HANDLE) } .map_err(|e| lifecycle_err(format!("GetStdHandle(stdin) failed: {}", e)))?; - enum StdinStopEvent { - ProcessGlobal(HANDLE), - PerExec(OwnedHandle), - } - - impl StdinStopEvent { - fn get(&self) -> HANDLE { - match self { - Self::ProcessGlobal(handle) => *handle, - Self::PerExec(handle) => handle.get(), - } - } - } - // Manual-reset stop event for the stdin relay. Effective for a waitable // `h_read` (console = TTY mode); for pipe handles it has no effect on a // blocked `ReadFile`, so that relay misses its join and ends only when - // the calling process exits. Interactive relays also install the Ctrl - // handler, so they use the process-lifetime event prepared and reset by - // `prepare_ctrl_handler_stop_event`; otherwise a handler thread that - // already read the raw handle could signal its event after it was closed. - // Non-interactive relays do not install the handler and have a - // per-exec owned event. - let stdin_stop_event = if options.interactive { - StdinStopEvent::ProcessGlobal( - prepare_ctrl_handler_stop_event() - .map_err(|e| lifecycle_err(format!("prepare stdin stop event: {}", e)))?, - ) - } else { - let handle = unsafe { - CreateEventW(None, true, false, PCWSTR::null()) - .map_err(|e| lifecycle_err(format!("CreateEventW(stdin stop): {}", e)))? - }; - StdinStopEvent::PerExec(OwnedHandle::new(handle)) + // the calling process exits. Only `wxc-exec` reaches the non-TTY case. + let stdin_stop_event = unsafe { + CreateEventW(None, true, false, PCWSTR::null()) + .map_err(|e| lifecycle_err(format!("CreateEventW(stdin stop): {}", e)))? }; + let stdin_stop_owned = OwnedHandle::new(stdin_stop_event); // Relay threads bridge the calling process's stdio to // `IsoSessionProcess`'s pipes: console-handle inheritance cannot cross @@ -467,9 +439,8 @@ impl IsolationSessionManager { // read their own duplicates, so `Close()` does not disturb them. // - The calling process's std handles (`GetStdHandle`): OS-owned. // We do NOT close them. - // - Stop event for stdin relay: process-lifetime when interactive - // because the Ctrl handler caches its raw value; otherwise - // per-exec and RAII-closed via `OwnedHandle`. + // - Stop event for stdin relay (`CreateEventW`): RAII-closed via + // `OwnedHandle`. // - Relay thread handles: RAII-closed via `OwnedHandle` after we // `WaitForSingleObject` on each. // @@ -482,7 +453,7 @@ impl IsolationSessionManager { let stderr_handle_val = started.stderr; let stdin_handle_val = started.stdin; - let mut scope = RelayScope::new(process, stdin_stop_event.get()); + let mut scope = RelayScope::new(process, stdin_stop_owned.get()); // In interactive mode, switch the calling process's console to raw VT // so only the agent's ConPTY echoes and renders; otherwise both do. @@ -499,11 +470,10 @@ impl IsolationSessionManager { // `ExitProcess`. Interactive mode only — non-interactive mode // wants the default behavior so the parent can terminate // wxc-exec via Ctrl-C. Installed after the workload starts: the - // process-global manual-reset event was reset at the start of this - // interactive exec, so a signal from a previous exec cannot leak - // forward into this relay. + // event is manual-reset and never cleared, so an earlier Ctrl-C + // would stop the stdin relay for the session's lifetime. let _ctrl_guard = if options.interactive { - Some(CtrlHandlerGuard::install(stdin_stop_event.get())) + Some(CtrlHandlerGuard::install(stdin_stop_owned.get())) } else { None }; @@ -565,7 +535,7 @@ impl IsolationSessionManager { duplicate_handle(HANDLE(stdin_handle_val as *mut core::ffi::c_void)) .map_err(|e| lifecycle_err(format!("duplicate stdin handle: {}", e)))?; // Shares the event object, so `signal_stop` still reaches the relay. - let stdin_h_stop = duplicate_handle(stdin_stop_event.get()) + let stdin_h_stop = duplicate_handle(stdin_stop_owned.get()) .map_err(|e| lifecycle_err(format!("duplicate stdin stop event: {}", e)))?; if options.interactive { // Clone the WinRT process handle so the relay thread holds From a81f7b0894522d8ca7eda05776ef79d6b4c98c98 Mon Sep 17 00:00:00 2001 From: Dom Giandinoto Date: Thu, 15 Oct 2026 16:50:46 -0700 Subject: [PATCH 4/5] update tests --- .../isolation_session/common/console_mode.rs | 133 +++++++++++++----- 1 file changed, 99 insertions(+), 34 deletions(-) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs index de4f89ca3..d347a9cc9 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs @@ -198,6 +198,8 @@ unsafe extern "system" fn ctrl_handler(ctrl_type: u32) -> BOOL { let _lock = lock_ctrl_handler(); let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); + #[cfg(test)] + tests::ctrl_handler_pause_point(); if raw != 0 { let h = HANDLE(raw as *mut core::ffi::c_void); let _ = SetEvent(h); @@ -279,9 +281,11 @@ pub(super) fn get_local_console_size() -> Option<(u16, u16)> { #[cfg(test)] mod tests { use super::*; + use std::sync::mpsc; + use std::time::Duration; use crate::mxc_common::process_util::OwnedHandle; - use windows::Win32::Foundation::{HANDLE, WAIT_OBJECT_0}; + use windows::Win32::Foundation::WAIT_OBJECT_0; use windows::Win32::System::Console::{ CTRL_BREAK_EVENT, CTRL_LOGOFF_EVENT, CTRL_SHUTDOWN_EVENT, }; @@ -296,6 +300,25 @@ mod tests { /// not touch these, so a module-local mutex is sufficient. static CTRL_HANDLER_TEST_LOCK: Mutex<()> = Mutex::new(()); + /// One-shot pause installed by a test: `(loaded_tx, resume_rx)`. When set, + /// the next `ctrl_handler` call reports that it has loaded its target (while + /// still holding `CTRL_HANDLER_LOCK`) and blocks until the test resumes it. + type PausePoint = (mpsc::Sender<()>, mpsc::Receiver<()>); + static CTRL_HANDLER_PAUSE: Mutex> = Mutex::new(None); + + /// Called by `ctrl_handler` between loading its target and `SetEvent`. + /// Must not panic: it runs inside an `extern "system"` function. + pub(super) fn ctrl_handler_pause_point() { + let pause = CTRL_HANDLER_PAUSE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .take(); + if let Some((loaded_tx, resume_rx)) = pause { + let _ = loaded_tx.send(()); + let _ = resume_rx.recv(); + } + } + /// Create a per-execution manual-reset stop event, mirroring the one /// `create_process` owns via `OwnedHandle`. fn make_stop_event() -> OwnedHandle { @@ -421,47 +444,89 @@ mod tests { } #[test] - fn test_stale_generation_signal_does_not_disturb_next_execution() { - // Regression for reviewer finding (1): a late callback aimed at a prior - // execution's stop event must not stop the next execution's stdin - // relay. The next execution is prepared (its own event installed) - // before the stale callback is allowed to fire; because each execution - // owns a distinct event, signaling execution A's handle leaves - // execution B's event untouched. + fn test_guard_teardown_waits_for_in_flight_handler() { + // Regression for the callback/teardown race. Pause the real handler + // after it has loaded its target (holding CTRL_HANDLER_LOCK), start + // guard teardown on another thread, and verify teardown cannot finish + // until the handler has signaled. Without the lock, teardown would + // complete while the handler still held the raw value, and production + // would then close the event out from under the pending SetEvent. + let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); + + // Owner outlives teardown, matching create_process's drop order. + let owned = make_stop_event(); + let event = owned.get(); + let guard = CtrlHandlerGuard::install(event); + + let (loaded_tx, loaded_rx) = mpsc::channel(); + let (resume_tx, resume_rx) = mpsc::channel(); + *CTRL_HANDLER_PAUSE.lock().unwrap() = Some((loaded_tx, resume_rx)); + + let handler = std::thread::spawn(|| unsafe { ctrl_handler(CTRL_C_EVENT) }.as_bool()); + loaded_rx + .recv_timeout(Duration::from_secs(5)) + .expect("handler should reach the pause point"); + + let (done_tx, done_rx) = mpsc::channel(); + let teardown = std::thread::spawn(move || { + drop(guard); + let _ = done_tx.send(()); + }); + + assert!( + matches!( + done_rx.recv_timeout(Duration::from_millis(250)), + Err(mpsc::RecvTimeoutError::Timeout) + ), + "guard teardown must block while the handler is between load and SetEvent" + ); + assert_ne!( + STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), + 0, + "target must not be cleared while the handler is in flight" + ); + + resume_tx.send(()).unwrap(); + assert!(handler.join().unwrap(), "handler must return TRUE"); + done_rx + .recv_timeout(Duration::from_secs(5)) + .expect("teardown should finish once the handler is done"); + teardown.join().unwrap(); + + assert_eq!( + unsafe { WaitForSingleObject(event, 0) }, + WAIT_OBJECT_0, + "the in-flight handler signals the still-open event" + ); + assert_eq!(STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst), 0); + } + + #[test] + fn test_ctrl_handler_after_rollover_targets_only_current_execution() { + // A handler that runs after execution A has torn down and execution B + // has installed reads B's target, so it signals only B's event. A's + // retired event is never touched. let _lock = CTRL_HANDLER_TEST_LOCK.lock().unwrap(); - // Execution A installs and tears down, capturing its (still test-owned) - // raw handle value. let owned_a = make_stop_event(); - let event_a = owned_a.get(); - let raw_a = { - let guard_a = CtrlHandlerGuard::install(event_a); - let raw = STDIN_STOP_EVENT_RAW.load(Ordering::SeqCst); - drop(guard_a); - raw - }; - assert_ne!(raw_a, 0); - - // Execution B begins with its own event, prepared before A's late - // callback is allowed to fire. + drop(CtrlHandlerGuard::install(owned_a.get())); + let owned_b = make_stop_event(); - let event_b = owned_b.get(); - let _guard_b = CtrlHandlerGuard::install(event_b); - unsafe { - ResetEvent(event_b).unwrap(); - } + let guard_b = CtrlHandlerGuard::install(owned_b.get()); - // A's delayed callback now signals A's own handle. - unsafe { - SetEvent(HANDLE(raw_a as *mut core::ffi::c_void)).unwrap(); - } + assert!(unsafe { ctrl_handler(CTRL_C_EVENT) }.as_bool()); - // B's relay keys off event_b, which must remain unsignaled. - let b_state = unsafe { WaitForSingleObject(event_b, 0) }; assert_ne!( - b_state, WAIT_OBJECT_0, - "a stale signal to execution A must not stop execution B's relay" + unsafe { WaitForSingleObject(owned_a.get(), 0) }, + WAIT_OBJECT_0, + "the retired execution's event must not be signaled" + ); + assert_eq!( + unsafe { WaitForSingleObject(owned_b.get(), 0) }, + WAIT_OBJECT_0, + "the current execution's event is signaled" ); + drop(guard_b); } #[test] From 6a3504b3c1ab6f5910cdfd314221f439305af60e Mon Sep 17 00:00:00 2001 From: Dom Giandinoto Date: Fri, 16 Oct 2026 10:04:55 -0700 Subject: [PATCH 5/5] removing unnecessary comment edits --- .../isolation_session/common/console_mode.rs | 18 +++++++++--------- .../isolation_session/common/manager.rs | 3 ++- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs index d347a9cc9..ee13742a3 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/console_mode.rs @@ -145,12 +145,12 @@ impl Drop for ConsoleModeRestorer { // ── Console Ctrl handler RAII guard ────────────────────────────────────── // -// When the executor or in-process/attached SDK path runs interactively, the -// local console owns the keyboard. A Ctrl-C from the user — or the user clicking -// the cmd.exe X button — fires `CTRL_C_EVENT` / `CTRL_CLOSE_EVENT` on the -// hosting process. The OS default for `CTRL_CLOSE_EVENT` is to call -// `ExitProcess` a short time later, which kills the relay threads mid-write and -// drops the agent's final output. +// When wxc-exec runs interactively, the local console owns the keyboard. A +// Ctrl-C from the user — or the user clicking the cmd.exe X button — fires +// `CTRL_C_EVENT` / `CTRL_CLOSE_EVENT` on wxc-exec. The OS default for +// `CTRL_CLOSE_EVENT` is to call `ExitProcess` on wxc-exec a short time +// later, which kills the relay threads mid-write and drops the agent's +// final output. // // `CtrlHandlerGuard` installs a process-wide handler for the relay // lifetime that signals the stdin relay's stop event on either event, @@ -216,9 +216,9 @@ unsafe extern "system" fn ctrl_handler(ctrl_type: u32) -> BOOL { /// `CTRL_CLOSE_EVENT`, allowing the stdin relay to exit gracefully rather /// than being killed by the OS default `ExitProcess`. /// -/// Only one guard is alive at a time across the executor relay path and the -/// in-process/attached SDK relay path. The static target handle is set at -/// install and cleared at drop, both under [`CTRL_HANDLER_LOCK`]. +/// Only one guard is alive at a time in wxc-exec's lifecycle (calls into +/// the manager are sequential per invocation). The static target handle +/// is set at install and cleared at drop, both under [`CTRL_HANDLER_LOCK`]. pub(super) struct CtrlHandlerGuard { installed: bool, } diff --git a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs index 2fff5c711..7ec4c8b79 100644 --- a/src/mxc-sdk/src/backends/isolation_session/common/manager.rs +++ b/src/mxc-sdk/src/backends/isolation_session/common/manager.rs @@ -426,6 +426,7 @@ impl IsolationSessionManager { CreateEventW(None, true, false, PCWSTR::null()) .map_err(|e| lifecycle_err(format!("CreateEventW(stdin stop): {}", e)))? }; + // Declared before `scope`, which holds a raw copy: it must drop last. let stdin_stop_owned = OwnedHandle::new(stdin_stop_event); // Relay threads bridge the calling process's stdio to @@ -464,7 +465,7 @@ impl IsolationSessionManager { None }; - // Install a console Ctrl handler that signals `stdin_stop_event` + // Install a console Ctrl handler that signals `stdin_stop_owned` // on Ctrl-C or terminal-close events, so the relay loops drain // cleanly instead of being terminated by the OS default // `ExitProcess`. Interactive mode only — non-interactive mode