Add --wait-for-debugger flag for BaseContainer/AppContainer runners #712
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| //! `--wait-for-debugger` support. | ||
| //! | ||
| //! The mechanism: the runners create the sandboxed child suspended and | ||
| //! assign it to the job object exactly as before, but instead of calling | ||
| //! `ResumeThread` immediately, they hand off to | ||
| //! [`wait_for_debugger_then_resume`], which: | ||
| //! | ||
| //! 1. Logs the PID and blocks — with **no timeout** — polling | ||
| //! `CheckRemoteDebuggerPresent` on the child's process handle. The main | ||
| //! thread has not run any code yet — not even its own loader | ||
| //! initialization, so no dependent DLL past the main image is mapped — | ||
| //! so nothing here races an operator who takes an arbitrarily long time | ||
| //! to attach. | ||
| //! 2. The instant a debugger attaches, calls `ResumeThread` exactly once. | ||
| //! | ||
| //! Why step 2 doesn't let the target run away before the operator sets | ||
| //! breakpoints: attaching a debugger (`DebugActiveProcess`) synthesizes an | ||
| //! initial batch of debug events (process/thread/module state) that stay | ||
| //! *outstanding* — and every thread stays frozen — until the debugger | ||
| //! itself acknowledges them via `ContinueDebugEvent`/`g`. That freeze is a | ||
| //! second, independent contribution to the same suspend-count field | ||
| //! `CREATE_SUSPENDED` used, which is why attaching used to leave the count | ||
| //! at 2 (our 1 + the debugger's 1) and required an operator-side `~0 m` | ||
| //! before `g`. By calling `ResumeThread` ourselves the moment attach is | ||
| //! detected, we cancel only *our own* contribution — the debugger's | ||
| //! independent freeze is still holding the thread — so all the operator | ||
| //! needs is a plain `g`. Every breakpoint set beforehand is still | ||
| //! necessarily a deferred/pending one (nothing but the main image has | ||
| //! loaded), so it resolves normally once the target actually starts running | ||
| //! after `g`. | ||
| //! | ||
| //! Once resumed this way, `wxc-exec` falls into its normal (already | ||
| //! unbounded when `script_timeout` is 0) wait for the child to exit — no | ||
| //! special-casing there. Ctrl-C during the attach-wait is handled by the | ||
| //! existing `sandbox_tracking` console-control handler (registered well | ||
| //! before this point), which runs on its own OS thread independent of this | ||
| //! poll loop. | ||
| //! | ||
| //! Failure handling: a `CheckRemoteDebuggerPresent` error (e.g. the process | ||
| //! handle going bad because the suspended child was killed externally while | ||
| //! we were polling) is a permanent condition, not "no debugger yet" — retrying | ||
| //! it forever would hang `wxc-exec` indefinitely. Likewise a failing | ||
| //! `ResumeThread` after attach must not be reported as success. Both cases | ||
| //! terminate the child and return an error, mirroring | ||
| //! `SpawnedChild::resume`'s fail-closed behavior; the caller only needs to | ||
| //! tear down its own sandbox resources and surface the error. | ||
|
|
||
| use std::fmt::Write as _; | ||
| use std::time::Duration; | ||
|
|
||
| use windows::Win32::Foundation::{GetLastError, HANDLE}; | ||
| use windows::Win32::System::Diagnostics::Debug::CheckRemoteDebuggerPresent; | ||
| use windows::Win32::System::Threading::{ResumeThread, TerminateProcess}; | ||
| use windows_core::BOOL; | ||
|
|
||
| use wxc_common::error::WxcError; | ||
| use wxc_common::logger::Logger; | ||
|
|
||
| /// How often to poll `CheckRemoteDebuggerPresent` while waiting. | ||
| const POLL_INTERVAL: Duration = Duration::from_millis(100); | ||
|
|
||
| /// Block (no timeout) until a debugger attaches to `process`, then resume | ||
| /// `thread` exactly once — cancelling only the `CREATE_SUSPENDED` increment | ||
| /// `wxc-exec` itself added, leaving the debugger's own attach-time freeze in | ||
| /// place so the operator's plain `g` is what actually starts it running. | ||
| /// | ||
| /// On failure the child is terminated (fail closed) before returning | ||
| /// [`WxcError::Process`]; the caller only needs to tear down its own sandbox | ||
| /// resources and surface the error. | ||
| /// | ||
| /// # Safety | ||
| /// `process` and `thread` must be valid, still-open handles to the just | ||
| /// created, still-suspended child for the duration of this call; the caller | ||
| /// retains ownership (this function does not close either handle). | ||
| pub fn wait_for_debugger_then_resume( | ||
|
Member
There was a problem hiding this comment. The wait loop's state machine can't be tested without a real debugger and a real process.
The practical cost is visible in this PR: the branch coverage that exists is on the guards around the feature ( A closure-based seam is enough here — no trait or new module needed: fn wait_loop(
mut is_attached: impl FnMut() -> Result<bool, WxcError>,
mut is_alive: impl FnMut() -> bool,
mut tick: impl FnMut(),
) -> Result<(), WxcError> { /* the loop, and only the loop */ }The Win32 version passes real closures; tests pass canned sequences ( I'd treat this as a should-fix rather than a merge blocker — but it's the difference between the two hang fixes being verified and being merely believed, so it's worth doing in the same change rather than as follow-up. |
||
| process: HANDLE, | ||
| thread: HANDLE, | ||
| pid: u32, | ||
| logger: &mut Logger, | ||
| ) -> Result<(), WxcError> { | ||
| let banner = format!("Process suspended, PID: {pid}. Attach a debugger to continue."); | ||
| let _ = writeln!(logger, "{banner}"); | ||
| eprintln!("{banner}"); | ||
|
|
||
| loop { | ||
|
Member
There was a problem hiding this comment. Ctrl-C during the attach-wait orphans the suspended child permanently. The module doc (lines 41-44) says this case is covered:
That handler runs, but it doesn't touch the child. Tracing what it actually does:
Net effect: Two options, both cheap:
I'd lean toward (2) gated on the flag, with (1) as the fallback if kill-on-close turns out to interact badly with the existing teardown ordering. Worth pairing with a regression test: launch with Either way, please also fix the lines 41-44 comment — right now it documents this as solved, which is what would stop the next reader from noticing. |
||
| let mut present = BOOL(0); | ||
|
Member
There was a problem hiding this comment. The loop has no cooperative exit — every way out is external. Flagging this mostly to note it's not fully addressed by the other two loop findings, in case it looks redundant. Adding a liveness check fixes the dead-child case, and killing the child from the Ctrl-C path fixes the orphan case — but both work by destroying the thing the loop is polling. The loop itself still has exactly one voluntary exit ( Two places that matters:
An |
||
| // SAFETY: `process` is a valid, open handle for the duration of this | ||
| // call (caller contract); `present` is a valid out-param on the stack. | ||
| let checked = unsafe { CheckRemoteDebuggerPresent(process, &mut present) }; | ||
| match checked { | ||
| Ok(()) if present.as_bool() => break, | ||
| Ok(()) => std::thread::sleep(POLL_INTERVAL), | ||
|
Comment on lines
+93
to
+95
Member
There was a problem hiding this comment. If the suspended child dies before a debugger attaches, this loop spins forever. The
But the handle can't go bad. So on external termination the loop takes the
match checked {
Ok(()) if present.as_bool() => break,
Ok(()) => {
// Doubles as the poll interval: returns early only if the child
// died, which can never resolve into an attach.
if unsafe { WaitForSingleObject(process, POLL_INTERVAL.as_millis() as u32) }
== WAIT_OBJECT_0
{
return Err(WxcError::Process(format!(
"PID {pid} exited before a debugger attached"
)));
}
}
Err(e) => { /* unchanged */ }
}Keep the This is also the cheapest of the findings to lock in with a test: spawn any suspended process, |
||
| Err(e) => { | ||
| // The handle went bad (e.g. the still-suspended child was | ||
| // terminated externally) -- this can never resolve on its | ||
| // own, so fail closed instead of polling forever. | ||
| // SAFETY: `process` is the caller's still-owned handle; | ||
| // terminating it before returning matches the fail-closed | ||
| // contract every other launch-failure path in the runners | ||
| // follows. | ||
| unsafe { | ||
| let _ = TerminateProcess(process, u32::MAX); | ||
| } | ||
| return Err(WxcError::Process(format!( | ||
| "CheckRemoteDebuggerPresent failed while waiting for a debugger to \ | ||
| attach to PID {pid}: {e}" | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: `thread` is the caller's still-owned, still-suspended main | ||
| // thread handle; ResumeThread only adjusts its suspend count. This | ||
| // cancels exactly the one increment wxc-exec added via CREATE_SUSPENDED | ||
| // — the debugger's own attach-time freeze (a separate contribution to | ||
| // the same counter) is unaffected and keeps the thread from actually | ||
| // running until the operator issues `g`. | ||
| let resumed = unsafe { ResumeThread(thread) }; | ||
| if resumed == u32::MAX { | ||
| let err = unsafe { GetLastError() }; | ||
| // SAFETY: same fail-closed contract as above. | ||
| unsafe { | ||
| let _ = TerminateProcess(process, u32::MAX); | ||
| } | ||
| return Err(WxcError::Process(format!( | ||
| "ResumeThread failed after debugger attach for PID {pid}: {err:?}" | ||
| ))); | ||
| } | ||
| let _ = writeln!(logger, "Process started, PID: {pid}."); | ||
| Ok(()) | ||
|
Comment on lines
+121
to
+133
Member
There was a problem hiding this comment. The feature's entire runtime behavior ships with zero automated coverage. The PR's validation section lists build, Concretely, all of these would ship green today:
That last pair is the part I'd push on regardless of how the seam question (see the testability comment above) is resolved, because it needs no Win32 fixture at all:
Both are a few lines each and lock in the plumbing half of the feature. For the Win32 half, if the loop gets a closure seam then the interesting cases (not-attached-then-attached, resume failure, check failure, child dies mid-wait) are all cheap unit tests. Without a seam, the fallback is a |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
prefer_base_containerfix made this branch dead code — worth deleting it now.Following on from the thread above: routing
--wait-for-debuggeraway from BaseContainer viaprefer_base_container = !request.wait_for_debuggeris the right fix, and thevalidate()rejection is a sensible backstop for direct callers. Together they mean this branch inspawn_basecan no longer execute.spawn_baseis private, its only caller runsself.validate(request)first, andvalidaterejects the flag unconditionally at line 1293 — sorequest.wait_for_debuggeris provablyfalseby the time control reaches line 1181.That leaves ~50 lines of unreachable code carrying its own error strings (
WAIT_FOR_DEBUGGER_FAILED_MSG), its own teardown sequence, and its ownScriptResponseconstruction, all of which have to be kept in step with the job-setup cleanup path next to them and with the AppContainer copy of the same logic. Defense-in-depth is cheap when it's an assertion; it's less cheap when it's a full parallel error path that no test can reach and no reader can verify.Suggest deleting the branch and keeping the unconditional resume. If you'd rather keep a tripwire for the "someone refactors
spawn_base's caller and drops thevalidatecall" scenario, a one-liner does the same job without the maintenance surface:Non-blocking — but the two guards you added are strictly better than this branch, and leaving all three in place obscures which one is actually load-bearing.