audio/ownership: pin the no-lost-carrier invariant, and harden the byte scan
Verification round on the round-10 review fixes. Adds the property the whole of finding 3 is about, stated directly: over 20,000 deterministic inputs built from the exact characters that break SPA-JSON (braces, brackets, quotes, separators, comment marks, escapes, newlines, multi-byte characters), the merge always emits both carriers in an object it can read back. Either outcome — parse and rebuild, or overwrite — has to end that way, and now nothing can quietly change which. Also replaces two byte-index steps with character-boundary steps. Both were correct on the ASCII input they actually see, but `index + 1` after a reverse find would have split a multi-byte character and panicked the slice. scan_token gains multi-byte cases for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+83
-3
@@ -872,10 +872,13 @@ mod tests {
|
||||
.expect("main has a body");
|
||||
// Back up over the module path the call is written with, so the
|
||||
// statement's own `peerspeak::audio::ownership::` is not mistaken for
|
||||
// code preceding it.
|
||||
// code preceding it. Stepping by `len_utf8` rather than by one byte
|
||||
// keeps the slice below on a character boundary whatever precedes it.
|
||||
let call_start = code[..call]
|
||||
.rfind(|c: char| !(c.is_alphanumeric() || c == '_' || c == ':'))
|
||||
.map_or(0, |index| index + 1);
|
||||
.char_indices()
|
||||
.rev()
|
||||
.find(|(_, c)| !(c.is_alphanumeric() || *c == '_' || *c == ':'))
|
||||
.map_or(0, |(index, c)| index + c.len_utf8());
|
||||
assert!(
|
||||
call_start > body,
|
||||
"the call must be inside main, not above it"
|
||||
@@ -1173,6 +1176,13 @@ mod tests {
|
||||
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.
|
||||
@@ -1216,6 +1226,76 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// **The invariant the whole finding is about: no inherited value, however
|
||||
/// hostile, may cost us a carrier.**
|
||||
///
|
||||
/// 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.
|
||||
#[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",
|
||||
];
|
||||
// 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
|
||||
};
|
||||
|
||||
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()]);
|
||||
}
|
||||
|
||||
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}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The warning names the variable that was actually malformed. It used to
|
||||
/// always say `PIPEWIRE_PROPS`, including when `tag_child` merged
|
||||
/// `PIPEWIRE_ALSA` (round 10 review, finding 3's second half).
|
||||
|
||||
Reference in New Issue
Block a user