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

audio/ownership: let libspa say where the object closes

Browse Source 
Round 12 review, finding 1 — a measured fail-open, and the third
distinct door into the same failure.

A trailing comment is valid SPA-JSON and *ends the document*
(`case __COMMENT: return 0` in spa/utils/json-core.h), so an object may
close before the last `}` in the string. `merge_pipewire_props` located
the closing brace with `rfind('}')`, which is a byte scan and not a
parse, so for

    { "target.object" = "my-sink" } # trailing }

it selected the comment's brace and spliced both ownership carriers
*into the comment*. The re-validation did not catch it, because the
result parses perfectly well — as `{ target.object = "my-sink" }`, with
neither carrier present. Confirmed against `spa-json-dump -s`.

That is an untagged node, so no taint root, so echo — exactly what
rounds 10 and 11 each closed by a different route. Latent rather than
live: pixelpass's evaluate() is still audit-only, so today it corrupts
an audit classification and becomes a leak when phase 6 consumes
eligibility.

The whole thesis of round 11 was "do not re-implement someone else's
grammar". The scanner went, but this brace hunt stayed behind in the
caller, which is the same defect wearing different clothes.

So spa_object now reports the object's own closer, taken from libspa:
closing a container at depth 0 writes the brace's position back to the
parent iterator, and spa_json_enter made `outer` that parent. Read
before the trailing check, which advances past it.

Also:
- whatever followed the object is preserved, so a user's trailing
  comment survives instead of being silently deleted;
- the output check now asks whether the object closes where we put our
  brace, not merely whether the string parses. A parse-only check is
  what this finding defeated.

Mutation-verified: restoring `rfind` fails the new test, and dropping
the tail fails it on the deleted comment. Honest note in the code —
mutation cannot distinguish the closer comparison or the is-object
test; both are labelled belt-and-braces rather than presented as
tested.

621 -> 622 lib tests, fmt clean, clippy clean, and the ignored
spa-json-dump differential still agrees.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
mollusk
2026-07-26 01:51:14 -04:00
co-authored by Claude Opus 5
parent 21ba633825
commit 9eab6c118d
1 changed files with 156 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/audio/ownership.rs
+156 -42
View File
@@ -301,11 +301,28 @@ 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.
/// 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.
///
/// 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 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 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<usize> {
fn spa_object(text: &str) -> Option<SpaObject> {
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<usize> {
}
}
// 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<usize> {
/// 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> {
fn spa_object(_text: &str) -> Option<SpaObject> {
None
}
@@ -419,29 +454,44 @@ fn spa_object_tokens(_text: &str) -> Option<usize> {
/// 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}"