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\" = ")