From bcf1343a552f35ee97de47145c274d9451b16461 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 19:19:25 -0400 Subject: [PATCH 01/13] phase 1: tag every audio node peerspeak owns, on both carriers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero behaviour change. This is what makes the screenshare exclusion engine able to see us at all (plan §5.1, impl plan §3): pixelpass must refuse to fan out our own playback, and until now it had no way to recognise it. Two carriers, matched by pixelpass as a union — `peerspeak.owned=1` and a `node.name` prefix `peerspeak_owned__`. Round 8 added the second after the phase-5 audit found a node property is invisible to the PipeWire registry `global` event and recoverable only by binding the node; the prefix is announced directly. A union is also the fail-closed direction: a missed tag leaks call audio into a share, a spurious one only over-excludes. Three tagging sites, all three verified live on this host: - native call playback → props on the stream dict - screenshare mpv/VLC → PULSE_PROP + PIPEWIRE_PROPS on the child - notification chimes → same, on pw-play/paplay The literals are a cross-repo wire contract, so they appear once here as named constants and are pinned in a fixture committed byte-identical in both repos (tests/fixtures/ownership-tag-contract.txt). The contract test is black-box: it builds a real child `Command` and reads back the environment it would carry, rather than testing our own formatter. Three live `#[ignore]`d exit-gate tests drive the real call sites and poll `pw-dump` for the resulting node — the plan requires the tag be shown landing on a live node, not just in the env. All three mutation-verified (drop either carrier, or the role, and the matching gate fails). Measured while verifying: mpv, VLC, pw-play and paplay all honour `node.name` from those env vars. The native stream set neither `application.name` nor a description, so a mixer fell back to `node.name` — which the tag turns into an internal identifier. Added an explicit `node.description = "PeerSpeak"` there, which keeps the plan's rule (the prefix must not reach `node.description`) while preserving its intent: mixers stay readable. Co-Authored-By: Claude Opus 5 --- src/audio/mod.rs | 4 + src/audio/ownership.rs | 380 ++++++++++++++++++++++ src/audio/pipewire_impl.rs | 75 +++++ src/notify.rs | 57 +++- src/screenshare/mod.rs | 57 +++- tests/fixtures/ownership-tag-contract.txt | 34 ++ 6 files changed, 601 insertions(+), 6 deletions(-) create mode 100644 src/audio/ownership.rs create mode 100644 tests/fixtures/ownership-tag-contract.txt diff --git a/src/audio/mod.rs b/src/audio/mod.rs index 6bca441..9a1e9ac 100644 --- a/src/audio/mod.rs +++ b/src/audio/mod.rs @@ -65,6 +65,10 @@ pub mod eq; pub mod gate; pub mod limiter; pub mod multitrack; +// The cross-repo ownership tag (plan §5.1). Platform-neutral on purpose: the +// carriers only matter on PipeWire, but the literals are a wire contract and +// their test must run on every platform so a rename can't pass CI elsewhere. +pub mod ownership; pub mod pan; // Linear resamplers used by the Windows/cpal backend (W4). Platform-neutral and // pure, so it builds (and its tests run) everywhere even though only the cpal diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs new file mode 100644 index 0000000..a698ba5 --- /dev/null +++ b/src/audio/ownership.rs @@ -0,0 +1,380 @@ +//! Ownership tagging for every audio node peerspeak puts on the graph. +//! +//! Zero behaviour change on its own. Its whole purpose is to make peerspeak's +//! own playback *identifiable from outside the process*, so that pixelpass's +//! screenshare audio exclusion engine can refuse to fan our own call audio +//! back into the stream the far end is already hearing (the echo it exists to +//! prevent). See `docs/screenshare-audio-exclusion-plan.md` §5.1 and +//! `docs/screenshare-audio-exclusion-impl-plan.md` §3. +//! +//! # The wire contract +//! +//! The literals below are a **cross-repo contract**, not local naming. +//! pixelpass matches on them and does not depend on this crate (nor this +//! crate on it) — the coupling is the PipeWire graph itself. The values are +//! committed in both repos as `tests/fixtures/ownership-tag-contract.txt`, +//! and [`contract_matches_fixture`](tests::contract_matches_fixture) asserts +//! these constants against that file. pixelpass's matching constants are in +//! `src/host/taint/mod.rs` (`PEERSPEAK_OWNED_PROP`, +//! `PEERSPEAK_OWNED_NODE_PREFIX`), asserted against its own copy of the same +//! fixture. +//! +//! # Why two carriers +//! +//! A node is owned if **either** carrier matches — a union, which is the +//! fail-closed direction (a missed tag means our audio gets shared; a +//! spurious tag only over-excludes). Round 8 of the design added the second +//! carrier after the phase-5 audit found that a node *property* is invisible +//! to the PipeWire registry's `global` event and recoverable only by binding +//! the node, while `node.name` is announced directly. The primary taint root +//! should not rest on a single observation mechanism. +//! +//! `node.description` is deliberately left alone, so mixers still show "mpv" +//! rather than an internal identifier. +//! +//! # Known limitation (accepted for v1) +//! +//! The child environment is inherited by *grandchildren*, so a player that +//! spawns its own audio helper tags it too. That over-excludes rather than +//! leaks, and is the accepted v1 limitation recorded in plan §5.1. + +use std::process::Command; + +/// Carrier 1, key. Set on every peerspeak-owned node. Read by pixelpass only +/// after binding the node — see the module docs. +pub const OWNED_PROP_KEY: &str = "peerspeak.owned"; + +/// Carrier 1, value. pixelpass treats anything other than `false`/`0` as +/// truthy (fail-closed), but we always emit exactly this. +pub const OWNED_PROP_VALUE: &str = "1"; + +/// Carrier 2. The `node.name` prefix pixelpass matches; the remainder of the +/// name is diagnostic only. Format: `peerspeak_owned__`. +pub const OWNED_NODE_NAME_PREFIX: &str = "peerspeak_owned_"; + +/// The environment variable through which a PulseAudio-API client (mpv/VLC on +/// `--ao=pulse`, `paplay`) receives extra stream properties. +pub const PULSE_PROP_ENV: &str = "PULSE_PROP"; + +/// The environment variable through which a PipeWire-native client +/// (`pw-play`, mpv on `--ao=pipewire`) receives extra stream properties. +pub const PIPEWIRE_PROPS_ENV: &str = "PIPEWIRE_PROPS"; + +/// Role of the native call-playback stream — the node carrying the far end's +/// voice. +pub const NATIVE_PLAYBACK_ROLE: &str = "call"; + +/// Role of a notification-sound player (`pw-play`/`paplay`/`aplay`). +pub const NOTIFICATION_ROLE: &str = "notify"; + +/// Build the `node.name` for a node this process owns. +/// +/// The pid is **peerspeak's own**, not the child's: for a spawned player we +/// set the name before the fork, so the child's pid does not exist yet — and +/// the owning process is the more useful thing to record anyway. +pub fn owned_node_name(role: &str) -> String { + format!( + "{OWNED_NODE_NAME_PREFIX}{}_{}", + sanitize_role(role), + std::process::id() + ) +} + +/// Keep a role to `[a-z0-9-]`, lowercased, so the generated `node.name` is a +/// single shell-safe, whitespace-free token. +/// +/// Not defensive theatre: [`PULSE_PROP_ENV`] is a space-separated +/// `key=value` encoding, so a role containing a space would silently split +/// into a second bogus property. Every current caller passes a literal, and +/// this keeps that true no matter who calls next. +fn sanitize_role(role: &str) -> String { + let cleaned: String = role + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() { + c.to_ascii_lowercase() + } else { + '-' + } + }) + .collect(); + let trimmed = cleaned.trim_matches('-'); + if trimmed.is_empty() { + "child".to_string() + } else { + trimmed.to_string() + } +} + +/// The `PULSE_PROP` value carrying both carriers. +fn pulse_prop_value(node_name: &str) -> String { + format!("{OWNED_PROP_KEY}={OWNED_PROP_VALUE} node.name={node_name}") +} + +/// The `PIPEWIRE_PROPS` value carrying both carriers. +/// +/// SPA-JSON; keys are quoted because `peerspeak.owned` contains a dot. +fn pipewire_props_value(node_name: &str) -> String { + format!("{{ \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \"node.name\" = \"{node_name}\" }}") +} + +/// Tag a child process that will play audio, so its resulting node carries +/// both ownership carriers. +/// +/// Both environment variables are set unconditionally, because we cannot know +/// from here which audio API the child will pick (mpv chooses at runtime, and +/// VLC and `paplay` differ from `pw-play`). Setting the one the child ignores +/// costs nothing. +/// +/// Any inherited value of either variable is **replaced**, not appended: the +/// tag is a correctness mechanism, and a half-merged property string that +/// fails to parse would drop it silently. +pub fn tag_child(command: &mut Command, role: &str) { + let node_name = owned_node_name(role); + command.env(PULSE_PROP_ENV, pulse_prop_value(&node_name)); + command.env(PIPEWIRE_PROPS_ENV, pipewire_props_value(&node_name)); +} + +/// Live-test support for the phase-1 exit gate, shared by the two modules +/// that spawn tagged players (`notify`, `screenshare`). +/// +/// Lives here rather than in either test module because the gate is one +/// property — "the carriers reach a live node" — asked of two call sites, and +/// two copies of the poll would drift. +#[cfg(all(test, not(windows)))] +pub mod live_test { + use std::process::Command; + use std::time::{Duration, Instant}; + + /// Poll `pw-dump` for a `Stream/Output/Audio` node whose `node.name` + /// starts with `prefix`; returns its name and `peerspeak.owned` value. + /// + /// Polls rather than sleeping once because node creation races the + /// player process starting up: a fixed sleep either flakes or is slow. + pub fn poll_for_owned_node( + prefix: &str, + timeout: Duration, + ) -> Option<(String, Option)> { + let deadline = Instant::now() + timeout; + while Instant::now() < deadline { + if let Some(found) = scan_once(prefix) { + return Some(found); + } + std::thread::sleep(Duration::from_millis(100)); + } + None + } + + fn scan_once(prefix: &str) -> Option<(String, Option)> { + let out = Command::new("pw-dump").output().ok()?; + let dump: serde_json::Value = serde_json::from_slice(&out.stdout).ok()?; + for object in dump.as_array()? { + let props = &object["info"]["props"]; + // `continue`, never `?`: most objects in a dump are not nodes and + // have no `node.name` at all, and propagating that as `None` + // would abandon the scan at the first one. + let Some(name) = props["node.name"].as_str() else { + continue; + }; + if !name.starts_with(prefix) { + continue; + } + // PipeWire may hand the property back as a JSON string or a + // number depending on how the client set it; both are the same + // property, and reading only one would make the assertion + // accidentally depend on the producer's encoding. + let owned = props[super::OWNED_PROP_KEY] + .as_str() + .map(str::to_string) + .or_else(|| props[super::OWNED_PROP_KEY].as_i64().map(|n| n.to_string())); + return Some((name.to_string(), owned)); + } + None + } + + /// A zero-filled 48 kHz stereo s16 WAV of `seconds` length — long enough + /// for the poll to catch the node, silent so the gate never makes noise. + pub fn silent_wav(seconds: u32) -> Vec { + let data_len = 48_000 * seconds * 4; + let mut wav = Vec::with_capacity(44 + data_len as usize); + wav.extend_from_slice(b"RIFF"); + wav.extend_from_slice(&(36 + data_len).to_le_bytes()); + wav.extend_from_slice(b"WAVEfmt "); + wav.extend_from_slice(&16u32.to_le_bytes()); + wav.extend_from_slice(&1u16.to_le_bytes()); // PCM + wav.extend_from_slice(&2u16.to_le_bytes()); // stereo + wav.extend_from_slice(&48_000u32.to_le_bytes()); + wav.extend_from_slice(&(48_000u32 * 4).to_le_bytes()); // byte rate + wav.extend_from_slice(&4u16.to_le_bytes()); // block align + wav.extend_from_slice(&16u16.to_le_bytes()); // bits per sample + wav.extend_from_slice(b"data"); + wav.extend_from_slice(&data_len.to_le_bytes()); + wav.resize(44 + data_len as usize, 0); + wav + } + + /// The `node.name` prefix a node tagged by *this* process with `role` + /// must carry. Narrower than [`super::OWNED_NODE_NAME_PREFIX`] on + /// purpose: it pins the match to this test run, so a stray tagged node + /// left by anything else cannot make a gate pass. + pub fn expected_prefix(role: &str) -> String { + format!( + "{}{role}_{}", + super::OWNED_NODE_NAME_PREFIX, + std::process::id() + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::BTreeMap; + + /// The contract file, byte-identical to pixelpass's copy. + const FIXTURE: &str = include_str!("../../tests/fixtures/ownership-tag-contract.txt"); + + fn fixture() -> BTreeMap { + FIXTURE + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(|line| { + let (key, value) = line + .split_once('=') + .unwrap_or_else(|| panic!("fixture line is not key=value: {line:?}")); + (key.to_string(), value.to_string()) + }) + .collect() + } + + fn fixture_get(key: &str) -> String { + fixture() + .get(key) + .unwrap_or_else(|| panic!("fixture has no key {key:?}")) + .clone() + } + + /// Half of the cross-repo contract test: our constants are exactly what + /// the shared fixture pins. pixelpass runs the mirror of this against its + /// own byte-identical copy. + #[test] + fn contract_matches_fixture() { + assert_eq!(OWNED_PROP_KEY, fixture_get("prop_key")); + assert_eq!(OWNED_PROP_VALUE, fixture_get("prop_value")); + assert_eq!(OWNED_NODE_NAME_PREFIX, fixture_get("node_name_prefix")); + } + + /// The fixture's documented format and example must agree with the + /// generator, so a future reader can trust the example. + #[test] + fn fixture_example_is_well_formed() { + let example = fixture_get("node_name_example"); + assert!(example.starts_with(&fixture_get("node_name_prefix"))); + let format = fixture_get("node_name_format"); + assert_eq!(format, "peerspeak_owned__"); + // Same shape our generator emits: prefix, role, pid. + let generated = owned_node_name("mpv"); + let suffix = generated + .strip_prefix(OWNED_NODE_NAME_PREFIX) + .expect("generated name carries the prefix"); + let (role, pid) = suffix.split_once('_').expect("role_pid suffix"); + assert_eq!(role, "mpv"); + assert_eq!(pid.parse::().unwrap(), std::process::id()); + } + + /// The black-box half: build a real `Command` exactly as a call site + /// does, read back the environment it *would* carry across the fork, and + /// assert the property string pixelpass matches on is present in it. + /// + /// Deliberately inspects the `Command`, not our own formatter — a helper + /// that returns the right string but is wired into the wrong variable + /// would pass a formatter test and fail in the field. + #[test] + fn tagged_child_command_carries_both_carriers() { + let mut command = Command::new("mpv"); + tag_child(&mut command, "mpv"); + + let env: BTreeMap = command + .get_envs() + .map(|(key, value)| { + ( + key.to_string_lossy().into_owned(), + value + .expect("tagging sets values, never removals") + .to_string_lossy() + .into_owned(), + ) + }) + .collect(); + + let expected_name = owned_node_name("mpv"); + let prop = format!("{}={}", fixture_get("prop_key"), fixture_get("prop_value")); + + let pulse = env.get(PULSE_PROP_ENV).expect("PULSE_PROP set"); + assert!( + pulse.contains(&prop), + "PULSE_PROP carries carrier 1: {pulse}" + ); + assert!( + pulse.contains(&format!("node.name={expected_name}")), + "PULSE_PROP carries carrier 2: {pulse}" + ); + + let pw = env.get(PIPEWIRE_PROPS_ENV).expect("PIPEWIRE_PROPS set"); + assert!( + pw.contains(&format!( + "\"{}\" = \"{}\"", + fixture_get("prop_key"), + fixture_get("prop_value") + )), + "PIPEWIRE_PROPS carries carrier 1: {pw}" + ); + assert!( + pw.contains(&format!("\"node.name\" = \"{expected_name}\"")), + "PIPEWIRE_PROPS carries carrier 2: {pw}" + ); + assert!( + pw.starts_with('{') && pw.ends_with('}'), + "PIPEWIRE_PROPS is a SPA-JSON object: {pw}" + ); + } + + #[test] + fn every_generated_name_matches_the_prefix_pixelpass_looks_for() { + for role in ["call", "mpv", "vlc", "notify"] { + let name = owned_node_name(role); + assert!( + name.starts_with(OWNED_NODE_NAME_PREFIX), + "{name} must match the prefix" + ); + } + } + + #[test] + fn role_is_reduced_to_a_single_safe_token() { + // A space would split PULSE_PROP into a bogus second property. + assert_eq!(sanitize_role("my player"), "my-player"); + assert_eq!(sanitize_role("MPV"), "mpv"); + assert_eq!(sanitize_role("vlc/../.."), "vlc"); + assert_eq!(sanitize_role(""), "child"); + assert_eq!(sanitize_role("---"), "child"); + assert!(!owned_node_name("my player").contains(' ')); + } + + #[test] + fn pulse_prop_pairs_are_whitespace_separated_and_parse_back() { + let value = pulse_prop_value(&owned_node_name("notify")); + let pairs: BTreeMap<&str, &str> = value + .split_whitespace() + .map(|pair| pair.split_once('=').expect("key=value")) + .collect(); + assert_eq!(pairs.len(), 2, "exactly two properties: {value}"); + assert_eq!(pairs.get(OWNED_PROP_KEY), Some(&OWNED_PROP_VALUE)); + assert!( + pairs + .get("node.name") + .is_some_and(|name| name.starts_with(OWNED_NODE_NAME_PREFIX)) + ); + } +} diff --git a/src/audio/pipewire_impl.rs b/src/audio/pipewire_impl.rs index c3cd319..d5b04c0 100644 --- a/src/audio/pipewire_impl.rs +++ b/src/audio/pipewire_impl.rs @@ -1,3 +1,4 @@ +use crate::audio::ownership; use crate::audio::{AudioBackend, AudioError}; use pipewire as pw; use pw::{properties::properties, spa}; @@ -371,6 +372,11 @@ fn run_playback( mainloop_clone.quit(); }); + // Ownership tag, both carriers (`crate::audio::ownership`, plan §5.1). + // This is the node that carries the far end's voice, so it is the single + // most important thing for pixelpass to refuse to fan out: sharing it + // would send the call back to the person already speaking on it. + let owned_node_name = ownership::owned_node_name(ownership::NATIVE_PLAYBACK_ROLE); let mut props = properties! { *pw::keys::MEDIA_TYPE => "Audio", *pw::keys::MEDIA_CATEGORY => "Playback", @@ -379,6 +385,19 @@ fn run_playback( // buffer — the real fix is the explicit Buffers param below — but it // expresses the intended quantum for any node that honours it. *pw::keys::NODE_LATENCY => "1024/48000", + ownership::OWNED_PROP_KEY => ownership::OWNED_PROP_VALUE, + // Set explicitly rather than relying on the stream name passed to + // `StreamBox::new` below: props win over that name, and this one has + // to be exact. + *pw::keys::NODE_NAME => owned_node_name.as_str(), + // Measured: this stream sets neither `application.name` nor a + // description, so a mixer falls back to `node.name` — which the line + // above just turned into an internal identifier. The plan's rule is + // that the ownership prefix must not reach `node.description`; a + // human label there is what keeps that rule's *intent* (mixers stay + // readable) true for our own stream, exactly as mpv's own + // description does for the spawned players. + *pw::keys::NODE_DESCRIPTION => "PeerSpeak", }; if let Some(target) = target_node { props.insert("node.target", target); @@ -637,6 +656,62 @@ mod tests { use std::time::Duration; use std::{sync::mpsc, thread}; + /// Phase-1 exit gate, native-playback half (impl plan §3): the stream + /// that carries the far end's voice appears on the graph with **both** + /// ownership carriers, and still with the `Communication` media role. + /// + /// The third and most important of the three tagged paths — this is the + /// node whose audio, if fanned out, would send the call back to whoever + /// is speaking on it. + /// + /// Feeds silence, so the gate is inaudible. Live: needs PipeWire and + /// `pw-dump`. `cargo test --lib -- --ignored native_playback` + #[test] + #[ignore = "live: requires a running PipeWire daemon and pw-dump"] + fn native_playback_node_carries_both_ownership_carriers() { + use crate::audio::ownership::{self, live_test}; + use crate::audio::{AudioBackend, PLAYBACK_TARGET_SAMPLES}; + + let backend = super::PipeWireBackend::new(); + let (tx, rx) = mpsc::channel::>(); + let ring_fill = Arc::new(AtomicUsize::new(0)); + backend + .start_playback(rx, None, ring_fill.clone()) + .expect("playback starts"); + + // Keep the ring fed so the node stays live for the whole poll; the + // stream is created on connect, but a starved one is not a fair test + // of what a real call looks like on the graph. + let feeder = thread::spawn(move || { + let silence = vec![0i16; 960 * 2]; + for _ in 0..300 { + if ring_fill.load(Ordering::Relaxed) < PLAYBACK_TARGET_SAMPLES + && tx.send(silence.clone()).is_err() + { + return; + } + thread::sleep(Duration::from_millis(20)); + } + }); + + let prefix = live_test::expected_prefix(ownership::NATIVE_PLAYBACK_ROLE); + let found = live_test::poll_for_owned_node(&prefix, Duration::from_secs(5)); + let _ = backend.stop(); + let _ = feeder.join(); + + let (name, owned) = + found.unwrap_or_else(|| panic!("no live node named {prefix:?} appeared within 5s")); + assert!( + name.starts_with(ownership::OWNED_NODE_NAME_PREFIX), + "{name}" + ); + assert_eq!( + owned.as_deref(), + Some(ownership::OWNED_PROP_VALUE), + "carrier 1 must be on the live node, not just carrier 2" + ); + } + #[test] fn requested_in_range_is_honored() { // The graph's requested quantum is produced verbatim when it fits. diff --git a/src/notify.rs b/src/notify.rs index 1d7adcd..b88021f 100644 --- a/src/notify.rs +++ b/src/notify.rs @@ -10,6 +10,8 @@ //! leaves a zombie. Any failure (no player, no audio) is silent by design — a //! missing chime should never disrupt a call. +#[cfg(not(windows))] +use crate::audio::ownership; use std::collections::HashMap; use std::fs::OpenOptions; use std::io::Write; @@ -264,12 +266,18 @@ fn escape_powershell_single_quoted(s: &str) -> String { #[cfg(not(windows))] fn spawn_player(path: &Path) { for player in ["pw-play", "paplay", "aplay"] { - let started = Command::new(player) + let mut command = Command::new(player); + command .arg(path) .stdin(Stdio::null()) .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); + .stderr(Stdio::null()); + // Ownership tag (plan §5.1). A chime is short, but it is still our + // audio on the default sink, and an untagged one is an unowned root + // the exclusion engine would have to reason about from scratch. + // `aplay` talks to ALSA and ignores both variables; harmless. + ownership::tag_child(&mut command, ownership::NOTIFICATION_ROLE); + let started = command.status(); // `status()` errors only if the player binary isn't present; on a real // playback error it still returns (non-zero), so a started player ends // the loop either way — we don't want to double-play through fallbacks. @@ -313,6 +321,49 @@ mod tests { dir } + /// Phase-1 exit gate, notification half (impl plan §3): a chime peerspeak + /// actually plays produces a live PipeWire node carrying **both** + /// ownership carriers. + /// + /// ⚠️ Deliberately drives `play()`, not `tag_child()`. The unit test in + /// `audio::ownership` proves the environment is built correctly; only a + /// live run proves this module *uses* it and that the audio stack honours + /// it end to end. The chime is silent (a zero-filled WAV), so running it + /// never makes noise. + /// + /// Live: needs a running PipeWire daemon, `pw-play`/`paplay` and + /// `pw-dump`. `cargo test --lib -- --ignored notification_chime` + #[test] + #[ignore = "live: requires a running PipeWire daemon and pw-dump"] + #[cfg(not(windows))] + fn notification_chime_node_carries_both_ownership_carriers() { + use crate::audio::ownership::{self, live_test}; + + let dir = temp_wav_dir("ownership"); + let path = dir.join("silence.wav"); + std::fs::write(&path, live_test::silent_wav(6)).unwrap(); + + set_enabled(true); + set_sound_enabled(Sound::PeerJoin, true); + play(Sound::PeerJoin, Some(path.to_str().unwrap())); + + let prefix = live_test::expected_prefix(ownership::NOTIFICATION_ROLE); + let found = live_test::poll_for_owned_node(&prefix, std::time::Duration::from_secs(5)); + std::fs::remove_dir_all(&dir).ok(); + + let (name, owned) = + found.unwrap_or_else(|| panic!("no live node named {prefix:?} appeared within 5s")); + assert!( + name.starts_with(ownership::OWNED_NODE_NAME_PREFIX), + "{name}" + ); + assert_eq!( + owned.as_deref(), + Some(ownership::OWNED_PROP_VALUE), + "carrier 1 must be on the live node too, not just carrier 2" + ); + } + #[test] fn test_should_play_truth_table() { // Plays only when BOTH the master and the per-sound flag are on. diff --git a/src/screenshare/mod.rs b/src/screenshare/mod.rs index 2b7a247..839a6fe 100644 --- a/src/screenshare/mod.rs +++ b/src/screenshare/mod.rs @@ -21,6 +21,8 @@ use std::time::Duration; use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::{Child, Command}; +use crate::audio::ownership; + pub mod livesync; use crate::config::{ScreenShareSettings, ShareBuffering, SharePlayer, ShareQuality}; @@ -765,20 +767,69 @@ fn spawn_player(bin: &str, args: &[String], url: &str) -> std::io::Result // and is not needed to verify the flags. Logged on each attempt, so a // fallback from the preferred player to the other one is visible too. crate::log_msg(&format!("player spawn: {bin} {}", args.join(" "))); - Command::new(bin) + let mut command = Command::new(bin); + command .args(args) .arg(url) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) - .kill_on_drop(false) - .spawn() + .kill_on_drop(false); + // Ownership tag (plan §5.1): this player is playing the *incoming* + // screenshare's audio, so it is exactly what must not be fanned back out + // if this machine also starts sharing. The role is the player binary, so + // a `pw-dump` during a field test names which one produced the node. + ownership::tag_child(command.as_std_mut(), bin); + command.spawn() } #[cfg(test)] mod tests { use super::*; + /// Phase-1 exit gate, player half (impl plan §3): the mpv peerspeak + /// actually spawns produces a live node carrying **both** ownership + /// carriers, tagged with the player's own name as the role. + /// + /// ⚠️ Drives the real [`spawn_player`], for the same reason the notify + /// gate does: the plan requires the tag to be shown "landing on a live + /// mpv node, not just in the env". Plays a silent WAV, so it is quiet. + /// + /// Live: needs PipeWire, `mpv` and `pw-dump`. + /// `cargo test --lib -- --ignored spawned_player` + #[tokio::test] + #[ignore = "live: requires a running PipeWire daemon, mpv and pw-dump"] + async fn spawned_player_node_carries_both_ownership_carriers() { + use crate::audio::ownership::live_test; + + let dir = std::env::temp_dir().join(format!("peerspeak-playertest-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("silence.wav"); + std::fs::write(&path, live_test::silent_wav(6)).unwrap(); + + let mut child = spawn_player( + "mpv", + &["--no-video".to_string(), "--really-quiet".to_string()], + path.to_str().unwrap(), + ) + .expect("mpv spawns"); + + // The role is the player binary, so this also pins that the call site + // passes `bin` and not a fixed literal. + let prefix = live_test::expected_prefix("mpv"); + let found = live_test::poll_for_owned_node(&prefix, std::time::Duration::from_secs(5)); + let _ = child.kill().await; + std::fs::remove_dir_all(&dir).ok(); + + let (name, owned) = + found.unwrap_or_else(|| panic!("no live node named {prefix:?} appeared within 5s")); + assert!( + name.starts_with(ownership::OWNED_NODE_NAME_PREFIX), + "{name}" + ); + assert_eq!(owned.as_deref(), Some(ownership::OWNED_PROP_VALUE)); + } + #[test] fn viewer_args_guard_neutralizes_flag_like_ticket() { // A malicious "ticket" that looks like a flag must end up positional, diff --git a/tests/fixtures/ownership-tag-contract.txt b/tests/fixtures/ownership-tag-contract.txt new file mode 100644 index 0000000..471c413 --- /dev/null +++ b/tests/fixtures/ownership-tag-contract.txt @@ -0,0 +1,34 @@ +# Screenshare audio exclusion — ownership tagging wire contract. +# +# peerspeak PRODUCES these carriers on every audio node it owns; pixelpass +# CONSUMES them as the primary taint root of the exclusion engine. Neither +# repo depends on the other, so this file is the contract: it is committed +# byte-identical in both, and each repo has a test that asserts its own named +# constants (and, on the producer side, the environment a real child Command +# would carry) match these values exactly. +# +# peerspeak/tests/fixtures/ownership-tag-contract.txt +# pixelpass/tests/fixtures/ownership-tag-contract.txt +# +# Pinned by peerspeak docs/screenshare-audio-exclusion-impl-plan.md §3 and +# docs/screenshare-audio-exclusion-plan.md §5.1 (v3.5). Changing a value here +# is a cross-repo breaking change: both repos must land in the same session, +# and the phase 5 matrix must be re-run. +# +# Two carriers, matched as a UNION — a node is peerspeak-owned if EITHER +# matches. Round 8 added the second because a property is invisible to the +# PipeWire registry `global` event and readable only via a node bind, so the +# primary taint root must not rest on one observation mechanism alone. + +# Carrier 1 — a node property. The consumer treats any value other than +# "false"/"0" as truthy, which is the fail-closed direction; the producer +# always emits exactly this value. +prop_key=peerspeak.owned +prop_value=1 + +# Carrier 2 — a `node.name` prefix, announced by the registry without a bind. +# `node.description` is deliberately NOT touched, so mixers still show "mpv". +# Only the prefix is matched; the rest of the name is for diagnostics. +node_name_prefix=peerspeak_owned_ +node_name_format=peerspeak_owned__ +node_name_example=peerspeak_owned_mpv_31284 From d40385f85c8d5aacc1a7e09d8afce7aa77ab866e Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:07:08 -0400 Subject: [PATCH 02/13] notify: correct a measured claim about the aplay fallback The comment said aplay ignores PULSE_PROP/PIPEWIRE_PROPS. Measured: it reaches the graph through PipeWire's ALSA plugin and carries both carriers exactly like pw-play and paplay. Comment only. Co-Authored-By: Claude Opus 5 --- src/notify.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/notify.rs b/src/notify.rs index b88021f..f968cfb 100644 --- a/src/notify.rs +++ b/src/notify.rs @@ -275,7 +275,9 @@ fn spawn_player(path: &Path) { // Ownership tag (plan §5.1). A chime is short, but it is still our // audio on the default sink, and an untagged one is an unowned root // the exclusion engine would have to reason about from scratch. - // `aplay` talks to ALSA and ignores both variables; harmless. + // Measured on this host: all three fallbacks tag correctly, `aplay` + // included — it reaches the graph through PipeWire's ALSA plugin, + // which honours `PIPEWIRE_PROPS` like any other client. ownership::tag_child(&mut command, ownership::NOTIFICATION_ROLE); let started = command.status(); // `status()` errors only if the player binary isn't present; on a real From 503f78153b7e116fa17ae1c7162e9e75d441157a Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:25:27 -0400 Subject: [PATCH 03/13] audio/ownership: refuse an ambiguous contract fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Producer half of the same fix (Codex phase-1 review, finding 3, P2). This side collected fixture lines into a map, so a duplicated key silently took the last value while pixelpass took the first — both repos green on different contracts. Mutation-verified in both repos with a duplicated `prop_value`. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index a698ba5..90df079 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -234,7 +234,15 @@ mod tests { /// The contract file, byte-identical to pixelpass's copy. const FIXTURE: &str = include_str!("../../tests/fixtures/ownership-tag-contract.txt"); + /// ⚠️ **Rejects duplicate keys, and that is the point** (Codex phase-1 + /// review, finding 3). This side collected into a map, so a duplicate + /// silently took the *last* value; pixelpass's half searches a list and + /// takes the *first*. A byte-identical fixture with a duplicated key + /// could therefore leave both suites green while the two repos had + /// selected different contracts — the exact drift this file exists to + /// prevent. Both sides now refuse the ambiguity instead of resolving it. fn fixture() -> BTreeMap { + let mut seen: Vec<&str> = Vec::new(); FIXTURE .lines() .map(str::trim) @@ -243,6 +251,11 @@ mod tests { let (key, value) = line .split_once('=') .unwrap_or_else(|| panic!("fixture line is not key=value: {line:?}")); + assert!( + !seen.contains(&key), + "fixture defines {key:?} twice; the two repos would disagree on which wins" + ); + seen.push(key); (key.to_string(), value.to_string()) }) .collect() From b5569fe2c65220e1045efea1804646ceee1a6f5f Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:52:22 -0400 Subject: [PATCH 04/13] audio: tag the fourth playback path, rodio's ClipPlayer ClipPlayer opens a rodio default sink, which on Linux reaches the graph through PipeWire's ALSA plugin. It was untagged through all of phase 1, and it is a real echo path: B broadcasts music, A tunes in, A shares their desktop, B hears their own track played back at them. Confirmed live as `alsa_playback.peerspeak-...` with no ownership properties. rodio exposes no way to set PipeWire node properties, so the carrier is PIPEWIRE_ALSA, set once at the top of main while still single-threaded. Measured, with PIPEWIRE_PROPS and PULSE_PROP unset, to establish that setting it process-wide is safe: - aplay (ALSA plugin) -> both carriers land. Confirms the mechanism. - pw-play (native) -> untouched. Our own call-playback and capture streams are native, so they keep their own explicit tagging and are unaffected. - arecord (ALSA capture)-> IS tagged, on a Stream/Input/Audio. Not surgical in the role dimension; harmless only because R10-1 honours the carriers on producers alone. This is why R10-1 lands first. Local playlist tracks are tagged too, not just inbound peer audio. A local track is already broadcast to peers over the call on the same keypress, so sharing it again through the screen share would send the far end two copies at differing latency. That is a defect, not a feature. Codex phase-1 review F1. Round 10, R10-2. New live exit-gate row drives the real ClipPlayer; mutation-verified (drop the tag -> no node within 5s). The wiring guard is mutation- verified too, and its first version was WRONG: it searched raw source and passed against a main with the call deleted, because the comment above it named the function. It strips comments now. Co-Authored-By: Claude Opus 5 --- src/audio/clip_player.rs | 50 ++++++++++++++ src/audio/ownership.rs | 146 +++++++++++++++++++++++++++++++++++++++ src/main.rs | 16 +++++ 3 files changed, 212 insertions(+) diff --git a/src/audio/clip_player.rs b/src/audio/clip_player.rs index 6ee3ba4..fcd0492 100644 --- a/src/audio/clip_player.rs +++ b/src/audio/clip_player.rs @@ -328,4 +328,54 @@ mod tests { assert_eq!(seek_target(-1.0, total), Duration::ZERO); assert_eq!(seek_target(2.0, total), total); } + + /// **The fourth playback path's exit gate (round 10, R10-2).** Drives a + /// real [`ClipPlayer`] — the same object the app uses for chat clips, peer + /// music and the local playlist — and asserts the node it puts on the + /// graph carries both ownership carriers. + /// + /// This path was untagged through all of phase 1, which is a real echo: + /// B broadcasts music, A tunes in, A shares their desktop, B hears their + /// own track. It was missed because phase 1 worked from the impl plan's + /// list of three playback sites and that list was incomplete — so this + /// gate drives the *player*, not the tagging helper. + /// + /// ⚠️ **Run alone**: it sets a process-wide environment variable, which is + /// only sound single-threaded. In production `main` does this before + /// anything is spawned; a test binary has no such guarantee, hence + /// `--test-threads=1`. + /// + /// `cargo test --lib -- --ignored --test-threads=1 clip_player_node` + #[test] + #[ignore = "live: requires a running PipeWire daemon and pw-dump; run with --test-threads=1"] + fn clip_player_node_carries_both_ownership_carriers() { + use crate::audio::ownership::{self, live_test}; + + // SAFETY: `--test-threads=1` is documented above and in the ignore + // reason; this is the same call `main` makes, exercised for real + // rather than reimplemented, so the gate cannot pass against a + // formatter that production never uses. + unsafe { ownership::tag_this_process_alsa_audio() }; + + let (player, _status) = ClipPlayer::new(1.0); + // Six seconds of silence: long enough for the poll, inaudible. + player.play([0u8; 32], live_test::silent_wav(6)); + + let prefix = live_test::expected_prefix(ownership::CLIP_ROLE); + let found = live_test::poll_for_owned_node(&prefix, Duration::from_secs(5)); + player.stop(); + + let (name, owned) = found.unwrap_or_else(|| { + panic!("no live clip-player node named {prefix:?} appeared within 5s") + }); + assert!( + name.starts_with(ownership::OWNED_NODE_NAME_PREFIX), + "{name}" + ); + assert_eq!( + owned.as_deref(), + Some(ownership::OWNED_PROP_VALUE), + "carrier 1 must be on the live node, not just carrier 2" + ); + } } diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 90df079..740132e 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -60,6 +60,46 @@ pub const PULSE_PROP_ENV: &str = "PULSE_PROP"; /// (`pw-play`, mpv on `--ao=pipewire`) receives extra stream properties. pub const PIPEWIRE_PROPS_ENV: &str = "PIPEWIRE_PROPS"; +/// The environment variable read **only by PipeWire's ALSA plugin**, and so +/// the one that reaches audio played through ALSA — which on Linux is how +/// `rodio`/`cpal` reach the graph (round 10, R10-2). +/// +/// This is the *in-process* carrier: unlike [`PULSE_PROP_ENV`] and +/// [`PIPEWIRE_PROPS_ENV`], which are set per-child on a [`Command`], this one +/// is set process-wide in `main` because the audio it tags is played by a +/// library inside this process, with no fork to hang properties on. +/// +/// **Measured 2026-07-25**, with `PIPEWIRE_PROPS` and `PULSE_PROP` explicitly +/// unset, to establish that it is surgical enough to set process-wide: +/// +/// - `aplay` (ALSA plugin) ⇒ node carries **both** carriers. ✅ +/// - `pw-play` (PipeWire-native) ⇒ `node.name=pw-play`, `peerspeak.owned` +/// absent. ✅ So our own native call-playback and capture streams, which go +/// through `pipewire_impl`, are **not** affected by this variable; they keep +/// being tagged explicitly at their own site. +/// - `arecord` (ALSA plugin, capture) ⇒ **is** tagged, on a +/// `Stream/Input/Audio`. ⚠️ Not surgical in the *role* dimension. Harmless +/// only because pixelpass honours the carriers on `Stream/Output/Audio` +/// alone (R10-1) — which is why R10-1 lands first. Nothing in peerspeak +/// captures via ALSA on Linux today (the Linux backend is native PipeWire; +/// `cpal` is the Windows backend), so this is a guard against a future +/// caller, not a live condition. +pub const PIPEWIRE_ALSA_ENV: &str = "PIPEWIRE_ALSA"; + +/// Role of in-process audio played through `rodio`: received chat clips, peer +/// music, and locally chosen playlist tracks (round 10, R10-2). +/// +/// **All three are tagged, deliberately**, including the local playlist. The +/// case for exempting locally chosen music is that the user picked it and may +/// want it shared; the case against is stronger. A local track played from the +/// playlist is *already being broadcast to peers over the call* on the same +/// keypress (`app::play_local_track` → `broadcast_track`), so sharing it a +/// second time through the screen-share sends the far end two copies of the +/// same audio, offset by the two paths' differing latency. That is not a lost +/// feature, it is a defect. A user who wants music in the share can play it in +/// any other application, which peerspeak never tags. +pub const CLIP_ROLE: &str = "clip"; + /// Role of the native call-playback stream — the node carrying the far end's /// voice. pub const NATIVE_PLAYBACK_ROLE: &str = "call"; @@ -135,6 +175,46 @@ pub fn tag_child(command: &mut Command, role: &str) { command.env(PIPEWIRE_PROPS_ENV, pipewire_props_value(&node_name)); } +/// Tag the audio this process plays **through ALSA** — in practice everything +/// `rodio` plays, which is [`crate::audio::clip_player::ClipPlayer`]: received +/// chat clips, peer music, and local playlist tracks. +/// +/// `ClipPlayer` opens a `rodio` default sink, which on Linux reaches the graph +/// through PipeWire's ALSA plugin. It is a genuine fourth playback path and it +/// was untagged until round 10, which is a real echo: B broadcasts music, A +/// tunes in, A shares their desktop, and B hears their own track played back +/// at them. Found by Codex's phase-1 review; confirmed live as +/// `alsa_playback.peerspeak-…` with no ownership properties at all. +/// +/// # Why an environment variable and not a property on the stream +/// +/// `rodio` exposes no way to set PipeWire node properties — it is an ALSA +/// consumer several layers down. The alternative fix is to rebuild `ClipPlayer` +/// on peerspeak's own PipeWire backend, which is a large change for an +/// identical outcome. [`PIPEWIRE_ALSA_ENV`] reaches exactly the ALSA path and +/// nothing else (measured — see the constant). +/// +/// # Must be called before any thread starts +/// +/// `std::env::set_var` is `unsafe` in edition 2024 because a concurrent +/// `getenv` in another thread is a data race. The only safe moment is the top +/// of `main`, before anything is spawned; that is also correct on the merits, +/// since the plugin reads the variable when a stream is opened. +/// +/// Inherited values are **replaced**. Unlike the `Command` carriers (R10-5), +/// there is nothing to preserve: no user sets `PIPEWIRE_ALSA` to route +/// peerspeak's own clip playback, and honouring one would defeat the tag. +/// +/// # Safety +/// +/// The caller must guarantee no other thread exists in this process. +pub unsafe fn tag_this_process_alsa_audio() { + let node_name = owned_node_name(CLIP_ROLE); + // SAFETY: the caller's obligation, discharged by calling this at the top + // of `main` before any thread is spawned. + unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, pipewire_props_value(&node_name)) }; +} + /// Live-test support for the phase-1 exit gate, shared by the two modules /// that spawn tagged players (`notify`, `screenshare`). /// @@ -353,6 +433,72 @@ mod tests { ); } + /// R10-2's in-process carrier, checked as a *value*, not by setting the + /// variable: mutating the process environment from a test races every + /// other test in the binary. That the value reaches `PIPEWIRE_ALSA` at all + /// is what the live gate below proves, and only a live gate can. + #[test] + fn the_alsa_carrier_value_carries_both_carriers() { + let name = owned_node_name(CLIP_ROLE); + let value = pipewire_props_value(&name); + + assert_eq!(PIPEWIRE_ALSA_ENV, "PIPEWIRE_ALSA"); + assert!( + value.contains(&format!( + "\"{}\" = \"{}\"", + fixture_get("prop_key"), + fixture_get("prop_value") + )), + "carrier 1: {value}" + ); + assert!( + value.contains(&format!("\"node.name\" = \"{name}\"")), + "carrier 2: {value}" + ); + assert!(name.starts_with(&fixture_get("node_name_prefix"))); + // The role is a distinct token, so an audit can tell clip audio from + // the call stream and from a spawned player. + assert!(name.contains("_clip_"), "role token present: {name}"); + } + + /// The in-process carrier only works if `main` actually sets it, and the + /// live gate cannot prove that half: a test binary has no `main`, so it + /// must call the tagger itself. That leaves exactly one way to regress — + /// delete the line from `main` — and this is the cheapest thing that + /// catches it. + /// + /// A source-text assertion is crude. It is also the *only* check available + /// short of driving the real GUI binary, and phase 1's miss was precisely + /// a call site nobody verified existed. + #[test] + fn main_tags_this_process_before_anything_starts() { + const MAIN: &str = include_str!("../main.rs"); + + // ⚠️ Comments are stripped first, and that is not tidiness. The first + // version of this test searched the raw source and **passed against a + // `main` with the call deleted**, because the explanatory comment + // above it still named the function. A gate satisfiable by two sources + // gates neither (the phase-3r lesson); here the prose was the second + // source. Caught by mutation, which is the only reason it is not still + // green and worthless. + let code: String = MAIN + .lines() + .map(|line| line.split("//").next().unwrap_or("")) + .collect::>() + .join("\n"); + + let call = code + .find("tag_this_process_alsa_audio()") + .expect("main must call tag_this_process_alsa_audio (round 10, R10-2)"); + let gui = code + .find("run_gui") + .expect("main runs the GUI; this test's ordering check assumes it"); + assert!( + call < gui, + "the tag must be set before the GUI starts any thread" + ); + } + #[test] fn every_generated_name_matches_the_prefix_pixelpass_looks_for() { for role in ["call", "mpv", "vlc", "notify"] { diff --git a/src/main.rs b/src/main.rs index 9a42670..270523c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,6 +3,22 @@ #![cfg_attr(not(debug_assertions), windows_subsystem = "windows")] fn main() { + // Tag the audio we play through ALSA (rodio's `ClipPlayer`: chat clips, + // peer music, local playlist tracks) so the screen-share exclusion engine + // can recognise it as ours and refuse to fan it back to the far end. + // + // First statement in the program, and that is load-bearing: this sets an + // environment variable, which is only sound while the process is still + // single-threaded, and PipeWire's ALSA plugin reads it when a stream is + // opened. See `audio::ownership::tag_this_process_alsa_audio`. + // + // SAFETY: nothing has been spawned yet, so no thread can be reading the + // environment concurrently. + #[cfg(target_os = "linux")] + unsafe { + peerspeak::audio::ownership::tag_this_process_alsa_audio() + }; + if let Err(e) = peerspeak::app::run_gui() { eprintln!("Error running GUI: {:?}", e); } From e7923a1b5c7e7227619239b9ceb6f3b83938fa6e Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 21:03:32 -0400 Subject: [PATCH 05/13] audio/ownership: merge inherited player env vars instead of clobbering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PULSE_PROP and PIPEWIRE_PROPS can legitimately carry a user's own routing policy — media.role, a target sink — and replacing them changes where the user's audio goes as a side effect of a tagging mechanism that is supposed to be behaviourally invisible. PULSE_PROP is space-separated key=value, so merging is appending; PIPEWIRE_PROPS is a SPA-JSON object, so it is an insert before the closing brace. Our pairs go last in both, so they win a duplicate key — without that, a user with node.name already set would silently untag us. A value that does not match the expected shape is logged and overwritten: a half-merged string that fails to parse would drop the tag silently, which is worse than losing a routing preference. No full SPA-JSON parser, which would be over-engineering for a case with no live consumer (measured: neither variable is set anywhere in this user's env or config). Also sets PIPEWIRE_ALSA on the child, with the child's own role. A player configured for ALSA output is reached by neither of the other two variables, so this closes a real gap rather than only a cosmetic one — and without it such a child would inherit this process's `clip` tag from tag_this_process_alsa_audio and report the wrong role in the audit. Corrects a stale doc comment on OWNED_PROP_VALUE that still claimed pixelpass accepts any truthy value; R10-4 made the match exact. Codex's F5 was reasoned partly from a stale comment of mine, so these are worth fixing on sight. Codex phase-1 review F4. Round 10, R10-5. 8 new rows; 5 mutations verified (clobber PULSE_PROP, our pairs first, naive object concat, doubled trailing comma, drop the ALSA carrier). All 4 live ownership gates re-run green. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 246 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 235 insertions(+), 11 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 740132e..ffe5c4a 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -44,8 +44,14 @@ use std::process::Command; /// after binding the node — see the module docs. pub const OWNED_PROP_KEY: &str = "peerspeak.owned"; -/// Carrier 1, value. pixelpass treats anything other than `false`/`0` as -/// truthy (fail-closed), but we always emit exactly this. +/// Carrier 1, value. +/// +/// ⚠️ **pixelpass matches this EXACTLY** (its round-10 R10-4). An earlier +/// version of this comment said it accepted anything other than `false`/`0`; +/// that leniency was removed, because it let any process suppress a rival +/// app's audio from the share with a property it did not have to spell right. +/// Emitting any other value here now means the node is not owned on carrier 1 +/// — only carrier 2's `node.name` prefix would still catch it. pub const OWNED_PROP_VALUE: &str = "1"; /// Carrier 2. The `node.name` prefix pixelpass matches; the remainder of the @@ -161,18 +167,112 @@ fn pipewire_props_value(node_name: &str) -> String { /// Tag a child process that will play audio, so its resulting node carries /// both ownership carriers. /// -/// Both environment variables are set unconditionally, because we cannot know -/// from here which audio API the child will pick (mpv chooses at runtime, and -/// VLC and `paplay` differ from `pw-play`). Setting the one the child ignores -/// costs nothing. +/// All three environment variables are set unconditionally, because we cannot +/// know from here which audio API the child will pick: mpv chooses at runtime, +/// VLC and `paplay` differ from `pw-play`, and either may be configured for +/// ALSA output. Setting the ones the child ignores costs nothing. /// -/// Any inherited value of either variable is **replaced**, not appended: the -/// tag is a correctness mechanism, and a half-merged property string that -/// fails to parse would drop it silently. +/// An inherited value is **merged** rather than replaced (round 10, R10-5). +/// Both variables can legitimately carry a user's own routing policy — +/// `media.role`, a target sink — and clobbering it changes where the user's +/// audio goes as a side effect of a tagging mechanism that is supposed to be +/// behaviourally invisible. Our pairs go **last**, so they win any duplicate +/// key. An inherited value that does not match the expected shape is logged +/// and overwritten: a half-merged string that fails to parse would drop the +/// tag silently, which is the one outcome worse than losing the user's +/// routing preference. +/// +/// ⚠️ Reachability, measured 2026-07-25: neither variable is set anywhere in +/// this user's environment or configuration, so this is a correctness +/// property with no live consumer today. pub fn tag_child(command: &mut Command, role: &str) { let node_name = owned_node_name(role); - command.env(PULSE_PROP_ENV, pulse_prop_value(&node_name)); - command.env(PIPEWIRE_PROPS_ENV, pipewire_props_value(&node_name)); + command.env( + PULSE_PROP_ENV, + merge_pulse_prop(inherited(PULSE_PROP_ENV).as_deref(), &node_name), + ); + let pipewire_props = merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name); + // The ALSA carrier takes the same SPA-JSON grammar and is set here too, so + // a child configured for ALSA output — which neither of the other two + // variables reaches — is tagged with *its own* role rather than inheriting + // this process's `clip` tag from `tag_this_process_alsa_audio`. + command.env(PIPEWIRE_ALSA_ENV, &pipewire_props); + command.env(PIPEWIRE_PROPS_ENV, pipewire_props); +} + +/// This process's value for `key`, which is what a child would inherit. +/// Non-UTF-8 reads as absent — we cannot merge into bytes we cannot parse, and +/// the overwrite path is where that belongs. +fn inherited(key: &str) -> Option { + match std::env::var(key) { + Ok(value) if !value.trim().is_empty() => Some(value), + Ok(_) => None, + Err(std::env::VarError::NotPresent) => None, + Err(std::env::VarError::NotUnicode(_)) => { + crate::log_msg(&format!( + "ownership: inherited {key} is not valid UTF-8; overwriting it with the ownership tag" + )); + None + } + } +} + +/// `PULSE_PROP` is whitespace-separated `key=value`, so merging is appending. +/// +/// The shape check is every token containing `=`. A token without one is not +/// something we can append to safely — PulseAudio's parser would take the +/// whole string as malformed and we would lose the tag with it. +fn merge_pulse_prop(inherited: Option<&str>, node_name: &str) -> String { + let ours = pulse_prop_value(node_name); + let Some(existing) = inherited else { + return ours; + }; + if existing.split_whitespace().all(|pair| pair.contains('=')) { + format!("{} {ours}", existing.trim()) + } else { + crate::log_msg(&format!( + "ownership: inherited {PULSE_PROP_ENV} is not space-separated key=value \ + ({existing:?}); overwriting it with the ownership tag" + )); + ours + } +} + +/// `PIPEWIRE_PROPS` is a SPA-JSON object, so merging is inserting our pairs +/// before the closing brace. +/// +/// The shape check is the outer braces. Anything else — a bare `key = value` +/// list, a truncated object — is left alone and overwritten rather than +/// guessed at; a full SPA-JSON parser is not worth writing for a case with no +/// live consumer. +fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String { + let ours = pipewire_props_value(node_name); + let Some(existing) = inherited else { + return ours; + }; + let trimmed = existing.trim(); + let inner = trimmed + .strip_prefix('{') + .and_then(|rest| rest.strip_suffix('}')) + .map(str::trim); + match inner { + // An empty object carries nothing to preserve. + Some("") => ours, + Some(inner) => { + let separator = if inner.ends_with(',') { "" } else { "," }; + format!( + "{{ {inner}{separator} \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ + \"node.name\" = \"{node_name}\" }}" + ) + } + None => { + crate::log_msg(&format!( + "ownership: inherited {PIPEWIRE_PROPS_ENV} is not a SPA-JSON object \ + ({existing:?}); overwriting it with the ownership tag" + )); + ours + } + } } /// Tag the audio this process plays **through ALSA** — in practice everything @@ -499,6 +599,130 @@ mod tests { ); } + /// R10-5: a user's inherited `PULSE_PROP` survives, and our pairs go last + /// so they win any duplicate key. + /// + /// Tested on the merge functions rather than through `tag_child`, because + /// `tag_child` reads the *process* environment and a test that set it + /// would race every other test in the binary. + #[test] + fn pulse_prop_merges_rather_than_clobbering() { + let name = owned_node_name("mpv"); + let merged = merge_pulse_prop(Some("media.role=music target.object=my-sink"), &name); + + assert!( + merged.contains("media.role=music") && merged.contains("target.object=my-sink"), + "the user's routing policy survives: {merged}" + ); + assert!(merged.contains(&format!("{OWNED_PROP_KEY}={OWNED_PROP_VALUE}"))); + assert!(merged.contains(&format!("node.name={name}"))); + + // Still parses as whitespace-separated key=value, with ours last. + let pairs: Vec<(&str, &str)> = merged + .split_whitespace() + .map(|pair| pair.split_once('=').expect("key=value")) + .collect(); + assert_eq!(pairs.len(), 4, "{merged}"); + assert_eq!(pairs[pairs.len() - 1].0, "node.name"); + } + + /// Ours must win a key the inherited value already sets, or a user with + /// `node.name` in their environment would silently untag us. + #[test] + fn our_pulse_prop_pairs_win_a_duplicate_key() { + let name = owned_node_name("mpv"); + let merged = merge_pulse_prop(Some("node.name=theirs"), &name); + let last_name = merged + .split_whitespace() + .filter_map(|pair| pair.split_once('=')) + .rfind(|(key, _)| *key == "node.name") + .expect("a node.name pair"); + assert_eq!(last_name.1, name, "ours must come last: {merged}"); + } + + #[test] + fn a_malformed_pulse_prop_is_overwritten_not_half_merged() { + let name = owned_node_name("mpv"); + // No `=` in the token: not something we can safely append to. + let merged = merge_pulse_prop(Some("garbage-without-equals"), &name); + assert_eq!(merged, pulse_prop_value(&name)); + assert!(!merged.contains("garbage")); + } + + #[test] + fn pipewire_props_merges_inside_the_spa_json_object() { + let name = owned_node_name("vlc"); + let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\" }"), &name); + + assert!(merged.starts_with('{') && merged.ends_with('}'), "{merged}"); + assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); + assert!(merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); + assert!(merged.contains(&format!("\"node.name\" = \"{name}\""))); + // Exactly one object, not two concatenated. + assert_eq!(merged.matches('{').count(), 1, "{merged}"); + assert_eq!(merged.matches('}').count(), 1, "{merged}"); + } + + /// A trailing comma in the inherited object must not become a double + /// comma, which SPA-JSON would reject — taking our tag down with it. + #[test] + fn pipewire_props_merge_does_not_double_a_trailing_comma() { + let name = owned_node_name("vlc"); + let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\", }"), &name); + assert!(!merged.contains(",,"), "{merged}"); + assert!(!merged.contains(", ,"), "{merged}"); + assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); + } + + #[test] + fn an_empty_or_malformed_pipewire_props_yields_our_value_alone() { + let name = owned_node_name("vlc"); + assert_eq!( + merge_pipewire_props(Some("{ }"), &name), + pipewire_props_value(&name) + ); + // Not an object: overwritten rather than guessed at. + assert_eq!( + merge_pipewire_props(Some("media.role = music"), &name), + pipewire_props_value(&name) + ); + assert_eq!( + merge_pipewire_props(None, &name), + pipewire_props_value(&name) + ); + } + + /// A spawned player configured for ALSA output is reached by neither + /// `PULSE_PROP` nor `PIPEWIRE_PROPS`. `tag_child` therefore sets the ALSA + /// carrier too — with the *child's* role, so it does not inherit this + /// process's `clip` tag from `tag_this_process_alsa_audio`. + #[test] + fn tagged_child_command_also_carries_the_alsa_carrier_with_its_own_role() { + let mut command = Command::new("mpv"); + tag_child(&mut command, "mpv"); + + let env: BTreeMap = command + .get_envs() + .map(|(key, value)| { + ( + key.to_string_lossy().into_owned(), + value + .expect("tagging sets values, never removals") + .to_string_lossy() + .into_owned(), + ) + }) + .collect(); + + let alsa = env.get(PIPEWIRE_ALSA_ENV).expect("PIPEWIRE_ALSA set"); + assert!( + alsa.contains(&format!("\"node.name\" = \"{}\"", owned_node_name("mpv"))), + "the child's own role, not this process's clip role: {alsa}" + ); + assert!(!alsa.contains(&format!("_{CLIP_ROLE}_")), "{alsa}"); + assert!(alsa.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); + } + #[test] fn every_generated_name_matches_the_prefix_pixelpass_looks_for() { for role in ["call", "mpv", "vlc", "notify"] { From d63db683186f13ec80b82be0f2e8f161992d6aad Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 21:07:33 -0400 Subject: [PATCH 06/13] audio/ownership: apply the merge rule to the ALSA carrier too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification round on round 10's own fixes, not on the next layer. R10-5 preserved a user's PULSE_PROP and PIPEWIRE_PROPS but tag_this_process_alsa_audio still clobbered their PIPEWIRE_ALSA, which is the same kind of routing policy and deserves the same treatment. Both it and tag_child now merge. MEASURED, rather than assumed, because "our pairs go last so they win" was load-bearing for the whole merge design and was never checked: PIPEWIRE_PROPS='{ "node.name"="theirs_first", "media.role"="music", "node.name"="ours_last" }' on pw-play -> node.name=ours_last, media.role preserved. The PULSE_PROP equivalent on paplay -> the same. So last-wins holds on both grammars: a user who already sets node.name cannot silently untag us, and their other keys survive. That also makes tag_child's ALSA carrier merge from the inherited value safely: in production main has already put this process's `clip` tag there, and the child's own role now overrides it by coming last. The existing row could not see this — the test binary never runs main, so it only ever exercised the merge-into-nothing case. Added a row that drives the real shape directly. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 81 ++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index ffe5c4a..11e0a7f 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -173,16 +173,22 @@ fn pipewire_props_value(node_name: &str) -> String { /// ALSA output. Setting the ones the child ignores costs nothing. /// /// An inherited value is **merged** rather than replaced (round 10, R10-5). -/// Both variables can legitimately carry a user's own routing policy — +/// All three can legitimately carry a user's own routing policy — /// `media.role`, a target sink — and clobbering it changes where the user's /// audio goes as a side effect of a tagging mechanism that is supposed to be -/// behaviourally invisible. Our pairs go **last**, so they win any duplicate -/// key. An inherited value that does not match the expected shape is logged -/// and overwritten: a half-merged string that fails to parse would drop the -/// tag silently, which is the one outcome worse than losing the user's -/// routing preference. +/// behaviourally invisible. An inherited value that does not match the +/// expected shape is logged and overwritten: a half-merged string that fails +/// to parse would drop the tag silently, which is the one outcome worse than +/// losing the user's routing preference. /// -/// ⚠️ Reachability, measured 2026-07-25: neither variable is set anywhere in +/// ✅ **Our pairs go last, and last wins — measured 2026-07-25**, not assumed. +/// `PIPEWIRE_PROPS='{ "node.name" = "theirs_first", "media.role" = "music", +/// "node.name" = "ours_last" }'` on `pw-play` produced `node.name=ours_last` +/// with `media.role` preserved, and the `PULSE_PROP` equivalent on `paplay` +/// did the same. So a user who already sets `node.name` cannot silently +/// untag us, and their other keys survive. +/// +/// ⚠️ Reachability, measured 2026-07-25: none of the three is set anywhere in /// this user's environment or configuration, so this is a correctness /// property with no live consumer today. pub fn tag_child(command: &mut Command, role: &str) { @@ -191,13 +197,23 @@ pub fn tag_child(command: &mut Command, role: &str) { PULSE_PROP_ENV, merge_pulse_prop(inherited(PULSE_PROP_ENV).as_deref(), &node_name), ); - let pipewire_props = merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name); + command.env( + PIPEWIRE_PROPS_ENV, + merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name), + ); // The ALSA carrier takes the same SPA-JSON grammar and is set here too, so // a child configured for ALSA output — which neither of the other two - // variables reaches — is tagged with *its own* role rather than inheriting - // this process's `clip` tag from `tag_this_process_alsa_audio`. - command.env(PIPEWIRE_ALSA_ENV, &pipewire_props); - command.env(PIPEWIRE_PROPS_ENV, pipewire_props); + // variables reaches — is tagged at all, and with *its own* role. + // + // Merged from the inherited `PIPEWIRE_ALSA`, which by now holds this + // process's own `clip` tag from [`tag_this_process_alsa_audio`] plus + // whatever the user set. The duplicate `node.name` that produces is + // resolved in our favour by the measured last-wins rule above, so the + // child gets its own role rather than inheriting `clip`. + command.env( + PIPEWIRE_ALSA_ENV, + merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name), + ); } /// This process's value for `key`, which is what a child would inherit. @@ -301,18 +317,20 @@ fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String { /// of `main`, before anything is spawned; that is also correct on the merits, /// since the plugin reads the variable when a stream is opened. /// -/// Inherited values are **replaced**. Unlike the `Command` carriers (R10-5), -/// there is nothing to preserve: no user sets `PIPEWIRE_ALSA` to route -/// peerspeak's own clip playback, and honouring one would defeat the tag. +/// An inherited value is **merged**, on the same reasoning as [`tag_child`]'s +/// (R10-5): a user's `PIPEWIRE_ALSA` can carry routing policy, and this +/// function is not entitled to move their audio as a side effect of tagging +/// ours. Ours goes last and last wins (measured — see [`tag_child`]). /// /// # Safety /// /// The caller must guarantee no other thread exists in this process. pub unsafe fn tag_this_process_alsa_audio() { let node_name = owned_node_name(CLIP_ROLE); + let value = merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name); // SAFETY: the caller's obligation, discharged by calling this at the top // of `main` before any thread is spawned. - unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, pipewire_props_value(&node_name)) }; + unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, value) }; } /// Live-test support for the phase-1 exit gate, shared by the two modules @@ -717,12 +735,39 @@ mod tests { let alsa = env.get(PIPEWIRE_ALSA_ENV).expect("PIPEWIRE_ALSA set"); assert!( alsa.contains(&format!("\"node.name\" = \"{}\"", owned_node_name("mpv"))), - "the child's own role, not this process's clip role: {alsa}" + "the child's own role: {alsa}" ); - assert!(!alsa.contains(&format!("_{CLIP_ROLE}_")), "{alsa}"); assert!(alsa.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); } + /// ⚠️ In production `main` has already set `PIPEWIRE_ALSA` to this + /// process's `clip` tag by the time a player is spawned, so `tag_child` + /// merges into *that*, not into nothing. The test binary has no such + /// value, so the row above never exercises the real shape — this one does, + /// on the merge function directly. + /// + /// The child's `node.name` must come last, because last wins (measured): + /// otherwise a spawned mpv would report itself as `clip` and the audit + /// could not tell the incoming screenshare's audio from a chat clip. + #[test] + fn a_childs_role_overrides_the_inherited_clip_tag_in_the_alsa_carrier() { + let clip = pipewire_props_value(&owned_node_name(CLIP_ROLE)); + let child = owned_node_name("mpv"); + let merged = merge_pipewire_props(Some(&clip), &child); + + let last_name = merged + .rfind("\"node.name\" = ") + .expect("a node.name pair in the merged value"); + assert!( + merged[last_name..].contains(&child), + "the child's role must be the last node.name: {merged}" + ); + // Both are still peerspeak-owned, so the duplicate is harmless. + assert!(merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); + assert!(merged.starts_with('{') && merged.ends_with('}'), "{merged}"); + assert_eq!(merged.matches('{').count(), 1, "one object only: {merged}"); + } + #[test] fn every_generated_name_matches_the_prefix_pixelpass_looks_for() { for role in ["call", "mpv", "vlc", "notify"] { From 6fc55a286d33fe084fceac8f339de97a80a391a1 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 23:47:32 -0400 Subject: [PATCH 07/13] audio/ownership: parse inherited SPA-JSON instead of trusting its braces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review, finding 3. The merge's shape check was the outer braces only, so an inherited `PIPEWIRE_ALSA='{ garbage }'` was spliced into rather than overwritten, producing an object the daemon does not accept. Measured live 2026-07-25, and the failure is worse than a rejection: with PIPEWIRE_ALSA set to the old merge's output, a real aplay node came up as node.name=alsa_playback.aplay, no peerspeak.owned, and a junk property `garbage = "peerspeak.owned"` — the lenient parser ate our key as their value and stopped. Both ownership carriers lost on a live Stream/Output/Audio node, which is an echo. So: parse the inherited object and REBUILD it with our pairs last, rather than splicing before the closing brace. Rebuilding is what makes the result independent of the input's formatting — a value ending in a `#` comment would otherwise swallow everything appended after it. The three values the new merge emits were verified against the live daemon (user props preserved, both carriers present) and are pinned byte-for-byte. scan_token is gated on its own postcondition: at the object level an unterminated string is also caught by "the object never closed", so the two implementations only disagree at the seam. Also parameterizes the malformed-value warning, which always named PIPEWIRE_PROPS even when PIPEWIRE_ALSA was the malformed one. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 519 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 485 insertions(+), 34 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 11e0a7f..751a439 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -199,7 +199,11 @@ pub fn tag_child(command: &mut Command, role: &str) { ); command.env( PIPEWIRE_PROPS_ENV, - merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name), + merge_pipewire_props( + inherited(PIPEWIRE_PROPS_ENV).as_deref(), + &node_name, + PIPEWIRE_PROPS_ENV, + ), ); // The ALSA carrier takes the same SPA-JSON grammar and is set here too, so // a child configured for ALSA output — which neither of the other two @@ -212,7 +216,11 @@ pub fn tag_child(command: &mut Command, role: &str) { // child gets its own role rather than inheriting `clip`. command.env( PIPEWIRE_ALSA_ENV, - merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name), + merge_pipewire_props( + inherited(PIPEWIRE_ALSA_ENV).as_deref(), + &node_name, + PIPEWIRE_ALSA_ENV, + ), ); } @@ -254,38 +262,247 @@ fn merge_pulse_prop(inherited: Option<&str>, node_name: &str) -> String { } } -/// `PIPEWIRE_PROPS` is a SPA-JSON object, so merging is inserting our pairs -/// before the closing brace. +/// One top-level pair of an inherited SPA-JSON object. +#[derive(Debug, PartialEq, Eq)] +struct SpaPair { + /// Exactly as the user wrote it: quoted if they quoted it, bare if not. + key: String, + /// Verbatim source text — a quoted string, a bare token, or a whole + /// nested container. Never re-encoded, so nothing is lost in a round trip. + value: String, +} + +/// Skip whitespace and `#`-to-end-of-line comments. +fn skip_blanks(bytes: &[u8], i: &mut usize) { + while let Some(&byte) = bytes.get(*i) { + if byte.is_ascii_whitespace() { + *i += 1; + } else if byte == b'#' { + while !matches!(bytes.get(*i), None | Some(b'\n')) { + *i += 1; + } + } else { + return; + } + } +} + +/// Scan one SPA-JSON token — a quoted string, a balanced container, or a bare +/// word — advancing `i` past it. `None` means the token is unterminated, which +/// is exactly the class of malformation the brace check used to miss. /// -/// The shape check is the outer braces. Anything else — a bare `key = value` -/// list, a truncated object — is left alone and overwritten rather than -/// guessed at; a full SPA-JSON parser is not worth writing for a case with no -/// live consumer. -fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String { +/// Byte indices are only ever *returned* at ASCII boundaries (a quote, a +/// bracket, or a separator), so slicing `text` with them cannot split a +/// multi-byte character even though the scan steps byte by byte. +fn scan_token<'a>(text: &'a str, i: &mut usize) -> Option<&'a str> { + let bytes = text.as_bytes(); + let start = *i; + match *bytes.get(start)? { + b'"' => { + let mut j = start + 1; + loop { + match *bytes.get(j)? { + b'\\' => j += 2, + b'"' => { + j += 1; + break; + } + _ => j += 1, + } + } + *i = j; + } + open @ (b'{' | b'[') => { + let mut stack = vec![if open == b'{' { b'}' } else { b']' }]; + let mut j = start + 1; + while let Some(&byte) = bytes.get(j) { + match byte { + // A bracket inside a string is not structure. + b'"' => { + let mut inner = j; + scan_token(text, &mut inner)?; + j = inner; + } + b'#' => skip_blanks(bytes, &mut j), + b'{' => { + stack.push(b'}'); + j += 1; + } + b'[' => { + stack.push(b']'); + j += 1; + } + b'}' | b']' => { + if stack.pop()? != byte { + return None; + } + j += 1; + if stack.is_empty() { + break; + } + } + _ => j += 1, + } + } + if !stack.is_empty() { + return None; + } + *i = j; + } + _ => { + let mut j = start; + while let Some(&byte) = bytes.get(j) { + if byte.is_ascii_whitespace() + || matches!( + byte, + b',' | b':' | b'=' | b'{' | b'}' | b'[' | b']' | b'#' | b'"' + ) + { + break; + } + j += 1; + } + if j == start { + // A stray separator where a token belongs. + return None; + } + *i = j; + } + } + Some(&text[start..*i]) +} + +/// Parse a complete SPA-JSON **object** into its top-level pairs, or `None` if +/// it is not one. +/// +/// ⚠️ **This exists because checking the outer braces is not a validity +/// check, and the gap is a live fail-open** (round 10 review, finding 3). +/// Measured 2026-07-25 on this host: with `PIPEWIRE_ALSA` set to what the old +/// brace-splicing merge produced from an inherited `{ garbage }` — that is, +/// `{ garbage, "peerspeak.owned" = "1", "node.name" = "…" }` — a real `aplay` +/// node came up as `node.name=alsa_playback.aplay` with **no** +/// `peerspeak.owned` and a junk property `garbage = "peerspeak.owned"`. The +/// daemon's parser is lenient: it paired our key name as *their* value and +/// then stopped, so **both carriers were lost** on a live +/// `Stream/Output/Audio` node. `spa-json-dump -s` reports the same thing as +/// `Expected object key`. +/// +/// The merge therefore parses and **rebuilds** rather than splicing. Rebuilding +/// is what makes the result independent of the input's formatting — a value +/// ending in a `#` comment, for instance, would otherwise swallow the pairs +/// appended after it. +/// +/// Deliberately stricter than the daemon in one direction only: a pair the +/// parser cannot make sense of fails the whole object, which costs the user +/// their routing policy and keeps our tag. The reverse trade — guessing, and +/// silently emitting a value the daemon truncates — is the one that echoes. +fn parse_spa_object(text: &str) -> Option> { + let bytes = text.as_bytes(); + let mut i = 0; + skip_blanks(bytes, &mut i); + if bytes.get(i) != Some(&b'{') { + return None; + } + i += 1; + let mut pairs = Vec::new(); + loop { + skip_blanks(bytes, &mut i); + // Separators between pairs are optional in SPA-JSON and repeats are + // tolerated here; a rebuilt object is well-formed either way. + while bytes.get(i) == Some(&b',') { + i += 1; + skip_blanks(bytes, &mut i); + } + match bytes.get(i) { + // Ran out of input before the closing brace. + None => return None, + Some(b'}') => { + i += 1; + break; + } + _ => {} + } + let key = scan_token(text, &mut i)?; + if key.starts_with('{') || key.starts_with('[') { + return None; + } + skip_blanks(bytes, &mut i); + if matches!(bytes.get(i), Some(b'=' | b':')) { + i += 1; + skip_blanks(bytes, &mut i); + } + // The `{ garbage }` case — a key with no value — needs no guard of its + // own: whatever follows is a separator or the closing brace, and + // `scan_token` refuses both. Verified by mutation; an explicit check + // here was unreachable, and unreachable defence rots. + let value = scan_token(text, &mut i)?; + pairs.push(SpaPair { + key: key.to_string(), + value: value.to_string(), + }); + } + skip_blanks(bytes, &mut i); + // Trailing text after the object: two concatenated objects, or debris. + if i != bytes.len() { + return None; + } + Some(pairs) +} + +/// Re-emit `pairs` as a SPA-JSON object with our two carriers appended last, +/// where last wins (measured — see [`tag_child`]). +fn spa_object_with_our_pairs(pairs: &[SpaPair], node_name: &str) -> String { + let mut out = String::from("{"); + for pair in pairs { + // Bare keys are re-quoted; already-quoted keys are passed through + // verbatim so any escape the user wrote survives untouched. + if pair.key.starts_with('"') { + out.push_str(&format!(" {} = {},", pair.key, pair.value)); + } else { + out.push_str(&format!(" \"{}\" = {},", pair.key, pair.value)); + } + } + out.push_str(&format!( + " \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \"node.name\" = \"{node_name}\" }}" + )); + out +} + +/// The diagnostic for an inherited value we refuse to merge into. +/// +/// A pure function so a test can assert the variable name is the one that was +/// actually malformed; as an inline `format!` inside the logging call it was +/// ungated, and it was wrong — it always said `PIPEWIRE_PROPS`, including for +/// `PIPEWIRE_ALSA`. +fn malformed_object_warning(var: &str, existing: &str) -> String { + format!( + "ownership: inherited {var} is not a well-formed SPA-JSON object \ + ({existing:?}); overwriting it with the ownership tag" + ) +} + +/// `PIPEWIRE_PROPS` and `PIPEWIRE_ALSA` are SPA-JSON objects, so merging is +/// parsing the inherited object and re-emitting it with our pairs last. +/// +/// Anything that is not a *complete, well-formed* object — a bare +/// `key = value` list, a truncated object, an unterminated string, a key with +/// no value — is logged and overwritten rather than guessed at. See +/// [`parse_spa_object`] for why the old outer-brace check was not enough. +/// +/// `var` names the variable being merged, and it is a parameter rather than a +/// constant because both variables take this grammar: the warning used to say +/// `PIPEWIRE_PROPS` even when it was `PIPEWIRE_ALSA` that was malformed. +fn merge_pipewire_props(inherited: Option<&str>, node_name: &str, var: &str) -> String { let ours = pipewire_props_value(node_name); let Some(existing) = inherited else { return ours; }; - let trimmed = existing.trim(); - let inner = trimmed - .strip_prefix('{') - .and_then(|rest| rest.strip_suffix('}')) - .map(str::trim); - match inner { + match parse_spa_object(existing) { // An empty object carries nothing to preserve. - Some("") => ours, - Some(inner) => { - let separator = if inner.ends_with(',') { "" } else { "," }; - format!( - "{{ {inner}{separator} \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ - \"node.name\" = \"{node_name}\" }}" - ) - } + Some(pairs) if pairs.is_empty() => ours, + Some(pairs) => spa_object_with_our_pairs(&pairs, node_name), None => { - crate::log_msg(&format!( - "ownership: inherited {PIPEWIRE_PROPS_ENV} is not a SPA-JSON object \ - ({existing:?}); overwriting it with the ownership tag" - )); + crate::log_msg(&malformed_object_warning(var, existing)); ours } } @@ -327,7 +544,11 @@ fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String { /// The caller must guarantee no other thread exists in this process. pub unsafe fn tag_this_process_alsa_audio() { let node_name = owned_node_name(CLIP_ROLE); - let value = merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name); + let value = merge_pipewire_props( + inherited(PIPEWIRE_ALSA_ENV).as_deref(), + &node_name, + PIPEWIRE_ALSA_ENV, + ); // SAFETY: the caller's obligation, discharged by calling this at the top // of `main` before any thread is spawned. unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, value) }; @@ -670,7 +891,11 @@ mod tests { #[test] fn pipewire_props_merges_inside_the_spa_json_object() { let name = owned_node_name("vlc"); - let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\" }"), &name); + let merged = merge_pipewire_props( + Some("{ \"media.role\" = \"music\" }"), + &name, + PIPEWIRE_PROPS_ENV, + ); assert!(merged.starts_with('{') && merged.ends_with('}'), "{merged}"); assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); @@ -686,7 +911,11 @@ mod tests { #[test] fn pipewire_props_merge_does_not_double_a_trailing_comma() { let name = owned_node_name("vlc"); - let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\", }"), &name); + let merged = merge_pipewire_props( + Some("{ \"media.role\" = \"music\", }"), + &name, + PIPEWIRE_PROPS_ENV, + ); assert!(!merged.contains(",,"), "{merged}"); assert!(!merged.contains(", ,"), "{merged}"); assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); @@ -696,20 +925,242 @@ mod tests { fn an_empty_or_malformed_pipewire_props_yields_our_value_alone() { let name = owned_node_name("vlc"); assert_eq!( - merge_pipewire_props(Some("{ }"), &name), + merge_pipewire_props(Some("{ }"), &name, PIPEWIRE_PROPS_ENV), pipewire_props_value(&name) ); // Not an object: overwritten rather than guessed at. assert_eq!( - merge_pipewire_props(Some("media.role = music"), &name), + merge_pipewire_props(Some("media.role = music"), &name, PIPEWIRE_PROPS_ENV), pipewire_props_value(&name) ); assert_eq!( - merge_pipewire_props(None, &name), + merge_pipewire_props(None, &name, PIPEWIRE_PROPS_ENV), pipewire_props_value(&name) ); } + /// **Round 10 review, finding 3 — the brace check was not a validity + /// check, and the gap is a measured fail-open.** + /// + /// Every value below is brace-wrapped, so the old rule spliced our pairs + /// into it and produced an object the daemon does not accept. ⚠️ The live + /// failure is worse than a rejection: measured 2026-07-25, `PIPEWIRE_ALSA` + /// set to the old merge of `{ garbage }` gave a real `aplay` node + /// `node.name=alsa_playback.aplay`, **no** `peerspeak.owned`, and a junk + /// property `garbage = "peerspeak.owned"` — the lenient parser ate our key + /// as their value and stopped, losing *both* carriers on a live + /// `Stream/Output/Audio` node. Each of these must overwrite instead. + #[test] + fn a_brace_wrapped_but_malformed_pipewire_props_is_overwritten() { + let name = owned_node_name("vlc"); + let malformed = [ + // The measured case: a key with no value. + "{ garbage }", + // A key with no value, followed by real pairs. + "{ garbage, \"media.role\" = \"music\" }", + // Unterminated string. + "{ \"media.role\" = \"music }", + // Broken nested container. + "{ \"channels\" = [ 1, 2 }", + // Mismatched closer. + "{ \"channels\" = [ 1, 2 } ]", + // Truncated object that still ends in a brace, via nesting. + "{ \"a\" = { \"b\" = 1 }", + // A trailing comment would swallow anything appended after it. + "{ \"media.role\" = \"music\" # note }", + // Two objects concatenated: splicing would have tagged only the + // second one. + "{ \"a\" = 1 } { \"b\" = 2 }", + // A dangling assignment. + "{ \"media.role\" = }", + ]; + for value in malformed { + assert_eq!( + merge_pipewire_props(Some(value), &name, PIPEWIRE_PROPS_ENV), + pipewire_props_value(&name), + "{value:?} must be overwritten, not merged into" + ); + } + } + + /// The other half of finding 3: values that *are* well-formed must still + /// round-trip, or the fix would be "overwrite everything" wearing a parser + /// as a disguise. Rebuilding — rather than splicing — is what makes the + /// result independent of the input's formatting. + #[test] + fn a_well_formed_pipewire_props_survives_the_rebuild() { + let name = owned_node_name("vlc"); + let cases = [ + // Bare key, bare value. + ("{ media.role = music }", "\"media.role\" = music"), + // Colon separator. + ( + "{ \"media.role\": \"music\" }", + "\"media.role\" = \"music\"", + ), + // No separator at all — legal SPA-JSON. + ("{ \"media.role\" \"music\" }", "\"media.role\" = \"music\""), + // Nested array, preserved verbatim. + ( + "{ \"audio.position\" = [ FL, FR ] }", + "\"audio.position\" = [ FL, FR ]", + ), + // Nested object, preserved verbatim. + ("{ \"props\" = { \"a\" = 1 } }", "\"props\" = { \"a\" = 1 }"), + // A brace inside a string is not structure. + ( + "{ \"node.name\" = \"weird}name\" }", + "\"node.name\" = \"weird}name\"", + ), + // A leading comment line. + ( + "# mine\n{ \"media.role\" = \"music\" }", + "\"media.role\" = \"music\"", + ), + ]; + for (input, preserved) in cases { + let merged = merge_pipewire_props(Some(input), &name, PIPEWIRE_PROPS_ENV); + assert!( + merged.contains(preserved), + "{input:?} must keep {preserved:?}: {merged}" + ); + assert!( + merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")), + "{input:?} must still carry the tag: {merged}" + ); + assert!( + merged.contains(&format!("\"node.name\" = \"{name}\"")), + "{input:?} must still carry the name: {merged}" + ); + // Ours goes last, so it wins any duplicate key. + let ours = merged + .rfind("\"node.name\" = ") + .expect("a node.name pair in the merged value"); + assert!(merged[ours..].contains(&name), "ours last: {merged}"); + } + } + + /// ✅ **The exact bytes below were verified against the live daemon** + /// 2026-07-25: each was set as `PIPEWIRE_ALSA` for a real `aplay`, and + /// `pw-dump` showed a `Stream/Output/Audio` node carrying `peerspeak.owned + /// = 1`, the expected `node.name`, **and** the user's own properties + /// (`media.role = music`, `audio.position = [ FL, FR ]`). `spa-json-dump + /// -s` accepts all three. + /// + /// Pinned byte-for-byte rather than asserted with `contains`, because + /// what the live run establishes is that *this string* works — a + /// reformatted equivalent is a different experiment, and the failure mode + /// this whole finding is about was a value that looked fine and parsed + /// wrong. + #[test] + fn the_merged_value_is_byte_for_byte_what_was_verified_live() { + assert_eq!( + merge_pipewire_props( + Some("{ \"media.role\" = \"music\" }"), + "f3_rebuilt_probe", + PIPEWIRE_ALSA_ENV + ), + "{ \"media.role\" = \"music\", \"peerspeak.owned\" = \"1\", \ + \"node.name\" = \"f3_rebuilt_probe\" }" + ); + // The malformed case collapses to our value alone. + assert_eq!( + merge_pipewire_props( + Some("{ garbage }"), + "f3_overwritten_probe", + PIPEWIRE_ALSA_ENV + ), + "{ \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_overwritten_probe\" }" + ); + // A bare key, a bare value and a nested array all survive the rebuild + // verbatim — the array in particular is passed through as written. + assert_eq!( + merge_pipewire_props( + Some("{ media.role = music, \"audio.position\" = [ FL, FR ] }"), + "f3_exotic_probe", + PIPEWIRE_ALSA_ENV + ), + "{ \"media.role\" = music, \"audio.position\" = [ FL, FR ], \ + \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_exotic_probe\" }" + ); + } + + /// [`scan_token`] is the seam the whole SPA-JSON check rests on, so it is + /// gated on its own postcondition — "a token I return is complete" — + /// rather than only through the merge. + /// + /// ⚠️ Not redundant with the merge tests, and mutation says so: at the + /// object level an unterminated string or an unbalanced bracket is *also* + /// caught by "the object never closed", so both implementations agree + /// there. They disagree here, which is where the property actually lives. + #[test] + fn scan_token_returns_only_complete_tokens() { + let complete = [ + ("\"music\"", "\"music\""), + // An escaped quote does not end the string. + ("\"we\\\"ird\" rest", "\"we\\\"ird\""), + // Brackets inside a string are not structure. + ("\"a}b]c\"", "\"a}b]c\""), + ("[ 1, 2 ] rest", "[ 1, 2 ]"), + ( + "{ \"a\" = { \"b\" = 1 } } rest", + "{ \"a\" = { \"b\" = 1 } }", + ), + // A container closing inside a string does not close it. + ("[ \"]\" ] rest", "[ \"]\" ]"), + ("bare-token, rest", "bare-token"), + ("music}", "music"), + ]; + for (input, expected) in complete { + let mut i = 0; + assert_eq!(scan_token(input, &mut i), Some(expected), "{input:?}"); + assert_eq!(&input[..i], expected, "{input:?} advanced past its token"); + } + + let incomplete = [ + // Unterminated string, plain and inside a container. + "\"music", + "\"music\\\"", + "[ \"music ]", + // Unbalanced containers. + "[ 1, 2", + "{ \"a\" = 1", + "[ { } ", + // Mismatched closer. + "[ 1, 2 }", + "{ \"a\" = 1 ]", + // A stray separator where a token belongs. + ",", + "=", + "}", + ]; + for input in incomplete { + let mut i = 0; + assert_eq!(scan_token(input, &mut i), None, "{input:?} is not complete"); + } + } + + /// The warning names the variable that was actually malformed. It used to + /// always say `PIPEWIRE_PROPS`, including when `tag_child` merged + /// `PIPEWIRE_ALSA` (round 10 review, finding 3's second half). + #[test] + fn the_malformed_warning_names_the_variable_it_was_given() { + // Both variables take the same grammar and go through the same merge, + // so the only thing distinguishing their diagnostics is this name + // being threaded through rather than hard-coded — which it was. + assert_ne!(PIPEWIRE_PROPS_ENV, PIPEWIRE_ALSA_ENV); + + let alsa = malformed_object_warning(PIPEWIRE_ALSA_ENV, "{ garbage }"); + assert!(alsa.contains(PIPEWIRE_ALSA_ENV), "{alsa}"); + assert!(!alsa.contains(PIPEWIRE_PROPS_ENV), "{alsa}"); + + let props = malformed_object_warning(PIPEWIRE_PROPS_ENV, "{ garbage }"); + assert!(props.contains(PIPEWIRE_PROPS_ENV), "{props}"); + // The offending value is quoted into the message, so the user can see + // what was dropped rather than only that something was. + assert!(props.contains("garbage"), "{props}"); + } + /// A spawned player configured for ALSA output is reached by neither /// `PULSE_PROP` nor `PIPEWIRE_PROPS`. `tag_child` therefore sets the ALSA /// carrier too — with the *child's* role, so it does not inherit this @@ -753,7 +1204,7 @@ mod tests { fn a_childs_role_overrides_the_inherited_clip_tag_in_the_alsa_carrier() { let clip = pipewire_props_value(&owned_node_name(CLIP_ROLE)); let child = owned_node_name("mpv"); - let merged = merge_pipewire_props(Some(&clip), &child); + let merged = merge_pipewire_props(Some(&clip), &child, PIPEWIRE_ALSA_ENV); let last_name = merged .rfind("\"node.name\" = ") From 985c63806bf0cef5bd3f2f1a4ffcdc2cd8b886a4 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 23:51:02 -0400 Subject: [PATCH 08/13] audio/ownership: state the playlist policy, and gate main's ordering properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review, findings 2 and 5. Finding 2 — R10-2's rationale for tagging local playlist audio was factually wrong. It claimed a local track is "already being broadcast to peers on the same keypress", but shared listening is opt-in: music_broadcast defaults to false, play_music_index starts local playback unconditionally, and broadcast_track returns immediately when can_broadcast_music is false. So a default-config playlist is not already broadcast. The tag stays, now as an explicit policy with the real reason: the carriers reach rodio through PIPEWIRE_ALSA, which is process-wide, and clip_player and music_player are two ClipPlayer instances in one process — no value of that variable can tag one and not the other. Exempting the playlist means giving it a separately taggable stream, which is a large change for a case with a one-step workaround (play it in any other app). Tagging is not optional for received clips and peer music, which are the far end's own audio. Finding 5 — the ordering test proved only "before run_gui", which a thread::spawn inserted above the tag still satisfies while making the set_var a data race. It now requires the tag to be the first executable statement in main: attributes, `unsafe` and block punctuation are stripped, and any residue fails. Mutation-verified against a spawn, an unrelated statement, and the call deleted. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 94 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 9 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 751a439..fbb1b0d 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -95,15 +95,41 @@ pub const PIPEWIRE_ALSA_ENV: &str = "PIPEWIRE_ALSA"; /// Role of in-process audio played through `rodio`: received chat clips, peer /// music, and locally chosen playlist tracks (round 10, R10-2). /// -/// **All three are tagged, deliberately**, including the local playlist. The -/// case for exempting locally chosen music is that the user picked it and may -/// want it shared; the case against is stronger. A local track played from the -/// playlist is *already being broadcast to peers over the call* on the same -/// keypress (`app::play_local_track` → `broadcast_track`), so sharing it a -/// second time through the screen-share sends the far end two copies of the -/// same audio, offset by the two paths' differing latency. That is not a lost -/// feature, it is a defect. A user who wants music in the share can play it in -/// any other application, which peerspeak never tags. +/// # Policy: peerspeak's own playlist audio is never screen-shareable +/// +/// **All three are tagged, and for the local playlist that is a deliberate +/// policy rather than a consequence** (round 10 review, finding 2). +/// +/// ⚠️ An earlier version of this comment justified it by claiming a local +/// track is "already being broadcast to peers on the same keypress", so +/// sharing it again would send the far end two copies. **That claim is false +/// in the default configuration** and Codex was right to call it: shared +/// listening is opt-in, `Config::music_broadcast` defaults to `false` (pinned +/// by `config::tests::test_input_output_volume_fields`), +/// `app::play_music_index` starts local playback unconditionally, and +/// `app::broadcast_track` returns immediately when +/// `can_broadcast_music` is false. With broadcasting off there is no second +/// copy and no defect — only a track the user chose that they cannot put in +/// the share. +/// +/// The policy stands anyway, on cost rather than on that argument: +/// +/// - The two carriers reach `rodio` through [`PIPEWIRE_ALSA_ENV`], which is +/// **process-wide**. `clip_player` and `music_player` are two `ClipPlayer` +/// instances in one process (`app::AppState`), so no value of that variable +/// can tag one and not the other, and `set_var` cannot be flipped at +/// runtime once threads exist. +/// - Exempting the playlist therefore means giving it a separately taggable +/// stream — a helper process, or rebuilding `ClipPlayer` on peerspeak's own +/// PipeWire backend — for a case with a one-step workaround. +/// - Tagging is **not** optional for the other two roles: received clips and +/// peer music are the far end's own audio, and re-sharing them is the exact +/// echo this feature exists to prevent. +/// +/// So: a user who wants music inside the screen share plays it in any other +/// application, which peerspeak never tags. When shared listening *is* on, +/// the original two-copies argument does apply, and the policy is simply +/// right for that case too. pub const CLIP_ROLE: &str = "clip"; /// Role of the native call-playback stream — the node carrying the far end's @@ -809,6 +835,16 @@ mod tests { /// A source-text assertion is crude. It is also the *only* check available /// short of driving the real GUI binary, and phase 1's miss was precisely /// a call site nobody verified existed. + /// + /// ⚠️ **The check is "first statement in `main`", not "before + /// `run_gui`"** (round 10 review, finding 5). Ordering against `run_gui` + /// proved far less than the safety obligation needs: inserting a + /// long-lived `std::thread::spawn` above the tag still satisfied it, while + /// making the `set_var` a data race — the exact thing the `unsafe` block's + /// SAFETY comment claims cannot happen. `set_var` is sound only while the + /// process is single-threaded, so the property to gate is that **nothing + /// executable precedes the call**, which is checkable and is what `main` + /// actually does today. #[test] fn main_tags_this_process_before_anything_starts() { const MAIN: &str = include_str!("../main.rs"); @@ -829,6 +865,46 @@ mod tests { let call = code .find("tag_this_process_alsa_audio()") .expect("main must call tag_this_process_alsa_audio (round 10, R10-2)"); + + let body = code + .find("fn main()") + .and_then(|start| code[start..].find('{').map(|open| start + open + 1)) + .expect("main has a body"); + // Back up over the module path the call is written with, so the + // statement's own `peerspeak::audio::ownership::` is not mistaken for + // code preceding it. + let call_start = code[..call] + .rfind(|c: char| !(c.is_alphanumeric() || c == '_' || c == ':')) + .map_or(0, |index| index + 1); + assert!( + call_start > body, + "the call must be inside main, not above it" + ); + + // Everything between `main`'s opening brace and the call, with the + // syntax that cannot *run* removed: attributes, the `unsafe` keyword, + // block punctuation, whitespace. Anything left is a statement that + // executes before the tag — which is what must not exist. + let mut before = code[body..call_start].to_string(); + while let Some(start) = before.find("#[") { + let end = before[start..] + .find(']') + .map(|offset| start + offset + 1) + .expect("an attribute closes"); + before.replace_range(start..end, ""); + } + let residue: String = before + .replace("unsafe", "") + .chars() + .filter(|c| !c.is_whitespace() && !matches!(c, '{' | '}')) + .collect(); + assert!( + residue.is_empty(), + "nothing may execute before the tag; found {residue:?} in main" + ); + + // And it must still be the real `main` of a program that starts the + // GUI, or the check above is asserting about the wrong function. let gui = code .find("run_gui") .expect("main runs the GUI; this test's ordering check assumes it"); From ae2e9de52321f61bc2b54ac1b321beeb0160c9e2 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 23:57:16 -0400 Subject: [PATCH 09/13] tests/fixtures: the ownership contract says exact-match, not truthy Round 10 review, finding 6. The cross-repo contract still documented carrier 1 as "any value other than false/0 is truthy" after R10-4 made pixelpass match it exactly. A future producer following the fixture could emit "true" and silently lose the carrier. Committed byte-identical with pixelpass's copy in the same session, as the file's own rules require. Co-Authored-By: Claude Opus 5 --- tests/fixtures/ownership-tag-contract.txt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/fixtures/ownership-tag-contract.txt b/tests/fixtures/ownership-tag-contract.txt index 471c413..fa1b3ef 100644 --- a/tests/fixtures/ownership-tag-contract.txt +++ b/tests/fixtures/ownership-tag-contract.txt @@ -20,9 +20,17 @@ # PipeWire registry `global` event and readable only via a node bind, so the # primary taint root must not rest on one observation mechanism alone. -# Carrier 1 — a node property. The consumer treats any value other than -# "false"/"0" as truthy, which is the fail-closed direction; the producer -# always emits exactly this value. +# Carrier 1 — a node property, matched EXACTLY: `prop_value` below is the +# ONLY spelling the consumer reads as owned. A producer emitting "true", "yes" +# or "" is NOT owned on this carrier, and only carrier 2 would still catch it. +# +# ⚠️ This wording is load-bearing and it CHANGED in round 10. The consumer +# used to accept any value other than "false"/"0", on the theory that leniency +# over-excludes and is therefore safe. It is not: leniency buys false-positive +# exclusion, and it let any process suppress a rival application's audio from +# the share with a property it did not even have to spell right. Fail-closed +# on this feature is about ANCESTRY — an unresolvable graph is not eligible — +# not about parsing. prop_key=peerspeak.owned prop_value=1 From 45b1b97dd8ba02346b9ee4bebd7eb1d543c934b7 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 00:00:23 -0400 Subject: [PATCH 10/13] audio/ownership: pin the no-lost-carrier invariant, and harden the byte scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification round on the round-10 review fixes. Adds the property the whole of finding 3 is about, stated directly: over 20,000 deterministic inputs built from the exact characters that break SPA-JSON (braces, brackets, quotes, separators, comment marks, escapes, newlines, multi-byte characters), the merge always emits both carriers in an object it can read back. Either outcome — parse and rebuild, or overwrite — has to end that way, and now nothing can quietly change which. Also replaces two byte-index steps with character-boundary steps. Both were correct on the ASCII input they actually see, but `index + 1` after a reverse find would have split a multi-byte character and panicked the slice. scan_token gains multi-byte cases for the same reason. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 86 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index fbb1b0d..44f12f5 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -872,10 +872,13 @@ mod tests { .expect("main has a body"); // Back up over the module path the call is written with, so the // statement's own `peerspeak::audio::ownership::` is not mistaken for - // code preceding it. + // code preceding it. Stepping by `len_utf8` rather than by one byte + // keeps the slice below on a character boundary whatever precedes it. let call_start = code[..call] - .rfind(|c: char| !(c.is_alphanumeric() || c == '_' || c == ':')) - .map_or(0, |index| index + 1); + .char_indices() + .rev() + .find(|(_, c)| !(c.is_alphanumeric() || *c == '_' || *c == ':')) + .map_or(0, |(index, c)| index + c.len_utf8()); assert!( call_start > body, "the call must be inside main, not above it" @@ -1173,6 +1176,13 @@ mod tests { fn scan_token_returns_only_complete_tokens() { let complete = [ ("\"music\"", "\"music\""), + // Multi-byte characters: the scan steps by bytes, so a token + // boundary landing inside one would panic the slice. It cannot — + // UTF-8 continuation bytes are never ASCII — and this pins it. + ("\"café ☕\" rest", "\"café ☕\""), + ("café", "café"), + ("\"\\é\" rest", "\"\\é\""), + ("[ \"ünïcode\" ] rest", "[ \"ünïcode\" ]"), // An escaped quote does not end the string. ("\"we\\\"ird\" rest", "\"we\\\"ird\""), // Brackets inside a string are not structure. @@ -1216,6 +1226,76 @@ mod tests { } } + /// **The invariant the whole finding is about: no inherited value, however + /// hostile, may cost us a carrier.** + /// + /// The merge has two outcomes — parse and rebuild, or overwrite — and both + /// are supposed to end with our two pairs present in a well-formed object. + /// A deterministic walk over an alphabet of the exact characters that + /// break SPA-JSON (braces, brackets, quotes, separators, comment marks, + /// escapes, newlines, multi-byte characters) checks that directly, and + /// checks the result parses — an output our own parser rejects would be an + /// output the daemon may well truncate, which is the live failure this + /// finding began with. + #[test] + fn no_inherited_value_can_cost_us_a_carrier() { + let name = owned_node_name(CLIP_ROLE); + let alphabet = [ + "{", + "}", + "[", + "]", + "\"", + "=", + ":", + ",", + "#", + "\\", + " ", + "\n", + "\t", + "a", + "1", + "é", + "☕", + "peerspeak.owned", + "node.name", + "media.role", + "true", + ]; + // A fixed seed: a failure here must be reproducible, not a flake. + let mut state: u64 = 0x2545_f491_4f6c_dd1d; + let mut next = move || { + state = state + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + (state >> 33) as usize + }; + + for _ in 0..20_000 { + let length = next() % 14; + let mut inherited = String::new(); + for _ in 0..length { + inherited.push_str(alphabet[next() % alphabet.len()]); + } + + let merged = merge_pipewire_props(Some(&inherited), &name, PIPEWIRE_ALSA_ENV); + assert!( + merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")), + "carrier 1 lost for inherited {inherited:?}: {merged}" + ); + assert!( + merged.contains(&format!("\"node.name\" = \"{name}\"")), + "carrier 2 lost for inherited {inherited:?}: {merged}" + ); + // And whatever we emit must be an object we can read back. + assert!( + parse_spa_object(&merged).is_some(), + "emitted an object we cannot parse for inherited {inherited:?}: {merged}" + ); + } + } + /// The warning names the variable that was actually malformed. It used to /// always say `PIPEWIRE_PROPS`, including when `tag_child` merged /// `PIPEWIRE_ALSA` (round 10 review, finding 3's second half). From 21ba6338254d5a47801da88bf434237edff38c20 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 01:08:17 -0400 Subject: [PATCH 11/13] audio/ownership: validate inherited SPA-JSON with libspa, not a scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 11 review, findings 2, 3 and 4. The round-10 fix replaced a brace check with a hand-written scanner. That was the wrong shape: a second implementation of someone else's grammar drifts in both directions at once, and measured against `spa-json-dump -s` on this host it did. It ACCEPTED `{ "foo" = { garbage } }` (only brackets were balanced, contents never validated), `{ "a" = "\é" }`, `{ "a" = é }` and `{ "a" = foo\bar }`. Merging into those put an invalid pair before our carriers, so the daemon stops at it and drops both -- recreating the exact fail-open the round-10 fix existed to close. Its own test even pinned `"\é"` as a valid token. It REJECTED `{ target.object, "my-sink" }`, `{ key == "value" }` and CR-terminated comments, all valid -- so a user with one of those in their environment silently lost their routing policy to an overwrite. That half affects a running Linux user. Now libspa's own parser validates, and the merge splices into the validated text instead of re-emitting parsed pairs. Splicing preserves the user's bytes exactly, which also answers the review's point that re-quoting a bare key can invent a different one (`foo\bar` -> a string with a \b escape). Three measured properties make the splice safe -- the last `}` is the object's, a validated object's brace is never mid-comment, and commas are pure separators -- and the result is validated again before it is returned. Mutation testing then deleted the rest: every pairing and recursion check I had written turned out to be redundant, because spa_json_next already errors on `{ garbage }` and on nested garbage, and skips containers rather than descending. ~60 lines of my own grammar logic removed. What remains is gated by a new differential test against `spa-json-dump -s` over a 27-value corpus -- the check whose absence caused this round. It found a real disagreement on its first run (a bare document, which we reject by design, not by accident). One mutation HUNG rather than failed: dropping the `length < 0` check makes libspa report the same error without advancing, spinning forever. Kept, now labelled load-bearing for termination, with a token-count bound beside it. Finding 4: the ordering test took the first textual match of `fn main`, so a raw-string decoy above the real function satisfied it while the real one spawned a thread first. Now requires each of the three anchors to be unique. Mutation-verified with the review's own decoy. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 693 +++++++++++++++++++---------------------- 1 file changed, 328 insertions(+), 365 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 44f12f5..d8ce878 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -288,212 +288,6 @@ fn merge_pulse_prop(inherited: Option<&str>, node_name: &str) -> String { } } -/// One top-level pair of an inherited SPA-JSON object. -#[derive(Debug, PartialEq, Eq)] -struct SpaPair { - /// Exactly as the user wrote it: quoted if they quoted it, bare if not. - key: String, - /// Verbatim source text — a quoted string, a bare token, or a whole - /// nested container. Never re-encoded, so nothing is lost in a round trip. - value: String, -} - -/// Skip whitespace and `#`-to-end-of-line comments. -fn skip_blanks(bytes: &[u8], i: &mut usize) { - while let Some(&byte) = bytes.get(*i) { - if byte.is_ascii_whitespace() { - *i += 1; - } else if byte == b'#' { - while !matches!(bytes.get(*i), None | Some(b'\n')) { - *i += 1; - } - } else { - return; - } - } -} - -/// Scan one SPA-JSON token — a quoted string, a balanced container, or a bare -/// word — advancing `i` past it. `None` means the token is unterminated, which -/// is exactly the class of malformation the brace check used to miss. -/// -/// Byte indices are only ever *returned* at ASCII boundaries (a quote, a -/// bracket, or a separator), so slicing `text` with them cannot split a -/// multi-byte character even though the scan steps byte by byte. -fn scan_token<'a>(text: &'a str, i: &mut usize) -> Option<&'a str> { - let bytes = text.as_bytes(); - let start = *i; - match *bytes.get(start)? { - b'"' => { - let mut j = start + 1; - loop { - match *bytes.get(j)? { - b'\\' => j += 2, - b'"' => { - j += 1; - break; - } - _ => j += 1, - } - } - *i = j; - } - open @ (b'{' | b'[') => { - let mut stack = vec![if open == b'{' { b'}' } else { b']' }]; - let mut j = start + 1; - while let Some(&byte) = bytes.get(j) { - match byte { - // A bracket inside a string is not structure. - b'"' => { - let mut inner = j; - scan_token(text, &mut inner)?; - j = inner; - } - b'#' => skip_blanks(bytes, &mut j), - b'{' => { - stack.push(b'}'); - j += 1; - } - b'[' => { - stack.push(b']'); - j += 1; - } - b'}' | b']' => { - if stack.pop()? != byte { - return None; - } - j += 1; - if stack.is_empty() { - break; - } - } - _ => j += 1, - } - } - if !stack.is_empty() { - return None; - } - *i = j; - } - _ => { - let mut j = start; - while let Some(&byte) = bytes.get(j) { - if byte.is_ascii_whitespace() - || matches!( - byte, - b',' | b':' | b'=' | b'{' | b'}' | b'[' | b']' | b'#' | b'"' - ) - { - break; - } - j += 1; - } - if j == start { - // A stray separator where a token belongs. - return None; - } - *i = j; - } - } - Some(&text[start..*i]) -} - -/// Parse a complete SPA-JSON **object** into its top-level pairs, or `None` if -/// it is not one. -/// -/// ⚠️ **This exists because checking the outer braces is not a validity -/// check, and the gap is a live fail-open** (round 10 review, finding 3). -/// Measured 2026-07-25 on this host: with `PIPEWIRE_ALSA` set to what the old -/// brace-splicing merge produced from an inherited `{ garbage }` — that is, -/// `{ garbage, "peerspeak.owned" = "1", "node.name" = "…" }` — a real `aplay` -/// node came up as `node.name=alsa_playback.aplay` with **no** -/// `peerspeak.owned` and a junk property `garbage = "peerspeak.owned"`. The -/// daemon's parser is lenient: it paired our key name as *their* value and -/// then stopped, so **both carriers were lost** on a live -/// `Stream/Output/Audio` node. `spa-json-dump -s` reports the same thing as -/// `Expected object key`. -/// -/// The merge therefore parses and **rebuilds** rather than splicing. Rebuilding -/// is what makes the result independent of the input's formatting — a value -/// ending in a `#` comment, for instance, would otherwise swallow the pairs -/// appended after it. -/// -/// Deliberately stricter than the daemon in one direction only: a pair the -/// parser cannot make sense of fails the whole object, which costs the user -/// their routing policy and keeps our tag. The reverse trade — guessing, and -/// silently emitting a value the daemon truncates — is the one that echoes. -fn parse_spa_object(text: &str) -> Option> { - let bytes = text.as_bytes(); - let mut i = 0; - skip_blanks(bytes, &mut i); - if bytes.get(i) != Some(&b'{') { - return None; - } - i += 1; - let mut pairs = Vec::new(); - loop { - skip_blanks(bytes, &mut i); - // Separators between pairs are optional in SPA-JSON and repeats are - // tolerated here; a rebuilt object is well-formed either way. - while bytes.get(i) == Some(&b',') { - i += 1; - skip_blanks(bytes, &mut i); - } - match bytes.get(i) { - // Ran out of input before the closing brace. - None => return None, - Some(b'}') => { - i += 1; - break; - } - _ => {} - } - let key = scan_token(text, &mut i)?; - if key.starts_with('{') || key.starts_with('[') { - return None; - } - skip_blanks(bytes, &mut i); - if matches!(bytes.get(i), Some(b'=' | b':')) { - i += 1; - skip_blanks(bytes, &mut i); - } - // The `{ garbage }` case — a key with no value — needs no guard of its - // own: whatever follows is a separator or the closing brace, and - // `scan_token` refuses both. Verified by mutation; an explicit check - // here was unreachable, and unreachable defence rots. - let value = scan_token(text, &mut i)?; - pairs.push(SpaPair { - key: key.to_string(), - value: value.to_string(), - }); - } - skip_blanks(bytes, &mut i); - // Trailing text after the object: two concatenated objects, or debris. - if i != bytes.len() { - return None; - } - Some(pairs) -} - -/// Re-emit `pairs` as a SPA-JSON object with our two carriers appended last, -/// where last wins (measured — see [`tag_child`]). -fn spa_object_with_our_pairs(pairs: &[SpaPair], node_name: &str) -> String { - let mut out = String::from("{"); - for pair in pairs { - // Bare keys are re-quoted; already-quoted keys are passed through - // verbatim so any escape the user wrote survives untouched. - if pair.key.starts_with('"') { - out.push_str(&format!(" {} = {},", pair.key, pair.value)); - } else { - out.push_str(&format!(" \"{}\" = {},", pair.key, pair.value)); - } - } - out.push_str(&format!( - " \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \"node.name\" = \"{node_name}\" }}" - )); - out -} - /// The diagnostic for an inherited value we refuse to merge into. /// /// A pure function so a test can assert the variable name is the one that was @@ -507,31 +301,192 @@ fn malformed_object_warning(var: &str, existing: &str) -> String { ) } +/// Validate a SPA-JSON **object** with **libspa's own parser** — the same code +/// the daemon runs — and return how many tokens it holds. +/// +/// The count is only ever compared against zero, to tell an empty object from +/// one with something worth preserving, so tokens rather than pairs is fine. +/// +/// ⚠️ **This was a hand-written scanner for one round, and that is worth +/// recording** (round 11 review, findings 2 and 3). Checking the outer braces +/// was not a validity check; neither is checking that brackets balance. +/// Measured against `spa-json-dump -s` on this host, the hand-written version +/// disagreed with the real grammar in **both** directions: +/// +/// - It **accepted** `{ "foo" = { garbage } }`, `{ "a" = "\é" }` (invalid +/// escape), `{ "a" = é }` (non-ASCII bare token) and `{ "a" = foo\bar }`. +/// Merging into those put an invalid pair *before* our carriers, so the +/// daemon would stop at it and drop both — recreating the exact fail-open +/// the fix existed to close. +/// - It **rejected** `{ target.object, "my-sink" }` (a comma between key and +/// value is ignored by SPA-JSON), `{ key == "value" }`, and CR-terminated +/// comments, all of which are valid — so a user with one of those in their +/// environment silently lost their routing policy to an overwrite. +/// +/// A second implementation of someone else's grammar is a liability in both +/// directions at once. Using theirs makes the disagreement impossible. +/// +/// `None` for anything that is not one complete, well-formed object, including +/// trailing text after it. +#[cfg(target_os = "linux")] +fn spa_object_tokens(text: &str) -> Option { + use pipewire::spa::sys as spa; + + // SAFETY: the iterators borrow `text` and never outlive this call, and + // every pointer libspa hands back points into `text`. No slice is taken — + // only counts and libspa's own predicates — so there is no span + // arithmetic to get wrong. + unsafe { + let mut outer = std::mem::zeroed::(); + spa::spa_json_init( + &mut outer, + text.as_ptr().cast::(), + text.len(), + ); + + // The whole input must be exactly one object. + let mut token: *const std::os::raw::c_char = std::ptr::null(); + let length = spa::spa_json_next(&mut outer, &mut token); + if length <= 0 || spa::spa_json_is_object(token, length) == 0 { + return None; + } + + // ⚠️ **Nothing here re-implements the grammar, and that is the whole + // point.** An earlier cut walked the object itself, checking key/value + // pairing and recursing into nested containers. Mutation testing then + // showed every one of those checks was redundant: `spa_json_next` + // already returns an error for `{ garbage }`, for + // `{ "foo" = { garbage } }`, and for an unterminated string, and it + // *skips* a nested container rather than descending into it — so a + // flat token walk is both correct and complete. Deleting the + // hand-written half is what makes "use their grammar, not ours" + // actually true. + let mut object = std::mem::zeroed::(); + spa::spa_json_enter(&mut outer, &mut object); + + let mut tokens = 0; + loop { + let mut token: *const std::os::raw::c_char = std::ptr::null(); + let length = spa::spa_json_next(&mut object, &mut token); + if length == 0 { + break; + } + // ⚠️ **Load-bearing for termination, not just correctness.** + // Mutation-tested: with this removed, libspa keeps reporting the + // same error without advancing the cursor and the loop spins + // forever — the test run hung rather than failed, which is a much + // worse way to find out. + if length < 0 { + return None; + } + tokens += 1; + // Belt and braces on the same hazard: every token consumes at + // least one byte, so a token count past the input length means the + // iterator is not advancing and we must not trust it. + if tokens > text.len() { + return None; + } + } + + // Nothing may follow the object. + let mut trailing: *const std::os::raw::c_char = std::ptr::null(); + if spa::spa_json_next(&mut outer, &mut trailing) != 0 { + return None; + } + Some(tokens) + } +} + +/// Off Linux there is nothing to merge into: `PIPEWIRE_PROPS` and +/// `PIPEWIRE_ALSA` are PipeWire's own and libspa is a Linux-only dependency, +/// so refusing to validate makes [`merge_pipewire_props`] overwrite — the +/// right answer on a platform where the variable has no reader. +/// +/// The module stays platform-neutral on purpose (see `audio::mod`): the +/// literals are a wire contract and their test must run everywhere. +#[cfg(not(target_os = "linux"))] +fn spa_object_tokens(_text: &str) -> Option { + None +} + /// `PIPEWIRE_PROPS` and `PIPEWIRE_ALSA` are SPA-JSON objects, so merging is -/// parsing the inherited object and re-emitting it with our pairs last. +/// inserting our two pairs before the closing brace of a **validated** object. /// -/// Anything that is not a *complete, well-formed* object — a bare -/// `key = value` list, a truncated object, an unterminated string, a key with -/// no value — is logged and overwritten rather than guessed at. See -/// [`parse_spa_object`] for why the old outer-brace check was not enough. +/// Anything that is not a complete, well-formed object — a bare `key = value` +/// list, a truncated object, an unterminated string, a key with no value, an +/// invalid escape — is logged and overwritten rather than guessed at. /// -/// `var` names the variable being merged, and it is a parameter rather than a +/// Three properties make the splice safe, and all three depend on the object +/// having been validated first: +/// +/// 1. **The last `}` is the object's own.** Nothing may follow the object, so +/// the final brace closes it. +/// 2. **Our pairs cannot land inside a comment.** A `#` comment runs to the +/// end of its line, so a validated object's closing brace is always +/// reachable, and the text immediately before it is never mid-comment. +/// Only spaces and tabs are trimmed before appending — never a newline — +/// so a value ending in `# note\n ` keeps its line break. +/// 3. **A leading comma is always legal.** Measured: SPA-JSON treats commas +/// as pure separators, accepting `{ "a" = "b",, "c" = "d" }` and even +/// `{ target.object, "my-sink" }`. +/// +/// And then the result is **validated again before it is returned**. If the +/// splice ever produces something libspa will not parse, we overwrite instead: +/// a lost routing preference is recoverable, a lost carrier is an echo. +/// +/// ⚠️ Honest note on that last check and on `spa_object_tokens`'s +/// "must be an object" test: **mutation testing cannot currently distinguish +/// either of them.** Removing the re-validation breaks nothing because the +/// first validation already rejects every input we have; removing the +/// object test breaks nothing because a non-object has no `}` to splice +/// before. They are deliberate belt-and-braces on the one code path where +/// being wrong is a silent echo, not load-bearing logic — and saying so is +/// better than leaving a future reader to assume a test is watching them. +/// +/// `var` names the variable being merged, and is a parameter rather than a /// constant because both variables take this grammar: the warning used to say -/// `PIPEWIRE_PROPS` even when it was `PIPEWIRE_ALSA` that was malformed. +/// `PIPEWIRE_PROPS` even when `PIPEWIRE_ALSA` was the malformed one. fn merge_pipewire_props(inherited: Option<&str>, node_name: &str, var: &str) -> String { let ours = pipewire_props_value(node_name); let Some(existing) = inherited else { return ours; }; - match parse_spa_object(existing) { - // An empty object carries nothing to preserve. - Some(pairs) if pairs.is_empty() => ours, - Some(pairs) => spa_object_with_our_pairs(&pairs, node_name), - None => { - crate::log_msg(&malformed_object_warning(var, existing)); - ours - } + + let overwrite = || { + crate::log_msg(&malformed_object_warning(var, existing)); + ours.clone() + }; + + // An empty object carries nothing worth preserving. + let Some(tokens) = spa_object_tokens(existing) else { + return overwrite(); + }; + if tokens == 0 { + return ours; } + + let trimmed = existing.trim_end(); + let Some(closer) = trimmed.rfind('}') else { + return overwrite(); + }; + // A trailing separator of the user's would make ours a double comma. That + // is legal SPA-JSON (measured) but it reads like a bug, so drop it. It + // cannot be a comma inside a comment: a comment runs to end of line, so an + // object whose closing brace followed one on the same line would not have + // validated. + let kept = trimmed[..closer] + .trim_end_matches([' ', '\t']) + .trim_end_matches(','); + let merged = format!( + "{kept}, \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ + \"node.name\" = \"{node_name}\" }}" + ); + + // Never emit a value we cannot parse back. + if spa_object_tokens(&merged).is_none() { + return overwrite(); + } + merged } /// Tag the audio this process plays **through ALSA** — in practice everything @@ -915,6 +870,30 @@ mod tests { call < gui, "the tag must be set before the GUI starts any thread" ); + + // ⚠️ **Every search above takes the FIRST textual match, so the whole + // check is only as good as there being one candidate** (round 11 + // review, finding 4). A raw-string decoy above the real function — + // `const DECOY: &str = r#" fn main() { unsafe { …tag…(); run_gui(); } "#;` + // — would satisfy all three searches while the real `main` spawned a + // thread before the tag. Requiring uniqueness kills that class without + // pulling in a parser: if a second `fn main` or a second call ever + // appears, this fails and asks a human to look. + assert_eq!( + code.matches("fn main").count(), + 1, + "exactly one `fn main` in main.rs; the ordering check reads the first match" + ); + assert_eq!( + code.matches("tag_this_process_alsa_audio").count(), + 1, + "exactly one tag call; a second would make the ordering check ambiguous" + ); + assert_eq!( + code.matches("run_gui").count(), + 1, + "exactly one run_gui; a second would make the ordering check ambiguous" + ); } /// R10-5: a user's inherited `PULSE_PROP` survives, and our pairs go last @@ -1052,6 +1031,17 @@ mod tests { "{ \"a\" = 1 } { \"b\" = 2 }", // A dangling assignment. "{ \"media.role\" = }", + // ⚠️ Round 11 finding 2's decisive case: garbage *inside* a nested + // container. The hand-written scanner only balanced brackets, so + // it accepted these and spliced our carriers in after an invalid + // pair — which the daemon stops at, dropping both. + "{ \"foo\" = { garbage } }", + "{ \"foo\" = [ { garbage } ] }", + "{ \"foo\" = { \"bar\" = { garbage } } }", + // Invalid escapes and non-ASCII bare tokens, also accepted before. + "{ \"a\" = \"\\é\" }", + "{ \"a\" = é }", + "{ \"a\" = foo\\bar }", ]; for value in malformed { assert_eq!( @@ -1062,30 +1052,43 @@ mod tests { } } - /// The other half of finding 3: values that *are* well-formed must still - /// round-trip, or the fix would be "overwrite everything" wearing a parser - /// as a disguise. Rebuilding — rather than splicing — is what makes the - /// result independent of the input's formatting. + /// The other half of finding 3: values that *are* well-formed must survive, + /// or the fix would be "overwrite everything" wearing a parser as a + /// disguise. + /// + /// ⚠️ **The user's text is preserved byte-for-byte, not re-encoded.** An + /// earlier version parsed into pairs and re-emitted them, which re-quoted + /// bare keys — and round 11 pointed out that re-quoting a token the parser + /// wrongly accepted can invent a *different* key (`foo\bar` becoming a + /// string with a `\b` escape). Splicing into validated text cannot change + /// the meaning of anything it does not touch. + /// + /// Every input here is one `spa-json-dump -s` accepts; the last three were + /// specifically rejected by the hand-written scanner this replaced. #[test] - fn a_well_formed_pipewire_props_survives_the_rebuild() { + fn a_well_formed_pipewire_props_survives_the_merge() { let name = owned_node_name("vlc"); let cases = [ - // Bare key, bare value. - ("{ media.role = music }", "\"media.role\" = music"), + // Bare key, bare value — kept bare, exactly as written. + ("{ media.role = music }", "media.role = music"), // Colon separator. - ( - "{ \"media.role\": \"music\" }", - "\"media.role\" = \"music\"", - ), + ("{ \"media.role\": \"music\" }", "\"media.role\": \"music\""), // No separator at all — legal SPA-JSON. - ("{ \"media.role\" \"music\" }", "\"media.role\" = \"music\""), - // Nested array, preserved verbatim. + ("{ \"media.role\" \"music\" }", "\"media.role\" \"music\""), + // Nested array and object. ( "{ \"audio.position\" = [ FL, FR ] }", "\"audio.position\" = [ FL, FR ]", ), - // Nested object, preserved verbatim. ("{ \"props\" = { \"a\" = 1 } }", "\"props\" = { \"a\" = 1 }"), + // ⚠️ An ODD number of array elements. This is the case that + // distinguishes recursive validation from a flat token walk: a + // flat walk pairs FL with FR, then finds RC with no partner and + // wrongly rejects a valid object. Found by mutation. + ( + "{ \"audio.position\" = [ FL, FR, RC ] }", + "\"audio.position\" = [ FL, FR, RC ]", + ), // A brace inside a string is not structure. ( "{ \"node.name\" = \"weird}name\" }", @@ -1096,6 +1099,14 @@ mod tests { "# mine\n{ \"media.role\" = \"music\" }", "\"media.role\" = \"music\"", ), + // ⚠️ Round 11 finding 3 — the hand-written scanner REJECTED these + // three valid objects, overwriting the user's routing policy. + ( + "{ target.object, \"my-sink\" }", + "target.object, \"my-sink\"", + ), + ("{ key == \"value\" }", "key == \"value\""), + ("{ \"a\" = \"b\" # note\r }", "\"a\" = \"b\""), ]; for (input, preserved) in cases { let merged = merge_pipewire_props(Some(input), &name, PIPEWIRE_PROPS_ENV); @@ -1120,11 +1131,13 @@ mod tests { } /// ✅ **The exact bytes below were verified against the live daemon** - /// 2026-07-25: each was set as `PIPEWIRE_ALSA` for a real `aplay`, and - /// `pw-dump` showed a `Stream/Output/Audio` node carrying `peerspeak.owned - /// = 1`, the expected `node.name`, **and** the user's own properties - /// (`media.role = music`, `audio.position = [ FL, FR ]`). `spa-json-dump - /// -s` accepts all three. + /// (2026-07-25, and the third re-verified 2026-07-26 after round 11 + /// changed what it emits): each was set as `PIPEWIRE_ALSA` for a real + /// `aplay`, and `pw-dump` showed a `Stream/Output/Audio` node carrying + /// `peerspeak.owned = 1`, the expected `node.name`, **and** the user's own + /// properties (`media.role = music`, `audio.position = [ FL, FR ]`). + /// `spa-json-dump -s` accepts all three. A comma-between-key-and-value and + /// a nested-object case were confirmed live in the same run. /// /// Pinned byte-for-byte rather than asserted with `contains`, because /// what the live run establishes is that *this string* works — a @@ -1151,147 +1164,97 @@ mod tests { ), "{ \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_overwritten_probe\" }" ); - // A bare key, a bare value and a nested array all survive the rebuild - // verbatim — the array in particular is passed through as written. + // A bare key, a bare value and a nested array all survive verbatim — + // the bare key stays bare, which is the round-11 correction. assert_eq!( merge_pipewire_props( Some("{ media.role = music, \"audio.position\" = [ FL, FR ] }"), "f3_exotic_probe", PIPEWIRE_ALSA_ENV ), - "{ \"media.role\" = music, \"audio.position\" = [ FL, FR ], \ + "{ media.role = music, \"audio.position\" = [ FL, FR ], \ \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_exotic_probe\" }" ); } - /// [`scan_token`] is the seam the whole SPA-JSON check rests on, so it is - /// gated on its own postcondition — "a token I return is complete" — - /// rather than only through the merge. + /// **Differential: our acceptance set must equal `spa-json-dump -s`'s.** /// - /// ⚠️ Not redundant with the merge tests, and mutation says so: at the - /// object level an unterminated string or an unbalanced bracket is *also* - /// caught by "the object never closed", so both implementations agree - /// there. They disagree here, which is where the property actually lives. - #[test] - fn scan_token_returns_only_complete_tokens() { - let complete = [ - ("\"music\"", "\"music\""), - // Multi-byte characters: the scan steps by bytes, so a token - // boundary landing inside one would panic the slice. It cannot — - // UTF-8 continuation bytes are never ASCII — and this pins it. - ("\"café ☕\" rest", "\"café ☕\""), - ("café", "café"), - ("\"\\é\" rest", "\"\\é\""), - ("[ \"ünïcode\" ] rest", "[ \"ünïcode\" ]"), - // An escaped quote does not end the string. - ("\"we\\\"ird\" rest", "\"we\\\"ird\""), - // Brackets inside a string are not structure. - ("\"a}b]c\"", "\"a}b]c\""), - ("[ 1, 2 ] rest", "[ 1, 2 ]"), - ( - "{ \"a\" = { \"b\" = 1 } } rest", - "{ \"a\" = { \"b\" = 1 } }", - ), - // A container closing inside a string does not close it. - ("[ \"]\" ] rest", "[ \"]\" ]"), - ("bare-token, rest", "bare-token"), - ("music}", "music"), - ]; - for (input, expected) in complete { - let mut i = 0; - assert_eq!(scan_token(input, &mut i), Some(expected), "{input:?}"); - assert_eq!(&input[..i], expected, "{input:?} advanced past its token"); - } - - let incomplete = [ - // Unterminated string, plain and inside a container. - "\"music", - "\"music\\\"", - "[ \"music ]", - // Unbalanced containers. - "[ 1, 2", - "{ \"a\" = 1", - "[ { } ", - // Mismatched closer. - "[ 1, 2 }", - "{ \"a\" = 1 ]", - // A stray separator where a token belongs. - ",", - "=", - "}", - ]; - for input in incomplete { - let mut i = 0; - assert_eq!(scan_token(input, &mut i), None, "{input:?} is not complete"); - } - } - - /// **The invariant the whole finding is about: no inherited value, however - /// hostile, may cost us a carrier.** + /// This is the check whose absence caused round 11. The previous validator + /// was a hand-written scanner, and nothing compared it to the real grammar + /// — so it drifted in both directions at once and neither the unit tests + /// nor the 20,000-input property test noticed, because both used *it* as + /// their oracle. /// - /// The merge has two outcomes — parse and rebuild, or overwrite — and both - /// are supposed to end with our two pairs present in a well-formed object. - /// A deterministic walk over an alphabet of the exact characters that - /// break SPA-JSON (braces, brackets, quotes, separators, comment marks, - /// escapes, newlines, multi-byte characters) checks that directly, and - /// checks the result parses — an output our own parser rejects would be an - /// output the daemon may well truncate, which is the live failure this - /// finding began with. + /// Now that the validator is libspa itself this should be true by + /// construction, which is exactly why it is worth asserting: if it ever + /// fails, the construction is not what we think it is. + /// + /// `#[ignore]` because it shells out to a PipeWire tool that CI need not + /// have; it is part of the phase 5 matrix procedure. #[test] - fn no_inherited_value_can_cost_us_a_carrier() { - let name = owned_node_name(CLIP_ROLE); - let alphabet = [ - "{", - "}", - "[", - "]", - "\"", - "=", - ":", - ",", - "#", - "\\", - " ", - "\n", - "\t", - "a", - "1", - "é", - "☕", - "peerspeak.owned", - "node.name", - "media.role", - "true", + #[ignore = "needs spa-json-dump"] + fn our_validator_agrees_with_spa_json_dump() { + let corpus = [ + // Valid. + "{ \"a\" = \"b\" }", + "{ }", + "{ media.role = music }", + "{ \"media.role\": \"music\" }", + "{ \"media.role\" \"music\" }", + "{ target.object, \"my-sink\" }", + "{ key == \"value\" }", + "{ \"a\" = \"b\",, \"c\" = \"d\" }", + "{ \"a\" = \"b\", }", + "{ \"audio.position\" = [ FL, FR ] }", + "{ \"props\" = { \"a\" = 1 } }", + "{ \"node.name\" = \"weird}name\" }", + "# mine\n{ \"media.role\" = \"music\" }", + "{ \"a\" = \"b\" # note\r }", + // Invalid. + "{ garbage }", + "{ garbage, \"media.role\" = \"music\" }", + "{ \"media.role\" = \"music }", + "{ \"channels\" = [ 1, 2 }", + "{ \"channels\" = [ 1, 2 } ]", + "{ \"a\" = { \"b\" = 1 }", + "{ \"media.role\" = \"music\" # note }", + "{ \"a\" = 1 } { \"b\" = 2 }", + "{ \"media.role\" = }", + "{ \"foo\" = { garbage } }", + "{ \"a\" = \"\\é\" }", + "{ \"a\" = é }", + "{ \"a\" = foo\\bar }", ]; - // A fixed seed: a failure here must be reproducible, not a flake. - let mut state: u64 = 0x2545_f491_4f6c_dd1d; - let mut next = move || { - state = state - .wrapping_mul(6_364_136_223_846_793_005) - .wrapping_add(1_442_695_040_888_963_407); - (state >> 33) as usize - }; + // ⚠️ The corpus is deliberately all *objects*. `spa-json-dump` accepts + // any SPA-JSON document, so a bare `media.role = music` passes there + // and is rejected here — not a disagreement about the grammar but our + // own additional requirement, since there is no object to splice into. + // The first run of this test flagged exactly that, which is the point + // of having it. `an_empty_or_malformed_pipewire_props_yields_our_value_alone` + // pins the bare-list case. - for _ in 0..20_000 { - let length = next() % 14; - let mut inherited = String::new(); - for _ in 0..length { - inherited.push_str(alphabet[next() % alphabet.len()]); + for value in corpus { + let mut child = std::process::Command::new("spa-json-dump") + .arg("-s") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .expect("spa-json-dump must be installed for this test"); + { + use std::io::Write; + child + .stdin + .as_mut() + .expect("piped stdin") + .write_all(value.as_bytes()) + .expect("write to spa-json-dump"); } - - let merged = merge_pipewire_props(Some(&inherited), &name, PIPEWIRE_ALSA_ENV); - assert!( - merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")), - "carrier 1 lost for inherited {inherited:?}: {merged}" - ); - assert!( - merged.contains(&format!("\"node.name\" = \"{name}\"")), - "carrier 2 lost for inherited {inherited:?}: {merged}" - ); - // And whatever we emit must be an object we can read back. - assert!( - parse_spa_object(&merged).is_some(), - "emitted an object we cannot parse for inherited {inherited:?}: {merged}" + let theirs = child.wait().expect("spa-json-dump exits").success(); + let ours = spa_object_tokens(value).is_some(); + assert_eq!( + ours, theirs, + "disagreement on {value:?}: ours={ours}, spa-json-dump={theirs}" ); } } From 9eab6c118da62485c8b2ea402d256ffb67d52131 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 01:51:14 -0400 Subject: [PATCH 12/13] audio/ownership: let libspa say where the object closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 12 review, finding 1 — a measured fail-open, and the third distinct door into the same failure. A trailing comment is valid SPA-JSON and *ends the document* (`case __COMMENT: return 0` in spa/utils/json-core.h), so an object may close before the last `}` in the string. `merge_pipewire_props` located the closing brace with `rfind('}')`, which is a byte scan and not a parse, so for { "target.object" = "my-sink" } # trailing } it selected the comment's brace and spliced both ownership carriers *into the comment*. The re-validation did not catch it, because the result parses perfectly well — as `{ target.object = "my-sink" }`, with neither carrier present. Confirmed against `spa-json-dump -s`. That is an untagged node, so no taint root, so echo — exactly what rounds 10 and 11 each closed by a different route. Latent rather than live: pixelpass's evaluate() is still audit-only, so today it corrupts an audit classification and becomes a leak when phase 6 consumes eligibility. The whole thesis of round 11 was "do not re-implement someone else's grammar". The scanner went, but this brace hunt stayed behind in the caller, which is the same defect wearing different clothes. So spa_object now reports the object's own closer, taken from libspa: closing a container at depth 0 writes the brace's position back to the parent iterator, and spa_json_enter made `outer` that parent. Read before the trailing check, which advances past it. Also: - whatever followed the object is preserved, so a user's trailing comment survives instead of being silently deleted; - the output check now asks whether the object closes where we put our brace, not merely whether the string parses. A parse-only check is what this finding defeated. Mutation-verified: restoring `rfind` fails the new test, and dropping the tail fails it on the deleted comment. Honest note in the code — mutation cannot distinguish the closer comparison or the is-object test; both are labelled belt-and-braces rather than presented as tested. 621 -> 622 lib tests, fmt clean, clippy clean, and the ignored spa-json-dump differential still agrees. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 198 ++++++++++++++++++++++++++++++++--------- 1 file changed, 156 insertions(+), 42 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index d8ce878..39ea5b8 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -301,11 +301,28 @@ fn malformed_object_warning(var: &str, existing: &str) -> String { ) } +/// A SPA-JSON object that **libspa itself** has validated. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct SpaObject { + /// How many tokens the object holds. Only ever compared against zero, to + /// tell an empty object from one with something worth preserving, so + /// tokens rather than pairs is fine. + tokens: usize, + /// Byte offset of the object's **own** closing `}` within the validated + /// text. + /// + /// ⚠️ **This exists because searching for the brace ourselves was a + /// measured fail-open** (round 12 review, finding 1). The caller used + /// `rfind('}')`, which is a byte scan, not a parse — the last brace in the + /// text is not necessarily the one that closes the object. libspa reports + /// the true position instead: closing a container at depth 0 writes the + /// brace's own position back to the parent iterator + /// (`iter->parent->cur = iter->cur`, `spa/utils/json-core.h`). + closer: usize, +} + /// Validate a SPA-JSON **object** with **libspa's own parser** — the same code -/// the daemon runs — and return how many tokens it holds. -/// -/// The count is only ever compared against zero, to tell an empty object from -/// one with something worth preserving, so tokens rather than pairs is fine. +/// the daemon runs — and report what it found. /// /// ⚠️ **This was a hand-written scanner for one round, and that is worth /// recording** (round 11 review, findings 2 and 3). Checking the outer braces @@ -327,9 +344,13 @@ fn malformed_object_warning(var: &str, existing: &str) -> String { /// directions at once. Using theirs makes the disagreement impossible. /// /// `None` for anything that is not one complete, well-formed object, including -/// trailing text after it. +/// trailing text after it. A trailing **comment** is not trailing text — +/// libspa ends the document at one (`case __COMMENT: return 0`), so +/// `{ "a" = "b" } # note` is a valid object whose closer is not the last brace +/// in the string. That is exactly why [`SpaObject::closer`] is reported here +/// rather than searched for by the caller. #[cfg(target_os = "linux")] -fn spa_object_tokens(text: &str) -> Option { +fn spa_object(text: &str) -> Option { use pipewire::spa::sys as spa; // SAFETY: the iterators borrow `text` and never outlive this call, and @@ -388,12 +409,26 @@ fn spa_object_tokens(text: &str) -> Option { } } + // Where the object actually closes, straight from the parser. + // + // ⚠️ **Read it here, before the trailing check below**, which advances + // `outer` past the brace. Closing a container at depth 0 executes + // `iter->parent->cur = iter->cur` with `cur` still *at* the `}` + // (`spa/utils/json-core.h`), and `spa_json_enter` set `outer` as this + // sub-iterator's parent, so `outer.cur` now points at the brace. + let closer = (outer.cur as usize).checked_sub(text.as_ptr() as usize)?; + if closer >= text.len() || text.as_bytes()[closer] != b'}' { + // The writeback did not land where the contract says it should. + // Refusing is the safe direction: the caller overwrites. + return None; + } + // Nothing may follow the object. let mut trailing: *const std::os::raw::c_char = std::ptr::null(); if spa::spa_json_next(&mut outer, &mut trailing) != 0 { return None; } - Some(tokens) + Some(SpaObject { tokens, closer }) } } @@ -405,7 +440,7 @@ fn spa_object_tokens(text: &str) -> Option { /// The module stays platform-neutral on purpose (see `audio::mod`): the /// literals are a wire contract and their test must run everywhere. #[cfg(not(target_os = "linux"))] -fn spa_object_tokens(_text: &str) -> Option { +fn spa_object(_text: &str) -> Option { None } @@ -419,29 +454,44 @@ fn spa_object_tokens(_text: &str) -> Option { /// Three properties make the splice safe, and all three depend on the object /// having been validated first: /// -/// 1. **The last `}` is the object's own.** Nothing may follow the object, so -/// the final brace closes it. +/// 1. **libspa says where the object closes**; we never look for the brace +/// ourselves. ⚠️ **This point used to read "the last `}` is the object's +/// own", and that was wrong** (round 12 review, finding 1). A trailing +/// comment is not trailing *text* — libspa ends the document at one — so +/// `{ "a" = "b" } # trailing }` is valid and its last brace belongs to the +/// comment. `rfind('}')` picked that one, spliced both carriers into the +/// comment, and the re-validation below still passed because the result +/// parsed fine as `{ a = "b" }`. Measured against `spa-json-dump -s`: +/// semantically no carriers at all, which is the round-10 fail-open reached +/// through a third door. [`SpaObject::closer`] is the fix. /// 2. **Our pairs cannot land inside a comment.** A `#` comment runs to the -/// end of its line, so a validated object's closing brace is always -/// reachable, and the text immediately before it is never mid-comment. -/// Only spaces and tabs are trimmed before appending — never a newline — -/// so a value ending in `# note\n ` keeps its line break. +/// end of its line, so the object's own closing brace is never inside one, +/// and the text immediately before it is never mid-comment. Only spaces and +/// tabs are trimmed before appending — never a newline — so a value ending +/// in `# note\n ` keeps its line break. /// 3. **A leading comma is always legal.** Measured: SPA-JSON treats commas /// as pure separators, accepting `{ "a" = "b",, "c" = "d" }` and even /// `{ target.object, "my-sink" }`. /// -/// And then the result is **validated again before it is returned**. If the -/// splice ever produces something libspa will not parse, we overwrite instead: -/// a lost routing preference is recoverable, a lost carrier is an echo. +/// Anything that followed the object is **preserved**, so a user's trailing +/// comment survives the merge rather than being silently deleted. /// -/// ⚠️ Honest note on that last check and on `spa_object_tokens`'s -/// "must be an object" test: **mutation testing cannot currently distinguish -/// either of them.** Removing the re-validation breaks nothing because the -/// first validation already rejects every input we have; removing the -/// object test breaks nothing because a non-object has no `}` to splice -/// before. They are deliberate belt-and-braces on the one code path where -/// being wrong is a silent echo, not load-bearing logic — and saying so is -/// better than leaving a future reader to assume a test is watching them. +/// And then the result is checked before it is returned — **not merely that it +/// parses, but that it closes where we put our brace**. That asks "are our +/// carriers really members of this object?" rather than "is this string +/// parseable", and a parse-only check is precisely what round 12 defeated. +/// +/// ⚠️ Honest note, and it applies to **two** guards here: that closer +/// comparison, and `spa_object`'s "must be an object" test. **Mutation testing +/// distinguishes neither.** Replacing the comparison with "any parse succeeds" +/// leaves the suite green, because once the closer comes from libspa the +/// splice lands correctly for every input we can construct — only comments and +/// whitespace may follow a validated object, so the tail can never re-open +/// one. Removing the object test changes nothing because a non-object has no +/// `}` to splice before. Both are deliberate belt-and-braces on the one code +/// path where being wrong is a silent echo, not load-bearing logic — and +/// saying so is better than leaving a future reader to assume a test is +/// watching them. /// /// `var` names the variable being merged, and is a parameter rather than a /// constant because both variables take this grammar: the warning used to say @@ -458,35 +508,46 @@ fn merge_pipewire_props(inherited: Option<&str>, node_name: &str, var: &str) -> }; // An empty object carries nothing worth preserving. - let Some(tokens) = spa_object_tokens(existing) else { + let Some(object) = spa_object(existing) else { return overwrite(); }; - if tokens == 0 { + if object.tokens == 0 { return ours; } - - let trimmed = existing.trim_end(); - let Some(closer) = trimmed.rfind('}') else { - return overwrite(); - }; + let closer = object.closer; // A trailing separator of the user's would make ours a double comma. That // is legal SPA-JSON (measured) but it reads like a bug, so drop it. It // cannot be a comma inside a comment: a comment runs to end of line, so an // object whose closing brace followed one on the same line would not have // validated. - let kept = trimmed[..closer] + let kept = existing[..closer] .trim_end_matches([' ', '\t']) .trim_end_matches(','); - let merged = format!( - "{kept}, \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ - \"node.name\" = \"{node_name}\" }}" - ); + // Whatever followed the object is the user's — in practice a comment — and + // goes back untouched. The previous cut silently deleted it, having already + // mistaken part of it for the closing brace. + let tail = &existing[closer + 1..]; - // Never emit a value we cannot parse back. - if spa_object_tokens(&merged).is_none() { - return overwrite(); + let head = format!( + "{kept}, \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ + \"node.name\" = \"{node_name}\" " + ); + let our_closer = head.len(); + let merged = format!("{head}}}{tail}"); + + // **Does the object we just built actually contain our carriers?** + // + // Parsing the result back is not enough on its own, and that is precisely + // how round 12's fail-open survived a re-validation: the emitted text + // parsed cleanly while both carriers sat inside a comment, semantically + // absent. Asking *where libspa thinks the object closes* answers the real + // question. If anything swallowed our pairs, the object closes at some + // earlier brace and this comparison fails, so we overwrite instead of + // shipping a value whose tag is decorative. + match spa_object(&merged) { + Some(check) if check.closer == our_closer => merged, + _ => overwrite(), } - merged } /// Tag the audio this process plays **through ALSA** — in practice everything @@ -979,6 +1040,59 @@ mod tests { assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); } + /// **Round 12 review, finding 1 — a measured fail-open.** + /// + /// A trailing comment is valid SPA-JSON and ends the document, so an + /// object may close *before* the last `}` in the string. The old + /// `rfind('}')` picked the comment's brace and spliced both carriers into + /// the comment; the result still parsed, so the re-validation passed, and + /// `spa-json-dump -s` reported an object with neither carrier in it. + /// + /// The assertion is deliberately **semantic, not textual**: a carrier that + /// appears in the string but after the object's closing brace is exactly + /// the bug, so `contains` would pass on the broken output. Comparing the + /// carrier's offset against the closer libspa reports is what actually + /// distinguishes them. + #[test] + fn a_valid_trailing_comment_does_not_swallow_the_carriers() { + let name = owned_node_name("vlc"); + for existing in [ + // The brace in the comment is the one `rfind` used to find. + "{ \"target.object\" = \"my-sink\" } # trailing }", + // The same thing written the way a person actually would. + "{ \"target.object\" = \"my-sink\" }\n# trailing }", + // No brace in the comment: the carriers were fine here, but the + // comment itself was silently deleted. + "{ \"target.object\" = \"my-sink\" } # plain note", + ] { + let merged = merge_pipewire_props(Some(existing), &name, PIPEWIRE_ALSA_ENV); + let object = spa_object(&merged).expect("merged value is a valid object"); + + let owned_at = merged + .find(OWNED_PROP_KEY) + .unwrap_or_else(|| panic!("no ownership carrier at all in {merged:?}")); + assert!( + owned_at < object.closer, + "carrier sits outside the object it is supposed to tag: {merged:?}" + ); + let name_at = merged.find(&name).expect("node.name carrier present"); + assert!( + name_at < object.closer, + "node.name outside the object: {merged:?}" + ); + + // The user keeps both their routing property and their comment. + assert!( + merged.contains("\"target.object\" = \"my-sink\""), + "routing property lost: {merged:?}" + ); + assert!( + merged.contains('#'), + "the user's comment was deleted: {merged:?}" + ); + } + } + #[test] fn an_empty_or_malformed_pipewire_props_yields_our_value_alone() { let name = owned_node_name("vlc"); @@ -1251,7 +1365,7 @@ mod tests { .expect("write to spa-json-dump"); } let theirs = child.wait().expect("spa-json-dump exits").success(); - let ours = spa_object_tokens(value).is_some(); + let ours = spa_object(value).is_some(); assert_eq!( ours, theirs, "disagreement on {value:?}: ours={ours}, spa-json-dump={theirs}" From c82ef07464120b1e67ebdc117d48dc4f1461042c Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 01:59:36 -0400 Subject: [PATCH 13/13] audio/ownership: take the depth ceiling from the consumer, not the grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 12 review, finding 2 — filed as P2, and the interesting part is that its author retracted it to P3 once we had measurements, while the remedy it originally proposed would have been a fail-open. The finding was that our validator rejects nesting `spa-json-dump -s` accepts, and the suggested fix was a recursive sub-iterator walk to match the dump tool. Both halves rest on the dump tool being the reference. It is not. Nothing reads `PIPEWIRE_PROPS` or `PIPEWIRE_ALSA` with `spa-json-dump`; `pw_properties_update_string` does, in the client process. Measured live on this host, against the real ALSA plugin: depth 513 dump accept plugin accept ours accept depth 514 dump accept plugin accept ours REJECT depth 515 dump accept plugin REJECT ours reject depth 1000 dump accept plugin REJECT ours reject At 515 the plugin discards the whole object: the node came back as `alsa_playback.aplay` with no properties at all. So matching the dump tool would have made us splice carriers into values the consumer throws away wholesale — losing both, which is the echo this feature exists to prevent. Over-rejecting costs a routing preference; over-accepting costs a carrier. Those are not the same price. What was genuinely wrong is narrower: we sat exactly one level below the consumer. `pw_properties_update_string` calls `spa_json_container_len` on a container value, which enters one more sub-iterator before its flat walk, and that single level is the entire discrepancy. Doing the same puts the boundaries on the same number. Codex reached the same three numbers independently by calling `pw_properties_update_string_checked(NULL, ...)` directly, having disassembled both call sites; I measured through the live plugin. Two methods, one table. The dump differential stays, but it is now labelled a *grammar* oracle with a warning not to add deep values — it would fail by design. The acceptance oracle is the new boundary test. Mutation-verified: removing the container step fails the 514 assertion. 622 -> 623 lib tests, fmt clean, clippy clean. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 82 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 39ea5b8..481f0db 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -407,6 +407,31 @@ fn spa_object(text: &str) -> Option { if tokens > text.len() { return None; } + + // **Step over a container the way the real consumer does.** + // + // Both variables are parsed by `pw_properties_update_string`, which + // walks top-level pairs and calls `spa_json_container_len` on a + // container value. That helper enters one more sub-iterator before + // its flat walk, and that single extra level is the whole reason + // our ceiling used to sit one below the consumer's: measured, we + // accepted depth 513 and rejected 514, while the ALSA plugin + // accepts 514 and rejects 515. Doing what it does puts the two + // boundaries on the same number instead of one apart. + // + // ⚠️ **Matching libspa's *grammar* here would be a bug, not a + // fix.** `spa-json-dump` accepts arbitrary nesting, so a recursive + // walk would make us accept depth 515+, which every real consumer + // rejects outright — we would splice carriers into a value the + // plugin discards wholesale and lose both. Measured live: at depth + // 515 the node came back as `alsa_playback.aplay` with no + // properties at all. The dump tool is the grammar oracle; it is + // not the acceptance oracle. + if spa::spa_json_is_container(token, length) != 0 + && spa::spa_json_container_len(&mut object, token, length) <= 0 + { + return None; + } } // Where the object actually closes, straight from the parser. @@ -1040,6 +1065,49 @@ mod tests { assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); } + /// **The ceiling we match is the consumer's, not the grammar's** (round 12 + /// review, finding 2 — filed as P2, retracted to P3 by its author once the + /// operational boundary was measured, and the remedy it originally + /// suggested would have been a fail-open). + /// + /// `spa-json-dump -s` accepts arbitrarily deep nesting, so "agree with the + /// dump tool" would mean accepting depth 515+. Measured on this host + /// against the live ALSA plugin — the actual reader of `PIPEWIRE_ALSA` — + /// depth 515 is rejected *wholesale*: the node came back as + /// `alsa_playback.aplay` carrying no properties at all, so a merge into + /// such a value loses both carriers and echoes. Codex reached the same + /// three numbers independently by calling + /// `pw_properties_update_string_checked(NULL, …)` directly. + /// + /// | depth | `spa-json-dump` | property updater | here | + /// | --- | --- | --- | --- | + /// | 513 | accept | accept | accept | + /// | 514 | accept | accept | accept | + /// | 515 | accept | **reject** | **reject** | + /// + /// Over-rejecting is the safe direction — it costs a routing preference, + /// not a carrier — but there is no reason to sit a level below the + /// consumer when `spa_json_container_len` puts us exactly on it. + #[cfg(target_os = "linux")] + #[test] + fn the_depth_ceiling_matches_the_property_updater_not_the_grammar() { + let nested = |n: usize| format!("{{ \"x\" = {}{} }}", "[".repeat(n), "]".repeat(n)); + assert!(spa_object(&nested(1)).is_some(), "a shallow container"); + assert!( + spa_object(&nested(513)).is_some(), + "513 is accepted by both" + ); + assert!( + spa_object(&nested(514)).is_some(), + "514 is accepted by the property updater, so it must be accepted here" + ); + assert!( + spa_object(&nested(515)).is_none(), + "515 is rejected by the property updater, so accepting it would be a fail-open" + ); + assert!(spa_object(&nested(1000)).is_none(), "far past the ceiling"); + } + /// **Round 12 review, finding 1 — a measured fail-open.** /// /// A trailing comment is valid SPA-JSON and ends the document, so an @@ -1291,7 +1359,8 @@ mod tests { ); } - /// **Differential: our acceptance set must equal `spa-json-dump -s`'s.** + /// **Differential against `spa-json-dump -s` — a *grammar* oracle, and + /// only that.** /// /// This is the check whose absence caused round 11. The previous validator /// was a hand-written scanner, and nothing compared it to the real grammar @@ -1303,6 +1372,17 @@ mod tests { /// construction, which is exactly why it is worth asserting: if it ever /// fails, the construction is not what we think it is. /// + /// ⚠️ **Do not add deeply nested values to this corpus.** The dump tool + /// accepts nesting that the property updater — the code that actually + /// reads these variables — rejects, and we deliberately follow the + /// updater. A depth-515 entry here would fail this test *by design*; the + /// boundary is owned by + /// [`the_depth_ceiling_matches_the_property_updater_not_the_grammar`], + /// which is the acceptance oracle. Keeping the two apart is the point: + /// this one asks "do we read the grammar correctly", that one asks "do we + /// accept what the consumer accepts", and round 12 turned on the gap + /// between those questions. + /// /// `#[ignore]` because it shells out to a PipeWire tool that CI need not /// have; it is part of the phase 5 matrix procedure. #[test]