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

audio/ownership: validate inherited SPA-JSON with libspa, not a scanner

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
mollusk
2026-07-26 01:08:17 -04:00
co-authored by Claude Opus 5
parent 45b1b97dd8
commit 21ba633825
1 changed files with 328 additions and 365 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/audio/ownership.rs
+326 -363
View File
@@ -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<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. /// 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 /// 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<usize> {
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>();
spa::spa_json_init(
&mut outer,
text.as_ptr().cast::<std::os::raw::c_char>(),
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>();
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<usize> {
None
}
/// `PIPEWIRE_PROPS` and `PIPEWIRE_ALSA` are SPA-JSON objects, so merging is /// `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 /// Anything that is not a complete, well-formed object — a bare `key = value`
/// `key = value` list, a truncated object, an unterminated string, a key with /// list, a truncated object, an unterminated string, a key with no value, an
/// no value — is logged and overwritten rather than guessed at. See /// invalid escape — is logged and overwritten rather than guessed at.
/// [`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 /// 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 /// 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 { fn merge_pipewire_props(inherited: Option<&str>, node_name: &str, var: &str) -> String {
let ours = pipewire_props_value(node_name); let ours = pipewire_props_value(node_name);
let Some(existing) = inherited else { let Some(existing) = inherited else {
return ours; return ours;
}; };
match parse_spa_object(existing) {
// An empty object carries nothing to preserve. let overwrite = || {
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)); crate::log_msg(&malformed_object_warning(var, existing));
ours 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 /// Tag the audio this process plays **through ALSA** — in practice everything
@@ -915,6 +870,30 @@ mod tests {
call < gui, call < gui,
"the tag must be set before the GUI starts any thread" "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 /// 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\" = 1 } { \"b\" = 2 }",
// A dangling assignment. // A dangling assignment.
"{ \"media.role\" = }", "{ \"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 { for value in malformed {
assert_eq!( assert_eq!(
@@ -1062,30 +1052,43 @@ mod tests {
} }
} }
/// The other half of finding 3: values that *are* well-formed must still /// The other half of finding 3: values that *are* well-formed must survive,
/// round-trip, or the fix would be "overwrite everything" wearing a parser /// or the fix would be "overwrite everything" wearing a parser as a
/// as a disguise. Rebuilding — rather than splicing — is what makes the /// disguise.
/// result independent of the input's formatting. ///
/// ⚠️ **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] #[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 name = owned_node_name("vlc");
let cases = [ let cases = [
// Bare key, bare value. // Bare key, bare value — kept bare, exactly as written.
("{ media.role = music }", "\"media.role\" = music"), ("{ media.role = music }", "media.role = music"),
// Colon separator. // Colon separator.
( ("{ \"media.role\": \"music\" }", "\"media.role\": \"music\""),
"{ \"media.role\": \"music\" }",
"\"media.role\" = \"music\"",
),
// No separator at all — legal SPA-JSON. // No separator at all — legal SPA-JSON.
("{ \"media.role\" \"music\" }", "\"media.role\" = \"music\""), ("{ \"media.role\" \"music\" }", "\"media.role\" \"music\""),
// Nested array, preserved verbatim. // Nested array and object.
( (
"{ \"audio.position\" = [ FL, FR ] }", "{ \"audio.position\" = [ FL, FR ] }",
"\"audio.position\" = [ FL, FR ]", "\"audio.position\" = [ FL, FR ]",
), ),
// Nested object, preserved verbatim.
("{ \"props\" = { \"a\" = 1 } }", "\"props\" = { \"a\" = 1 }"), ("{ \"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. // A brace inside a string is not structure.
( (
"{ \"node.name\" = \"weird}name\" }", "{ \"node.name\" = \"weird}name\" }",
@@ -1096,6 +1099,14 @@ mod tests {
"# mine\n{ \"media.role\" = \"music\" }", "# mine\n{ \"media.role\" = \"music\" }",
"\"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 { for (input, preserved) in cases {
let merged = merge_pipewire_props(Some(input), &name, PIPEWIRE_PROPS_ENV); 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** /// ✅ **The exact bytes below were verified against the live daemon**
/// 2026-07-25: each was set as `PIPEWIRE_ALSA` for a real `aplay`, and /// (2026-07-25, and the third re-verified 2026-07-26 after round 11
/// `pw-dump` showed a `Stream/Output/Audio` node carrying `peerspeak.owned /// changed what it emits): each was set as `PIPEWIRE_ALSA` for a real
/// = 1`, the expected `node.name`, **and** the user's own properties /// `aplay`, and `pw-dump` showed a `Stream/Output/Audio` node carrying
/// (`media.role = music`, `audio.position = [ FL, FR ]`). `spa-json-dump /// `peerspeak.owned = 1`, the expected `node.name`, **and** the user's own
/// -s` accepts all three. /// 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 /// Pinned byte-for-byte rather than asserted with `contains`, because
/// what the live run establishes is that *this string* works — a /// 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\" }" "{ \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_overwritten_probe\" }"
); );
// A bare key, a bare value and a nested array all survive the rebuild // A bare key, a bare value and a nested array all survive verbatim —
// verbatim — the array in particular is passed through as written. // the bare key stays bare, which is the round-11 correction.
assert_eq!( assert_eq!(
merge_pipewire_props( merge_pipewire_props(
Some("{ media.role = music, \"audio.position\" = [ FL, FR ] }"), Some("{ media.role = music, \"audio.position\" = [ FL, FR ] }"),
"f3_exotic_probe", "f3_exotic_probe",
PIPEWIRE_ALSA_ENV 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\" }" \"peerspeak.owned\" = \"1\", \"node.name\" = \"f3_exotic_probe\" }"
); );
} }
/// [`scan_token`] is the seam the whole SPA-JSON check rests on, so it is /// **Differential: our acceptance set must equal `spa-json-dump -s`'s.**
/// 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 /// This is the check whose absence caused round 11. The previous validator
/// object level an unterminated string or an unbalanced bracket is *also* /// was a hand-written scanner, and nothing compared it to the real grammar
/// caught by "the object never closed", so both implementations agree /// — so it drifted in both directions at once and neither the unit tests
/// there. They disagree here, which is where the property actually lives. /// nor the 20,000-input property test noticed, because both used *it* as
#[test] /// their oracle.
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.**
/// ///
/// The merge has two outcomes — parse and rebuild, or overwrite — and both /// Now that the validator is libspa itself this should be true by
/// are supposed to end with our two pairs present in a well-formed object. /// construction, which is exactly why it is worth asserting: if it ever
/// A deterministic walk over an alphabet of the exact characters that /// fails, the construction is not what we think it is.
/// break SPA-JSON (braces, brackets, quotes, separators, comment marks, ///
/// escapes, newlines, multi-byte characters) checks that directly, and /// `#[ignore]` because it shells out to a PipeWire tool that CI need not
/// checks the result parses — an output our own parser rejects would be an /// have; it is part of the phase 5 matrix procedure.
/// output the daemon may well truncate, which is the live failure this
/// finding began with.
#[test] #[test]
fn no_inherited_value_can_cost_us_a_carrier() { #[ignore = "needs spa-json-dump"]
let name = owned_node_name(CLIP_ROLE); fn our_validator_agrees_with_spa_json_dump() {
let alphabet = [ 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 ] }",
"\n", "{ \"props\" = { \"a\" = 1 } }",
"\t", "{ \"node.name\" = \"weird}name\" }",
"a", "# mine\n{ \"media.role\" = \"music\" }",
"1", "{ \"a\" = \"b\" # note\r }",
"é", // Invalid.
"☕", "{ garbage }",
"peerspeak.owned", "{ garbage, \"media.role\" = \"music\" }",
"node.name", "{ \"media.role\" = \"music }",
"media.role", "{ \"channels\" = [ 1, 2 }",
"true", "{ \"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. // ⚠️ The corpus is deliberately all *objects*. `spa-json-dump` accepts
let mut state: u64 = 0x2545_f491_4f6c_dd1d; // any SPA-JSON document, so a bare `media.role = music` passes there
let mut next = move || { // and is rejected here — not a disagreement about the grammar but our
state = state // own additional requirement, since there is no object to splice into.
.wrapping_mul(6_364_136_223_846_793_005) // The first run of this test flagged exactly that, which is the point
.wrapping_add(1_442_695_040_888_963_407); // of having it. `an_empty_or_malformed_pipewire_props_yields_our_value_alone`
(state >> 33) as usize // pins the bare-list case.
};
for _ in 0..20_000 { for value in corpus {
let length = next() % 14; let mut child = std::process::Command::new("spa-json-dump")
let mut inherited = String::new(); .arg("-s")
for _ in 0..length { .stdin(std::process::Stdio::piped())
inherited.push_str(alphabet[next() % alphabet.len()]); .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 theirs = child.wait().expect("spa-json-dump exits").success();
let merged = merge_pipewire_props(Some(&inherited), &name, PIPEWIRE_ALSA_ENV); let ours = spa_object_tokens(value).is_some();
assert!( assert_eq!(
merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")), ours, theirs,
"carrier 1 lost for inherited {inherited:?}: {merged}" "disagreement on {value:?}: ours={ours}, spa-json-dump={theirs}"
);
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}"
); );
} }
} }