From 104a95a6d78644d3e4832187d4e09fa9a34ff365 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Mon, 27 Jul 2026 02:18:52 -0400 Subject: [PATCH] repair: let the token beat the namespace guesswork, and ask liveness first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5. Two blocking P2s, both in the execution shell rather than the token design, plus an overstated claim of mine. **The degradation signals were defeating the token in exactly the case it exists for.** A host crashing inside a container leaves a token that matches this machine, boot and pid namespace perfectly — but the probe saw a container marker or a multi-entry `NSpid` and answered `Unknown` for everything, so token-qualified repair did nothing precisely where it had just become safe. Those signals are guesswork about whether a bare pid is meaningful, and for an attributed pid that is no longer a guess. Liveness is now asked with the module's attribution in hand: a tokened pid goes straight to `kill`, while an untagged one still has to get past the signals, because there they are the only protection left. **Liveness now runs before the fresh snapshot, not after it.** The natural order — verify the module, check liveness, unload — leaves the dangerous window open: after `kill` returns ESRCH this process can be descheduled while the planned module vanishes, a new host inherits both the pid and the module index, and its differently-nonced arguments occupy that index. Nothing re-read those arguments, so the reused index would have been unloaded. Asking liveness first means the post-liveness fingerprint check catches that replacement, leaving only the irreducible snapshot-to-unload interval. **The nonce claim was overstated and is now true.** A token was minted once per `Routing` session and reused for every subsequent reload, making it a host-session nonce rather than a per-load one. It is now minted inside `load_module`, mixing a bumped counter with the clock, so two loads by the same pid really do render different arguments — which, combined with the reordering above, is what lets a fingerprint tell a module from its replacement at the same index. ⚠️ **The first version of the attribution fix had no gate, and the mutation said so.** Swapping `of_attributed` back to `of` passed all 254 tests, because on an ordinary desktop the probe is not degraded and the two paths agree, while the planner tests use a fake liveness closure that never touches the probe at all. The new test constructs a *deliberately degraded* probe, which is the only state where the distinction is observable. Both mutants — routing a tokened pid through `of`, and re-applying the degradation gate inside `of_attributed` — now fail it. Codex's answers to the questions I raised, recorded because they close them: omitting the pid from the token loses nothing, since the canonical argument already binds the token to exactly one pid; namespace inode reuse is real but only after the old namespace is destroyed, so its host is necessarily gone and no live owner is endangered; and refusing foreign tokens even under `--repair-legacy-untagged` is the right line, because the flag speaks to missing evidence rather than wrong evidence. No finding against the two-hole template derivation. 255 tests, clippy clean, fmt clean. All four live field gates re-run green: tokened cleaned, foreign refused with and without the flag, untagged refused then cleaned on request, A/B orphan removal byte-identical elsewhere, reference gate still firing. Co-Authored-By: Claude Opus 5 --- src/host/audio.rs | 51 ++++++++-------- src/repair/mod.rs | 144 ++++++++++++++++++++++++++++++++++++--------- src/repair/plan.rs | 76 ++++++++++++++++++++---- 3 files changed, 208 insertions(+), 63 deletions(-) diff --git a/src/host/audio.rs b/src/host/audio.rs index c0bf2e3..a2d3030 100644 --- a/src/host/audio.rs +++ b/src/host/audio.rs @@ -67,13 +67,12 @@ impl Routing { let pid = std::process::id(); let sink_name = repair_plan::sink_name_for(pid); - // Every module this host loads carries an ownership token, so `--repair` - // can tell whose pid the name refers to instead of assuming the number - // means the same thing everywhere. Without it a repair run in another pid - // namespace can unload a live host's audio; see `repair::plan::OwnerToken`. - let owner = owner_token(pid).context("could not build an audio ownership token")?; - - let sink_module = load_module(Shape::LegacyCaptureSink, pid, &owner) + // Every module this host loads carries an ownership token, minted per + // load, so `--repair` can tell whose pid the name refers to instead of + // assuming the number means the same thing everywhere. Without it a repair + // run in another pid namespace can unload a live host's audio; see + // `repair::plan::OwnerToken`. + let sink_module = load_module(Shape::LegacyCaptureSink, pid) .context("failed to load module-null-sink")?; // In strict per-app mode we never mirror the default sink: the viewer @@ -89,7 +88,7 @@ impl Routing { None } else { Some( - load_module(Shape::LoopbackIntoCapture, pid, &owner) + load_module(Shape::LoopbackIntoCapture, pid) .context("failed to load module-loopback (null-sink cleaned up on Drop)")?, ) }; @@ -117,7 +116,6 @@ impl Routing { let (router, mut event_rx) = StreamRouter::spawn(app.clone(), sink_name.clone())?; let loopback_for_task = Arc::clone(&loopback_arc); let local_monitor_for_task = Arc::clone(&local_monitor_arc); - let owner_for_task = owner.clone(); let strict = opts.strict_audio; let event_task = tokio::spawn(async move { use crate::common::output::{self, AppAudioState}; @@ -139,8 +137,7 @@ impl Routing { // only, never the desktop/call — so it can't echo into // the capture. if local_monitor_for_task.lock().unwrap().is_none() { - match load_module(Shape::LoopbackOutOfCapture, pid, &owner_for_task) - { + match load_module(Shape::LoopbackOutOfCapture, pid) { Ok(id) => { tracing::info!( module = id, @@ -193,7 +190,7 @@ impl Routing { tracing::info!( "audio routing: last routed stream gone → restoring default-sink loopback" ); - match load_module(Shape::LoopbackIntoCapture, pid, &owner_for_task) { + match load_module(Shape::LoopbackIntoCapture, pid) { Ok(id) => { *loopback_for_task.lock().unwrap() = Some(id); } @@ -342,24 +339,31 @@ struct SinkInputProperties { // pactl module helpers // ────────────────────────────────────────────────────────────────────── -/// Mint this host's ownership token. +/// Mint an ownership token for one module load. /// -/// The nonce is what makes two loads by the same pid distinguishable, which is why -/// it is per-call rather than per-process: it narrows the window where a module that -/// vanished and a replacement that inherited its index look byte-identical. +/// **Per load, not per session.** The nonce is what makes two loads by the same pid +/// render different arguments, which is what lets a fingerprint tell a module from +/// its replacement at the same index. A token minted once and reused for every +/// reload would be a host-session nonce and would not do that, so the counter is +/// bumped on every call and mixed with the clock. fn owner_token(pid: u32) -> Result { + use std::sync::atomic::{AtomicU64, Ordering}; + static LOADS: AtomicU64 = AtomicU64::new(0); + let local = crate::repair::local_identity()?; - // A nonce only has to be unlikely to repeat, not unguessable. - let nonce = std::time::SystemTime::now() + // A nonce only has to be unlikely to repeat, not unguessable. The counter makes + // two loads within the same clock tick distinct; the clock keeps two runs of the + // same process distinct. + let counter = LOADS.fetch_add(1, Ordering::Relaxed); + let nanos = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_nanos() as u64) - .unwrap_or(0) - ^ u64::from(pid) << 32; + .unwrap_or(0); Ok(repair_plan::OwnerToken { machine: local.machine, boot: local.boot, pid_ns: local.pid_ns, - nonce, + nonce: nanos ^ (counter << 48) ^ (u64::from(pid) << 32), }) } @@ -370,11 +374,12 @@ fn owner_token(pid: u32) -> Result { /// `--repair`'s exact-form matcher and this loader are one source of truth. A /// latency or argument change that moved only one of them would leave repair /// silently unable to recognise the modules this build loads. -fn load_module(shape: Shape, pid: u32, owner: &repair_plan::OwnerToken) -> Result { +fn load_module(shape: Shape, pid: u32) -> Result { + let owner = owner_token(pid).context("could not build an audio ownership token")?; let output = Command::new("pactl") .arg("load-module") .arg(shape.module_name()) - .args(shape.render_args(pid, Some(owner))) + .args(shape.render_args(pid, Some(&owner))) .output() .context("failed to run pactl load-module")?; if !output.status.success() { diff --git a/src/repair/mod.rs b/src/repair/mod.rs index 45d1cf2..7e3ff5a 100644 --- a/src/repair/mod.rs +++ b/src/repair/mod.rs @@ -82,7 +82,9 @@ pub async fn run(clean_untagged: bool) -> Result<()> { } } - let planned = plan::plan(&modules, &policy, |pid| liveness.of(pid)); + let planned = plan::plan(&modules, &policy, |pid, attribution| { + liveness_for(&liveness, attribution, pid) + }); // Ours by shape, but carrying no proof of whose pid they name. Never unloaded by // default — listed, so an explicit legacy run has something to look at first. @@ -149,9 +151,43 @@ pub async fn run(clean_untagged: bool) -> Result<()> { let mut failed = 0u32; for fp in &planned.unload { - // Fresh snapshot per action. Deliberately not hoisted out of the loop: - // each unload changes the module table, and the point is to decide - // against the table as it is *now*, not as it was when the plan was made. + // ORDER MATTERS, and it is the opposite of what reads naturally. + // + // Liveness is asked FIRST, and the fresh snapshot is taken AFTER it. The + // tempting order — verify the module, then check liveness, then unload — + // leaves the dangerous window wide open: between `kill` returning ESRCH and + // the unload, this process can be descheduled long enough for the planned + // module to vanish, a new host to inherit both the pid and the module index, + // and its differently-nonced arguments to occupy that index. Nothing would + // re-read those arguments, so the reused index gets unloaded. + // + // Asking liveness first and re-verifying the fingerprint after it means a + // replacement arriving in that window is caught by the argument comparison, + // and only the irreducible snapshot-to-unload interval remains. + match attributed_liveness(&liveness, fp) { + Liveness::Dead => {} + Liveness::Alive => { + println!( + "[pixelpass] --repair: pid {} is alive again; leaving module #{} alone", + fp.pid, fp.id + ); + skipped += 1; + continue; + } + Liveness::Unknown => { + eprintln!( + "[pixelpass] --repair: pid {}'s liveness became undeterminable; \ + leaving module #{} alone", + fp.pid, fp.id + ); + skipped += 1; + continue; + } + } + + // Fresh snapshot per action, taken after the liveness answer. Deliberately + // not hoisted out of the loop: each unload changes the module table, and the + // point is to decide against the table as it is *now*. let current = pulse .list_modules() .context("could not re-list Pulse modules")?; @@ -189,29 +225,6 @@ pub async fn run(clean_untagged: bool) -> Result<()> { skipped += 1; continue; } - // Liveness last, and closest to the unload: a host that came back — or - // a stranger that inherited the pid — outranks any amount of evidence - // that this module looked orphaned. Undecidable counts as back. - match liveness.of(fp.pid) { - Liveness::Dead => {} - Liveness::Alive => { - println!( - "[pixelpass] --repair: pid {} is alive again; leaving module #{} alone", - fp.pid, fp.id - ); - skipped += 1; - continue; - } - Liveness::Unknown => { - eprintln!( - "[pixelpass] --repair: pid {}'s liveness became undeterminable; \ - leaving module #{} alone", - fp.pid, fp.id - ); - skipped += 1; - continue; - } - } match pulse.unload_module(fp.id) { Ok(()) => { @@ -259,6 +272,34 @@ fn describe(fp: &Fingerprint) -> String { ) } +/// Ask liveness with the module's *attribution* in hand. +/// +/// A module whose token matches this machine, boot and pid namespace has already +/// proven that its pid is a number meaningful here — that is the token's entire +/// job. Running such a module through the probe's degradation checks would defeat +/// it in exactly the situation it exists for: a host crashing inside a container +/// leaves a token that matches perfectly, while a container marker or a multi-entry +/// `NSpid` makes the probe answer `Unknown` for everything, so token-qualified +/// repair would do nothing precisely where it is now safe. +/// +/// The degradation signals therefore guard only the *untagged* path, where a bare +/// pid is all there is and those signals are the only protection left. +fn liveness_for(probe: &LivenessProbe, attribution: plan::Attribution, pid: u32) -> Liveness { + match attribution { + plan::Attribution::Tokened => probe.of_attributed(pid), + plan::Attribution::Untagged => probe.of(pid), + } +} + +/// The same rule, for a fingerprint at execution time. +fn attributed_liveness(probe: &LivenessProbe, fp: &Fingerprint) -> Liveness { + let attribution = match fp.owner { + Some(_) => plan::Attribution::Tokened, + None => plan::Attribution::Untagged, + }; + liveness_for(probe, attribution, fp.pid) +} + // ────────────────────────────────────────────────────────────────────── // Identity // ────────────────────────────────────────────────────────────────────── @@ -400,10 +441,21 @@ impl LivenessProbe { self.degraded.as_deref() } + /// Liveness for a pid this process has **no** independent reason to trust — + /// an untagged module. Here the degradation signals are the only protection. fn of(&self, pid: u32) -> Liveness { if self.degraded.is_some() { return Liveness::Unknown; } + self.of_attributed(pid) + } + + /// Liveness for a pid already proven to belong to this machine, boot and pid + /// namespace by an [`plan::OwnerToken`]. + /// + /// The degradation checks are deliberately skipped: they exist to guess at + /// whether a bare pid is meaningful, and here that is not a guess any more. + fn of_attributed(&self, pid: u32) -> Liveness { // `kill(0, …)` signals our whole process group and a negative pid signals // another group, so neither may ever reach `kill`. Neither is a pid we // could have written into a sink name anyway. @@ -455,4 +507,42 @@ mod tests { assert_eq!(probe.of(0), Liveness::Unknown); assert_eq!(probe.of(u32::MAX), Liveness::Unknown); } + + /// A token proves the pid is meaningful here, so the probe's namespace + /// guesswork must not veto it. Without this, a host crashing inside a + /// container leaves a perfectly matching token while a container marker makes + /// every answer `Unknown` — and token-qualified repair does nothing in exactly + /// the situation the token was built for. + /// + /// This runs against a *degraded* probe deliberately: on an ordinary desktop + /// the two paths agree, so a test using the real probe's state would pass + /// whether or not the distinction exists. + #[test] + fn a_token_beats_the_degradation_signals_but_a_bare_pid_does_not() { + let degraded = LivenessProbe { + degraded: Some("pretending to be in a container".to_string()), + }; + let me = std::process::id(); + + assert_eq!( + degraded.of_attributed(me), + Liveness::Alive, + "an attributed pid must still be answered when the probe is degraded" + ); + assert_eq!( + degraded.of(me), + Liveness::Unknown, + "a bare pid must not be, since the signals are all it has" + ); + + // And the routing between them, which is what the caller actually uses. + assert_eq!( + liveness_for(°raded, plan::Attribution::Tokened, me), + Liveness::Alive + ); + assert_eq!( + liveness_for(°raded, plan::Attribution::Untagged, me), + Liveness::Unknown + ); + } } diff --git a/src/repair/plan.rs b/src/repair/plan.rs index 5a72ad9..03feb50 100644 --- a/src/repair/plan.rs +++ b/src/repair/plan.rs @@ -279,6 +279,20 @@ impl LocalIdentity { } } +/// Whether a module proved which pid space its pid belongs to. +/// +/// Passed to the liveness callback because the answer changes *how* the question +/// may be asked: an attributed pid needs no guessing about namespaces, while an +/// untagged one is a bare number whose meaning has to be guarded some other way. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Attribution { + /// Carries a token matching this machine, boot and pid namespace. + Tokened, + /// Carries no token; judged by pid alone, and only under + /// [`UntaggedPolicy::CleanByPidAlone`]. + Untagged, +} + /// What repair may do about modules that carry no token at all. /// /// Every module loaded before tokens existed is untagged, and there is no way to @@ -625,7 +639,7 @@ pub fn sink_still_referenced( pub fn plan( observations: &[ModuleObservation], policy: &Policy, - liveness: impl Fn(u32) -> Liveness, + liveness: impl Fn(u32, Attribution) -> Liveness, ) -> Plan { // Deduplicate by module id: a snapshot should not repeat one, but a repeated // entry must not become a repeated unload of an index that has since been @@ -642,13 +656,15 @@ pub fn plan( // one cannot see, and an absent token names nothing at all. let mut untagged: Vec = Vec::new(); let mut foreign: Vec = Vec::new(); - let mut judgeable: Vec = Vec::new(); + let mut judgeable: Vec<(Fingerprint, Attribution)> = Vec::new(); for fp in seen.into_values() { match &fp.owner { - Some(token) if policy.local.can_judge(token) => judgeable.push(fp), + Some(token) if policy.local.can_judge(token) => { + judgeable.push((fp, Attribution::Tokened)) + } Some(_) => foreign.push(fp), None => match policy.untagged { - UntaggedPolicy::CleanByPidAlone => judgeable.push(fp), + UntaggedPolicy::CleanByPidAlone => judgeable.push((fp, Attribution::Untagged)), UntaggedPolicy::Refuse => untagged.push(fp), }, } @@ -659,8 +675,12 @@ pub fn plan( let mut live_pids = BTreeSet::new(); let mut dead_pids = BTreeSet::new(); let mut unknown_pids = BTreeSet::new(); - for pid in judgeable.iter().map(|fp| fp.pid).collect::>() { - match liveness(pid) { + for (pid, attribution) in judgeable + .iter() + .map(|(fp, attribution)| (fp.pid, *attribution)) + .collect::>() + { + match liveness(pid, attribution) { Liveness::Alive => live_pids.insert(pid), Liveness::Dead => dead_pids.insert(pid), Liveness::Unknown => unknown_pids.insert(pid), @@ -669,6 +689,7 @@ pub fn plan( let mut unload: Vec = judgeable .into_iter() + .map(|(fp, _)| fp) .filter(|fp| dead_pids.contains(&fp.pid)) .collect(); // `Shape`'s declaration order is the unload order: loopbacks before the sink @@ -744,7 +765,7 @@ mod tests { obs(id, Shape::LoopbackIntoCapture, pid, None) } - fn nothing_is_alive(_: u32) -> Liveness { + fn nothing_is_alive(_: u32, _: Attribution) -> Liveness { Liveness::Dead } @@ -885,7 +906,7 @@ mod tests { null_sink(6, 200), mirror(12, 200), ]; - let plan = plan(&modules, &our_policy(), |pid| { + let plan = plan(&modules, &our_policy(), |pid, _| { if pid == 200 { Liveness::Alive } else { @@ -903,7 +924,7 @@ mod tests { #[test] fn an_undecidable_pid_is_never_touched() { let modules = [null_sink(5, 100), mirror(10, 100), local_monitor(11, 200)]; - let plan = plan(&modules, &our_policy(), |pid| { + let plan = plan(&modules, &our_policy(), |pid, _| { if pid == 100 { Liveness::Unknown } else { @@ -934,7 +955,7 @@ mod tests { null_sink(6, 200), mirror(12, 200), ]; - let plan = plan(&modules, &our_policy(), |_| Liveness::Alive); + let plan = plan(&modules, &our_policy(), |_, _| Liveness::Alive); assert!(plan.is_empty(), "no live host may be touched: {plan:?}"); assert_eq!(plan.live_pids, BTreeSet::from([100, 200])); assert!(plan.dead_pids.is_empty()); @@ -1049,7 +1070,7 @@ mod tests { mirror(12, 99), local_monitor(13, 99), ]; - let plan = plan(&modules, &our_policy(), |pid| { + let plan = plan(&modules, &our_policy(), |pid, _| { *calls.borrow_mut().entry(pid).or_insert(0) += 1; Liveness::Dead }); @@ -1240,7 +1261,7 @@ mod tests { legacy_mirror(11, 5555), ]; let asked = std::cell::RefCell::new(Vec::new()); - let plan = plan(&modules, &our_policy(), |pid| { + let plan = plan(&modules, &our_policy(), |pid, _| { asked.borrow_mut().push(pid); Liveness::Dead }); @@ -1288,7 +1309,7 @@ mod tests { local: our_identity(), untagged: UntaggedPolicy::CleanByPidAlone, }, - |_| Liveness::Alive, + |_, _| Liveness::Alive, ); assert!(live.is_empty(), "{live:?}"); } @@ -1309,6 +1330,35 @@ mod tests { ); } + /// The attribution handed to the liveness callback decides *how* the question + /// may be asked, so it must be right per module. Getting this wrong made the + /// token useless in a container — the one place it exists for — because the + /// probe's namespace guesswork answered `Unknown` for a pid the token had + /// already proven local. + #[test] + fn attribution_is_reported_per_module() { + let modules = [mirror(10, 100), legacy_mirror(11, 200)]; + let asked = std::cell::RefCell::new(Vec::new()); + let plan = plan( + &modules, + &Policy { + local: our_identity(), + untagged: UntaggedPolicy::CleanByPidAlone, + }, + |pid, attribution| { + asked.borrow_mut().push((pid, attribution)); + Liveness::Dead + }, + ); + let asked = asked.into_inner(); + assert_eq!( + asked, + vec![(100, Attribution::Tokened), (200, Attribution::Untagged)], + "each pid must be asked about with its own module's attribution" + ); + assert_eq!(ids(&plan), vec![10, 11]); + } + /// Tokened and legacy forms must both classify, and carry the difference. #[test] fn both_forms_classify_and_record_whether_they_are_attributable() {