mollusk/peerspeak
Public Access
1
0
Fork 0
Code Issues Pull Requests 3 Actions 3 Packages Projects Releases 10 Wiki Activity

audio/ownership: parse inherited SPA-JSON instead of trusting its braces

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
mollusk
2026-07-25 23:47:32 -04:00
co-authored by Claude Opus 5
parent d63db68318
commit 6fc55a286d
1 changed files with 485 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/audio/ownership.rs
+485 -34
View File
@@ -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<Vec<SpaPair>> {
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\" = ")