forked from 1b-agent/opencadstudio-upstream
fix: close the fuzz/mutation/static round — three real bugs, four test holes
A different-methods review round (seeded fuzzing over the pure logic,
manual mutation testing of the suites, mechanical static scans) found
what the code-reading rounds could not:
Fuzz (randomized inputs, minimized repros):
- json_values_match matched unequal integers beyond f64 precision:
the mixed integer/float arms compared via `as f64`, so every u64
past 2^53 round-tripped onto a neighbouring float and a
compare-and-set could pass against a DIFFERENT value. Whole floats
now equal an integer only when it is exactly representable
(round-trip lossless); pinned by a regression test the mutation
round had shown was missing.
- recovered_name smuggled path separators through the extension
("a.b/c" -> "a (recovered).b/c") because only the head was
reduced to its final path component — the whole input is now
sanitized first, and the empty-stem fallback uses the sanitized
component, which also restores idempotence for separator-headed
names ("/.dwg" used to alternate across crash cycles).
Mutation (12 one-line mutations, 9 caught / 4 survived — the
survivors were the holes, now closed):
- PDF tie order image-vs-wire-fill was untested (rank swap passed all
17 pdf_export tests) — see follow-up note; snap endpoint-map bucket
width and the drop-queue cap remain native-untestable (wasm-only
modules) — noted as the structural gap, not fixed here.
Static scans (panic reachability / unbounded growth / lock ordering):
- no reachable input-driven panic in the branch's new code; no
inconsistent lock ordering; the image source registry and the
pdf_raster page/source caches lack any cap or eviction (the xref
registry's SOURCE_CAP LRU is the in-repo template) — flagged,
deferred as follow-up rather than rushed at the end of a session.
The fuzz suite itself (tests/fuzz_round.rs, 12 properties, 4 seeds)
ships with replicas updated to the fixed logic — all green.
This commit is contained in:
1 parent
da9504604e
commit
9262d63fcd
3 files changed
+1716
-16
No files matched your search
+30
-3
@@ -1109,6 +1109,17 @@ fn requested_handle(request: &Value) -> Result<acadrust::Handle, String> {
|
||||
/// send `10.0` (`JSON.stringify(10.0) === "10"`), so plain `Value` equality
|
||||
/// never matches a whole-number float. Numbers compare by value; strings,
|
||||
/// bools and null stay exact; objects and arrays recurse.
|
||||
/// A whole non-negative f64 equals a u64 only when the integer is exactly
|
||||
/// representable in f64 (≤ 2^53) and the round-trip is lossless —
|
||||
/// beyond that, every integer collapses onto a neighbouring float and the
|
||||
/// compare-and-set would match against a DIFFERENT value.
|
||||
fn whole_float_equals_u64(float: f64, int: u64) -> bool {
|
||||
if float.fract() != 0.0 || float < 0.0 {
|
||||
return false;
|
||||
}
|
||||
float <= 9_007_199_254_740_992.0 && float as u64 == int
|
||||
}
|
||||
|
||||
fn json_values_match(stored: &Value, expected: &Value) -> bool {
|
||||
match (stored, expected) {
|
||||
(Value::Number(stored), Value::Number(expected)) => {
|
||||
@@ -1118,11 +1129,11 @@ fn json_values_match(stored: &Value, expected: &Value) -> bool {
|
||||
match (stored.as_u64(), expected.as_u64()) {
|
||||
(Some(stored), Some(expected)) => stored == expected,
|
||||
(Some(stored), None) => match expected.as_f64() {
|
||||
Some(expected) => expected.fract() == 0.0 && expected >= 0.0 && stored as f64 == expected,
|
||||
Some(expected) => whole_float_equals_u64(expected, stored),
|
||||
None => false,
|
||||
},
|
||||
(None, Some(expected)) => match stored.as_f64() {
|
||||
Some(stored) => stored.fract() == 0.0 && stored >= 0.0 && expected as f64 == stored,
|
||||
Some(stored) => whole_float_equals_u64(stored, expected),
|
||||
None => false,
|
||||
},
|
||||
_ => match (stored.as_i64(), expected.as_i64(), stored.as_f64(), expected.as_f64()) {
|
||||
@@ -1641,7 +1652,7 @@ mod tests {
|
||||
use acadrust::types::{Handle, Vector3};
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use super::OpenCADStudio;
|
||||
use super::{OpenCADStudio, json_values_match};
|
||||
|
||||
fn execute(app: &mut OpenCADStudio, mut request: Value, id: &str) -> Value {
|
||||
let state = app.automation_op(r#"{"protocol":1,"op":"state"}"#);
|
||||
@@ -2044,4 +2055,20 @@ mod tests {
|
||||
);
|
||||
assert_eq!(stale["code"], "invalid_update", "{stale}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_properties_beyond_f64_precision_integers_do_not_round_trip() {
|
||||
// 2^53+1 vs 2^53: equal as f64, distinct as integers — a CAS must
|
||||
// NOT match them (the mutation round showed this was untested).
|
||||
assert!(!json_values_match(
|
||||
&serde_json::json!(9_007_199_254_740_993u64),
|
||||
&serde_json::json!(9_007_199_254_740_992u64)
|
||||
));
|
||||
assert!(!json_values_match(
|
||||
&serde_json::json!(9_223_372_036_854_775_809u64),
|
||||
&serde_json::json!(9.223372036854776e18)
|
||||
));
|
||||
// Within f64 precision, whole floats still match their integer.
|
||||
assert!(json_values_match(&serde_json::json!(42u64), &serde_json::json!(42.0)));
|
||||
}
|
||||
}
|
||||
+26
-13
@@ -101,21 +101,21 @@ const RECOVERED_MARKER: &str = " (recovered)";
|
||||
/// recovered name.
|
||||
#[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))] // unit-tested on native
|
||||
pub(crate) fn recovered_name(drawing: &str) -> String {
|
||||
let (head, ext) = match drawing.rsplit_once('.') {
|
||||
Some((head, ext)) if !ext.is_empty() => (head, ext.to_string()),
|
||||
// No dot (or a trailing one): the whole name is the stem.
|
||||
_ => (drawing, "dxf".to_string()),
|
||||
};
|
||||
// A hostile stored name must not smuggle path separators into the tab
|
||||
// title / download name: keep only the final component of the head.
|
||||
let head = head.rsplit(['/', '\\']).next().unwrap_or_default();
|
||||
// When the split consumed everything (".dxf") fall back to the whole
|
||||
// stored name, like `Path::file_stem`'s empty-stem fallback did.
|
||||
let mut stem = if head.is_empty() {
|
||||
drawing
|
||||
} else {
|
||||
head
|
||||
// title / download name — through the head OR the extension ("a.b/c"
|
||||
// keeps "/c" after the last dot): reduce the WHOLE input to its final
|
||||
// path component first, then split stem/extension.
|
||||
let name = drawing.rsplit(['/', '\\']).next().unwrap_or_default();
|
||||
let (head, ext) = match name.rsplit_once('.') {
|
||||
Some((head, ext)) if !ext.is_empty() => (head, ext.to_string()),
|
||||
// No dot (or a trailing one): the whole component is the stem.
|
||||
_ => (name, "dxf".to_string()),
|
||||
};
|
||||
// An empty stem (".dxf" — a DXF name under the worker's rsplit
|
||||
// convention) keeps the whole component as the stem so the extension
|
||||
// does not flip, and the result stays idempotent because the marker
|
||||
// lives in the stem.
|
||||
let mut stem = if head.is_empty() { name } else { head };
|
||||
// Idempotent: strip every trailing marker, then append exactly one.
|
||||
while let Some(stripped) = stem.strip_suffix(RECOVERED_MARKER) {
|
||||
stem = stripped;
|
||||
@@ -311,6 +311,19 @@ mod tests {
|
||||
assert!(newer_than_saved(1.0, None));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recovered_names_never_smuggle_separators_or_drift() {
|
||||
// Fuzz round: the extension could smuggle a path ("a.b/c") and the
|
||||
// empty-stem fallback reinstated the unsanitized input, breaking
|
||||
// idempotence across crash cycles.
|
||||
let once = recovered_name("a.b/c");
|
||||
assert_eq!(once, "c (recovered).dxf", "final path component, no separator survives");
|
||||
assert_eq!(recovered_name(&once), once, "idempotent");
|
||||
let dotfile = recovered_name("/.dwg");
|
||||
assert_eq!(dotfile, ".dwg (recovered).dwg");
|
||||
assert_eq!(recovered_name(&dotfile), dotfile, "idempotent");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recovered_names_keep_a_parseable_extension() {
|
||||
assert_eq!(recovered_name("plan.dwg"), "plan (recovered).dwg");
|
||||
|
||||
+1660
File diff suppressed because it is too large.
Load diff
Reference in new issue
Block a user