diff --git a/src/host/audio.rs b/src/host/audio.rs index 9238a60..de36328 100644 --- a/src/host/audio.rs +++ b/src/host/audio.rs @@ -39,7 +39,7 @@ use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; use crate::cli::HostOpts; -use crate::repair::plan as repair_plan; +use crate::repair::plan::{self as repair_plan, Shape}; /// Owns the pactl-loaded modules plus, when filtering is active, the /// libpipewire stream-router thread. Drop unloads modules as a backstop; @@ -67,7 +67,7 @@ impl Routing { let pid = std::process::id(); let sink_name = repair_plan::sink_name_for(pid); - let sink_module = load_module("module-null-sink", &repair_plan::null_sink_args(pid)) + 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 @@ -83,7 +83,7 @@ impl Routing { None } else { Some( - load_module("module-loopback", &repair_plan::mirror_args(pid)) + load_module(Shape::LoopbackIntoCapture, pid) .context("failed to load module-loopback (null-sink cleaned up on Drop)")?, ) }; @@ -132,10 +132,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( - "module-loopback", - &repair_plan::local_monitor_args(pid), - ) { + match load_module(Shape::LoopbackOutOfCapture, pid) { Ok(id) => { tracing::info!( module = id, @@ -188,7 +185,7 @@ impl Routing { tracing::info!( "audio routing: last routed stream gone → restoring default-sink loopback" ); - match load_module("module-loopback", &repair_plan::mirror_args(pid)) { + match load_module(Shape::LoopbackIntoCapture, pid) { Ok(id) => { *loopback_for_task.lock().unwrap() = Some(id); } @@ -337,17 +334,18 @@ struct SinkInputProperties { // pactl module helpers // ────────────────────────────────────────────────────────────────────── -/// Load one Pulse module and return its index. +/// Load the Pulse module for one [`Shape`] and return its index. /// -/// `args` comes from the renderers in [`crate::repair::plan`] rather than being -/// written out here, so that `--repair`'s exact-form matcher and this loader are -/// one source of truth. A latency or argument change that only moved one of them -/// would leave repair silently unable to recognise the modules this build loads. -fn load_module(module: &str, args: &[String]) -> Result { +/// Both the module name and its arguments come from the shape itself +/// ([`crate::repair::plan::Shape`]) rather than being written out here, so that +/// `--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) -> Result { let output = Command::new("pactl") .arg("load-module") - .arg(module) - .args(args) + .arg(shape.module_name()) + .args(shape.render_args(pid)) .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 cfa61b9..e107e1c 100644 --- a/src/repair/mod.rs +++ b/src/repair/mod.rs @@ -16,22 +16,39 @@ //! connection-owned and removes itself when its host dies, so there is nothing //! there for repair to do and no safe way for it to help. //! -//! # Where the observations come from +//! # Where the observations come from: one listing, atomically //! -//! Two `pactl` invocations, because neither alone is sufficient: +//! `pactl list short modules`, in a single invocation, and nothing else. Every +//! observation's `(index, name, argument)` therefore comes from **one** server +//! response and cannot be mis-assembled. //! -//! - `pactl list short modules` carries the **module index**, which is the only -//! thing `unload-module` accepts. -//! - `pactl -f json list modules` carries the **exact argument string**. Measured -//! on pactl 17.0: its records have no index at all (`"index": null`), so it -//! cannot be used on its own. Its arguments are exact, though, where the short -//! listing's are tab-delimited text that a module argument may itself contain. +//! This replaced a two-listing scheme (indices from the short listing, exact +//! arguments from `pactl -f json list modules`, correlated by position). The JSON +//! listing was needed because it carries the exact argument where the short +//! listing's is tab-delimited text — but on pactl 17 its records carry **no module +//! index at all** (`"index": null`) while `unload-module` accepts only an index, so +//! it can never stand alone. Correlating the two by position is unsound with +//! repeated module names: if another client loads one module and unloads another +//! between the two calls, the counts and names still line up while the arguments +//! have shifted by one, and a *foreign* module can inherit a canonical +//! fingerprint. The name check cannot see that, and the retry never fires because +//! correlation "succeeded". //! -//! They are correlated **positionally** and the pairing is *checked*: same count, -//! same module name at every position, or the run refuses. That check is also what -//! makes a fabricated row harmless — a line crafted to look like a module in the -//! short listing has no JSON counterpart, so the sequences misalign and repair -//! stops instead of unloading an index it inferred from text. +//! Two consequences of using the short listing alone, both handled rather than +//! hoped away: +//! +//! - **A tab inside an argument is invisible to this format.** Such a row is marked +//! `args_complete: false`; [`plan::classify`] refuses it outright (a truncation +//! could otherwise coincide with a canonical form) while the reference gate can +//! still see that the row names a sink. +//! - **A crafted argument containing a newline can fabricate a row.** A fabricated +//! row only does damage if it names a *real* module's index — which makes that +//! index appear twice — so a duplicated index refuses the whole run. +//! +//! The remaining exact route is libpulse introspection (`pa_module_info` carries +//! index, name and argument in one record). That is a new dependency plus a +//! mainloop in a one-shot CLI path, so it is recorded as the upgrade rather than +//! taken now; see the deferred item in the impl plan. pub mod plan; @@ -41,11 +58,6 @@ use std::process::Command; use plan::{Fingerprint, Liveness, ModuleObservation, Shape}; -/// How many times to re-take the pair of listings when they disagree. A module -/// loaded or unloaded by anyone between the two `pactl` calls shifts the pairing; -/// that is a transient, so retry a couple of times before giving up. -const SNAPSHOT_ATTEMPTS: u32 = 3; - pub async fn run() -> Result<()> { let liveness = LivenessProbe::new(); if let Some(reason) = liveness.degraded_reason() { @@ -220,28 +232,84 @@ fn describe(fp: &Fingerprint) -> String { // Liveness // ────────────────────────────────────────────────────────────────────── -/// Answers "is this pid still around", and knows when it cannot answer. +/// Answers "is this pid still around", and knows when it must refuse to answer. /// -/// `Path::exists()` maps every error — permission, missing `/proc`, a filtered -/// mount — to `false`, which in this module would read as "dead, go ahead and -/// unload". So the probe uses `try_exists()` and preflights `/proc` itself: if -/// procfs is not answering for *this* process, no pid can be called dead. +/// Two separate hazards, and the first one is the dangerous one: +/// +/// 1. **A pid can be alive and invisible.** Inside a pid namespace — a container, +/// a distrobox — `/proc/self` is perfectly visible while every process in the +/// *parent* namespace is not. Repair there can reach the host's Pulse socket, +/// see a live host's modules, conclude its pid is dead, and unload a running +/// host's audio. `hidepid` has the same self-visible/others-invisible shape. So +/// a "can I see myself" preflight proves nothing; what is needed is positive +/// confidence that our pid numbers mean the same thing as the ones in the +/// module names. `NSpid` in `/proc/self/status` answers that directly: more than +/// one entry means we are nested, and every verdict becomes `Unknown`. +/// 2. **An error is not an absence.** `Path::exists()` maps permission errors and a +/// missing `/proc` to `false`, which here reads as "dead, go ahead". Liveness is +/// asked with `kill(pid, 0)` instead, where `EPERM` *proves* existence. +/// +/// A remote Pulse server also fails closed: the module table then belongs to +/// another machine's processes, where our pids mean nothing at all. struct LivenessProbe { - /// `None` when procfs looks usable; `Some(reason)` when every answer must be - /// [`Liveness::Unknown`]. + /// `None` when pid numbers here are trustworthy; `Some(reason)` when every + /// answer must be [`Liveness::Unknown`]. degraded: Option, } impl LivenessProbe { fn new() -> Self { - // If our own entry is not visible, procfs is not a source of truth here - // (no /proc mounted, a sandbox filter, a foreign pid namespace). - let degraded = match Path::new("/proc/self/stat").try_exists() { - Ok(true) => None, - Ok(false) => Some("/proc/self/stat is not visible".to_string()), - Err(e) => Some(format!("/proc/self/stat could not be read: {e}")), - }; - Self { degraded } + Self { + degraded: Self::detect_degradation(), + } + } + + fn detect_degradation() -> Option { + // A remote server's modules belong to another machine's pids. + if let Some(server) = std::env::var_os("PULSE_SERVER") { + let server = server.to_string_lossy().to_string(); + let local = + server.starts_with("unix:") || server.starts_with('/') || server.starts_with("{"); + if !local { + return Some(format!("PULSE_SERVER={server} is not a local socket")); + } + } + // The authoritative namespace question: NSpid lists this process's pid in + // every namespace it is visible in, outermost first. More than one entry + // means our pid numbers are not the ones the outer namespace uses. + match std::fs::read_to_string("/proc/self/status") { + Ok(status) => { + let nspid = status + .lines() + .find_map(|line| line.strip_prefix("NSpid:")) + .map(|rest| rest.split_whitespace().count()); + match nspid { + Some(n) if n > 1 => { + return Some(format!( + "this process is in a nested pid namespace (NSpid has {n} entries), \ + so pids in module names may belong to processes it cannot see" + )); + } + // A kernel too old to report NSpid cannot rule nesting out. + None => { + return Some( + "/proc/self/status does not report NSpid, so pid-namespace identity \ + cannot be established" + .to_string(), + ); + } + Some(_) => {} + } + } + Err(e) => return Some(format!("/proc/self/status could not be read: {e}")), + } + // Belt and braces: container runtimes that leave a marker. + for marker in ["/run/.containerenv", "/.dockerenv"] { + if Path::new(marker).try_exists().unwrap_or(false) { + return Some(format!("{marker} exists, so this is a container")); + } + } + None } fn degraded_reason(&self) -> Option<&str> { @@ -252,11 +320,17 @@ impl LivenessProbe { if self.degraded.is_some() { return Liveness::Unknown; } - match Path::new(&format!("/proc/{pid}")).try_exists() { - Ok(true) => Liveness::Alive, - Ok(false) => Liveness::Dead, - // A pid we are not allowed to ask about is not a pid we may destroy - // state for. + // `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. + if pid == 0 || pid > i32::MAX as u32 { + return Liveness::Unknown; + } + match nix::sys::signal::kill(nix::unistd::Pid::from_raw(pid as i32), None) { + Ok(()) => Liveness::Alive, + // The process exists; we merely may not signal it. + Err(nix::errno::Errno::EPERM) => Liveness::Alive, + Err(nix::errno::Errno::ESRCH) => Liveness::Dead, Err(_) => Liveness::Unknown, } } @@ -266,54 +340,8 @@ impl LivenessProbe { // Observation // ────────────────────────────────────────────────────────────────────── -/// One correlated observation of the whole module table. +/// One observation of the whole module table, from a single `pactl` invocation. fn snapshot() -> Result> { - let mut last_error = None; - for attempt in 1..=SNAPSHOT_ATTEMPTS { - let short = list_short().context("failed to run `pactl list short modules`")?; - let json = list_json().context("failed to run `pactl -f json list modules`")?; - match correlate(short, json) { - Ok(observations) => return Ok(observations), - Err(e) => { - if attempt < SNAPSHOT_ATTEMPTS { - tracing::debug!("repair: module listings disagreed, retrying: {e:#}"); - } - last_error = Some(e); - } - } - } - Err(last_error.expect("at least one attempt ran")).context( - "the two pactl module listings never agreed (is something loading modules right now?)", - ) -} - -/// Pair `(id, name)` rows with `(name, argument)` records by position, checking -/// the pairing rather than trusting it. -fn correlate( - short: Vec<(u32, String)>, - json: Vec<(String, String)>, -) -> Result> { - if short.len() != json.len() { - bail!( - "pactl reported {} modules in the short listing and {} in JSON", - short.len(), - json.len() - ); - } - let mut observations = Vec::with_capacity(short.len()); - for ((id, short_name), (json_name, argument)) in short.into_iter().zip(json) { - if short_name != json_name { - bail!( - "module listings disagree at #{id}: short says {short_name:?}, JSON says \ - {json_name:?}" - ); - } - observations.push(ModuleObservation::new(id, &short_name, &argument)); - } - Ok(observations) -} - -fn list_short() -> Result> { let output = Command::new("pactl") .args(["list", "short", "modules"]) .output() @@ -325,60 +353,55 @@ fn list_short() -> Result> { ); } let text = String::from_utf8(output.stdout).context("pactl returned non-UTF-8")?; - Ok(parse_short(&text)) + parse_short(&text) } -/// `pactl list short modules` is tab-separated. Only the id and name are taken -/// from it — the argument comes from the JSON listing, because a module argument -/// can itself contain tabs and newlines, which this format cannot escape. +/// Parse `pactl list short modules`: `\t\t` per module, one +/// server response, so a row's three fields always belong together. /// -/// Some modules render their arguments as a multi-line `{ … }` block whose -/// continuation lines start with whitespace; those never parse as a u32 id, so -/// filtering on that keeps them out. -fn parse_short(text: &str) -> Vec<(u32, String)> { - let mut rows = Vec::new(); +/// Two format hazards, both handled rather than assumed away: +/// +/// - **Tabs.** pactl emits a trailing tab after the argument, so a well-formed row +/// has at most one empty field after it. Any *non-empty* content past the +/// argument means the argument itself contained a tab and this format cannot +/// show it in full: the row is marked `args_complete: false`, never classified, +/// but still visible to the reference gate. +/// - **Newlines.** Some modules render arguments as a multi-line `{ … }` block; the +/// continuation lines never parse as a u32 index, so they are dropped. A crafted +/// argument *could* still fabricate a row that does parse — but to do damage it +/// must claim a real module's index, which then appears twice. A duplicated index +/// therefore refuses the whole run. +fn parse_short(text: &str) -> Result> { + let mut rows: Vec = Vec::new(); for line in text.lines() { - let mut parts = line.split('\t'); - let Some(id_str) = parts.next() else { continue }; + let fields: Vec<&str> = line.split('\t').collect(); + let Some(id_str) = fields.first() else { + continue; + }; let Ok(id) = id_str.parse::() else { continue; }; - let Some(name) = parts.next() else { continue }; - rows.push((id, name.to_string())); + let Some(name) = fields.get(1) else { continue }; + let args = fields.get(2).copied().unwrap_or(""); + let complete = fields.iter().skip(3).all(|extra| extra.is_empty()); + rows.push(if complete { + ModuleObservation::new(id, name, args) + } else { + ModuleObservation::truncated(id, name, args) + }); } - rows -} - -fn list_json() -> Result> { - let output = Command::new("pactl") - .args(["-f", "json", "list", "modules"]) - .output() - .context("failed to run pactl")?; - if !output.status.success() { - bail!( - "pactl -f json list modules failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); + // A repeated index is the one shape a fabricated row needs in order to make us + // unload a module that exists. It is also never legitimate. + for (i, row) in rows.iter().enumerate() { + if rows[..i].iter().any(|earlier| earlier.id == row.id) { + bail!( + "module index #{} appears more than once in `pactl list short modules`; \ + refusing to unload anything", + row.id + ); + } } - parse_json(&output.stdout) -} - -/// The JSON listing carries the exact argument string but, on pactl 17, no module -/// index (`"index": null`) — hence the correlation with the short listing. -fn parse_json(stdout: &[u8]) -> Result> { - #[derive(serde::Deserialize)] - struct JsonModule { - name: String, - /// Absent or null for a module loaded without arguments. - #[serde(default)] - argument: Option, - } - let modules: Vec = - serde_json::from_slice(stdout).context("pactl returned unparseable JSON")?; - Ok(modules - .into_iter() - .map(|m| (m.name, m.argument.unwrap_or_default())) - .collect()) + Ok(rows) } fn unload_module(id: u32) -> Result<()> { @@ -400,17 +423,44 @@ fn unload_module(id: u32) -> Result<()> { mod tests { use super::*; + /// The whole row comes from one response: index, name and the exact argument, + /// including pactl's trailing tab. #[test] - fn parses_id_and_name_from_the_short_listing() { - let text = "5\tmodule-null-sink\tsink_name=pixelpass_capture_42\n\ - 10\tmodule-loopback\tsource=@DEFAULT_SINK@.monitor sink=pixelpass_capture_42\n"; - let rows = parse_short(text); + fn parses_index_name_and_exact_argument() { + let text = "5\tmodule-null-sink\tsink_name=pixelpass_capture_42\t\n\ + 10\tmodule-loopback\tsource=@DEFAULT_SINK@.monitor \ + sink=pixelpass_capture_42 latency_msec=20\t\n\ + 7\tmodule-always-sink\n"; + let rows = parse_short(text).expect("well-formed"); + assert_eq!(rows.len(), 3); + assert_eq!(rows[0].id, 5); + assert_eq!(rows[0].args, "sink_name=pixelpass_capture_42"); + assert!(rows[0].args_complete, "a trailing tab is not truncation"); + assert!(rows[1].args.ends_with("latency_msec=20")); + assert_eq!(rows[2].args, "", "a module without arguments still parses"); + } + + /// An argument containing a tab cannot be shown in full by this format. Such a + /// row must never be classified — a truncation could coincide with a canonical + /// form — but it must still be *present*, or the reference gate would not see + /// that something names our sink. + #[test] + fn a_tab_inside_an_argument_marks_the_row_incomplete() { + let canonical = "source=@DEFAULT_SINK@.monitor sink=pixelpass_capture_42 latency_msec=20"; + let text = format!("10\tmodule-loopback\t{canonical}\tremix=false\n"); + let rows = parse_short(&text).expect("well-formed"); + assert_eq!(rows.len(), 1); + assert!(!rows[0].args_complete); assert_eq!( - rows, - vec![ - (5, "module-null-sink".to_string()), - (10, "module-loopback".to_string()) - ] + plan::classify(&rows[0]), + None, + "a truncated argument that happens to match must not be ours" + ); + // Still visible to the gate that protects a sink from being destroyed. + assert_eq!( + plan::sink_still_referenced(&rows, 42, 999), + Some(10), + "an unreadable argument still counts as a reference" ); } @@ -424,81 +474,24 @@ mod tests { \x20 rt.prio = 88\n\ }\n\ 5\tmodule-null-sink\tsink_name=pixelpass_capture_42\n"; - let rows = parse_short(text); + let rows = parse_short(text).expect("well-formed"); assert_eq!(rows.len(), 2, "{rows:?}"); - assert_eq!(rows[1].0, 5); + assert_eq!(rows[1].id, 5); } + /// A crafted argument can fabricate a row that parses. It only does damage if + /// it claims a *real* module's index — which makes that index appear twice — so + /// a duplicate index refuses the entire run rather than unloading a stranger. #[test] - fn parses_the_json_listing_including_an_argumentless_module() { - let json = br#"[ - {"index":null,"name":"module-null-sink","argument":"sink_name=pixelpass_capture_42", - "properties":{},"usage_counter":null}, - {"index":null,"name":"module-metadata","argument":null,"properties":{}} - ]"#; - let records = parse_json(json).expect("valid JSON"); - assert_eq!( - records, - vec![ - ( - "module-null-sink".to_string(), - "sink_name=pixelpass_capture_42".to_string() - ), - ("module-metadata".to_string(), String::new()) - ] - ); - } - - /// The correlation is the whole reason two listings are safe to combine: the - /// id comes from one and the argument from the other, so a disagreement means - /// we do not know which argument belongs to which index. - #[test] - fn correlation_pairs_ids_with_exact_arguments() { - let short = vec![ - (5, "module-null-sink".to_string()), - (10, "module-loopback".to_string()), - ]; - let json = vec![ - ( - "module-null-sink".to_string(), - "sink_name=pixelpass_capture_42".to_string(), - ), - ( - "module-loopback".to_string(), - "source=@DEFAULT_SINK@.monitor sink=pixelpass_capture_42 latency_msec=20" - .to_string(), - ), - ]; - let observations = correlate(short, json).expect("aligned"); - assert_eq!(observations[0].id, 5); - assert!(observations[1].args.contains("latency_msec=20")); - } - - /// A row that exists in one listing and not the other must stop the run. This - /// is also what makes a fabricated short-listing row harmless rather than - /// exploitable: it has no JSON counterpart, so the sequences misalign. - #[test] - fn correlation_refuses_a_mismatched_pairing() { - let short = vec![ - (5, "module-null-sink".to_string()), - (10, "module-loopback".to_string()), - ]; - let json = vec![( - "module-null-sink".to_string(), - "sink_name=pixelpass_capture_42".to_string(), - )]; + fn a_duplicated_index_refuses_the_whole_run() { + // As it would arrive: a module whose argument embeds a newline and a row + // that re-uses index 5, which really belongs to something else. + let text = "5\tmodule-real-thing\tconfig={\n\ + 5\tmodule-null-sink\tsink_name=pixelpass_capture_4242\n"; + let err = parse_short(text).expect_err("a repeated index is never legitimate"); assert!( - correlate(short.clone(), json).is_err(), - "count mismatch must fail" - ); - - let reordered = vec![ - ("module-loopback".to_string(), "a=b".to_string()), - ("module-null-sink".to_string(), "c=d".to_string()), - ]; - assert!( - correlate(short, reordered).is_err(), - "a name mismatch at any position must fail" + format!("{err:#}").contains("more than once"), + "unexpected error: {err:#}" ); } @@ -507,25 +500,14 @@ mod tests { /// suite green while real `--repair` recognised nothing at all. #[test] fn raw_pactl_output_becomes_a_plan() { - let short_text = "1\tlibpipewire-module-rt\t{\n\ - \x20 nice.level = -11\n\ - }\n\ - 10\tmodule-loopback\tsource=@DEFAULT_SINK@.monitor \ - sink=pixelpass_capture_4242 latency_msec=20\n\ - 11\tmodule-loopback\tsource=pixelpass_capture_4242.monitor \ - sink=@DEFAULT_SINK@ latency_msec=20\n"; - let json_bytes = br#"[ - {"index":null,"name":"libpipewire-module-rt","argument":"{ nice.level = -11 }"}, - {"index":null,"name":"module-loopback", - "argument":"source=@DEFAULT_SINK@.monitor sink=pixelpass_capture_4242 latency_msec=20"}, - {"index":null,"name":"module-loopback", - "argument":"source=pixelpass_capture_4242.monitor sink=@DEFAULT_SINK@ latency_msec=20"} - ]"#; - let observations = correlate( - parse_short(short_text), - parse_json(json_bytes).expect("valid JSON"), - ) - .expect("aligned"); + let text = "1\tlibpipewire-module-rt\t{\n\ + \x20 nice.level = -11\n\ + }\n\ + 10\tmodule-loopback\tsource=@DEFAULT_SINK@.monitor \ + sink=pixelpass_capture_4242 latency_msec=20\t\n\ + 11\tmodule-loopback\tsource=pixelpass_capture_4242.monitor \ + sink=@DEFAULT_SINK@ latency_msec=20\t\n"; + let observations = parse_short(text).expect("well-formed"); let planned = plan::plan(&observations, |_| Liveness::Dead); assert_eq!( @@ -535,4 +517,27 @@ mod tests { ); assert_eq!(planned.dead_pids.len(), 1); } + + /// The probe must refuse to call anything dead when pid numbers here may not + /// mean what the module names mean. On this host it should be confident; the + /// point of the assertion is that `of()` never returns `Dead` while degraded. + #[test] + fn a_degraded_probe_never_reports_dead() { + let degraded = LivenessProbe { + degraded: Some("test".to_string()), + }; + assert_eq!(degraded.of(1), Liveness::Unknown); + assert_eq!(degraded.of(u32::MAX), Liveness::Unknown); + + let probe = LivenessProbe::new(); + // Our own pid is alive by construction — unless this test itself runs in a + // pid namespace, which is exactly when the probe must abstain. + let me = std::process::id(); + match probe.degraded_reason() { + None => assert_eq!(probe.of(me), Liveness::Alive), + Some(_) => assert_eq!(probe.of(me), Liveness::Unknown), + } + // pid 0 would signal our whole process group; it is never askable. + assert_eq!(probe.of(0), Liveness::Unknown); + } } diff --git a/src/repair/plan.rs b/src/repair/plan.rs index bf299d5..c0f67d0 100644 --- a/src/repair/plan.rs +++ b/src/repair/plan.rs @@ -55,12 +55,15 @@ //! # Why the templates are generated, not written out //! //! The matcher's prefixes and suffixes are derived at runtime from the *same* -//! renderers the loader uses ([`null_sink_args`], [`mirror_args`], -//! [`local_monitor_args`]). Hard-coding `latency_msec=20` in a matcher would mean -//! that changing the loader silently blinds repair to every module the new version -//! loads — the fail-closed-and-silent failure this project has now been bitten by -//! three times. With one source of truth, a loader change moves the matcher with -//! it or fails to compile. +//! renderer the loader uses — [`Shape::render_args`], which owns the module name +//! too. Hard-coding `latency_msec=20` in a matcher would mean that changing the +//! loader silently blinds repair to every module the new version loads: the +//! fail-closed-and-silent failure this project has now been bitten by three times. +//! With one source of truth, a loader change moves the matcher with it. +//! +//! The template is only a pre-filter, too. [`classify`] re-renders the pid it +//! extracted and demands byte equality, so the renderer — not a derived pair of +//! strings — is always the authority on what one of our modules looks like. //! //! Blindness is also reported rather than assumed impossible: //! [`unrecognised_pixelpass_modules`] finds modules that name a @@ -82,31 +85,6 @@ pub fn sink_name_for(pid: u32) -> String { format!("{SINK_NAME_PREFIX}{pid}") } -/// `pactl load-module` arguments for the capture sink (pre-0c hosts only). -pub fn null_sink_args(pid: u32) -> Vec { - vec![format!("sink_name={}", sink_name_for(pid))] -} - -/// `pactl load-module` arguments for the default-sink mirror: the viewer hears -/// system audio. -pub fn mirror_args(pid: u32) -> Vec { - vec![ - "source=@DEFAULT_SINK@.monitor".to_string(), - format!("sink={}", sink_name_for(pid)), - format!("latency_msec={LOOPBACK_LATENCY_MSEC}"), - ] -} - -/// `pactl load-module` arguments for the local monitor: the sharer hears the app -/// they are sharing. -pub fn local_monitor_args(pid: u32) -> Vec { - vec![ - format!("source={}.monitor", sink_name_for(pid)), - "sink=@DEFAULT_SINK@".to_string(), - format!("latency_msec={LOOPBACK_LATENCY_MSEC}"), - ] -} - /// How the server records an argument vector we passed as separate argv entries. /// /// Measured on pactl 17.0 against a live pipewire-pulse: the arguments come back @@ -129,6 +107,14 @@ pub struct ModuleObservation { pub id: u32, pub name: String, pub args: String, + /// False when the observation format could not carry the whole argument (the + /// short listing is tab-delimited, and an argument may itself contain a tab). + /// + /// A truncated argument is never ours — [`classify`] refuses it outright, + /// because a truncation could otherwise coincide with a canonical form. It is + /// still kept in the snapshot: [`sink_still_referenced`] must be able to see + /// that *something* names a sink even when it cannot read the whole argument. + pub args_complete: bool, } impl ModuleObservation { @@ -137,16 +123,26 @@ impl ModuleObservation { id, name: name.to_string(), args: args.to_string(), + args_complete: true, + } + } + + /// An observation whose argument the format could not fully carry. + pub fn truncated(id: u32, name: &str, args: &str) -> Self { + Self { + args_complete: false, + ..Self::new(id, name, args) } } } /// Whether the owner of a module is still around. /// -/// Three states, not two: `/proc` can fail to answer (a different pid namespace, -/// a permission error, `hidepid`), and `Path::exists` collapses every one of -/// those into "no". That collapse points the wrong way — it turns "cannot tell" -/// into "dead, go ahead and unload". +/// Three states, not two, because "I cannot see that process" and "that process +/// does not exist" are different answers and only one of them permits destroying +/// anything. A pid can be alive and invisible: inside a pid namespace every +/// process in the parent namespace is, and `hidepid` hides others while leaving +/// `self` visible. Collapsing those into "absent" would point the wrong way. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Liveness { Alive, @@ -187,10 +183,16 @@ pub const ALL_SHAPES: [Shape; 3] = [ /// The pid used to render a shape's argument string when deriving its template. /// Any value works as long as its decimal form appears exactly once in the -/// rendered arguments, which [`Template::derive`] asserts. +/// rendered arguments, which [`Template::derive`] asserts — unconditionally, so a +/// future shape that repeats the pid cannot slip through a release build. const TEMPLATE_SENTINEL_PID: u32 = u32::MAX; /// An exact-match matcher for one shape, derived from that shape's own renderer. +/// +/// The template is only ever a *pre-filter*: it finds the candidate pid cheaply, +/// and [`classify`] then re-renders that pid through the real renderer and demands +/// byte equality. So the authority is always the renderer the loader uses, never +/// this derived pair of strings. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Template { pub module_name: &'static str, @@ -200,17 +202,18 @@ pub struct Template { impl Template { /// Split a shape's rendered arguments around the pid, giving a total matcher. - fn derive(module_name: &'static str, rendered: String) -> Self { + fn derive(shape: Shape) -> Self { + let rendered = recorded_argument(&shape.render_args(TEMPLATE_SENTINEL_PID)); let sentinel = TEMPLATE_SENTINEL_PID.to_string(); let (prefix, suffix) = rendered .split_once(&sentinel) - .expect("a rendered shape must contain its pid exactly once"); - debug_assert!( + .expect("a rendered shape must contain its pid"); + assert!( !suffix.contains(&sentinel), - "the sentinel pid must appear exactly once in {rendered:?}" + "a shape must name its pid exactly once, but {shape:?} rendered {rendered:?}" ); Self { - module_name, + module_name: shape.module_name(), prefix: prefix.to_string(), suffix: suffix.to_string(), } @@ -238,24 +241,45 @@ impl Template { } impl Shape { - /// The exact-match matcher for this shape, generated from the loader's own - /// renderer so the two cannot drift apart. - pub fn template(self) -> Template { - let pid = TEMPLATE_SENTINEL_PID; + /// The Pulse module this shape loads. Owned here rather than written out at + /// the call site, so the module *name* is as drift-proof as the arguments — + /// the loader asks for it too. + pub fn module_name(self) -> &'static str { match self { - Shape::LoopbackIntoCapture => { - Template::derive("module-loopback", recorded_argument(&mirror_args(pid))) - } - Shape::LoopbackOutOfCapture => Template::derive( - "module-loopback", - recorded_argument(&local_monitor_args(pid)), - ), - Shape::LegacyCaptureSink => { - Template::derive("module-null-sink", recorded_argument(&null_sink_args(pid))) - } + Shape::LoopbackIntoCapture | Shape::LoopbackOutOfCapture => "module-loopback", + Shape::LegacyCaptureSink => "module-null-sink", } } + /// The exact `pactl load-module` arguments for this shape, for `pid`. + /// + /// **This is the single source of truth.** `host/audio.rs` loads through it and + /// `--repair` matches through it, so a change here moves both at once. + pub fn render_args(self, pid: u32) -> Vec { + let sink = sink_name_for(pid); + match self { + // The default-sink mirror: the viewer hears system audio. + Shape::LoopbackIntoCapture => vec![ + "source=@DEFAULT_SINK@.monitor".to_string(), + format!("sink={sink}"), + format!("latency_msec={LOOPBACK_LATENCY_MSEC}"), + ], + // The local monitor: the sharer hears the app they are sharing. + Shape::LoopbackOutOfCapture => vec![ + format!("source={sink}.monitor"), + "sink=@DEFAULT_SINK@".to_string(), + format!("latency_msec={LOOPBACK_LATENCY_MSEC}"), + ], + // The legacy capture sink (pre-0c hosts only). + Shape::LegacyCaptureSink => vec![format!("sink_name={sink}")], + } + } + + /// The exact-match pre-filter for this shape, generated from `render_args`. + pub fn template(self) -> Template { + Template::derive(self) + } + /// Human label for reporting. pub fn label(self) -> &'static str { match self { @@ -327,13 +351,27 @@ impl Plan { /// sink=pixelpass_capture_4242` — is **not** ours and must never be unloaded, and /// a `sink=` token nested inside a quoted `sink_input_properties` value cannot be /// mistaken for a top-level argument because the whole string must match. +/// +/// The template only proposes a pid; the shape's own renderer decides. Re-rendering +/// and demanding byte equality means the loader is the authority, so a shape that +/// grows an argument, changes a latency, or repeats the pid cannot leave a matcher +/// quietly accepting the old form. pub fn classify(obs: &ModuleObservation) -> Option { + // An argument we could not read in full is never ours: a truncation could + // coincide with a canonical form, and unloading on a coincidence is the one + // outcome none of these rules tolerate. + if !obs.args_complete { + return None; + } for shape in ALL_SHAPES { let template = shape.template(); if obs.name != template.module_name { continue; } if let Some(pid) = template.pid_of(&obs.args) { + if recorded_argument(&shape.render_args(pid)) != obs.args { + continue; + } return Some(Fingerprint { id: obs.id, module_name: obs.name.clone(), @@ -433,19 +471,23 @@ mod tests { ModuleObservation::new( id, "module-null-sink", - &recorded_argument(&null_sink_args(pid)), + &recorded_argument(&Shape::LegacyCaptureSink.render_args(pid)), ) } fn mirror(id: u32, pid: u32) -> ModuleObservation { - ModuleObservation::new(id, "module-loopback", &recorded_argument(&mirror_args(pid))) + ModuleObservation::new( + id, + "module-loopback", + &recorded_argument(&Shape::LoopbackIntoCapture.render_args(pid)), + ) } fn local_monitor(id: u32, pid: u32) -> ModuleObservation { ModuleObservation::new( id, "module-loopback", - &recorded_argument(&local_monitor_args(pid)), + &recorded_argument(&Shape::LoopbackOutOfCapture.render_args(pid)), ) } @@ -464,15 +506,15 @@ mod tests { #[test] fn the_canonical_argument_strings_are_what_the_server_records() { assert_eq!( - recorded_argument(&mirror_args(4242)), + recorded_argument(&Shape::LoopbackIntoCapture.render_args(4242)), "source=@DEFAULT_SINK@.monitor sink=pixelpass_capture_4242 latency_msec=20" ); assert_eq!( - recorded_argument(&local_monitor_args(4242)), + recorded_argument(&Shape::LoopbackOutOfCapture.render_args(4242)), "source=pixelpass_capture_4242.monitor sink=@DEFAULT_SINK@ latency_msec=20" ); assert_eq!( - recorded_argument(&null_sink_args(4242)), + recorded_argument(&Shape::LegacyCaptureSink.render_args(4242)), "sink_name=pixelpass_capture_4242" ); } @@ -485,11 +527,7 @@ mod tests { for shape in ALL_SHAPES { let template = shape.template(); for pid in [1_u32, 7, 4242, 999_999, u32::MAX - 1] { - let args = match shape { - Shape::LoopbackIntoCapture => recorded_argument(&mirror_args(pid)), - Shape::LoopbackOutOfCapture => recorded_argument(&local_monitor_args(pid)), - Shape::LegacyCaptureSink => recorded_argument(&null_sink_args(pid)), - }; + let args = recorded_argument(&shape.render_args(pid)); assert_eq!( template.pid_of(&args), Some(pid), @@ -748,6 +786,33 @@ mod tests { assert!(!fp.still_matches(&local_monitor(10, 4242))); } + /// The module *name* is part of the identity, and this is provable because the + /// argument grammars overlap: `module-echo-cancel sink_name=pixelpass_capture_9` + /// is byte-identical to the canonical null-sink argument. So a comparator using + /// only `id + args` accepts a foreign module — and unloads it. + /// + /// (I previously argued this case could not be constructed non-vacuously, on the + /// grounds that the name determines which grammar can match. That was wrong: the + /// grammars are not disjoint across names.) + #[test] + fn an_identical_argument_under_another_module_name_is_not_a_match() { + let ours = null_sink(5, 9); + let fp = classify(&ours).expect("ours"); + let impostor = ModuleObservation::new(5, "module-echo-cancel", &ours.args); + + assert_eq!(fp.args, impostor.args, "the arguments really are identical"); + assert_eq!(fp.id, impostor.id, "and so is the index"); + assert!( + !fp.still_matches(&impostor), + "only the module name distinguishes these, so it must be compared" + ); + assert_eq!( + classify(&impostor), + None, + "and it is not ours to begin with" + ); + } + /// Whitespace is identity, not layout. The exact recorded argument is /// compared, so a re-spaced string is a *different* argument — inside a /// quoted property value that difference can be semantic.