mollusk/pixelpass
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

repair: one atomic listing, namespace-aware liveness, renderer as authority

Browse Source 
Second review round. Two blocking P2s and two P3s, all applied.

**The two-listing correlation was unsound, so it is gone** (P2). Pairing the short
listing's indices with the JSON listing's exact arguments by position breaks 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 inherits a canonical fingerprint. The name
check cannot see it and the retry never fires, because correlation "succeeded".

Observations now come from a single `pactl list short modules` invocation, so every
`(index, name, argument)` comes from one server response and cannot be
mis-assembled. The two costs of that format are handled rather than hoped away:

- A tab inside an argument is invisible here, so the row is marked
  `args_complete: false`. `classify` refuses such a row outright — a truncation
  could otherwise coincide with a canonical form — while the reference gate can
  still see that it names a sink.
- A crafted argument containing a newline can fabricate a row, but it only does
  damage if it claims a *real* module's index, which makes that index appear twice.
  A duplicated index now refuses the whole run.

libpulse introspection (`pa_module_info` carries index, name and argument in one
record) remains the exact route. It is a new dependency plus a mainloop in a
one-shot CLI path, so it is recorded as the upgrade rather than taken unilaterally.

**Liveness was still converting invisible-but-alive into dead** (P2). A
`/proc/self` preflight proves nothing: inside a pid namespace — a container, a
distrobox — `self` is visible while every process in the parent namespace is not,
and `hidepid` has the same shape. Repair there can reach the host's Pulse socket,
see a live host's modules, call its pid dead and unload a running host's audio.

So the probe now asks for positive confidence instead: `NSpid` in
`/proc/self/status` reports this process's pid in every namespace it appears in, so
more than one entry means our pid numbers are not the outer namespace's and every
verdict becomes `Unknown`. A kernel that does not report `NSpid`, a container
marker, and a non-local `PULSE_SERVER` all fail closed the same way. Liveness
itself is `kill(pid, 0)` via the existing `nix` dep, where `EPERM` proves
existence; pid 0 and pids past `i32::MAX` are never asked, since `kill(0, …)`
would signal our own process group.

**The renderer is the authority, not the derived template** (P3). `classify` now
re-renders the pid it extracted and demands byte equality, so the template is only
a pre-filter. `Shape` also owns the module *name* now, and `host/audio.rs` loads
through `Shape::{module_name, render_args}` — previously the "cannot drift" claim
covered only arguments while the names were still written out at both ends. The
sentinel assertion is unconditional (`assert!`), so a future shape that repeats the
pid cannot slip through a release build.

**A test I wrongly called unclosable** (P3). I argued no non-vacuous case could
prove the module name is part of the identity, because the name determines which
argument grammar can match. That was wrong: the grammars are not disjoint —
`module-echo-cancel sink_name=pixelpass_capture_9` is byte-identical to the
canonical null-sink argument, which the suite already constructs. Same index, same
arguments, different name, and `still_matches` must say no.

252 tests (+1 net; the correlation tests were replaced by parser and probe tests),
clippy clean, fmt clean apart from the pre-existing taint/tests.rs:2683. Both live
field gates re-run against the rewritten observation path: the A/B orphan test
still removes exactly the two orphans with the module table otherwise identical,
and the reference/unrecognised fixture still leaves both modules alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
mollusk
2026-07-26 22:07:58 -04:00
co-authored by Claude Opus 5
parent 9145b2a726
commit 01c582427b
3 changed files with 378 additions and 310 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/repair/plan.rs
+131 -66
View File
@@ -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<String> {
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<String> {
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<String> {
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<String> {
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<Fingerprint> {
// 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.