From 1c3ec0160157dfce062f83f68ea01ea6115b1d7c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 06:39:18 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Add=20duplicate-immutable=20detector=20(Pla?= =?UTF-8?q?n.md=20=C2=A72=20cat.=2011=20=E2=80=94=20the=20"gold")?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A heap full of identical immutable values (the same "Country"/unit/currency string held by thousands of separate instances) is wasted memory that interning / a flyweight / reference-by-id would collapse. This adds that detector to the runtime layer, flowing through the same normalize -> score -> report pipeline. Testable core (pure Python, Linux-CI-gated): - ingest.py refactored around a shared _runtime_sarif() core, with a new duplicate_detector_to_sarif() adapter: type/value-based findings stay file-level, SARIF level warning (a P2 memory/perf finding), and a below-threshold finding (report:false) is dropped. --selftest extended to prove the dup path maps to category 11 and carries wastedBytes; ingest 15/15. - run_static.run() folds /duplicate-detector.sarif into aggregation as a runtime tier (alongside leak-harness.sarif), so a cat-11 finding reaches the report through the orchestrator; run_static 15/15. - RUNTIME-DUP-IMMUTABLE was already mapped to category 11 (P2) in #102, so no taxonomy change is needed. Windows / build-required skeleton (NOT CI-gated): - audit/runtime/DuplicateDetector/ (net472 C#, ClrMD): walks a full dump, groups live strings by value, computes per-group wasted bytes ((count-1)*bytesPerInstance), and emits JSON findings above --min-wasted-bytes. Reuses the hardened procdump pattern (drain pipes, bounded wait, exit-code check) and the ClrVersions guard. Strings first (the highest-value case); arbitrary immutable types deferred. All audit selftests green (normalize/score/report/run_static/ingest), ruff clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE --- .../DuplicateDetector.csproj | 22 ++ audit/runtime/DuplicateDetector/Program.cs | 223 ++++++++++++++++++ audit/runtime/README.md | 41 +++- audit/runtime/ingest.py | 164 +++++++++---- audit/static/run_static.py | 20 +- 5 files changed, 407 insertions(+), 63 deletions(-) create mode 100644 audit/runtime/DuplicateDetector/DuplicateDetector.csproj create mode 100644 audit/runtime/DuplicateDetector/Program.cs diff --git a/audit/runtime/DuplicateDetector/DuplicateDetector.csproj b/audit/runtime/DuplicateDetector/DuplicateDetector.csproj new file mode 100644 index 00000000..3e68f4a0 --- /dev/null +++ b/audit/runtime/DuplicateDetector/DuplicateDetector.csproj @@ -0,0 +1,22 @@ + + + + Exe + net472 + latest + enable + DuplicateDetector + OwnNet.Audit.Runtime + x64 + x64 + + + + + + diff --git a/audit/runtime/DuplicateDetector/Program.cs b/audit/runtime/DuplicateDetector/Program.cs new file mode 100644 index 00000000..56c8b1fd --- /dev/null +++ b/audit/runtime/DuplicateDetector/Program.cs @@ -0,0 +1,223 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.Diagnostics.Runtime; +using Newtonsoft.Json; + +namespace OwnNet.Audit.Runtime +{ + /// + /// Duplicate-immutable-data detector (Plan.md §2 cat. 11 — the project's "gold"). + /// Walks the TARGET process heap from a full dump (ClrMD) and groups identical + /// immutable values; a value held by many separate instances is wasted memory that + /// interning / a flyweight / a reference-by-id would collapse (units, countries, + /// currencies, repeated reference-table strings). + /// + /// This first cut covers STRINGS — the highest-value, most common case. Arbitrary + /// immutable reference types (field-by-field content equality) are a later + /// refinement. Emits a JSON result that audit/runtime/ingest.py converts to SARIF + /// (rule RUNTIME-DUP-IMMUTABLE -> category 11) for the unified report. + /// + /// Windows / build-required: needs a full dump (procdump) + ClrMD. Not part of the + /// Linux CI gate — the ingest bridge is what CI tests. + /// + internal static class Program + { + private static int Main(string[] args) + { + var opts = DupOptions.Parse(args); + if (opts == null) + { + return 2; + } + try + { + return Run(opts); + } + catch (Exception ex) + { + Console.Error.WriteLine($"duplicate-detector: failed — {ex.Message}"); + return 2; + } + } + + private static int Run(DupOptions opts) + { + // Analyze an existing dump, or take one of a live pid (then delete it). + var ownsDump = opts.DumpPath == null; + var dump = opts.DumpPath ?? CreateDump(opts); + var findings = new List>(); + try + { + using var dataTarget = DataTarget.LoadDump(dump); + var clr = dataTarget.ClrVersions.FirstOrDefault() + ?? throw new InvalidOperationException( + "dump contains no CLR — is the target a managed process?"); + using var runtime = clr.CreateRuntime(); + + // Group live strings by value: instance count + total bytes per value. + var groups = new Dictionary(); + foreach (var obj in runtime.Heap.EnumerateObjects()) + { + if (obj.Type == null || !obj.Type.IsString) + { + continue; + } + var val = obj.AsString(opts.MaxStringLength); + if (string.IsNullOrEmpty(val)) + { + continue; + } + var g = groups.TryGetValue(val!, out var cur) ? cur : (0L, 0UL); + groups[val!] = (g.Count + 1, g.Bytes + obj.Size); + } + + foreach (var kv in groups) + { + var count = kv.Value.Count; + if (count < 2) + { + continue; // a unique value is not duplication + } + var bytesPer = (long)(kv.Value.Bytes / (ulong)count); + var wasted = (count - 1) * bytesPer; // duplicates beyond one canonical instance + var report = wasted >= opts.MinWastedBytes; + if (!report && !opts.IncludeBelowThreshold) + { + continue; + } + findings.Add(new Dictionary + { + ["rule"] = "RUNTIME-DUP-IMMUTABLE", + ["type"] = "System.String", + ["value"] = Truncate(kv.Key, 80), + ["count"] = count, + ["bytesPerInstance"] = bytesPer, + ["wastedBytes"] = wasted, + ["report"] = report, + ["message"] = $"{count} duplicate \"{Truncate(kv.Key, 40)}\" strings " + + $"(~{wasted / 1024} KB wasted; intern or use a flyweight)", + }); + } + } + finally + { + if (ownsDump) + { + File.Delete(dump); // dumps are large; the findings are the artifact + } + } + + findings = findings.OrderByDescending(f => (long)f["wastedBytes"]).ToList(); + var result = new Dictionary + { + ["tool"] = "duplicate-detector", + ["target"] = opts.Target, + ["commit"] = opts.Commit, + ["minWastedBytes"] = opts.MinWastedBytes, + ["findings"] = findings, + }; + Directory.CreateDirectory(Path.GetDirectoryName(Path.GetFullPath(opts.OutPath))!); + File.WriteAllText(opts.OutPath, JsonConvert.SerializeObject(result, Formatting.Indented)); + + var reported = findings.Count(f => (bool)f["report"]); + Console.WriteLine( + $"duplicate-detector: {reported} duplicate group(s) over threshold -> {opts.OutPath}"); + return reported > 0 ? 1 : 0; + } + + private static string CreateDump(DupOptions opts) + { + if (opts.Pid == 0) + { + throw new ArgumentException("either --dump or --pid is required"); + } + Directory.CreateDirectory(opts.ScratchDir); + var dump = Path.Combine(opts.ScratchDir, $"dup-{opts.Pid}.dmp"); + var psi = new ProcessStartInfo(opts.ProcdumpPath, $"-accepteula -ma {opts.Pid} \"{dump}\"") + { + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + CreateNoWindow = true, + }; + using var p = Process.Start(psi)!; + // Drain pipes before waiting (avoid the full-buffer deadlock); bound the wait. + var stdout = p.StandardOutput.ReadToEndAsync(); + var stderr = p.StandardError.ReadToEndAsync(); + if (!p.WaitForExit(120_000)) + { + try { p.Kill(); } catch { /* best effort */ } + throw new IOException($"procdump timed out (>120s) for pid {opts.Pid}"); + } + Task.WaitAll(stdout, stderr); + if (p.ExitCode != 0 || !File.Exists(dump)) + { + throw new IOException( + $"procdump failed for pid {opts.Pid} (exit {p.ExitCode}): {stderr.Result}"); + } + return dump; + } + + private static string Truncate(string s, int n) => s.Length <= n ? s : s.Substring(0, n) + "…"; + } + + /// Command-line options for the duplicate detector. + internal sealed class DupOptions + { + public string? DumpPath { get; private set; } + public int Pid { get; private set; } + public string ProcdumpPath { get; private set; } = "procdump.exe"; + public string ScratchDir { get; private set; } = "artifacts/own-audit/dumps"; + public long MinWastedBytes { get; private set; } = 65536; // 64 KB + public int MaxStringLength { get; private set; } = 512; + public bool IncludeBelowThreshold { get; private set; } + public string OutPath { get; private set; } = "artifacts/own-audit/duplicate-detector.json"; + public string Target { get; private set; } = ""; + public string Commit { get; private set; } = ""; + + public static DupOptions? Parse(string[] args) + { + var o = new DupOptions(); + for (var i = 0; i < args.Length; i++) + { + switch (args[i]) + { + case "--dump": o.DumpPath = Next(args, ref i); break; + case "--pid": o.Pid = int.Parse(Next(args, ref i)); break; + case "--procdump": o.ProcdumpPath = Next(args, ref i); break; + case "--scratch": o.ScratchDir = Next(args, ref i); break; + case "--min-wasted-bytes": o.MinWastedBytes = long.Parse(Next(args, ref i)); break; + case "--max-string-length": o.MaxStringLength = int.Parse(Next(args, ref i)); break; + case "--include-below-threshold": o.IncludeBelowThreshold = true; break; + case "--out": o.OutPath = Next(args, ref i); break; + case "--target": o.Target = Next(args, ref i); break; + case "--commit": o.Commit = Next(args, ref i); break; + default: + Console.Error.WriteLine($"duplicate-detector: unknown arg {args[i]}"); + return null; + } + } + if (o.DumpPath == null && o.Pid == 0) + { + Console.Error.WriteLine( + "Usage: DuplicateDetector (--dump | --pid --procdump ) " + + "[--min-wasted-bytes N] [--out json] [--target owner/repo] [--commit sha]"); + return null; + } + return o; + } + + private static string Next(string[] args, ref int i) + { + if (i + 1 >= args.Length) + { + throw new ArgumentException($"{args[i]} requires a value"); + } + return args[++i]; + } + } +} diff --git a/audit/runtime/README.md b/audit/runtime/README.md index b05ab5a8..274cbdf7 100644 --- a/audit/runtime/README.md +++ b/audit/runtime/README.md @@ -29,14 +29,17 @@ the CoreCLR-only `dotnet-*` tools: ```text audit/runtime/ - ingest.py # leak-harness JSON -> SARIF -> the unified pipeline (PURE PYTHON, CI-gated) + ingest.py # runtime JSON -> SARIF -> the unified pipeline (PURE PYTHON, CI-gated) scenarios/ open-close-declaration.yml # one deterministic leak-harness scenario (+ schema docs) - LeakHarness/ # C# harness — Windows/build-required, NOT CI-gated + LeakHarness/ # C# leak-harness — Windows/build-required, NOT CI-gated LeakHarness.csproj # net472; FlaUI.UIA3 + Microsoft.Diagnostics.Runtime + YamlDotNet Program.cs # GC+snapshot loop, growth assertion, JSON result Scenario.cs # YAML model HeapCounter.cs # procdump + ClrMD: count live instances of suspect types + 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 ``` ## How the leak-harness works (Plan.md §4.1) @@ -64,6 +67,27 @@ python audit/runtime/ingest.py --leak-harness artifacts/own-audit/leak-harness.j # folds it in and a confirmed leak clusters with its static OWN014/OWN001. ``` +## Duplicate-immutable detector (Plan.md §2 cat. 11 — the "gold") + +A heap full of identical immutable values (the same `"Country"` / unit / currency +string held by thousands of separate instances) is wasted memory that interning, a +flyweight, or a reference-by-id would collapse. The detector walks a full dump with +ClrMD, groups strings by value, and reports each group whose duplicates waste more +than `--min-wasted-bytes`. (Strings first — the highest-value case; arbitrary +immutable types are a later refinement.) It needs no UI scenario — it's a one-shot +heap analysis. + +```bash +# on Windows, against a dump (or a live --pid with --procdump): +DuplicateDetector.exe --dump target.dmp --min-wasted-bytes 65536 \ + --out artifacts/own-audit/duplicate-detector.json --target acme/LegacyApp --commit "$COMMIT" + +# then, anywhere (CI exercises this conversion): +python audit/runtime/ingest.py --duplicate-detector artifacts/own-audit/duplicate-detector.json \ + --out artifacts/own-audit/duplicate-detector.sarif +# -> run_static folds duplicate-detector.sarif in as a category-11 (P2) finding set. +``` + ## Selftest `ingest.py` carries embedded-fixture selftests (no harness, no Windows needed) and @@ -76,9 +100,10 @@ python audit/runtime/ingest.py --selftest ## Status -- **Done:** the runtime→pipeline bridge (`ingest.py`, CI-gated), the leak-harness - scenario schema + one scenario, runtime rule mappings in the taxonomy (categories - 2/3/4/11), and the C# leak-harness skeleton. -- **Deferred:** the ClrMD duplicate-immutable detector and PropertyChanged-storm - profiler (more C# over the same dump/ETW), PerfView/SematixTrace wiring, and a - scenario corpus for the top-N screens. +- **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). +- **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. diff --git a/audit/runtime/ingest.py b/audit/runtime/ingest.py index ad8485fb..05f89925 100644 --- a/audit/runtime/ingest.py +++ b/audit/runtime/ingest.py @@ -9,12 +9,13 @@ with it → **high confidence** (the static→runtime confirmation in Plan.md §3.5, e.g. own-check OWN014 + leak-harness on ``VideoSource.xaml.cs:123``). -The C# leak-harness (``audit/runtime/LeakHarness/``, Windows / build-required) runs -the deterministic GC+snapshot loop and writes the JSON; this bridge is pure Python -and gates on Linux CI, like the aggregation selftests. +The C# harnesses (``audit/runtime/LeakHarness/`` and ``DuplicateDetector/``, Windows +/ build-required) write the JSON; this bridge is pure Python and gates on Linux CI, +like the aggregation selftests. Usage: - ingest.py --leak-harness result.json --out artifacts/own-audit/leak-harness.sarif + ingest.py --leak-harness result.json [--out leak-harness.sarif] + ingest.py --duplicate-detector result.json [--out duplicate-detector.sarif] ingest.py --selftest """ @@ -35,39 +36,27 @@ / "static" / "taxonomy" / "categories.yml") -def leak_harness_to_sarif(result: dict[str, Any]) -> dict[str, Any]: - """Turn one leak-harness JSON result into a SARIF 2.1.0 log. Only findings with - ``leaked: true`` (the deterministic growth assertion tripped) become results; a - clean iteration loop is evidence of *no* leak, not a finding.""" - tool = result.get("tool", "leak-harness") +def _runtime_sarif(tool: str, findings: list[dict[str, Any]], + run_props: dict[str, Any], level: str) -> dict[str, Any]: + """Build a SARIF 2.1.0 log from already-normalized runtime findings. Each finding + is ``{rule, message, uri, line, properties}``; a ``line < 1`` stays file-level (no + region) rather than fabricate line 1. The ``level`` is cosmetic for code scanning + — score.py derives severity from the taxonomy category, not the SARIF level.""" rules: dict[str, dict[str, Any]] = {} results: list[dict[str, Any]] = [] - for f in result.get("findings", []): - if not f.get("leaked"): - continue - rule_id = f.get("rule", "RUNTIME-LEAK") + for f in findings: + rule_id = f["rule"] rules.setdefault(rule_id, {"id": rule_id, "name": rule_id, - "defaultConfiguration": {"level": "error"}}) - # A type-based finding (e.g. duplicate-immutable) may have no source line; - # keep it file-level (no region) rather than fabricate line 1. parse_sarif - # drops results with NO location, so always carry at least an artifactLocation - # (the source file when known, else the type as a synthetic uri). - uri = f.get("location") or f.get("type") or "runtime" - phys: dict[str, Any] = {"artifactLocation": {"uri": uri}} - line = int(f.get("line", 0) or 0) - if line >= 1: - phys["region"] = {"startLine": line} - msg = (f.get("message", "") - + f" [scenario: {result.get('scenario', '?')}, " - + f"x{result.get('iterations', '?')} iterations]") + "defaultConfiguration": {"level": level}}) + phys: dict[str, Any] = {"artifactLocation": {"uri": f["uri"]}} + if f["line"] >= 1: + phys["region"] = {"startLine": f["line"]} results.append({ "ruleId": rule_id, - "level": "error", - "message": {"text": msg}, + "level": level, + "message": {"text": f["message"]}, "locations": [{"physicalLocation": phys}], - "properties": {k: f[k] for k in - ("type", "baseline", "final", "growthPerIteration", "threshold") - if k in f}, + "properties": f.get("properties", {}), }) return { "$schema": "https://json.schemastore.org/sarif-2.1.0.json", @@ -79,35 +68,94 @@ def leak_harness_to_sarif(result: dict[str, Any]) -> dict[str, Any]: "rules": list(rules.values()), }}, "results": results, - "properties": { - "target": result.get("target", ""), - "commit": result.get("commit", ""), - "scenario": result.get("scenario", ""), - "iterations": result.get("iterations", 0), - }, + "properties": run_props, }], } +def _location(f: dict[str, Any]) -> tuple[str, int]: + """A finding's (uri, line). parse_sarif drops results with NO location, so always + carry an artifactLocation: the source file when known, else the type as a + synthetic uri (type-based findings like duplicate-immutable have no source line).""" + return (f.get("location") or f.get("type") or "runtime", int(f.get("line", 0) or 0)) + + +def leak_harness_to_sarif(result: dict[str, Any]) -> dict[str, Any]: + """Leak-harness JSON → SARIF. Only ``leaked: true`` findings (the deterministic + growth assertion tripped) become results; a clean loop is evidence of *no* leak.""" + suffix = (f" [scenario: {result.get('scenario', '?')}, " + f"x{result.get('iterations', '?')} iterations]") + findings = [] + for f in result.get("findings", []): + if not f.get("leaked"): + continue + uri, line = _location(f) + findings.append({ + "rule": f.get("rule", "RUNTIME-LEAK"), + "message": f.get("message", "") + suffix, + "uri": uri, "line": line, + "properties": {k: f[k] for k in + ("type", "baseline", "final", "growthPerIteration", "threshold") + if k in f}, + }) + return _runtime_sarif( + result.get("tool", "leak-harness"), findings, + {"target": result.get("target", ""), "commit": result.get("commit", ""), + "scenario": result.get("scenario", ""), "iterations": result.get("iterations", 0)}, + level="error") + + +def duplicate_detector_to_sarif(result: dict[str, Any]) -> dict[str, Any]: + """Duplicate-immutable-detector JSON → SARIF (Plan.md §2 cat. 11, the project's + "gold"). Findings are type/value-based (a heap full of identical 'Country' + strings), so they have no source line — file-level, level ``warning`` (a P2 + memory/perf finding, not a correctness error). A finding with ``report: false`` + (below the wasted-bytes threshold) is dropped.""" + findings = [] + for f in result.get("findings", []): + if not f.get("report", True): + continue + uri, line = _location(f) + findings.append({ + "rule": f.get("rule", "RUNTIME-DUP-IMMUTABLE"), + "message": f.get("message", ""), + "uri": uri, "line": line, + "properties": {k: f[k] for k in + ("type", "value", "count", "bytesPerInstance", "wastedBytes") + if k in f}, + }) + return _runtime_sarif( + result.get("tool", "duplicate-detector"), findings, + {"target": result.get("target", ""), "commit": result.get("commit", ""), + "minWastedBytes": result.get("minWastedBytes", 0)}, + level="warning") + + def main(argv: list[str] | None = None) -> int: - ap = argparse.ArgumentParser(description="Convert a leak-harness JSON result to SARIF.") + ap = argparse.ArgumentParser(description="Convert a runtime tool's JSON result to SARIF.") ap.add_argument("--leak-harness", help="leak-harness result JSON") - ap.add_argument("--out", default="artifacts/own-audit/leak-harness.sarif") + ap.add_argument("--duplicate-detector", help="duplicate-immutable-detector 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) if args.selftest: return _selftest() - if not args.leak_harness: - ap.error("--leak-harness is required (or use --selftest)") + if args.leak_harness: + result = json.loads(Path(args.leak_harness).read_text(encoding="utf-8")) + sarif = leak_harness_to_sarif(result) + default_out = "artifacts/own-audit/leak-harness.sarif" + elif args.duplicate_detector: + 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" + else: + ap.error("one of --leak-harness / --duplicate-detector is required (or --selftest)") - result = json.loads(Path(args.leak_harness).read_text(encoding="utf-8")) - sarif = leak_harness_to_sarif(result) - out = Path(args.out) + out = Path(args.out or default_out) out.parent.mkdir(parents=True, exist_ok=True) out.write_text(json.dumps(sarif, indent=2), encoding="utf-8") - n = len(sarif["runs"][0]["results"]) - print(json.dumps({"out": str(out), "leaked_findings": n}, indent=2)) + print(json.dumps({"out": str(out), "results": len(sarif["runs"][0]["results"])}, indent=2)) return 0 @@ -192,6 +240,30 @@ def check(ok: bool, msg: str) -> None: f"static OWN014 + runtime leak in the same file must form 1 high-confidence cluster, " f"got {[(c.module, c.tools, c.confidence) for c in scored['clusters']]}") + # Duplicate-immutable detector (cat. 11, the project's "gold"): type/value-based, + # file-level, level warning; a below-threshold finding (report:false) is dropped. + dup = duplicate_detector_to_sarif({ + "tool": "duplicate-detector", "target": "acme/LegacyApp", "minWastedBytes": 65536, + "findings": [ + {"rule": "RUNTIME-DUP-IMMUTABLE", "type": "System.String", "value": "Country", + "count": 48211, "bytesPerInstance": 36, "wastedBytes": 1735560, + "location": "Acme.Ref.CountryTable", "line": 0, "report": True, + "message": "48211 duplicate 'Country' strings (~1.7 MB wasted)"}, + {"rule": "RUNTIME-DUP-IMMUTABLE", "type": "System.String", "value": "x", + "count": 3, "wastedBytes": 108, "report": False, "message": "below threshold"}, + ]}) + dres = dup["runs"][0]["results"] + check(len(dres) == 1, f"below-threshold dup finding must be dropped: got {len(dres)}") + check(dres[0]["level"] == "warning", "duplicate-immutable must be SARIF level warning (P2)") + check("region" not in dres[0]["locations"][0]["physicalLocation"], + "type-based duplicate finding must stay file-level (no region)") + check(dres[0]["properties"].get("wastedBytes") == 1735560, + "duplicate finding must carry wastedBytes in properties") + dcat = {f.rule: (f.category, f.category_name) + for f in normalize_results(parse_sarif(json.dumps(dup), "duplicate-detector", []), tax)} + check(dcat.get("RUNTIME-DUP-IMMUTABLE") == (11, "duplicate-immutable"), + f"duplicate-immutable -> category 11, got {dcat.get('RUNTIME-DUP-IMMUTABLE')}") + 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 4ce77d7d..946853ca 100755 --- a/audit/static/run_static.py +++ b/audit/static/run_static.py @@ -141,15 +141,17 @@ def run(target: str, profile: dict[str, Any], out_dir: Path, target_name: str = tiers.append({"tool": "infersharp", "tier": "build-required", "available": True, "sarif": str(infer), "reason": ""}) - # Runtime tier (Plan.md §4): the leak-harness SARIF (produced by - # audit/runtime/ingest.py from the harness JSON) is folded in here, so a - # runtime-confirmed leak clusters with its static OWN014/OWN001 -> high - # confidence (§3.5) through the orchestrator, not only the lower-level aggregate(). - leak = out_dir / "leak-harness.sarif" - if leak.exists(): - sarif_inputs.append(("leak-harness", str(leak))) - tiers.append({"tool": "leak-harness", "tier": "runtime", "available": True, - "sarif": str(leak), "reason": ""}) + # Runtime tier (Plan.md §4): SARIFs produced by audit/runtime/ingest.py from each + # runtime tool's JSON are folded in here, so a runtime-confirmed finding clusters + # 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")): + rt = out_dir / fname + if rt.exists(): + sarif_inputs.append((tool, str(rt))) + tiers.append({"tool": tool, "tier": "runtime", "available": True, + "sarif": str(rt), "reason": ""}) meta = { "target": target_name or target, "commit": commit, From 5ff16dc9125ef54b1e7d23ef94c97b52d18a2966 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 06:49:28 +0000 Subject: [PATCH 2/2] audit: fix duplicate-immutable grouping + clustering (PR #103 review) Address Codex + CodeRabbit findings on the duplicate-immutable detector: - DuplicateDetector/Program.cs: group strings by their FULL value, not the --max-string-length-truncated read. Distinct long strings sharing a prefix (JSON/XML blobs) no longer collapse into one bogus group with inflated count/wastedBytes. Overlong strings key on length + a SHA-256 of the full content so the dictionary never retains large blobs; a short sample is kept only for the value/message display. - ingest.py: the C# emits no source location for heap-wide duplicate groups, so every finding previously fell back to (type, line 0) and the scorer collapsed Country/Currency/... into a single cluster, corrupting totals/heatmap. Synthesize a UNIQUE per-value synthetic uri (heap:///-); all groups still roll up under one heap:// module. Harden the selftest with a realistic location-less fixture proving two distinct values stay two clusters. - ingest.py: make --leak-harness / --duplicate-detector mutually exclusive so a stray flag fails fast instead of silently converting the wrong file. - run_static.py: extend _selftest() to also drop a duplicate-detector.sarif and assert the runtime pickup folds it in, so a filename/tool-name typo fails on Linux CI, not only on a Windows run. Selftests: ingest 18/18, run_static 16/16, normalize/score/report unchanged; ruff clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QDPpNT9Uh8RoTrKPvgcoRE --- audit/runtime/DuplicateDetector/Program.cs | 40 ++++++++++++--- audit/runtime/ingest.py | 60 ++++++++++++++++++---- audit/static/run_static.py | 11 ++++ 3 files changed, 93 insertions(+), 18 deletions(-) diff --git a/audit/runtime/DuplicateDetector/Program.cs b/audit/runtime/DuplicateDetector/Program.cs index 56c8b1fd..db9868f0 100644 --- a/audit/runtime/DuplicateDetector/Program.cs +++ b/audit/runtime/DuplicateDetector/Program.cs @@ -3,6 +3,8 @@ using System.Diagnostics; using System.IO; using System.Linq; +using System.Security.Cryptography; +using System.Text; using System.Threading.Tasks; using Microsoft.Diagnostics.Runtime; using Newtonsoft.Json; @@ -59,20 +61,34 @@ private static int Run(DupOptions opts) using var runtime = clr.CreateRuntime(); // Group live strings by value: instance count + total bytes per value. - var groups = new Dictionary(); + // Group by the FULL string — the display cap (--max-string-length) must + // NOT be the grouping key, or distinct long strings that share the first + // N chars (JSON/XML blobs with a common prefix) collapse into one bogus + // group with inflated count/wastedBytes (Codex review on #103). Read the + // whole string for the key, hashing anything longer than the cap so the + // dictionary never retains large blobs, and keep a short readable sample + // only for display. + var groups = new Dictionary(); foreach (var obj in runtime.Heap.EnumerateObjects()) { if (obj.Type == null || !obj.Type.IsString) { continue; } - var val = obj.AsString(opts.MaxStringLength); - if (string.IsNullOrEmpty(val)) + var full = obj.AsString(int.MaxValue); // actual length, not capped + if (string.IsNullOrEmpty(full)) { continue; } - var g = groups.TryGetValue(val!, out var cur) ? cur : (0L, 0UL); - groups[val!] = (g.Count + 1, g.Bytes + obj.Size); + var key = full!.Length <= opts.MaxStringLength ? full : LongKey(full); + if (groups.TryGetValue(key, out var cur)) + { + groups[key] = (cur.Count + 1, cur.Bytes + obj.Size, cur.Sample); + } + else + { + groups[key] = (1, obj.Size, Truncate(full, opts.MaxStringLength)); + } } foreach (var kv in groups) @@ -89,16 +105,17 @@ private static int Run(DupOptions opts) { continue; } + var sample = kv.Value.Sample; findings.Add(new Dictionary { ["rule"] = "RUNTIME-DUP-IMMUTABLE", ["type"] = "System.String", - ["value"] = Truncate(kv.Key, 80), + ["value"] = Truncate(sample, 80), ["count"] = count, ["bytesPerInstance"] = bytesPer, ["wastedBytes"] = wasted, ["report"] = report, - ["message"] = $"{count} duplicate \"{Truncate(kv.Key, 40)}\" strings " + ["message"] = $"{count} duplicate \"{Truncate(sample, 40)}\" strings " + $"(~{wasted / 1024} KB wasted; intern or use a flyweight)", }); } @@ -163,6 +180,15 @@ private static string CreateDump(DupOptions opts) } private static string Truncate(string s, int n) => s.Length <= n ? s : s.Substring(0, n) + "…"; + + // For strings longer than the display cap, key on length + a hash of the FULL + // content so distinct long strings never merge, without retaining the blob. + private static string LongKey(string s) + { + using var sha = SHA256.Create(); + var hash = sha.ComputeHash(Encoding.Unicode.GetBytes(s)); + return "len=" + s.Length + ":" + BitConverter.ToString(hash).Replace("-", ""); + } } /// Command-line options for the duplicate detector. diff --git a/audit/runtime/ingest.py b/audit/runtime/ingest.py index 05f89925..222bc060 100644 --- a/audit/runtime/ingest.py +++ b/audit/runtime/ingest.py @@ -23,6 +23,7 @@ import argparse import json +import re import sys from pathlib import Path from typing import Any @@ -80,6 +81,22 @@ 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._-]+") + + +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 leak_harness_to_sarif(result: dict[str, Any]) -> dict[str, Any]: """Leak-harness JSON → SARIF. Only ``leaked: true`` findings (the deterministic growth assertion tripped) become results; a clean loop is evidence of *no* leak.""" @@ -115,11 +132,14 @@ def duplicate_detector_to_sarif(result: dict[str, Any]) -> dict[str, Any]: for f in result.get("findings", []): if not f.get("report", True): continue - uri, line = _location(f) + # heap-wide duplicate groups have no source line; synthesize a UNIQUE uri per + # value so distinct groups stay distinct clusters (see _dup_uri). Always + # file-level (line 0) -> no fabricated region. + uri = _dup_uri(str(f.get("type", "")), str(f.get("value", "")), len(findings)) findings.append({ "rule": f.get("rule", "RUNTIME-DUP-IMMUTABLE"), "message": f.get("message", ""), - "uri": uri, "line": line, + "uri": uri, "line": 0, "properties": {k: f[k] for k in ("type", "value", "count", "bytesPerInstance", "wastedBytes") if k in f}, @@ -133,8 +153,11 @@ def duplicate_detector_to_sarif(result: dict[str, Any]) -> dict[str, Any]: def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser(description="Convert a runtime tool's JSON result to SARIF.") - ap.add_argument("--leak-harness", help="leak-harness result JSON") - ap.add_argument("--duplicate-detector", help="duplicate-immutable-detector result JSON") + # exactly one source tool — reject both so a stray flag fails fast instead of + # silently converting the wrong file (CodeRabbit review on #103). + 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") 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) @@ -242,27 +265,42 @@ def check(ok: bool, msg: str) -> None: # Duplicate-immutable detector (cat. 11, the project's "gold"): type/value-based, # file-level, level warning; a below-threshold finding (report:false) is dropped. + # The real C# emits NO source location (heap-wide groups), so the bridge must + # synthesize a UNIQUE per-value uri — else distinct values (Country, Currency) + # collapse into one cluster, corrupting totals/heatmap (Codex review on #103). dup = duplicate_detector_to_sarif({ "tool": "duplicate-detector", "target": "acme/LegacyApp", "minWastedBytes": 65536, "findings": [ {"rule": "RUNTIME-DUP-IMMUTABLE", "type": "System.String", "value": "Country", - "count": 48211, "bytesPerInstance": 36, "wastedBytes": 1735560, - "location": "Acme.Ref.CountryTable", "line": 0, "report": True, + "count": 48211, "bytesPerInstance": 36, "wastedBytes": 1735560, "report": True, "message": "48211 duplicate 'Country' strings (~1.7 MB wasted)"}, + {"rule": "RUNTIME-DUP-IMMUTABLE", "type": "System.String", "value": "Currency", + "count": 31002, "bytesPerInstance": 38, "wastedBytes": 1178038, "report": True, + "message": "31002 duplicate 'Currency' strings (~1.1 MB wasted)"}, {"rule": "RUNTIME-DUP-IMMUTABLE", "type": "System.String", "value": "x", "count": 3, "wastedBytes": 108, "report": False, "message": "below threshold"}, ]}) dres = dup["runs"][0]["results"] - check(len(dres) == 1, f"below-threshold dup finding must be dropped: got {len(dres)}") - check(dres[0]["level"] == "warning", "duplicate-immutable must be SARIF level warning (P2)") - check("region" not in dres[0]["locations"][0]["physicalLocation"], + check(len(dres) == 2, f"below-threshold dup finding must be dropped: got {len(dres)}") + check(all(r["level"] == "warning" for r in dres), + "duplicate-immutable must be SARIF level warning (P2)") + check(all("region" not in r["locations"][0]["physicalLocation"] for r in dres), "type-based duplicate finding must stay file-level (no region)") + uris = {r["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] for r in dres} + check(len(uris) == 2, f"each duplicate value needs a unique synthetic uri, got {uris}") check(dres[0]["properties"].get("wastedBytes") == 1735560, "duplicate finding must carry wastedBytes in properties") - dcat = {f.rule: (f.category, f.category_name) - for f in normalize_results(parse_sarif(json.dumps(dup), "duplicate-detector", []), tax)} + dnorm = normalize_results(parse_sarif(json.dumps(dup), "duplicate-detector", []), tax) + dcat = {f.rule: (f.category, f.category_name) for f in dnorm} check(dcat.get("RUNTIME-DUP-IMMUTABLE") == (11, "duplicate-immutable"), f"duplicate-immutable -> category 11, got {dcat.get('RUNTIME-DUP-IMMUTABLE')}") + # distinct over-threshold values must NOT collapse into a single cluster. + dscored = score(dnorm, tax, line_tol=3) + check(dscored["totals"]["clusters"] == 2, + f"Country and Currency must stay 2 separate clusters, got " + f"{dscored['totals']['clusters']}") + check(dscored["by_category"].get("duplicate-immutable") == 2, + f"both duplicate values must count under category 11, got {dscored['by_category']}") fails = [c for c in checks if c] for f in fails: diff --git a/audit/static/run_static.py b/audit/static/run_static.py index 946853ca..555d5ea2 100755 --- a/audit/static/run_static.py +++ b/audit/static/run_static.py @@ -275,12 +275,23 @@ def check(ok: bool, msg: str) -> None: # total derives from the call count "locations": [{"physicalLocation": {"artifactLocation": {"uri": "src/App/Svc.cs"}, "region": {"startLine": 6}}}]}]}]} (out2 / "leak-harness.sarif").write_text(json.dumps(leak), encoding="utf-8") + # a runtime duplicate-detector SARIF (heap-wide, file-level) must be folded in + # too, so a filename/tool-name typo in the runtime pickup loop fails CI here, + # not only on a Windows run (CodeRabbit review on #103). + dup = {"runs": [{"tool": {"driver": {"name": "duplicate-detector"}}, "results": [ + {"ruleId": "RUNTIME-DUP-IMMUTABLE", "level": "warning", + "message": {"text": '48211 duplicate "Country" strings (~1.6 MB wasted)'}, + "locations": [{"physicalLocation": {"artifactLocation": { + "uri": "heap://System.String/0000-Country"}}}]}]}]} + (out2 / "duplicate-detector.sarif").write_text(json.dumps(dup), 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"]), "roslyn per-project SARIF under roslyn/ must be picked up") check(any(t["tool"] == "leak-harness" for t in res2["tiers"]), "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(res2["totals"]["high_confidence"] >= 1, "runtime leak + static finding in one file must form a high-confidence cluster")