buffers: complete the counter set + sensitive/clear enforcement (OWN024)#4
Conversation
Closes the small gaps from the stackalloc/logging design audit.
Counters (§10): emit the four metrics that were specified but missing —
ScratchTotalRequestedBytes / ScratchMaxRequestedBytes (via Requested()),
ScratchPoolBytesReturned (via PoolReturned(), inside the rented-null guard),
and ScratchForcedClears (via ForcedClear(), next to a clear-on-release). All
scratch-scoped and under [Conditional("OWNSHARP_COUNTERS")].
Sensitive buffers (OWN024, §12): a `sensitive` option / policy key, and a
check that a buffer marked sensitive but not cleared on release is rejected --
pooled/scratch arrays return to a shared ArrayPool, native memory goes back to
the allocator, so leaving secret bytes unzeroed is the silent leak the flag
exists to prevent. The report (json + text) now carries `sensitive`.
README: document OWN024 + the full counter set; the unsafe-contract (UNS0xx,
§6) and benchmark-matrix (§13) work stays flagged as roadmap.
Tests: 5 OWN024 analysis cases (option + policy, cleared/uncleared, malformed)
and 2 codegen-content cases for the new counter emission. Suite: analysis
100/100, codegen content 23/23, property fuzz clean (20k draws).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Changessensitive attribute, OWN024 diagnostic, and scratch counter instrumentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ownlang/codegen.py (1)
707-719: 💤 Low valueCAS loop is correct and thread-safe.
The compare-and-swap retry loop correctly updates the max value in a concurrent environment. The early return when
bytes <= curavoids unnecessary CAS attempts.Optional: Add comment explaining retry logic
Consider adding a brief comment above the do-while loop:
System.Threading.Interlocked.Add(ref ScratchTotalRequestedBytes, bytes); + // CAS retry loop: update max if bytes > current max; retry if concurrent update long cur; doThis helps future maintainers quickly understand the concurrency pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ownlang/codegen.py` around lines 707 - 719, Add a brief explanatory comment above the do-while loop in the Requested method to document the compare-and-swap (CAS) retry pattern. The comment should explain that this loop safely updates the ScratchMaxRequestedBytes maximum value in a concurrent environment by repeatedly reading the current value with Interlocked.Read and attempting to update it with Interlocked.CompareExchange, retrying if another thread modified the value in between. This will help future maintainers quickly understand the thread-safe concurrency pattern being used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ownlang/codegen.py`:
- Around line 707-719: Add a brief explanatory comment above the do-while loop
in the Requested method to document the compare-and-swap (CAS) retry pattern.
The comment should explain that this loop safely updates the
ScratchMaxRequestedBytes maximum value in a concurrent environment by repeatedly
reading the current value with Interlocked.Read and attempting to update it with
Interlocked.CompareExchange, retrying if another thread modified the value in
between. This will help future maintainers quickly understand the thread-safe
concurrency pattern being used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c808e4e-7c99-42b4-b828-c920151cf8d8
📒 Files selected for processing (7)
README.mdownlang/buffers.pyownlang/codegen.pyownlang/diagnostics.pyownlang/report.pytests/run_tests.pytests/test_codegen.py
… re-validation Leak #4 in the SystemEvents fixture: a FileStream that IS disposed, but the Dispose() sits inside the try after a may-throw call (WriteByte), so it's skipped on the exceptional path. This is CodeQL's cs/dispose-not-called-on-throw (cs/local-not-disposed also models it). Own.NET used to miss it — disposed somewhere looked balanced — until the exception-edge model (77b2edd) inserted a throw edge before each may-throw statement in a try. Re-running the oracle on this fixture should land #4 in "Agree" with CodeQL, joining #2/#3, with #1 (subscription) staying Own.NET-only. Bumps the oracle sentinel to trigger the push-run.
…oracle fixture
The exception-edge try-lowering injects an exceptional exit (a bare return while
the local is live) before each may-throw statement. A local never disposed then
leaks on BOTH that exit and the normal fall-through, so the core emits OWN001
once per exit. Every flow-local diagnostic remaps to the acquire line, so the two
collapse to byte-identical findings. The first oracle run surfaced this as
Program.cs:54 appearing twice in "Agree".
- ownir.py: drop byte-identical findings (same file/line/code/component/event/
handler/message/kind/severity) before sorting. Native-OwnLang leaks at distinct
lines stay distinct — the key includes line; only the bridge's line-collapse
makes them identical, so deduping is exactly right.
- test_ownir.py + flow_leak_two_exits fixture: a two-leaking-exits flow body
(acquire; if(*){return}; use) must yield exactly one OWN001 (TryNeverDisposed
'tfLeak'@105). Without the dedup it returns two.
Also tighten the SystemEvents fixture's dispose-on-throw case (#4): the three
tools anchor the same leak at different points (Own.NET acquire / CodeQL Dispose /
Infer# last-access), so a spread-out method puts them >3 lines apart and the
oracle's ±3 window splits one leak into own-only + oracle-only. Keep the try a
one-liner adjacent to the acquire so the anchors fall inside the window -> #4
joins #2/#3 in "Agree" across all three. Bumps the oracle sentinel.
fix(extractor): resolve member-group symbols in IsStaticHandler (mined ImageSharp FP #4/4)
Both reviewers converged; all corrections are factual/design refinements (no forks): - Crate DAG: own-codegen is a SIBLING of own-diagnostics (both consume own-cfg/own-analysis), not chained through it. Codegen is verdict-independent (AST/CFG-driven, matching Python codegen.generate), so the old own-diagnostics -> own-codegen edge would have forced diagnostics to re-export solver internals. Added a fitness function locking codegen !-> diagnostics (and !-> analysis for now). (Codex P2, CodeRabbit #1) - Oracle: do NOT reuse scripts/oracle_compare.py as the parity oracle — it is cross-tool fuzzy (leak-only, +-N line tolerance, coarse severity) and would mask off-by-one/label/subject/exit-status divergences. Spec a new exact harness over status+stdout+stderr+SARIF/JSON, with an exit/crash gate first, exact set-equality incl. evidence label text, and intra-tie ordering. (Codex P1, CodeRabbit #4) - CFG seam does not exist yet: python cfg prints human text, not JSON. Add+freeze a canonical cfg --format json before the ratchet uses that seam; added as migration step 0. (Codex P2) - State: arena+CoW likely wins this procedural workload; bench largest real function, wall-clock + RSS. Prior art: clippy lint-pass registry, prusti-viper encoding boundary. Repo-layout revisit trigger. (CodeRabbit #2/#3/#5/#6)
… object "Who holds this object" is ill-posed for an object reachable from many roots: there are as many answers as there are paths, and the shortest is an arbitrary pick. Reporting one of them was the tool's real weakness — the first version happily pointed at a prototype held by the stack while the leak was eight hops away through a delegate's invocation list. `roots` now samples the retained instances (--sample, default 200), computes every one's shortest path in a SINGLE BFS (breadth-first from the whole root set gives each node its shortest path for free), and reports the paths as a RANKED HISTOGRAM. On the live target this finds the leak on its own: #1 25/50 (50.0%) — via [static-event], 7 hops System.Object[] BrokerDataClasses.Property.KernelProperty BrokerDataClasses.Property.GBProperty (.fGBProperty) System.ComponentModel.PropertyChangedEventHandler (.PropertyChanged) System.Object[] (._invocationList) System.ComponentModel.PropertyChangedEventHandler BrokerDataClasses.GTD (._target) #4 1/50 (2.0%) — via [stack] — noise, and labelled as such Parent pointers are stored as addresses, not labels; type and field names are resolved only for the sampled paths. Carrying a label per node would cost hundreds of MB on a 4M-object heap. The run also exposes the remaining limit, and the README now says so plainly: the histogram partitions by SHORTEST path, so an object held by two references at once is attributed to the nearer one. Here 50% of the GTDs land under the static event and 44% under a static List<Object> — most likely many are held by BOTH, and detaching the event alone would not free them. "Which single reference, if cut, frees this object" is a different, well-posed question with a standard answer this tool does not implement: a dominator tree with retained sizes (Lengauer-Tarjan / Cooper-Harvey-Kennedy — what Eclipse MAT and dotMemory are built on). That is the honest next step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
Closes the small gaps from the stackalloc/logging design audit.
Counters (§10): emit the four metrics that were specified but missing — ScratchTotalRequestedBytes / ScratchMaxRequestedBytes (via Requested()), ScratchPoolBytesReturned (via PoolReturned(), inside the rented-null guard), and ScratchForcedClears (via ForcedClear(), next to a clear-on-release). All scratch-scoped and under [Conditional("OWNSHARP_COUNTERS")].
Sensitive buffers (OWN024, §12): a
sensitiveoption / policy key, and a check that a buffer marked sensitive but not cleared on release is rejected -- pooled/scratch arrays return to a shared ArrayPool, native memory goes back to the allocator, so leaving secret bytes unzeroed is the silent leak the flag exists to prevent. The report (json + text) now carriessensitive.README: document OWN024 + the full counter set; the unsafe-contract (UNS0xx, §6) and benchmark-matrix (§13) work stays flagged as roadmap.
Tests: 5 OWN024 analysis cases (option + policy, cleared/uncleared, malformed) and 2 codegen-content cases for the new counter emission. Suite: analysis 100/100, codegen content 23/23, property fuzz clean (20k draws).
Summary by CodeRabbit
Release Notes
New Features
sensitivebuffer attribute and policy key.New Diagnostics
sensitivebuffer isn’t cleared on release (clear = trueexpected).Documentation
Reports & Tests