From 91c4dedcb06d53d115208c39c21b536b4988bfae Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 02:32:07 -0400 Subject: [PATCH] host/observer: derive pipewire-pulse's PID from comm, not from repetition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MEASURED DEFECT, found by the §5.1 row-1 matrix run: the daemon PID was unresolvable on this host, permanently, which switched key 4's suppression off and fused every Pulse-emulated node into a single owner. Row 1's clean control forwarder was excluded, Firefox was excluded across an application.process.id bridge, and the taint reached the sunshine sinks and both sound cards -- the same machine-wide over-exclusion cascade as phase 5's F2, from a new cause. Stage 1 returned the one sec_pid shared by 2+ Clients, reasoning that only the Pulse shim repeats a value. WirePlumber repeats one too: it holds Clients 'WirePlumber' and 'WirePlumber [export]', both sec_pid 1747. Two values repeated, the rule called that ambiguous and returned None, and owner::keys_of's documented fail-closed asymmetry did the rest. The rule was wrong in both directions, so the prefilter is gone rather than patched: a second process holding two Clients defeats it (permanent, not a corner case), and a session where pipewire-pulse holds exactly one Client never repeats anything so the candidate is missed. comm was always the authoritative check; repetition stood in front of it and was a guess about other processes' Client counts. Now: candidates() lists every distinct sec_pid, resolve() picks the unique one whose /proc comm is exactly pipewire-pulse, and several matches still fail closed (a single Option cannot suppress two daemons -- recorded, not approximated). The adapter probes only PIDs entering the set, and retain_probed_comms bounds the map to live PIDs so a PID that leaves and returns is re-probed instead of answered from a stale comm. Row 1 now passes its exact partition, key named: tainted forwarder leg excluded on node.link-group, clean forwarder leg and Firefox eligible, taint confined to the tainted half. 225 tests green (WirePlumber-pair and single-Client regressions covered), clippy clean. --- src/host/observer/adapter.rs | 25 ++++-- src/host/observer/mod.rs | 31 +++++--- src/host/observer/pulse_pid.rs | 131 +++++++++++++++++++------------ src/host/observer/tests.rs | 137 +++++++++++++++++++++++++++++---- 4 files changed, 246 insertions(+), 78 deletions(-) diff --git a/src/host/observer/adapter.rs b/src/host/observer/adapter.rs index 85a41c7..7efcc5e 100644 --- a/src/host/observer/adapter.rs +++ b/src/host/observer/adapter.rs @@ -16,7 +16,7 @@ use crate::host::taint::{PEERSPEAK_OWNED_PROP, PEERSPEAK_OWNED_VALUE}; use anyhow::{Context, Result}; use pipewire::{self as pw, types::ObjectType}; use std::cell::{Cell, RefCell}; -use std::collections::{BTreeMap, VecDeque}; +use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::rc::Rc; use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; @@ -130,7 +130,9 @@ struct LiveGlobal { struct ObserverState { model: RegistryModel, latest: Arc>>, - last_candidate: Option, + /// The pulse-PID candidate set as of the last probe, so only PIDs entering + /// it are read from `/proc`. + last_candidates: BTreeSet, live_globals: BTreeMap>, sink: Option>, started_at: Instant, @@ -148,7 +150,7 @@ impl ObserverState { Self { model: RegistryModel::new(0, READINESS_TIMEOUT_MILLIS), latest, - last_candidate: None, + last_candidates: BTreeSet::new(), live_globals: BTreeMap::new(), sink, started_at, @@ -163,10 +165,17 @@ impl ObserverState { let event_outcome = self.model.apply(event); let mut outcome = event_outcome; - let candidate = self.model.pulse_pid_candidate(); - if candidate != self.last_candidate { - self.last_candidate = candidate; - if let Some(pid) = candidate { + // Round 10: a *set* of candidates, because repetition across Clients + // turned out not to identify pipewire-pulse (see `pulse_pid`'s module + // docs — WirePlumber repeats a PID too, which made the old single + // candidate permanently ambiguous on this host). + let candidates = self.model.pulse_pid_candidates(); + if candidates != self.last_candidates { + // Only PIDs *entering* the set are probed. A PID that left and came + // back is "entering" again and so is re-probed, which is what keeps + // the PID-reuse guard honest rather than answering from a cached + // `comm` for a number that now belongs to someone else. + for &pid in candidates.difference(&self.last_candidates) { let comm = std::fs::read_to_string(format!("/proc/{pid}/comm")) .ok() .map(|comm| comm.trim_end_matches(['\r', '\n']).to_string()); @@ -178,6 +187,8 @@ impl ObserverState { outcome = Outcome::Applied; } } + self.model.retain_probed_comms(&candidates); + self.last_candidates = candidates; } // v3.5 §6.7 decision 2: a projection the model proved identical is not diff --git a/src/host/observer/mod.rs b/src/host/observer/mod.rs index 87f5a27..82a70dd 100644 --- a/src/host/observer/mod.rs +++ b/src/host/observer/mod.rs @@ -91,7 +91,7 @@ use crate::host::taint::snapshot::{ PortSnapshot, Serial, }; use classify::{Classification, DeviceClaim, DeviceProps}; -use std::collections::{BTreeMap, VecDeque}; +use std::collections::{BTreeMap, BTreeSet, VecDeque}; /// A monotonic millisecond clock value, supplied by the adapter via /// [`RegEvent::Tick`]. Kept as a bare integer rather than @@ -399,12 +399,25 @@ impl RegistryModel { matches!(self.readiness, Readiness::Complete) && !self.obligations_outstanding() } - /// The pulse-PID candidate the adapter should be probing (`None` = no - /// repeated `sec_pid`, nothing to probe). Exposed so the adapter re-probes - /// only when the candidate changes. - pub fn pulse_pid_candidate(&self) -> Option { + /// The pulse-PID candidates the adapter should be probing — every distinct + /// `sec_pid` on the current Clients. Exposed so the adapter re-probes only + /// the PIDs *entering* the set rather than all of them on every event. + pub fn pulse_pid_candidates(&self) -> BTreeSet { let clients: Vec = self.clients.values().cloned().collect(); - pulse_pid::candidate(&clients) + pulse_pid::candidates(&clients) + } + + /// Forget probed `comm`s for PIDs no Client presents any more. + /// + /// Cannot change the projection — [`Self::pulse_pid`] only ever reads + /// `comm`s for PIDs in the current candidate set — so it is deliberately + /// not an [`Outcome`]-returning `apply` arm: it must not publish, and it + /// must not count as a graph event for O5. Its purpose is to bound the map + /// (one entry per live Client PID) in a host process that runs for hours, + /// and to guarantee a PID that leaves and returns is re-probed rather than + /// answered from a stale `comm`. + pub fn retain_probed_comms(&mut self, live: &BTreeSet) { + self.probed_comm.retain(|pid, _| live.contains(pid)); } /// Fold one observation into the model. The returned [`Outcome`] tells the @@ -701,9 +714,9 @@ impl RegistryModel { /// probed `comm`. `None` whenever anything is ambiguous or unconfirmed — /// the safe answer (key 4 unusable). fn pulse_pid(&self) -> Option { - let candidate = self.pulse_pid_candidate()?; - let comm = self.probed_comm.get(&candidate).and_then(|c| c.as_deref()); - pulse_pid::validate(candidate, comm) + pulse_pid::resolve(&self.pulse_pid_candidates(), |pid| { + self.probed_comm.get(&pid).cloned().flatten() + }) } /// Project the current state into the taint engine's inputs. diff --git a/src/host/observer/pulse_pid.rs b/src/host/observer/pulse_pid.rs index ccdf663..85d3160 100644 --- a/src/host/observer/pulse_pid.rs +++ b/src/host/observer/pulse_pid.rs @@ -11,20 +11,49 @@ //! The derivation is split into two pure stages so the I/O — reading //! `/proc//comm` — stays in the adapter: //! -//! 1. [`candidate`] finds the PID that *looks* like pulse from the graph -//! alone: the `pipewire.sec.pid` value shared across multiple Clients. -//! Native PipeWire clients carry their own distinct PID; only the -//! Pulse shim repeats one value, so a repeated value is the signal. -//! 2. [`validate`] confirms that candidate against the `comm` the adapter -//! read from `/proc`. This is what closes **PID reuse**: a recycled PID -//! that coincidentally repeats in the graph is rejected because -//! `/proc//comm` now names a different process. +//! 1. [`candidates`] lists the PIDs worth probing from the graph alone: every +//! distinct `pipewire.sec.pid` any Client presents. +//! 2. [`resolve`] picks the one whose `comm`, as read from `/proc` by the +//! adapter, is exactly pipewire-pulse's. This is also what closes **PID +//! reuse**: a recycled PID is rejected because `/proc//comm` now names +//! a different process. //! -//! Any failure at either stage — no repeated value, two repeated values, -//! the property missing, `/proc` gone, a `comm` mismatch — yields `None`. +//! Any failure — no Client carries the property, no `comm` matches, `/proc` +//! gone, or *several* PIDs claim to be pipewire-pulse — yields `None`. +//! +//! ## ⚠️ Round 10 (MEASURED): repetition is not the signal +//! +//! Stage 1 used to return a single candidate: the one `sec_pid` value shared by +//! two or more Clients, reasoning that "native PipeWire clients carry their own +//! distinct PID; only the Pulse shim repeats one value". **That is false on a +//! stock desktop, and the phase-5 §5.1 matrix caught it on row 1.** Measured on +//! this host (PipeWire 1.6.8 / WirePlumber 0.5.15): WirePlumber holds *two* +//! Clients — `WirePlumber` and `WirePlumber [export]` — both carrying +//! `sec_pid` 1747. So two values repeated (1747 and pipewire-pulse's 2528), the +//! old rule called that ambiguous and returned `None`, and the consequence was +//! not a missing optimisation but a machine-wide over-exclusion cascade: with +//! the daemon PID unknown, key 4's suppression never fires, every +//! Pulse-emulated node fuses into one owner, and the eligible half of every row +//! empties out (see `owner::keys_of`'s fail-closed asymmetry note). +//! +//! The rule failed in *both* directions, which is why the prefilter is gone +//! rather than patched: +//! +//! - **False ambiguity** — any second process holding two Clients defeats it. +//! WirePlumber always does, so this was permanent, not a corner case. +//! - **False absence** — a session where pipewire-pulse happens to hold exactly +//! one Client (one Pulse app running) never repeats a value at all, so the +//! candidate is missed and the same cascade follows. +//! +//! `comm` was always the authoritative check; repetition was a heuristic +//! standing in front of it, and it was wrong. Probing every distinct `sec_pid` +//! costs one `/proc` read per *distinct* PID (single digits — bounded by the +//! Client count, cached, and re-read only when the candidate set changes), +//! which is a cheap price for a signal that does not encode an assumption about +//! how many Clients anyone else opens. use crate::host::taint::snapshot::ClientSnapshot; -use std::collections::BTreeMap; +use std::collections::BTreeSet; /// The kernel `comm` of the pipewire-pulse process. `comm` is truncated to /// 15 bytes by the kernel; `pipewire-pulse` is 14 bytes, so it is exact — @@ -32,40 +61,19 @@ use std::collections::BTreeMap; /// recycled PID belonging to e.g. `pipewire-pulseX`. const PULSE_COMM: &str = "pipewire-pulse"; -/// Stage 1: the PID that looks like pipewire-pulse from the client graph. +/// Stage 1: every PID worth probing — the distinct `pipewire.sec.pid` values +/// the Clients present. /// -/// Returns `Some(pid)` only when **exactly one** `pipewire.sec.pid` value is -/// shared by two or more clients. Rationale, matched to the failure matrix: -/// -/// - **consistent** — one value repeats, the rest (native clients) are -/// distinct ⇒ that value. -/// - **inconsistent** — two or more values each repeat ⇒ we cannot tell which -/// is pulse ⇒ `None`. -/// - **missing property** — the Pulse clients carry no `sec_pid` ⇒ nothing -/// repeats ⇒ `None`. -/// -/// A count threshold of two is deliberate: a single client carrying a PID is -/// indistinguishable from a lone native app, and pulse always mints many. -pub fn candidate(clients: &[ClientSnapshot]) -> Option { - let mut counts: BTreeMap = BTreeMap::new(); - for client in clients { - if let Some(pid) = client.sec_pid { - *counts.entry(pid).or_insert(0) += 1; - } - } - - // Every PID seen on 2+ clients is a pulse candidate. If there is exactly - // one such PID we trust it; zero or several ⇒ fail closed. - let mut repeated = counts.iter().filter(|&(_, &n)| n >= 2).map(|(&pid, _)| pid); - let first = repeated.next()?; - if repeated.next().is_some() { - // Ambiguous: more than one value repeats. - return None; - } - Some(first) +/// No filtering, and deliberately so (see the module docs): any rule applied +/// here is a guess about other processes' Client counts, while stage 2 has the +/// kernel's own answer. A `BTreeSet` because the adapter diffs successive +/// candidate sets to decide what to re-probe, and that diff must not depend on +/// Client iteration order. +pub fn candidates(clients: &[ClientSnapshot]) -> BTreeSet { + clients.iter().filter_map(|client| client.sec_pid).collect() } -/// Stage 2: confirm the candidate against the `comm` read from +/// Stage 2: confirm one candidate against the `comm` read from /// `/proc//comm`. /// /// `comm` is `None` when the adapter's read failed — the `/proc` entry is @@ -79,11 +87,36 @@ pub fn validate(candidate: u32, comm: Option<&str>) -> Option { } } -/// The two stages composed, for callers that already hold the probed `comm`. -/// The model keeps them separate (it recomputes the candidate as clients -/// churn, and only re-probes when the candidate *changes*), so this is a -/// convenience for tests and for the fully-resolved path. -pub fn derive(clients: &[ClientSnapshot], comm_of: impl Fn(u32) -> Option) -> Option { - let candidate = candidate(clients)?; - validate(candidate, comm_of(candidate).as_deref()) +/// Stage 2 across the whole candidate set: the *unique* PID whose `comm` is +/// pipewire-pulse's. +/// +/// `None` when none matches (nothing to suppress that we can prove) and also +/// when **several** do. Several means either two pipewire-pulse daemons are +/// live — a nested or sandboxed session — or a `comm` collision, and a single +/// `Option` cannot suppress two owners. Failing closed here lands on the +/// over-exclusion side, matching the asymmetry `owner::keys_of` already +/// documents: broad over-exclusion is annoying, a missed suppression is an +/// echo. Suppressing a *set* of daemon PIDs is the real answer if a +/// multi-daemon host ever turns up; it is not v1, and it is recorded rather +/// than silently approximated. +pub fn resolve(candidates: &BTreeSet, comm_of: impl Fn(u32) -> Option) -> Option { + let mut found = None; + for &pid in candidates { + if validate(pid, comm_of(pid).as_deref()).is_some() { + if found.is_some() { + return None; + } + found = Some(pid); + } + } + found +} + +/// Both stages composed, for callers that can probe on demand. +/// +/// The model keeps them separate — it recomputes the candidate set as Clients +/// churn and only re-probes PIDs entering it — so this is a convenience for +/// tests and for the fully-resolved path. +pub fn derive(clients: &[ClientSnapshot], comm_of: impl Fn(u32) -> Option) -> Option { + resolve(&candidates(clients), comm_of) } diff --git a/src/host/observer/tests.rs b/src/host/observer/tests.rs index 3b38e74..e594f47 100644 --- a/src/host/observer/tests.rs +++ b/src/host/observer/tests.rs @@ -418,29 +418,107 @@ fn clients_with(pids: &[Option]) -> Vec { .collect() } +/// A `comm` probe of `pipewire-pulse` identifies the daemon regardless of how +/// many Clients it holds. #[test] -fn pid_candidate_consistent_repeated_value() { +fn pid_resolves_on_comm_not_on_repetition() { let clients = clients_with(&[Some(4137), Some(4137), Some(9001)]); - assert_eq!(pulse_pid::candidate(&clients), Some(4137)); + let pulse = pulse_pid::derive(&clients, |pid| { + Some( + if pid == 4137 { + "pipewire-pulse" + } else { + "firefox" + } + .to_string(), + ) + }); + assert_eq!(pulse, Some(4137)); +} + +/// 🔴 **The round-10 regression, measured on this host and caught by the §5.1 +/// row-1 matrix run.** WirePlumber holds two Clients (`WirePlumber` and +/// `WirePlumber [export]`) sharing one `sec_pid`, so two values repeat. The old +/// stage 1 called that ambiguous and returned `None`, which switched key 4's +/// suppression off and fused every Pulse-emulated node into a single owner — +/// a machine-wide over-exclusion cascade, on a stock desktop, permanently. +#[test] +fn a_second_process_holding_two_clients_does_not_defeat_the_derivation() { + // 1747 = WirePlumber x2, 2528 = pipewire-pulse x2, plus a native app. + let clients = clients_with(&[Some(1747), Some(1747), Some(2528), Some(2528), Some(9001)]); + let pulse = pulse_pid::derive(&clients, |pid| { + Some( + match pid { + 1747 => "wireplumber", + 2528 => "pipewire-pulse", + _ => "firefox", + } + .to_string(), + ) + }); + assert_eq!( + pulse, + Some(2528), + "the WirePlumber pair must not make this ambiguous" + ); +} + +/// The other direction the old rule failed in: pipewire-pulse holding exactly +/// one Client (a session with one Pulse app) repeated nothing, so it was never +/// even a candidate — same cascade, opposite cause. +#[test] +fn a_daemon_holding_a_single_client_is_still_found() { + let clients = clients_with(&[Some(2528), Some(9001)]); + let pulse = pulse_pid::derive(&clients, |pid| { + Some( + if pid == 2528 { + "pipewire-pulse" + } else { + "kwin_wayland" + } + .to_string(), + ) + }); + assert_eq!(pulse, Some(2528)); } #[test] -fn pid_candidate_inconsistent_two_repeats_is_none() { - let clients = clients_with(&[Some(4137), Some(4137), Some(9001), Some(9001)]); - assert_eq!(pulse_pid::candidate(&clients), None); +fn pid_candidates_are_every_distinct_sec_pid() { + let clients = clients_with(&[Some(4137), Some(4137), Some(9001), None]); + assert_eq!( + pulse_pid::candidates(&clients), + [4137, 9001].into_iter().collect() + ); } #[test] -fn pid_candidate_missing_property_is_none() { +fn pid_missing_property_leaves_nothing_to_probe() { let clients = clients_with(&[None, None, None]); - assert_eq!(pulse_pid::candidate(&clients), None); + assert!(pulse_pid::candidates(&clients).is_empty()); + assert_eq!(pulse_pid::derive(&clients, |_| None), None); } +/// No Client's `comm` is pipewire-pulse's: nothing to suppress that we can +/// prove, so `None` — and key 4 stays coarse rather than wrong. #[test] -fn pid_candidate_single_occurrence_is_none() { - // One client per pid: nothing repeats, so nothing is pipewire-pulse. +fn pid_resolve_no_match_is_none() { let clients = clients_with(&[Some(4137), Some(9001)]); - assert_eq!(pulse_pid::candidate(&clients), None); + assert_eq!( + pulse_pid::derive(&clients, |_| Some("firefox".to_string())), + None + ); +} + +/// Two live pipewire-pulse daemons: a single `Option` cannot suppress +/// both, so fail closed to over-exclusion rather than pick one and leak the +/// other's fusion. +#[test] +fn pid_resolve_two_daemons_is_none() { + let clients = clients_with(&[Some(4137), Some(9001)]); + assert_eq!( + pulse_pid::derive(&clients, |_| Some("pipewire-pulse".to_string())), + None + ); } #[test] @@ -473,9 +551,8 @@ fn pid_validate_reuse_named_other_process_is_none() { #[test] fn pid_derive_end_to_end_valid() { let clients = clients_with(&[Some(4137), Some(4137)]); - let candidate = pulse_pid::candidate(&clients).expect("candidate"); assert_eq!( - pulse_pid::validate(candidate, Some("pipewire-pulse")), + pulse_pid::derive(&clients, |_| Some("pipewire-pulse".to_string())), Some(4137) ); } @@ -490,7 +567,7 @@ fn model_pulse_pid_valid_through_projection() { m.apply(client(1, 200, Some(4137))); m.apply(client(2, 201, Some(4137))); m.apply(client(3, 202, Some(9001))); - assert_eq!(m.pulse_pid_candidate(), Some(4137)); + assert_eq!(m.pulse_pid_candidates(), [4137, 9001].into_iter().collect()); m.apply(RegEvent::ProcCommProbed { pid: 4137, comm: Some("pipewire-pulse".to_string()), @@ -507,6 +584,40 @@ fn model_pulse_pid_none_until_probed() { assert_eq!(m.project().pipewire_pulse_pid, None); } +/// Probed `comm`s are dropped once no Client presents the PID any more. +/// +/// Two reasons, and the second is the load-bearing one: the map is bounded by +/// the live Client count in a process that runs for hours, **and** a PID that +/// leaves and returns is re-probed rather than answered from the `comm` of +/// whoever held that number before. Pruning cannot change the projection — +/// `pulse_pid` only reads PIDs in the current candidate set — which is why it +/// is not an `apply` arm and must not publish. +#[test] +fn a_departed_pid_does_not_keep_its_probed_comm() { + let mut m = model(); + m.apply(client(1, 200, Some(4137))); + m.apply(RegEvent::ProcCommProbed { + pid: 4137, + comm: Some("pipewire-pulse".to_string()), + }); + assert_eq!(m.project().pipewire_pulse_pid, Some(4137)); + + // The daemon's Client goes away; the adapter prunes to the live set. + let live = m.pulse_pid_candidates(); + assert!(live.contains(&4137)); + m.retain_probed_comms(&std::collections::BTreeSet::new()); + + // A *different* process now holds 4137 and opens a Client. Without the + // prune this would answer from the stale `comm` and suppress a real app's + // owner key. + m.apply(client(2, 201, Some(4137))); + assert_eq!( + m.project().pipewire_pulse_pid, + None, + "the stale comm must not survive its PID leaving the graph" + ); +} + #[test] fn model_pulse_pid_none_on_comm_mismatch() { let mut m = model();