diff --git a/examples/abandoned_launch.rs b/examples/abandoned_launch.rs new file mode 100644 index 0000000..0c9066c --- /dev/null +++ b/examples/abandoned_launch.rs @@ -0,0 +1,134 @@ +//! Scratch measurement for dbgscope#141 (not part of the public API): a launch guard dropped +//! **before** anything pumps leaves its `Waiting` entry removed and no exclusion behind it, while +//! `CreateProcessWide` is still queued. Whose process does the *next* launch get? +//! +//! The issue was written from reasoning. Both of its load-bearing claims needed measuring, and the +//! first draft of this file measured the wrong thing: +//! +//! - **A.** How many queued creates does **one** `WaitForEvent` realise? If a single pump realises +//! both, then counting the session when `wait()` returns cannot tell a guard satisfied by its own +//! process from one satisfied by its predecessor's — which is what the first version of this +//! example did, and why it reported "waited for its own process" for a case it had not tested. +//! +//! - **B.** Does a launch whose image does not exist fail at `launch_process_begin`, or later +//! inside the wait? This decides whether the fix is hard. `deferred_arrival`'s arm C says "fails +//! *inside* the wait", but that arm calls the combined `launch_process`, which cannot tell the +//! two apart — the error comes back from one call either way. windbg-mcp's `worker.rs` says the +//! opposite and claims a live check. +//! +//! - **C.** The contract itself, observed rather than inferred: when the second guard's `wait()` +//! returns, is that guard's **own program** in the session? Distinguishable images (`cmd.exe` +//! then `ping.exe`) rather than a count, because a count cannot answer it. **This arm reports +//! 0 short and that is not the all-clear it looks like**: it reads *membership*, and membership +//! is exactly the weaker claim this crate is built on not confusing with having stopped. The +//! defect is real and is caught by reading the register instead -- +//! `test_an_abandoned_launch_does_not_hand_its_process_to_the_next_one`, which needs internals +//! this example does not have. Kept as the record of what the public surface can and cannot +//! see. + +use dbgscope::dbgeng::DebugEngine; +use std::time::Instant; + +/// The session's process listing, through the public surface only — the same `|` reading +/// `deferred_arrival` uses, since `session_processes` is private to the crate. +fn listing(e: &DebugEngine) -> String { + e.execute_command("|").unwrap_or_default() +} + +fn count(listing: &str) -> usize { + listing.lines().filter(|l| l.contains("id:")).count() +} + +/// Arm A: does one pump realise one queued create, or all of them? +fn how_many_creates_one_pump_realises() { + println!("\n=== A. two queued creates, one explicit wait ==="); + let e = DebugEngine::new(); + let first = e.launch_process_begin("cmd.exe /c ping -n 30 127.0.0.1"); + let second = e.launch_process_begin("ping.exe -n 30 127.0.0.2"); + println!( + " both begun: {:?} / {:?}, session {} process(es)", + first.is_ok(), + second.is_ok(), + count(&listing(&e)) + ); + // Neither guard is waited on: this arm is about the pump, not about delivery. + drop(first); + drop(second); + for pump in 1..=3 { + let started = Instant::now(); + let waited = e.wait_for_event(20_000); + println!( + " pump {pump}: {:?} in {:?}, session now {} process(es)", + waited.as_ref().map(|_| "Ok").map_err(|err| err.to_string()), + started.elapsed(), + count(&listing(&e)) + ); + } + let _ = e.end_session(); +} + +/// Arm B: the question the fix turns on — where does a bad image fail? +fn where_a_bad_image_fails() { + println!("\n=== B. launch_process_begin on an image that does not exist ==="); + let e = DebugEngine::new(); + let started = Instant::now(); + let begun = e.launch_process_begin("no_such_program_xyzzy.exe"); + println!( + " launch_process_begin -> {:?} in {:?}", + begun + .as_ref() + .map(|_| "Ok(guard)") + .map_err(|e| e.to_string()), + started.elapsed() + ); + match begun { + Err(_) => println!( + " => fails AT BEGIN, so a guard for a launch that never starts is not constructible \ + and nothing has to retire one." + ), + Ok(guard) => { + let waited = guard.wait(); + println!( + " guard.wait() -> {:?}", + waited.as_ref().map(|()| "Ok").map_err(|e| e.to_string()) + ); + println!(" => fails IN THE WAIT, so a kept entry needs something to retire it."); + } + } + let _ = e.end_session(); +} + +/// Arm C: the contract. Abandon a launch, launch a **different program**, and ask whether that +/// second guard's own process is in the session when its `wait()` returns. +fn does_the_next_launch_get_its_own_process(round: usize) -> bool { + let e = DebugEngine::new(); + let first = e.launch_process_begin("cmd.exe /c ping -n 30 127.0.0.1"); + drop(first); // abandoned before anything pumps: the create is still queued. + + let Ok(second) = e.launch_process_begin("ping.exe -n 30 127.0.0.2") else { + println!(" round {round}: the second launch would not begin"); + return true; + }; + let waited = second.wait(); + let text = listing(&e); + let mine = text.contains("ping.exe"); + println!( + " round {round}: wait() -> {:?}, {} process(es), own program present: {mine}{}", + waited.as_ref().map(|()| "Ok").map_err(|e| e.to_string()), + count(&text), + if mine { "" } else { " <-- SHORT" } + ); + let _ = e.end_session(); + mine +} + +fn main() { + where_a_bad_image_fails(); + how_many_creates_one_pump_realises(); + println!("\n=== C. abandon a launch, then launch a different program ==="); + println!(" (SHORT = wait() returned before this guard's own process was there)"); + let short = (1..=10) + .filter(|round| !does_the_next_launch_get_its_own_process(*round)) + .count(); + println!(" short in 10: {short}"); +} diff --git a/src/dbgeng.rs b/src/dbgeng.rs index 3aac69e..d43150b 100644 --- a/src/dbgeng.rs +++ b/src/dbgeng.rs @@ -541,6 +541,11 @@ struct Pending { /// Processes another open was given before it finished, which this one must not be given now /// that the record of that claim has gone with its guard. See [`Arrivals::forget`]. inherited: Vec<(u32, u32)>, + /// Its guard is gone and it was never delivered anything, but its `CreateProcessWide` is still + /// queued — so the entry stays to claim the process that create still produces. Nothing holds + /// its id any more; it exists only to keep that process from the *next* launch, and it goes + /// with the session ([`Arrivals::forget_all`]). See [`Arrivals::forget`], dbgscope#141. + abandoned: bool, } impl Pending { @@ -611,6 +616,7 @@ impl Arrivals { what, claim: Claim::Waiting, inherited: Vec::new(), + abandoned: false, }); id } @@ -647,13 +653,20 @@ impl Arrivals { /// A [`Claim::Departed`] one is handed to nobody: [`Self::forget_departed`] has already taken /// that process out of every inheritance, and putting it back is the stale claim that method /// exists to remove. - fn forget(&mut self, id: ArrivalId) { - let claimed = self - .pending - .iter() - .find(|pending| pending.id == id) - .and_then(|pending| pending.claim.held()); - self.pending.retain(|pending| pending.id != id); + /// Forgets an open **outright**, whatever state it is in — for an open whose wait has ended in + /// a way that says its process is not coming. + /// + /// [`Self::forget`] is the ordinary path and keeps an abandoned launch's place in the queue, + /// which is right when the guard was simply dropped: the create is still queued and its process + /// is still on its way. It is wrong when the open pumped its whole bound and nothing came, + /// because the entry then survives to take the *next* launch's stop — the failure keeping it + /// exists to prevent, from the other side. Raised in review on dbgscope#143. + fn discard(&mut self, id: ArrivalId) { + let Some(index) = self.pending.iter().position(|pending| pending.id == id) else { + return; + }; + let claimed = self.pending[index].claim.held(); + self.pending.remove(index); if let Some(entry) = claimed { for pending in &mut self.pending { pending.inherited.push(entry); @@ -661,6 +674,36 @@ impl Arrivals { } } + fn forget(&mut self, id: ArrivalId) { + let Some(index) = self.pending.iter().position(|pending| pending.id == id) else { + return; + }; + // **A launch abandoned before it was delivered anything keeps its place in the queue** + // (dbgscope#141). Dropping the guard does not un-queue the `CreateProcessWide` behind it, + // so that process still arrives — and it is new to the *next* launch's snapshot, because + // it did not exist when that snapshot was taken, so without this the next launch claims it + // and its `wait()` returns for a target it never asked for. Measured before it was fixed: + // a launch of `ping.exe` behind an abandoned `cmd.exe` was delivered the `cmd.exe`. + // + // The entry is kept rather than an exclusion left behind, because there is nothing to + // exclude yet — which process the create will produce is exactly what nobody knows. Being + // first in registration order is what makes it right: `deliver` offers in that order, and + // the engine realises queued creates in it. + // + // **Only a launch, and only one that has been given nothing.** An abandoned *attach* is + // already covered by the engine's `attached_processes`, which is what `Pending::wants` + // reads it for; and an entry that has a claim is removed and its claim inherited, as + // before. `Launched(None)` is removed too: it can never claim anything, so keeping it + // would leave an entry that does nothing at all. + if self.pending[index].claim == Claim::Waiting + && matches!(self.pending[index].what, Arrival::Launched(Some(_))) + { + self.pending[index].abandoned = true; + return; + } + self.discard(id); + } + /// Drops every claim and inherited exclusion naming a process the session no longer holds. /// /// **The reuse hazard the record this replaced was pruned for, arriving from the other side.** @@ -3138,8 +3181,17 @@ impl DebugEngine { // by a later process that inherits the pid. Not on the interrupted branch // above -- an open the host cut short says nothing about whether the // attach is still coming. + // + // The registration goes the same way and for the same reason, or a + // *launch* that timed out here would be kept as abandoned when its guard + // drops (dbgscope#141) and would then take the next launch's stop -- the + // very failure keeping it exists to prevent, arrived at from the other + // side. `discard` and not `forget`: this open pumped its whole bound and + // nothing came, which is the one ending that says nothing is coming. + // Raised in review on dbgscope#143. if waited { self.retire_deferred_attachment(registered); + self.discard_registration(registered); } Err(DbgEngError::LiveTargetTimeout) } @@ -3286,6 +3338,29 @@ impl DebugEngine { } } + /// Forgets an open outright, for a wait that ended saying its process is not coming. + /// + /// The guard's own drop calls [`Arrivals::forget`] shortly afterwards and finds nothing, which + /// is the point: `forget` would keep an abandoned launch's place in the queue, and an open that + /// pumped its whole bound for nothing must not hold one. See [`Arrivals::discard`]. + /// + /// **This call site is not pinned by a test, and says so rather than being assumed covered.** + /// The rule it reaches is (`..._keeps_its_place_and_an_attach_does_not`'s last block, which + /// fails if `discard` keeps the entry), but the *wiring* needs a launch whose process never + /// arrives inside `LIVE_WAIT_MS`, and a create that reaches the queue produces a process — + /// every way a launch fails with nothing created lands on `launch_process_begin`'s own `?` + /// (measured: a missing image is `0x80070002` in 1.5 ms, before any guard exists). Backing this + /// line out leaves the whole suite green; that was checked rather than supposed. The attach + /// side has `test_the_openers_prune_clears_a_claim_on_a_departed_process` for the equivalent + /// wiring because a *pid* that never joins is constructible where a launch is not. + fn discard_registration(&self, registered: &Registered<'_>) { + self.state + .arrivals + .lock() + .unwrap_or_else(|e| e.into_inner()) + .discard(registered.id); + } + /// A copy of the pids this engine attached to, taken rather than borrowed so that no lock is /// held across the arrival register's. fn attached_pids(&self) -> HashSet { @@ -6539,6 +6614,69 @@ mod tests { ); } + /// **An abandoned launch keeps its place in the queue; an abandoned attach does not.** + /// + /// dbgscope#141. Dropping a launch guard does not un-queue its `CreateProcessWide`, so that + /// process still arrives — new to the next launch's snapshot, because it did not exist when + /// that snapshot was taken. Removing the entry therefore hands it to the next launch. + /// + /// The attach half is the other rule and is asserted here rather than assumed, because it is + /// the one that says why this is not "keep every abandoned entry": an attach names a pid, and + /// the engine's own `attached_processes` already covers one whose guard has gone — which is + /// what [`Pending::wants`] reads that set for. Keeping the entry as well would leave + /// [`Arrivals::presence`] answering `Listed` for an id nobody holds. + #[test] + fn test_an_abandoned_launch_keeps_its_place_and_an_attach_does_not() { + let none = HashSet::new(); + let mut arrivals = Arrivals::default(); + + let abandoned = arrivals.register(Arrival::Launched(Some(Vec::new()))); + arrivals.forget(abandoned); + let mine = arrivals.register(Arrival::Launched(Some(Vec::new()))); + + // The abandoned launch's own process, arriving after its guard has gone. + arrivals.deliver((0, 100), &none); + assert_ne!( + arrivals.presence(mine, &[(0, 100)], &none), + Presence::Arrived, + "the next launch was given the abandoned launch's process, so its wait() returns for a \ + target it never asked for" + ); + + // And its own still reaches it, or the entry is refusing arrivals wholesale. + arrivals.deliver((1, 200), &none); + assert_eq!( + arrivals.presence(mine, &[(0, 100), (1, 200)], &none), + Presence::Arrived, + "the next launch was refused a process nobody else was entitled to" + ); + + // An abandoned attach is removed: `attached_processes` covers it, and a kept entry would + // answer `Listed` for an id nobody holds. + let attach = arrivals.register(Arrival::Attached(300)); + arrivals.forget(attach); + assert_eq!( + arrivals.presence(attach, &[(2, 300)], &none), + Presence::Absent, + "an abandoned attach was kept in the register, where the engine's own attachment record \ + is what covers it" + ); + + // And a launch whose wait ended saying nothing is coming is discarded rather than kept, + // or it survives to take the *next* launch's stop — the failure keeping an abandoned entry + // exists to prevent, reached from the other side. Raised in review on dbgscope#143. + let timed_out = arrivals.register(Arrival::Launched(Some(Vec::new()))); + arrivals.discard(timed_out); + let after = arrivals.register(Arrival::Launched(Some(Vec::new()))); + arrivals.deliver((3, 400), &none); + assert_eq!( + arrivals.presence(after, &[(3, 400)], &none), + Presence::Arrived, + "a launch whose wait timed out kept its place in the queue and took the next launch's \ + process" + ); + } + /// **A claim is inherited by every open still waiting, not handed along one at a time.** /// /// Put to *three* launches, because with two "every remaining open" and "the next one" are the @@ -7581,6 +7719,121 @@ mod tests { e.end_session().expect("end_session failed"); } + /// **A launch abandoned before anything pumps does not hand its process to the next launch** + /// (dbgscope#141). + /// + /// Dropping a guard takes a `Waiting` entry out of the register and leaves no exclusion behind + /// — there is nothing to leave, since nothing was ever delivered to it. The queued + /// `CreateProcessWide` still produces a process, and it is new to the *next* launch's snapshot + /// because it did not exist when that snapshot was taken. + /// + /// **This has to read which process was delivered, not what the session holds.** Two attempts + /// through the public surface both passed while the defect was live, which is why the assertion + /// is where it is: counting processes when `wait()` returns says nothing, because **one pump + /// realises both queued creates** (measured: session 0 → 2 on a single `WaitForEvent`); and + /// asking whether the second program is *listed* says nothing either, because membership is the + /// weaker claim this whole module is built on not confusing with having stopped. + /// + /// So it reads the register rather than the session, and asserts the property the fix actually + /// delivers: **the abandoned entry absorbs an arrival of its own**, distinct from the next + /// launch's. Before the fix nothing accounted for the abandoned create, so the next launch was + /// satisfied by the first stop to arrive and no one was waiting for the other process at all. + /// + /// **It deliberately does not assert that the next launch got its *own* process, because that + /// is not true and cannot be made true by this change.** `deliver` offers in registration + /// order and the abandoned entry is first, so it takes whichever process arrives first — and + /// one `WaitForEvent` realises *both* queued creates (measured: session 0 → 2 on a single + /// pump), so which one the event names is a coin flip. Asserting the image name here failed + /// about half of all runs with the fix in place, which is how this was found. Identifying a + /// launch by arrival order is the residual ambiguity [`Arrival`] has always documented for two + /// launches pending at once; dbgscope#139 closed "both guards get the same arrival", not "each + /// guard gets its own", and closing the second needs something to match on rather than an + /// order. See dbgscope#141. + /// + /// **Several rounds, because one is racy in the direction that matters.** Measured with the fix + /// backed out, a single round catches the defect 9 times in 10 and passes the other time — a + /// guard that goes green on broken code one run in ten is the wrong way round. + #[test] + #[cfg(not(miri))] + fn test_an_abandoned_launch_does_not_hand_its_process_to_the_next_one() { + let _debuggee = one_debuggee(); + for round in 1..=5 { + an_abandoned_launch_round(round); + } + } + + /// One round of [`test_an_abandoned_launch_does_not_hand_its_process_to_the_next_one`], in its + /// own session so a round cannot inherit the previous one's processes. + #[cfg(not(miri))] + fn an_abandoned_launch_round(round: usize) { + let e = DebugEngine::new(); + + // Abandoned before anything pumps, so its create is still queued. + let abandoned = e + .launch_process_begin("cmd.exe /c ping -n 30 127.0.0.1") + .expect("the first launch would not begin"); + drop(abandoned); + + let mine = e + .launch_process_begin("ping.exe -n 30 127.0.0.2") + .expect("the second launch would not begin"); + let WaitKind::Live(registered) = &mine.kind else { + panic!("a launch guard is not a live open"); + }; + let id = registered.id; + + // **Pumped directly rather than through `wait()`**, which consumes the guard and takes the + // registration with it — the claim has to be read while the open is still alive, and the + // first draft of this test read it afterwards and found nothing at all. Bounded rather than + // once, for the reason `test_the_openers_prune_clears_a_claim_on_a_departed_process` gives: + // one `WaitForEvent` is one *event*, however many creates it happens to realise. + let claim_of = |id: ArrivalId| { + e.state + .arrivals + .lock() + .unwrap_or_else(|err| err.into_inner()) + .pending + .iter() + .find(|pending| pending.id == id) + .and_then(|pending| pending.claim.held()) + }; + let mut claimed = None; + for _ in 0..5 { + claimed = claim_of(id); + if claimed.is_some() { + break; + } + e.wait_for_event(LIVE_WAIT_MS).expect("a pump failed"); + } + let mine = claimed.expect("nothing was delivered to the second launch in five pumps"); + + // The abandoned entry absorbed an arrival of its own, and a different one. **That** is + // what the fix delivers: before it, no entry accounted for the abandoned create, so the + // second launch was satisfied by the first stop to arrive and nothing was waiting for the + // other process at all. + let absorbed = e + .state + .arrivals + .lock() + .unwrap_or_else(|err| err.into_inner()) + .pending + .iter() + .find(|pending| pending.abandoned) + .and_then(|pending| pending.claim.held()); + let absorbed = absorbed.unwrap_or_else(|| { + panic!( + "round {round}: the abandoned launch's entry was given nothing, so its process was \ + left for whoever asked next" + ) + }); + assert_ne!( + absorbed, mine, + "round {round}: the abandoned launch and the one after it were given the same process" + ); + + let _ = e.end_session(); + } + /// Reads a debugger pseudo-register (`$t0`, …) as a number, via `? ` — whose output /// is `Evaluate expression: = `. `None` when no value came back. ///