From 21ba6338254d5a47801da88bf434237edff38c20 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 01:08:17 -0400 Subject: [PATCH] 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}" ); } }