Selectively Replace DeepDiff Due to Performance Issues - #3340
Conversation
Body comparison ran through DeepDiff with ignore_order=True, whose similarity pairing is quadratic in the number of differing lines. On an ordinary large page it stalled the whole scan: 8,007 lines took 72s and 32,007 lines took minutes, all while holding the GIL. Reached from is_http_wildcard_host, so any scan that probes a large catch-all host could hit it regardless of enabled modules. DeepDiff's own budgets don't help: max_passes and the cutoff_* knobs give no speedup, and max_diffs silently returns an empty diff for bodies that clearly differ, inverting the answer. Compare bodies directly instead. Lines are matched by content, ignoring positions that varied between the two baseline samples. This agrees with DeepDiff on 599/600 randomized realistic bodies and is ~400x faster. Baseline parsing also moves off the event loop, where it ran inline. Parse JSON alongside XML and compare both leaf-by-leaf, keyed by path. A JSON API answers on one line, so a body with any volatile field produced a single differing line covering the whole body; that filter then suppressed everything and compare_body returned "match" for every subsequent response, including unrelated documents. The comparison was not degraded but vacuous, silently disabling the core signal of every module built on HttpCompare against JSON endpoints, and making is_http_wildcard_host report real sites as catch-all responders. compare_body still takes raw response text verbatim, which lightfuzz's crypto submodule relies on after stripping its own reflected values.
📊 Performance Benchmark Report
📈 Detailed Results (All Benchmarks)
🎯 Performance Summary! 1 regression ⚠️
30 unchanged ✅🔍 Significant Changes (>10%)
🐍 Python Version 3.11.15 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #3340 +/- ##
======================================
+ Coverage 90% 90% +1%
======================================
Files 450 451 +1
Lines 46308 46577 +269
======================================
+ Hits 41569 41837 +268
- Misses 4739 4740 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
singlerider
left a comment
singlerider
left a comment
There was a problem hiding this comment.
Went through this one carefully since it changes behavior. Reproduced both bugs independently against dev, and both are real.
Vacuous JSON comparison. Simulated dev's baseline filter derivation on a 78-byte single-line JSON API body with a volatile csrf field. xmltodict fails, so the body becomes a 1-element list of lines, and the derived filter is root[0], the entire document. Result:
| subject | want | dev | PR |
|---|---|---|---|
| same body, new csrf token | match | match | match |
value changed in results[3] |
differ | match | differ |
added top-level error key |
differ | match | differ |
| completely unrelated JSON | differ | match | differ |
PR's filter is {('csrf',)} instead of the whole body. Matches your table exactly.
Quadratic blowup. On a synthetic catch-all page (8,007 lines / 1,600 echoing the request path, 516 KiB): dev 20.9s, PR 0.63s, 33x. Lower than your 71.9s, probably a different body shape, but the shape of the problem is confirmed.
One thing I couldn't reproduce: you attribute the worse exposure to _baseline() rather than compare_body. On my repro dev spent 3.1s in baseline and 17.6s in compare, the reverse. Not disputing your measurement, just noting it's shape-dependent enough that the "64s of the 72s" split may not generalize.
42/42 of the new tests pass. Type-mismatch guards do the right thing both directions (structured-vs-line returns a difference, and a JSON baseline against a valid-XML error page compares leaf-wise and differs).
The memory cost isn't mentioned anywhere
_StructuredBody holds roughly 12x the body size, for the lifetime of the HttpCompare:
| body | leaves | peak RSS | vs body |
|---|---|---|---|
| 597 KiB dense JSON | 39,602 | 7.1 MB | 12.1x |
| 1191 KiB dense JSON | 77,641 | 13.8 MB | 11.9x |
| 443 KiB realistic JSON | n/a | 6.4 MB | ~14x |
On dev these bodies cost essentially nothing, since a single-line body is a 1-element list. The benchmark bot flagged the only regression in the run here: Memory Use Parallel Chains 10.9 MB to 13.8 MB, +26.8%. That +2.9 MB absolute delta is the right order of magnitude for a single moderately sized JSON baseline at 12x.
MAX_STRUCTURED_BODY_SIZE bounds text length, not leaf count, and the two aren't proportional. Dense JSON packs far more leaves per byte than sparse. A body sitting just under 2 MB is the worst case at ~24 MB per baseline, per concurrent compare. Worth either lowering the bound, or bounding on leaf count after the parse and falling back to _LineBody past some ceiling.
Related: parse_body tries xmltodict before json, so well-formed XHTML becomes _StructuredBody and pays the same 12x. Only malformed HTML escapes to _LineBody. That ordering is inherited from dev, but it's newly expensive.
On the detection increase
Your caveat is the right call to flag. Since these modules were silently dead on JSON endpoints, there's no field data on their false-positive rate there. The 599/600 agreement measures agreement with a comparison that was returning a constant True for exactly the cases now changing. Not an argument against merging, but paramminer and lightfuzz against JSON APIs are worth watching on the first real scans after this lands.
Nice find on max_diffs returning an empty diff for differing bodies. That one would have been genuinely nasty to debug.
|
@singlerider Not too worried about the memory increase. Almost all of it is short-lived, freed when the event finishes. The one case worth more thought is |
liquidsec
changed the title
liquidsec
changed the title
2 participants
Fixes #3339.
The hang
HttpComparecompared bodies withDeepDiff(..., ignore_order=True), whose similarity pairing is quadratic in the number of differing lines. The expensive band needs those lines to be numerous but individually near-identical — which is what a catch-all page echoing the request path produces, and also an ordinary homepage-vs-404 pair sharing template boilerplate.Measured on a 583 KiB page:
Two things beyond the issue report: the worse exposure was
_baseline(), notcompare_body— its diff had noexclude_pathsto short-circuit pairing and ran directly on the event loop, accounting for 64s of the 72s above. And the XML path was exposed too (48s on a 248 KiB body), so this wasn't limited to line bodies.The budgets suggested in the issue don't work.
max_passesand bothcutoff_*knobs gave no measurable speedup, andmax_diffs=1000returned an empty diff for two clearly-different bodies — a false "match", which would make wildcard detection and paramminer report the opposite of the truth.The vacuous JSON comparison
Found while fixing the above; same root cause, so it's fixed here rather than separately.
The baseline filter's granularity was tied to the body's line breaks rather than its structure. A JSON API answers on one line, so a body containing any volatile field (CSRF token, timestamp, request id) produced exactly one differing line — index 0, the whole body. That single filter then suppressed everything.
The comparison wasn't degraded, it was vacuous — a constant
True. Because it fails in the "no difference detected" direction it was silent, and it disabled the core signal of every module built onHttpCompare(paramminer, lightfuzz, bypass403, url_manipulation, webbrute) against JSON endpoints, while makingis_http_wildcard_hostlog real sites asis an HTTP wildcard responder.On a 222-byte JSON API body:
results[3]errorkeyApproach
Both bugs come from comparing by line rather than by structure, so the unit of comparison changes:
ignore_order's intent, linear.run_in_executor_cpu.DeepDiff is still used for header comparison — small, flat, no blowup risk.
compare_bodystill accepts raw response text and compares it verbatim, which lightfuzz's crypto submodule depends on after stripping its own reflected probe values.Rather than assume the replacement matched, I characterized DeepDiff's actual boolean contract here — set membership, with differing items suppressed by whichever index its pairing heuristic reported, effectively arbitrary once order is declared irrelevant. The replacement agrees with it on 599/600 randomized realistic bodies, and accuracy is a wash on a labelled scenario suite (each got 1 missed detection and 1 false difference out of 14). So the quadratic work was buying a report
compare_bodydiscarded, not better answers.Caveat
The JSON change is a detection increase in a place that was silently dead, so these modules will start producing findings on JSON endpoints where they previously produced none.