diff --git a/audit/runtime/PropertyChangedStorm/Program.cs b/audit/runtime/PropertyChangedStorm/Program.cs new file mode 100644 index 00000000..0de2cf83 Binary files /dev/null and b/audit/runtime/PropertyChangedStorm/Program.cs differ diff --git a/audit/runtime/PropertyChangedStorm/PropertyChangedStorm.csproj b/audit/runtime/PropertyChangedStorm/PropertyChangedStorm.csproj new file mode 100644 index 00000000..e907c9fe --- /dev/null +++ b/audit/runtime/PropertyChangedStorm/PropertyChangedStorm.csproj @@ -0,0 +1,23 @@ + + + + Exe + net472 + latest + enable + PropertyChangedStorm + OwnNet.Audit.Runtime + x64 + x64 + + + + + + diff --git a/audit/runtime/README.md b/audit/runtime/README.md index 274cbdf7..cb4ebb9f 100644 --- a/audit/runtime/README.md +++ b/audit/runtime/README.md @@ -8,9 +8,10 @@ a static finding clusters with it → **high confidence** (Plan.md §3.5). This layer covers the categories static analysis honestly can't (Plan.md §2): event/subscription & timer leaks confirmed under load (cat. 2/3), the -`DependencyPropertyDescriptor.AddValueChanged` leak (cat. 4), and the -**duplicated-immutable-data** detector — the project's "gold" (cat. 11). For these, -the runtime layer is the *only* tool, so they were `NO-TOOL` until now. +`DependencyPropertyDescriptor.AddValueChanged` leak (cat. 4), **PropertyChanged +storms** measured by raise-frequency (cat. 6), and the **duplicated-immutable-data** +detector — the project's "gold" (cat. 11). For these, the runtime layer is the *only* +tool, so they were `NO-TOOL` until now. ## Stack (Windows / build-required — Plan.md §4) @@ -40,6 +41,9 @@ audit/runtime/ DuplicateDetector/ # C# duplicate-immutable detector — Windows/build-required, NOT CI-gated DuplicateDetector.csproj # net472; Microsoft.Diagnostics.Runtime Program.cs # ClrMD over a full dump: group identical strings, wasted-bytes findings + PropertyChangedStorm/ # C# PropertyChanged-storm profiler — Windows/build-required, NOT CI-gated + PropertyChangedStorm.csproj # net472; Microsoft.Diagnostics.Tracing.TraceEvent + Program.cs # TraceEvent over an .etl: per-property raise frequency, storm findings ``` ## How the leak-harness works (Plan.md §4.1) @@ -88,11 +92,41 @@ python audit/runtime/ingest.py --duplicate-detector artifacts/own-audit/duplicat # -> run_static folds duplicate-detector.sarif in as a category-11 (P2) finding set. ``` +## PropertyChanged-storm profiler (Plan.md §2 cat. 6) + +Frequency — not correctness — is a runtime property. The static `INPC0xx` tier (cat. 5) +catches a missing `nameof` or a broken arg; it cannot see that `Total` fires +PropertyChanged 4 000x for one keystroke, half of them with **no value change**, +thrashing every binding. The profiler reads an ETW trace (`.etl`) captured while a +FlaUI scenario drove the target — a diagnostic build emits one event per raise via an +EventSource (`OwnNet-Sematix-INPC` / `Raised`, payload `{Type, Property, ValueChanged, +[SourceFile, SourceLine]}`) — aggregates per (type, property), and reports each +property over its per-operation threshold. When the build resolved a source file, a +storm clusters with a static `INPC0xx` hit in the same file → **high confidence** +(§3.5); otherwise (file-only with no line, or no location at all) it gets a unique +`inpc:///-` synthetic uri — the `` index keeps distinct +storming properties in distinct clusters even when their slugs collide. + +```bash +# on Windows, against an .etl captured during the scenario (PerfView / xperf / logman): +PropertyChangedStorm.exe --trace artifacts/own-audit/scenario.etl --operations 1 \ + --per-op-threshold 50 --out artifacts/own-audit/propertychanged-storm.json \ + --scenario open-declaration --target acme/LegacyApp --commit "$COMMIT" + +# then, anywhere (CI exercises this conversion): +python audit/runtime/ingest.py --propertychanged-storm \ + artifacts/own-audit/propertychanged-storm.json \ + --out artifacts/own-audit/propertychanged-storm.sarif +# -> run_static folds propertychanged-storm.sarif in as a category-6 (P2) finding set; +# a located storm clusters with a static INPC0xx in the same file. +``` + ## Selftest `ingest.py` carries embedded-fixture selftests (no harness, no Windows needed) and -gates on Linux CI — including the end-to-end check that a static OWN014 plus a -runtime leak in the same file form one high-confidence cluster: +gates on Linux CI — including the end-to-end checks that a static OWN014 plus a +runtime leak (and a static `INPC0xx` plus a runtime storm) in the same file each form +one high-confidence cluster: ```bash python audit/runtime/ingest.py --selftest @@ -100,10 +134,12 @@ python audit/runtime/ingest.py --selftest ## Status -- **Done:** the runtime→pipeline bridge (`ingest.py`, CI-gated, for both the - leak-harness and the duplicate detector), the leak-harness scenario schema + one - scenario, runtime rule mappings in the taxonomy (categories 2/3/4/11), the C# - leak-harness skeleton, and the C# duplicate-immutable detector (strings). +- **Done:** the runtime→pipeline bridge (`ingest.py`, CI-gated, for the leak-harness, + the duplicate detector and the PropertyChanged-storm profiler), the leak-harness + scenario schema + one scenario, runtime rule mappings in the taxonomy (categories + 2/3/4/6/11), the C# leak-harness skeleton, the C# duplicate-immutable detector + (strings), and the C# PropertyChanged-storm profiler (ETW). - **Deferred:** duplicate detection for arbitrary immutable types (field-by-field - content equality), the PropertyChanged-storm profiler (C# over ETW), - PerfView/SematixTrace wiring, and a scenario corpus for the top-N screens. + content equality), the diagnostic-build INPC `EventSource` instrumentation in the + target + PerfView/SematixTrace capture wiring, and a scenario corpus for the top-N + screens. diff --git a/audit/runtime/ingest.py b/audit/runtime/ingest.py index 222bc060..daa7906c 100644 --- a/audit/runtime/ingest.py +++ b/audit/runtime/ingest.py @@ -81,20 +81,21 @@ def _location(f: dict[str, Any]) -> tuple[str, int]: return (f.get("location") or f.get("type") or "runtime", int(f.get("line", 0) or 0)) -_DUP_SLUG_RE = re.compile(r"[^0-9A-Za-z._-]+") +_SLUG_RE = re.compile(r"[^0-9A-Za-z._-]+") -def _dup_uri(type_name: str, value: str, index: int) -> str: - """A UNIQUE synthetic uri for one heap-wide duplicate group. These findings have - no source line, so without a distinct uri every group would share the same - ``(basename, line)`` and the scorer would collapse Country, Currency, ... into a - single cluster — corrupting totals/heatmap and hiding distinct remediation items - (Codex review on #103). The ``index`` guarantees uniqueness even when two display - values share a prefix; the slug keeps the path readable. All groups roll up under - one ``heap://`` module, so the heatmap still buckets them together.""" - typ = (type_name or "immutable").replace("\\", ".").replace("/", ".") - slug = _DUP_SLUG_RE.sub("_", value or "").strip("_")[:40] or "value" - return f"heap://{typ}/{index:04d}-{slug}" +def _synthetic_uri(scheme: str, type_name: str, value: str, index: int) -> str: + """A UNIQUE synthetic uri for a runtime finding with no source line — a heap-wide + duplicate group (``heap://``), a storming property (``inpc://``). Without a + distinct uri every such finding would share the same ``(basename, line)`` and the + scorer would collapse Country/Currency or Total/Subtotal into a single cluster, + corrupting totals/heatmap and hiding distinct remediation items (Codex review on + #103). The ``index`` guarantees uniqueness even when two display values share a + prefix; the slug keeps the path readable. All findings of one ``scheme://`` + roll up under one heatmap module, so it still buckets them together.""" + typ = (type_name or "runtime").replace("\\", ".").replace("/", ".") + slug = _SLUG_RE.sub("_", value or "").strip("_")[:40] or "value" + return f"{scheme}://{typ}/{index:04d}-{slug}" def leak_harness_to_sarif(result: dict[str, Any]) -> dict[str, Any]: @@ -133,9 +134,10 @@ def duplicate_detector_to_sarif(result: dict[str, Any]) -> dict[str, Any]: if not f.get("report", True): continue # heap-wide duplicate groups have no source line; synthesize a UNIQUE uri per - # value so distinct groups stay distinct clusters (see _dup_uri). Always + # value so distinct groups stay distinct clusters (see _synthetic_uri). Always # file-level (line 0) -> no fabricated region. - uri = _dup_uri(str(f.get("type", "")), str(f.get("value", "")), len(findings)) + uri = _synthetic_uri("heap", str(f.get("type", "")), str(f.get("value", "")), + len(findings)) findings.append({ "rule": f.get("rule", "RUNTIME-DUP-IMMUTABLE"), "message": f.get("message", ""), @@ -151,6 +153,49 @@ def duplicate_detector_to_sarif(result: dict[str, Any]) -> dict[str, Any]: level="warning") +def propertychanged_storm_to_sarif(result: dict[str, Any]) -> dict[str, Any]: + """PropertyChanged-storm profiler JSON → SARIF (Plan.md §2 cat. 6, §4.3). The + profiler counts, over one user operation, how often each property raises + PropertyChanged and how many of those raises carry no value change (a missing + equality guard). A property over its per-operation threshold is a storm. When the + instrumentation resolved a source file the finding keeps it — so a storm clusters + with a static ``INPC0xx`` hit (cat. 5) in the same file → high confidence (§3.5); + otherwise we synthesize a unique per-property uri so distinct storming properties + stay distinct clusters. ``report: false`` (below threshold) is dropped; level + ``warning`` (a P2 perf finding, not a correctness error).""" + findings = [] + for f in result.get("findings", []): + if not f.get("report", True): + continue + # A source location is usable for clustering ONLY when the line is resolved + # (>= 1). The C# falls back to line 0 when only the file is known; writing the + # bare file at line 0 would collapse every file-only storm into one cluster and + # could false-match a static finding on lines 1-3 (Codex review on #104). In + # that case keep a unique per-property synthetic uri instead. + loc = f.get("location") + line = int(f.get("line", 0) or 0) + if loc and line >= 1: + uri = str(loc) + else: + uri = _synthetic_uri("inpc", str(f.get("type", "")), + str(f.get("property", "")), len(findings)) + line = 0 + findings.append({ + "rule": f.get("rule", "RUNTIME-PROPCHANGED-STORM"), + "message": f.get("message", ""), + "uri": uri, "line": line, + "properties": {k: f[k] for k in + ("type", "property", "raises", "redundantRaises", + "perOperation", "threshold") + if k in f}, + }) + return _runtime_sarif( + result.get("tool", "propertychanged-storm"), findings, + {"target": result.get("target", ""), "commit": result.get("commit", ""), + "scenario": result.get("scenario", ""), "operations": result.get("operations", 0)}, + level="warning") + + def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser(description="Convert a runtime tool's JSON result to SARIF.") # exactly one source tool — reject both so a stray flag fails fast instead of @@ -158,6 +203,7 @@ def main(argv: list[str] | None = None) -> int: src = ap.add_mutually_exclusive_group() src.add_argument("--leak-harness", help="leak-harness result JSON") src.add_argument("--duplicate-detector", help="duplicate-immutable-detector result JSON") + src.add_argument("--propertychanged-storm", help="PropertyChanged-storm profiler result JSON") ap.add_argument("--out", default="", help="output SARIF (defaults per tool under artifacts/)") ap.add_argument("--selftest", action="store_true") args = ap.parse_args(argv) @@ -172,8 +218,13 @@ def main(argv: list[str] | None = None) -> int: result = json.loads(Path(args.duplicate_detector).read_text(encoding="utf-8")) sarif = duplicate_detector_to_sarif(result) default_out = "artifacts/own-audit/duplicate-detector.sarif" + elif args.propertychanged_storm: + result = json.loads(Path(args.propertychanged_storm).read_text(encoding="utf-8")) + sarif = propertychanged_storm_to_sarif(result) + default_out = "artifacts/own-audit/propertychanged-storm.sarif" else: - ap.error("one of --leak-harness / --duplicate-detector is required (or --selftest)") + ap.error("one of --leak-harness / --duplicate-detector / " + "--propertychanged-storm is required (or --selftest)") out = Path(args.out or default_out) out.parent.mkdir(parents=True, exist_ok=True) @@ -302,6 +353,81 @@ def check(ok: bool, msg: str) -> None: check(dscored["by_category"].get("duplicate-immutable") == 2, f"both duplicate values must count under category 11, got {dscored['by_category']}") + # PropertyChanged-storm profiler (cat. 6): per-operation raise frequency. A storm + # with a resolved source file keeps its line (so it clusters with a static INPC0xx + # hit in the same file); storms with no source line get unique per-property uris so + # distinct properties stay distinct clusters; below-threshold is dropped. + storm = propertychanged_storm_to_sarif({ + "tool": "propertychanged-storm", "target": "acme/LegacyApp", + "scenario": "open-declaration", "operations": 1, + "findings": [ + {"rule": "RUNTIME-PROPCHANGED-STORM", "type": "Acme.Vm.DeclarationViewModel", + "property": "Total", "location": "src/Vm/DeclarationViewModel.cs", "line": 88, + "raises": 4200, "redundantRaises": 3990, "perOperation": 4200.0, + "threshold": 50, "report": True, + "message": "Total raised PropertyChanged 4200x/op (3990 with no value change)"}, + {"rule": "RUNTIME-PROPCHANGED-STORM", "type": "Acme.Vm.DeclarationViewModel", + "property": "Subtotal", "raises": 900, "redundantRaises": 880, + "perOperation": 900.0, "threshold": 50, "report": True, + "message": "Subtotal raised PropertyChanged 900x/op (880 with no value change)"}, + # file resolved but LINE unresolved (C# falls back to 0): must NOT use the + # bare file uri — keep a synthetic uri so it can't collapse/false-match. + {"rule": "RUNTIME-PROPCHANGED-STORM", "type": "Acme.Vm.DeclarationViewModel", + "property": "Discount", "location": "src/Vm/DeclarationViewModel.cs", "line": 0, + "raises": 700, "redundantRaises": 690, "perOperation": 700.0, + "threshold": 50, "report": True, + "message": "Discount raised PropertyChanged 700x/op (file known, line unresolved)"}, + {"rule": "RUNTIME-PROPCHANGED-STORM", "type": "Acme.Vm.DeclarationViewModel", + "property": "Title", "raises": 2, "redundantRaises": 0, "perOperation": 2.0, + "threshold": 50, "report": False, "message": "below threshold"}, + ]}) + sres = storm["runs"][0]["results"] + check(len(sres) == 3, f"below-threshold storm must be dropped: got {len(sres)}") + check(all(r["level"] == "warning" for r in sres), + "propertychanged-storm must be SARIF level warning (P2)") + located = [r for r in sres + if r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + == "src/Vm/DeclarationViewModel.cs"] + check(len(located) == 1 + and located[0]["locations"][0]["physicalLocation"].get("region", {}) + .get("startLine") == 88, + "only the line-resolved storm keeps the bare source file (line>=1)") + # the file-only (line 0) storm falls back to a synthetic inpc:// uri, no region. + file_only = [r for r in sres if "Discount" in r["message"]["text"]] + check(len(file_only) == 1 + and file_only[0]["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + .startswith("inpc://") + and "region" not in file_only[0]["locations"][0]["physicalLocation"], + "a file-only storm (line 0) must fall back to a synthetic inpc:// uri") + suris = {r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] for r in sres} + check(len(suris) == 3, f"distinct storming properties need distinct uris, got {suris}") + # the TWO unresolved storms (Subtotal: no location, Discount: file-only line 0) + # must get DISTINCT synthetic uris — not collapse onto one (CodeRabbit review #104). + synthetic = [r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] for r in sres + if r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + .startswith("inpc://")] + check(len(synthetic) == 2 and len(set(synthetic)) == 2, + f"unresolved storming properties need distinct synthetic uris, got {synthetic}") + snorm = normalize_results(parse_sarif(json.dumps(storm), "propertychanged-storm", []), tax) + scat = {f.rule: (f.category, f.category_name) for f in snorm} + check(scat.get("RUNTIME-PROPCHANGED-STORM") == (6, "propertychanged-storm"), + f"propertychanged-storm -> category 6, got {scat.get('RUNTIME-PROPCHANGED-STORM')}") + # Plan §3.5: the located storm + a static INPC0xx in the same file -> high confidence. + inpc_sarif = json.dumps({"version": "2.1.0", "runs": [{ + "tool": {"driver": {"name": "Own.NET"}}, "results": [ + {"ruleId": "INPC003", "level": "warning", + "message": {"text": "raise PropertyChanged without an equality check"}, + "locations": [{"physicalLocation": { + "artifactLocation": {"uri": "src/Vm/DeclarationViewModel.cs"}, + "region": {"startLine": 87}}}]}]}]}) + both_s = normalize_results(parse_sarif(inpc_sarif, "own-check", []), tax) + snorm + sscored = score(both_s, tax, line_tol=3) + confirmed_s = [c for c in sscored["clusters"] if c.confidence == "high" + and set(c.tools) == {"own-check", "propertychanged-storm"}] + check(len(confirmed_s) == 1, + f"static INPC0xx + runtime storm in the same file must form 1 high-confidence " + f"cluster, got {[(c.module, c.tools, c.confidence) for c in sscored['clusters']]}") + fails = [c for c in checks if c] for f in fails: print(f"INGEST SELFTEST FAIL: {f}") diff --git a/audit/static/run_static.py b/audit/static/run_static.py index 555d5ea2..f9a80949 100755 --- a/audit/static/run_static.py +++ b/audit/static/run_static.py @@ -146,7 +146,8 @@ def run(target: str, profile: dict[str, Any], out_dir: Path, target_name: str = # with its static OWN014/OWN001 -> high confidence (§3.5) through the orchestrator, # not only the lower-level aggregate(). for fname, tool in (("leak-harness.sarif", "leak-harness"), - ("duplicate-detector.sarif", "duplicate-detector")): + ("duplicate-detector.sarif", "duplicate-detector"), + ("propertychanged-storm.sarif", "propertychanged-storm")): rt = out_dir / fname if rt.exists(): sarif_inputs.append((tool, str(rt))) @@ -284,6 +285,13 @@ def check(ok: bool, msg: str) -> None: # total derives from the call count "locations": [{"physicalLocation": {"artifactLocation": { "uri": "heap://System.String/0000-Country"}}}]}]}]} (out2 / "duplicate-detector.sarif").write_text(json.dumps(dup), encoding="utf-8") + # a runtime propertychanged-storm SARIF must be folded in by the same loop. + storm = {"runs": [{"tool": {"driver": {"name": "propertychanged-storm"}}, "results": [ + {"ruleId": "RUNTIME-PROPCHANGED-STORM", "level": "warning", + "message": {"text": "Total raised PropertyChanged 4200x/op"}, + "locations": [{"physicalLocation": {"artifactLocation": { + "uri": "inpc://Acme.Vm.DeclarationViewModel/0000-Total"}}}]}]}]} + (out2 / "propertychanged-storm.sarif").write_text(json.dumps(storm), encoding="utf-8") profile = {"name": "t", "severity_floor": "warning", "tiers": {"build_free": []}} res2 = run("/nonexistent-target", profile, out2, target_name="t/p") check(any(t["tool"] == "roslyn-pack" for t in res2["tiers"]), @@ -292,6 +300,8 @@ def check(ok: bool, msg: str) -> None: # total derives from the call count "runtime leak-harness.sarif must be folded in by run()") check(any(t["tool"] == "duplicate-detector" for t in res2["tiers"]), "runtime duplicate-detector.sarif must be folded in by run()") + check(any(t["tool"] == "propertychanged-storm" for t in res2["tiers"]), + "runtime propertychanged-storm.sarif must be folded in by run()") check(res2["totals"]["high_confidence"] >= 1, "runtime leak + static finding in one file must form a high-confidence cluster") diff --git a/audit/static/taxonomy/categories.yml b/audit/static/taxonomy/categories.yml index 4190f5b6..380b804f 100644 --- a/audit/static/taxonomy/categories.yml +++ b/audit/static/taxonomy/categories.yml @@ -64,6 +64,10 @@ rules: "RUNTIME-LEAK-TIMER": {category: 3, name: timer-leak} "RUNTIME-DPD-ADDVALUECHANGED": {category: 4, name: dpd-addvaluechanged-leak} "RUNTIME-DUP-IMMUTABLE": {category: 11, name: duplicate-immutable} + "RUNTIME-PROPCHANGED-STORM": {category: 6, name: propertychanged-storm} # raise-frequency, + # NOT INPC0xx correctness (cat. 5) — a storm + # in the same file as an INPC0xx hit clusters + # with it -> high confidence (§3.5) # Baseline P-level per category (Plan.md §3.5.2). Confidence (cross-tool # agreement) is a SEPARATE axis handled by score.py; this is severity only. @@ -73,6 +77,7 @@ category_severity: 3: P1 4: P1 # DependencyPropertyDescriptor.AddValueChanged leak (runtime-confirmed) 5: P2 + 6: P2 # PropertyChanged storms/cascades — runtime raise-frequency, perf-tier 9: P2 11: P2 # duplicated immutable data — memory bloat (the project's "gold"), perf-tier 14: P2