audio/ownership: take the depth ceiling from the consumer, not the grammar
Round 12 review, finding 2 — filed as P2, and the interesting part is
that its author retracted it to P3 once we had measurements, while the
remedy it originally proposed would have been a fail-open.
The finding was that our validator rejects nesting `spa-json-dump -s`
accepts, and the suggested fix was a recursive sub-iterator walk to
match the dump tool. Both halves rest on the dump tool being the
reference. It is not. Nothing reads `PIPEWIRE_PROPS` or `PIPEWIRE_ALSA`
with `spa-json-dump`; `pw_properties_update_string` does, in the client
process.
Measured live on this host, against the real ALSA plugin:
depth 513 dump accept plugin accept ours accept
depth 514 dump accept plugin accept ours REJECT
depth 515 dump accept plugin REJECT ours reject
depth 1000 dump accept plugin REJECT ours reject
At 515 the plugin discards the whole object: the node came back as
`alsa_playback.aplay` with no properties at all. So matching the dump
tool would have made us splice carriers into values the consumer throws
away wholesale — losing both, which is the echo this feature exists to
prevent. Over-rejecting costs a routing preference; over-accepting costs
a carrier. Those are not the same price.
What was genuinely wrong is narrower: we sat exactly one level below the
consumer. `pw_properties_update_string` calls `spa_json_container_len`
on a container value, which enters one more sub-iterator before its flat
walk, and that single level is the entire discrepancy. Doing the same
puts the boundaries on the same number.
Codex reached the same three numbers independently by calling
`pw_properties_update_string_checked(NULL, ...)` directly, having
disassembled both call sites; I measured through the live plugin. Two
methods, one table.
The dump differential stays, but it is now labelled a *grammar* oracle
with a warning not to add deep values — it would fail by design. The
acceptance oracle is the new boundary test.
Mutation-verified: removing the container step fails the 514 assertion.
622 -> 623 lib tests, fmt clean, clippy clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+81
-1
@@ -407,6 +407,31 @@ fn spa_object(text: &str) -> Option<SpaObject> {
|
||||
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]
|
||||
|
||||
Reference in New Issue
Block a user