diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index d8ce878..39ea5b8 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -301,11 +301,28 @@ fn malformed_object_warning(var: &str, existing: &str) -> String { ) } +/// A SPA-JSON object that **libspa itself** has validated. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct SpaObject { + /// How many tokens the object holds. Only ever compared against zero, to + /// tell an empty object from one with something worth preserving, so + /// tokens rather than pairs is fine. + tokens: usize, + /// Byte offset of the object's **own** closing `}` within the validated + /// text. + /// + /// ⚠️ **This exists because searching for the brace ourselves was a + /// measured fail-open** (round 12 review, finding 1). The caller used + /// `rfind('}')`, which is a byte scan, not a parse — the last brace in the + /// text is not necessarily the one that closes the object. libspa reports + /// the true position instead: closing a container at depth 0 writes the + /// brace's own position back to the parent iterator + /// (`iter->parent->cur = iter->cur`, `spa/utils/json-core.h`). + closer: usize, +} + /// Validate a SPA-JSON **object** with **libspa's own parser** — the same code -/// the daemon runs — and return how many tokens it holds. -/// -/// The count is only ever compared against zero, to tell an empty object from -/// one with something worth preserving, so tokens rather than pairs is fine. +/// the daemon runs — and report what it found. /// /// ⚠️ **This was a hand-written scanner for one round, and that is worth /// recording** (round 11 review, findings 2 and 3). Checking the outer braces @@ -327,9 +344,13 @@ fn malformed_object_warning(var: &str, existing: &str) -> String { /// directions at once. Using theirs makes the disagreement impossible. /// /// `None` for anything that is not one complete, well-formed object, including -/// trailing text after it. +/// trailing text after it. A trailing **comment** is not trailing text — +/// libspa ends the document at one (`case __COMMENT: return 0`), so +/// `{ "a" = "b" } # note` is a valid object whose closer is not the last brace +/// in the string. That is exactly why [`SpaObject::closer`] is reported here +/// rather than searched for by the caller. #[cfg(target_os = "linux")] -fn spa_object_tokens(text: &str) -> Option { +fn spa_object(text: &str) -> Option { use pipewire::spa::sys as spa; // SAFETY: the iterators borrow `text` and never outlive this call, and @@ -388,12 +409,26 @@ fn spa_object_tokens(text: &str) -> Option { } } + // Where the object actually closes, straight from the parser. + // + // ⚠️ **Read it here, before the trailing check below**, which advances + // `outer` past the brace. Closing a container at depth 0 executes + // `iter->parent->cur = iter->cur` with `cur` still *at* the `}` + // (`spa/utils/json-core.h`), and `spa_json_enter` set `outer` as this + // sub-iterator's parent, so `outer.cur` now points at the brace. + let closer = (outer.cur as usize).checked_sub(text.as_ptr() as usize)?; + if closer >= text.len() || text.as_bytes()[closer] != b'}' { + // The writeback did not land where the contract says it should. + // Refusing is the safe direction: the caller overwrites. + return None; + } + // Nothing may follow the object. let mut trailing: *const std::os::raw::c_char = std::ptr::null(); if spa::spa_json_next(&mut outer, &mut trailing) != 0 { return None; } - Some(tokens) + Some(SpaObject { tokens, closer }) } } @@ -405,7 +440,7 @@ fn spa_object_tokens(text: &str) -> Option { /// The module stays platform-neutral on purpose (see `audio::mod`): the /// literals are a wire contract and their test must run everywhere. #[cfg(not(target_os = "linux"))] -fn spa_object_tokens(_text: &str) -> Option { +fn spa_object(_text: &str) -> Option { None } @@ -419,29 +454,44 @@ fn spa_object_tokens(_text: &str) -> Option { /// Three properties make the splice safe, and all three depend on the object /// having been validated first: /// -/// 1. **The last `}` is the object's own.** Nothing may follow the object, so -/// the final brace closes it. +/// 1. **libspa says where the object closes**; we never look for the brace +/// ourselves. ⚠️ **This point used to read "the last `}` is the object's +/// own", and that was wrong** (round 12 review, finding 1). A trailing +/// comment is not trailing *text* — libspa ends the document at one — so +/// `{ "a" = "b" } # trailing }` is valid and its last brace belongs to the +/// comment. `rfind('}')` picked that one, spliced both carriers into the +/// comment, and the re-validation below still passed because the result +/// parsed fine as `{ a = "b" }`. Measured against `spa-json-dump -s`: +/// semantically no carriers at all, which is the round-10 fail-open reached +/// through a third door. [`SpaObject::closer`] is the fix. /// 2. **Our pairs cannot land inside a comment.** A `#` comment runs to the -/// end of its line, so a validated object's closing brace is always -/// reachable, and the text immediately before it is never mid-comment. -/// Only spaces and tabs are trimmed before appending — never a newline — -/// so a value ending in `# note\n ` keeps its line break. +/// end of its line, so the object's own closing brace is never inside one, +/// and the text immediately before it is never mid-comment. Only spaces and +/// tabs are trimmed before appending — never a newline — so a value ending +/// in `# note\n ` keeps its line break. /// 3. **A leading comma is always legal.** Measured: SPA-JSON treats commas /// as pure separators, accepting `{ "a" = "b",, "c" = "d" }` and even /// `{ target.object, "my-sink" }`. /// -/// And then the result is **validated again before it is returned**. If the -/// splice ever produces something libspa will not parse, we overwrite instead: -/// a lost routing preference is recoverable, a lost carrier is an echo. +/// Anything that followed the object is **preserved**, so a user's trailing +/// comment survives the merge rather than being silently deleted. /// -/// ⚠️ Honest note on that last check and on `spa_object_tokens`'s -/// "must be an object" test: **mutation testing cannot currently distinguish -/// either of them.** Removing the re-validation breaks nothing because the -/// first validation already rejects every input we have; removing the -/// object test breaks nothing because a non-object has no `}` to splice -/// before. They are deliberate belt-and-braces on the one code path where -/// being wrong is a silent echo, not load-bearing logic — and saying so is -/// better than leaving a future reader to assume a test is watching them. +/// And then the result is checked before it is returned — **not merely that it +/// parses, but that it closes where we put our brace**. That asks "are our +/// carriers really members of this object?" rather than "is this string +/// parseable", and a parse-only check is precisely what round 12 defeated. +/// +/// ⚠️ Honest note, and it applies to **two** guards here: that closer +/// comparison, and `spa_object`'s "must be an object" test. **Mutation testing +/// distinguishes neither.** Replacing the comparison with "any parse succeeds" +/// leaves the suite green, because once the closer comes from libspa the +/// splice lands correctly for every input we can construct — only comments and +/// whitespace may follow a validated object, so the tail can never re-open +/// one. Removing the object test changes nothing because a non-object has no +/// `}` to splice before. Both are deliberate belt-and-braces on the one code +/// path where being wrong is a silent echo, not load-bearing logic — and +/// saying so is better than leaving a future reader to assume a test is +/// watching them. /// /// `var` names the variable being merged, and is a parameter rather than a /// constant because both variables take this grammar: the warning used to say @@ -458,35 +508,46 @@ fn merge_pipewire_props(inherited: Option<&str>, node_name: &str, var: &str) -> }; // An empty object carries nothing worth preserving. - let Some(tokens) = spa_object_tokens(existing) else { + let Some(object) = spa_object(existing) else { return overwrite(); }; - if tokens == 0 { + if object.tokens == 0 { return ours; } - - let trimmed = existing.trim_end(); - let Some(closer) = trimmed.rfind('}') else { - return overwrite(); - }; + let closer = object.closer; // A trailing separator of the user's would make ours a double comma. That // is legal SPA-JSON (measured) but it reads like a bug, so drop it. It // cannot be a comma inside a comment: a comment runs to end of line, so an // object whose closing brace followed one on the same line would not have // validated. - let kept = trimmed[..closer] + let kept = existing[..closer] .trim_end_matches([' ', '\t']) .trim_end_matches(','); - let merged = format!( - "{kept}, \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ - \"node.name\" = \"{node_name}\" }}" - ); + // Whatever followed the object is the user's — in practice a comment — and + // goes back untouched. The previous cut silently deleted it, having already + // mistaken part of it for the closing brace. + let tail = &existing[closer + 1..]; - // Never emit a value we cannot parse back. - if spa_object_tokens(&merged).is_none() { - return overwrite(); + let head = format!( + "{kept}, \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \ + \"node.name\" = \"{node_name}\" " + ); + let our_closer = head.len(); + let merged = format!("{head}}}{tail}"); + + // **Does the object we just built actually contain our carriers?** + // + // Parsing the result back is not enough on its own, and that is precisely + // how round 12's fail-open survived a re-validation: the emitted text + // parsed cleanly while both carriers sat inside a comment, semantically + // absent. Asking *where libspa thinks the object closes* answers the real + // question. If anything swallowed our pairs, the object closes at some + // earlier brace and this comparison fails, so we overwrite instead of + // shipping a value whose tag is decorative. + match spa_object(&merged) { + Some(check) if check.closer == our_closer => merged, + _ => overwrite(), } - merged } /// Tag the audio this process plays **through ALSA** — in practice everything @@ -979,6 +1040,59 @@ mod tests { assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); } + /// **Round 12 review, finding 1 — a measured fail-open.** + /// + /// A trailing comment is valid SPA-JSON and ends the document, so an + /// object may close *before* the last `}` in the string. The old + /// `rfind('}')` picked the comment's brace and spliced both carriers into + /// the comment; the result still parsed, so the re-validation passed, and + /// `spa-json-dump -s` reported an object with neither carrier in it. + /// + /// The assertion is deliberately **semantic, not textual**: a carrier that + /// appears in the string but after the object's closing brace is exactly + /// the bug, so `contains` would pass on the broken output. Comparing the + /// carrier's offset against the closer libspa reports is what actually + /// distinguishes them. + #[test] + fn a_valid_trailing_comment_does_not_swallow_the_carriers() { + let name = owned_node_name("vlc"); + for existing in [ + // The brace in the comment is the one `rfind` used to find. + "{ \"target.object\" = \"my-sink\" } # trailing }", + // The same thing written the way a person actually would. + "{ \"target.object\" = \"my-sink\" }\n# trailing }", + // No brace in the comment: the carriers were fine here, but the + // comment itself was silently deleted. + "{ \"target.object\" = \"my-sink\" } # plain note", + ] { + let merged = merge_pipewire_props(Some(existing), &name, PIPEWIRE_ALSA_ENV); + let object = spa_object(&merged).expect("merged value is a valid object"); + + let owned_at = merged + .find(OWNED_PROP_KEY) + .unwrap_or_else(|| panic!("no ownership carrier at all in {merged:?}")); + assert!( + owned_at < object.closer, + "carrier sits outside the object it is supposed to tag: {merged:?}" + ); + let name_at = merged.find(&name).expect("node.name carrier present"); + assert!( + name_at < object.closer, + "node.name outside the object: {merged:?}" + ); + + // The user keeps both their routing property and their comment. + assert!( + merged.contains("\"target.object\" = \"my-sink\""), + "routing property lost: {merged:?}" + ); + assert!( + merged.contains('#'), + "the user's comment was deleted: {merged:?}" + ); + } + } + #[test] fn an_empty_or_malformed_pipewire_props_yields_our_value_alone() { let name = owned_node_name("vlc"); @@ -1251,7 +1365,7 @@ mod tests { .expect("write to spa-json-dump"); } let theirs = child.wait().expect("spa-json-dump exits").success(); - let ours = spa_object_tokens(value).is_some(); + let ours = spa_object(value).is_some(); assert_eq!( ours, theirs, "disagreement on {value:?}: ours={ours}, spa-json-dump={theirs}"