diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 39ea5b8..481f0db 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -407,6 +407,31 @@ fn spa_object(text: &str) -> Option { if tokens > text.len() { return None; } + + // **Step over a container the way the real consumer does.** + // + // Both variables are parsed by `pw_properties_update_string`, which + // walks top-level pairs and calls `spa_json_container_len` on a + // container value. That helper enters one more sub-iterator before + // its flat walk, and that single extra level is the whole reason + // our ceiling used to sit one below the consumer's: measured, we + // accepted depth 513 and rejected 514, while the ALSA plugin + // accepts 514 and rejects 515. Doing what it does puts the two + // boundaries on the same number instead of one apart. + // + // ⚠️ **Matching libspa's *grammar* here would be a bug, not a + // fix.** `spa-json-dump` accepts arbitrary nesting, so a recursive + // walk would make us accept depth 515+, which every real consumer + // rejects outright — we would splice carriers into a value the + // plugin discards wholesale and lose both. Measured live: at depth + // 515 the node came back as `alsa_playback.aplay` with no + // properties at all. The dump tool is the grammar oracle; it is + // not the acceptance oracle. + if spa::spa_json_is_container(token, length) != 0 + && spa::spa_json_container_len(&mut object, token, length) <= 0 + { + return None; + } } // Where the object actually closes, straight from the parser. @@ -1040,6 +1065,49 @@ mod tests { assert!(merged.contains("\"media.role\" = \"music\""), "{merged}"); } + /// **The ceiling we match is the consumer's, not the grammar's** (round 12 + /// review, finding 2 — filed as P2, retracted to P3 by its author once the + /// operational boundary was measured, and the remedy it originally + /// suggested would have been a fail-open). + /// + /// `spa-json-dump -s` accepts arbitrarily deep nesting, so "agree with the + /// dump tool" would mean accepting depth 515+. Measured on this host + /// against the live ALSA plugin — the actual reader of `PIPEWIRE_ALSA` — + /// depth 515 is rejected *wholesale*: the node came back as + /// `alsa_playback.aplay` carrying no properties at all, so a merge into + /// such a value loses both carriers and echoes. Codex reached the same + /// three numbers independently by calling + /// `pw_properties_update_string_checked(NULL, …)` directly. + /// + /// | depth | `spa-json-dump` | property updater | here | + /// | --- | --- | --- | --- | + /// | 513 | accept | accept | accept | + /// | 514 | accept | accept | accept | + /// | 515 | accept | **reject** | **reject** | + /// + /// Over-rejecting is the safe direction — it costs a routing preference, + /// not a carrier — but there is no reason to sit a level below the + /// consumer when `spa_json_container_len` puts us exactly on it. + #[cfg(target_os = "linux")] + #[test] + fn the_depth_ceiling_matches_the_property_updater_not_the_grammar() { + let nested = |n: usize| format!("{{ \"x\" = {}{} }}", "[".repeat(n), "]".repeat(n)); + assert!(spa_object(&nested(1)).is_some(), "a shallow container"); + assert!( + spa_object(&nested(513)).is_some(), + "513 is accepted by both" + ); + assert!( + spa_object(&nested(514)).is_some(), + "514 is accepted by the property updater, so it must be accepted here" + ); + assert!( + spa_object(&nested(515)).is_none(), + "515 is rejected by the property updater, so accepting it would be a fail-open" + ); + assert!(spa_object(&nested(1000)).is_none(), "far past the ceiling"); + } + /// **Round 12 review, finding 1 — a measured fail-open.** /// /// A trailing comment is valid SPA-JSON and ends the document, so an @@ -1291,7 +1359,8 @@ mod tests { ); } - /// **Differential: our acceptance set must equal `spa-json-dump -s`'s.** + /// **Differential against `spa-json-dump -s` — a *grammar* oracle, and + /// only that.** /// /// 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 @@ -1303,6 +1372,17 @@ mod tests { /// construction, which is exactly why it is worth asserting: if it ever /// fails, the construction is not what we think it is. /// + /// ⚠️ **Do not add deeply nested values to this corpus.** The dump tool + /// accepts nesting that the property updater — the code that actually + /// reads these variables — rejects, and we deliberately follow the + /// updater. A depth-515 entry here would fail this test *by design*; the + /// boundary is owned by + /// [`the_depth_ceiling_matches_the_property_updater_not_the_grammar`], + /// which is the acceptance oracle. Keeping the two apart is the point: + /// this one asks "do we read the grammar correctly", that one asks "do we + /// accept what the consumer accepts", and round 12 turned on the gap + /// between those questions. + /// /// `#[ignore]` because it shells out to a PipeWire tool that CI need not /// have; it is part of the phase 5 matrix procedure. #[test]