diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a1b388a..fe6d604b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1984,6 +1984,22 @@ jobs: echo "FAIL: a tree with leaks should exit non-zero under --fail-on-finding"; exit 1 fi echo "OK: --fail-on-finding surfaced the leaks as a non-zero exit" + - name: A broken stage 1 is a hard error, never the "findings" tier + run: | + # Exit 1 means "analysed, and there are findings". If a failed + # extractor build could also exit 1, then a caller that chooses NOT to + # gate on findings — which is now the Action's default — reads a run + # that analysed nothing as clean. So stage-1 failures are normalised + # into the >=2 tier, and this pins it: --root at a path with no + # extractor project makes `dotnet run` fail with 1. + set +e + scripts/own-check.sh --root "$RUNNER_TEMP/no-such-root" --format github \ + -- "$RUNNER_TEMP/owen-action-clean" >/dev/null 2>"$RUNNER_TEMP/stage1.err" + rc=$? + set -e + tail -3 "$RUNNER_TEMP/stage1.err" || true + [ "$rc" -ge 2 ] || { echo "FAIL: a broken stage 1 must exit >=2 (the tool did not look), got $rc"; exit 1; } + echo "OK: stage-1 failure landed in the hard-error tier (exit $rc)" - name: SARIF surface is a valid 2.1.0 log (the structure code scanning enforces) run: | # The contract GitHub's code-scanning ingest enforces, checked locally so @@ -2011,6 +2027,22 @@ jobs: ' "$out" >/dev/null \ || { echo "FAIL: a result is unlocated or references an undeclared rule"; exit 1; } echo "OK: SARIF 2.1.0 — Owen driver, every result rule-backed + located" + - name: Fixtures for the action's status contract (clean tree, broken config) + run: | + # Tier 0: a tree with nothing to report. + mkdir -p "$RUNNER_TEMP/owen-action-clean" + printf 'public class Clean { public int M() { return 1; } }\n' \ + > "$RUNNER_TEMP/owen-action-clean/Clean.cs" + # Asserted here rather than on the action step: a `uses:` step's + # stdout is not capturable, so "annotates nothing" is checked through + # the same script the action runs. + out=$(scripts/own-check.sh --format github -- "$RUNNER_TEMP/owen-action-clean") + [ -z "$(echo "$out" | grep '^::' || true)" ] \ + || { echo "FAIL: a clean tree must emit no annotations, got:"; echo "$out"; exit 1; } + echo "OK: clean tree, zero annotations" + # Tier >=2: a config own-check refuses to parse, so it exits 2 before + # analysing anything. + printf 'this is not = valid toml [[[\n' > "$RUNNER_TEMP/owen-broken.toml" - name: The composite action runs end-to-end (non-failing) uses: ./ with: @@ -2018,6 +2050,66 @@ jobs: format: github fail-on-finding: "false" + # The four tiers of the action's status contract, each pinned. The point + # of "annotations, not failures" is that it applies to FINDINGS ONLY; + # every other tier must keep behaving exactly as before, or the friendly + # default silently turns "the tool could not run" into a green check. + # + # Deliberately here and not only in action-marketplace-readiness.yml, + # which covers the same ground for the consumer fixture: that workflow is + # PATH-FILTERED (action.yml, own-check.sh, …), so a change to the Python + # core's exit codes — where these tiers actually originate — would never + # wake it. This job runs on every push and PR. + - name: "Tier 1 — findings, DEFAULT inputs: annotations published, step succeeds" + id: default_findings + uses: ./ + with: + path: frontend/roslyn/samples + format: github + # fail-on-finding deliberately NOT set: this is the out-of-the-box + # experience of someone who just added Owen to their repository. + - name: "Tier 1 — the default really was non-blocking" + run: | + [ "${{ steps.default_findings.outcome }}" = "success" ] \ + || { echo "FAIL: a leaky tree must not fail the step by default"; exit 1; } + echo "OK: findings did not fail the step under default inputs" + - name: "Tier 1 opt-in — fail-on-finding: true turns the same findings into a failure" + id: strict_findings + continue-on-error: true + uses: ./ + with: + path: frontend/roslyn/samples + format: github + fail-on-finding: "true" + - name: "Tier 1 opt-in — the strict mode really did fail" + run: | + [ "${{ steps.strict_findings.outcome }}" = "failure" ] \ + || { echo "FAIL: fail-on-finding: true must fail on a leaky tree (outcome=${{ steps.strict_findings.outcome }})"; exit 1; } + echo "OK: the same tree, the same annotations, a failing status" + - name: "Tier 0 — a clean tree succeeds and annotates nothing" + id: clean_tree + uses: ./ + with: + path: ${{ runner.temp }}/owen-action-clean + format: github + - name: "Tier >=2 — an operational failure fails the step even with fail-on-finding: false" + id: operational_failure + continue-on-error: true + uses: ./ + with: + path: frontend/roslyn/samples + format: github + fail-on-finding: "false" + # A malformed own.toml makes own-check exit 2 before it can analyse + # anything. That is the tool failing to LOOK, not a defect found in + # the caller's code — the friendly default must not absorb it. + config: ${{ runner.temp }}/owen-broken.toml + - name: "Tier >=2 — the operational failure really did fail" + run: | + [ "${{ steps.operational_failure.outcome }}" = "failure" ] \ + || { echo "FAIL: an operational failure (exit >=2) must fail the step regardless of fail-on-finding (outcome=${{ steps.operational_failure.outcome }})"; exit 1; } + echo "OK: 'could not look' did not become 'looked and found nothing'" + # Dog-food the code-scanning surface end-to-end (P-013): run the composite action # with format: sarif over the sample tree, then upload the log to GitHub code # scanning. The repo is public, so code scanning is free — this is the live proof @@ -2569,6 +2661,45 @@ jobs: print(f"ok: nothing durably retains the window — verdict " f"{doc.get('verdict')}, roots seen: {dict(kinds) or 'none'}") PY + - name: "attach denied by kernel policy is an honest exit 2, never a clean verdict (A4)" + if: runner.os == 'Linux' + run: | + # The operational contract, asserted rather than described: with Yama + # at its default scope, attaching to a NON-DESCENDANT process is + # refused by the kernel. The witness must say so, name the policy, and + # exit 2 — the tier that means "I did not look", distinct from 0 + # (looked, nothing retained) and 1 (looked, retention found). This is + # the failure the first CI round of this arc actually hit. + # + # Runs BEFORE the demo step relaxes the scope; the app is a sibling of + # the witness (both children of this shell), which is exactly the + # shape Yama scope 1 forbids. + sudo sysctl -w kernel.yama.ptrace_scope=1 + dotnet build "$GITHUB_WORKSPACE/examples/flagship/console/bad" -c Release -v quiet + APP="$GITHUB_WORKSPACE/examples/flagship/console/bad/bin/Release/net8.0/BadDocumentApp.dll" + WITNESS="$GITHUB_WORKSPACE/audit/runtime/RetentionPath/bin/Release/net8.0/RetentionPath.dll" + STOP="$RUNNER_TEMP/denied-stop"; LOG="$RUNNER_TEMP/denied-app.log" + rm -f "$STOP" + OWEN_FLAGSHIP_HOLD=1 OWEN_FLAGSHIP_STOP="$STOP" OWEN_FLAGSHIP_HOLD_SECONDS=120 \ + dotnet "$APP" > "$LOG" 2>&1 & + for _ in $(seq 1 60); do grep -q "holding (pid" "$LOG" 2>/dev/null && break; sleep 1; done + grep -q "holding (pid" "$LOG" || { cat "$LOG"; echo "FAIL: sample never held"; exit 1; } + PID=$(sed -n 's/.*holding (pid \([0-9]*\)).*/\1/p' "$LOG" | head -1) + set +e + out=$(dotnet "$WITNESS" roots --pid "$PID" --type Owen.Flagship.DocumentView \ + --out "$RUNNER_TEMP/denied.json" 2>&1) + rc=$? + set -e + touch "$STOP" + echo "$out" + [ "$rc" -eq 2 ] || { echo "FAIL: a denied attach must exit 2, got $rc"; exit 1; } + echo "$out" | grep -q "ptrace_scope" \ + || { echo "FAIL: the diagnostic must name the policy that refused"; exit 1; } + echo "$out" | grep -q "NOT a verdict" \ + || { echo "FAIL: the diagnostic must say it did not look"; exit 1; } + [ ! -s "$RUNNER_TEMP/denied.json" ] \ + || { echo "FAIL: a refused attach must not write a verdict artifact"; exit 1; } + echo "OK: denied attach -> exit 2, policy named, no artifact written" - name: "flagship demo orchestrator end-to-end: bad DEMONSTRATED, ok VERIFIED (A3/A4)" if: runner.os == 'Linux' run: | diff --git a/README.md b/README.md index c8f13979..2ef268e0 100644 --- a/README.md +++ b/README.md @@ -2,12 +2,55 @@ # Own.NET -> Own.NET finds lifetime/resource bugs that C# cannot express: WPF/event -> leaks, missing `Dispose`, DI lifetime mismatch, and pooled-buffer misuse. +> **Owen finds .NET lifetime bugs and confirms them at runtime — showing the +> reference path that keeps the object alive.** *Find leaks before the profiler.* GC collects unreachable objects; Own finds objects that should have become unreachable. `event +=` is acquire, `-=` is -release. +release. It also finds missing `Dispose`, DI lifetime mismatch, and +pooled-buffer misuse — the same ownership question in different skins. + +## Both halves, on one sample + +**The defect, at the line that causes it.** A window subscribes to a +process-lifetime settings hub; the unsubscribe exists, but the close path +never reaches it: + +```console +$ owen check examples/flagship/wpf/bad --fail-on-finding +DocumentWindow.xaml.cs:28: warning: [OWN001] event '_settings.PropertyChanged' + is subscribed (handler 'OnSettingsChanged') but never unsubscribed; its + source is an injected dependency whose lifetime is unknown, so it may + outlive and keep 'DocumentWindow' alive (possible leak) +``` + +**The same object at runtime, and what is actually holding it.** A separate +step — you run it when you want proof, not on every build: + +```text +verdict: RETAINED — DocumentWindow: 200 on the heap, 200 durably retained + +AppSettings + → PropertyChanged + → _invocationList + → handler + → DocumentWindow + +100% of them hang off ONE reference. +``` + +Both halves are held to a contract on every CI run, on Linux and Windows: + +```text +bad → RETAINED / exit 1 +ok → ABSENT or OBSERVED_ONLY / exit 0 / zero durable roots +``` + +The sample is real and runnable: [`examples/flagship/`](examples/flagship/) +(console and WPF, each a `bad`/`ok` pair whose only difference is where the +`-=` lives). What the runtime witness will and will not claim — and why +"reachable" is not "leaked" — is +[`docs/how-owen-proves-retention.md`](docs/how-owen-proves-retention.md). ## Run it in CI — 6 lines @@ -15,10 +58,26 @@ release. - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - uses: PhysShell/Own.NET@main # pre-release: no tagged release yet — pin a commit SHA for reproducibility with: + path: . format: github # inline PR annotations; use "sarif" for the Security tab - fail-on-finding: "true" ``` +Findings arrive as PR annotations and **the step stays green** — adding Owen +to a repository does not turn its CI red on day one. When you are ready to +gate on it: + +```yaml + with: + path: . + fail-on-finding: true +``` + +Both modes analyse identically and publish identical annotations/SARIF; only +the step's final status differs. And `fail-on-finding` governs *findings* +only — if Owen cannot complete the analysis (crash, unreadable input, no SARIF +written) the step fails in either mode. A friendly default must never turn +"could not look" into "looked and found nothing". + Once a release ships, prefer a pinned tag (`@v0.1.0`) or the moving major tag (`@v0`) over `@main` — see [`docs/notes/action-marketplace-readiness.md`](docs/notes/action-marketplace-readiness.md) diff --git a/README.ru.md b/README.ru.md index 3efff573..d6eb537d 100644 --- a/README.ru.md +++ b/README.ru.md @@ -2,13 +2,57 @@ # Own.NET -> Own.NET находит баги времени жизни/ресурсов, которые C# не может выразить: -> WPF/event-лики, забытый `Dispose`, рассинхрон DI lifetime и неправильное -> использование pooled-буферов. +> **Owen находит ошибки времени жизни в .NET и подтверждает их во время +> выполнения — показывая ссылочный путь, который держит объект живым.** *Находи лики до профайлера.* GC собирает недостижимые объекты; Own находит объекты, которые должны были стать недостижимыми. `event +=` — это acquire, -`-=` — это release. +`-=` — это release. Он также находит забытый `Dispose`, рассинхрон DI +lifetime и неправильное использование pooled-буферов — тот же вопрос о +владении в разных обличьях. + +## Обе половины, на одном образце + +**Дефект — на строке, которая его порождает.** Окно подписывается на хаб +настроек, живущий всё время процесса; отписка существует, но путь закрытия до +неё не доходит: + +```console +$ owen check examples/flagship/wpf/bad --fail-on-finding +DocumentWindow.xaml.cs:28: warning: [OWN001] event '_settings.PropertyChanged' + is subscribed (handler 'OnSettingsChanged') but never unsubscribed; its + source is an injected dependency whose lifetime is unknown, so it may + outlive and keep 'DocumentWindow' alive (possible leak) +``` + +**Тот же объект во время выполнения — и то, что его действительно держит.** +Отдельный шаг: он запускается, когда нужно доказательство, а не на каждой +сборке: + +```text +verdict: RETAINED — DocumentWindow: 200 on the heap, 200 durably retained + +AppSettings + → PropertyChanged + → _invocationList + → handler + → DocumentWindow + +100% из них висят на ОДНОЙ ссылке. +``` + +Обе половины держатся контракта на каждом прогоне CI, под Linux и Windows: + +```text +bad → RETAINED / exit 1 +ok → ABSENT or OBSERVED_ONLY / exit 0 / zero durable roots +``` + +Образец настоящий и запускаемый: [`examples/flagship/`](examples/flagship/) +(консоль и WPF, каждый — пара `bad`/`ok`, отличающаяся только тем, где живёт +`-=`). Что свидетель времени выполнения утверждает, а что — нет, и почему +«достижим» не значит «утёк»: +[`docs/how-owen-proves-retention.md`](docs/how-owen-proves-retention.md). ## Запустить в CI — 6 строк @@ -16,10 +60,26 @@ - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - uses: PhysShell/Own.NET@main # пре-релиз: тегов ещё нет — для воспроизводимости пиньте commit SHA with: + path: . format: github # инлайн-аннотации в PR; "sarif" — для вкладки Security - fail-on-finding: "true" ``` +Находки приходят аннотациями в PR, а шаг **остаётся зелёным** — добавление +Owen в репозиторий не красит чужой CI в первый же день. Когда готовы на нём +гейтить: + +```yaml + with: + path: . + fail-on-finding: true +``` + +Оба режима анализируют одинаково и публикуют одинаковые аннотации/SARIF; +отличается только итоговый статус шага. И `fail-on-finding` управляет только +*находками*: если Owen не смог довести анализ до конца (краш, нечитаемый вход, +SARIF не записан), шаг падает в любом режиме. Дружелюбный дефолт не имеет +права превращать «не смог посмотреть» в «посмотрел и ничего не нашёл». + После первого релиза предпочитайте закреплённый тег (`@v0.1.0`) или подвижный major-тег (`@v0`) вместо `@main` — политика версионирования в [`docs/notes/action-marketplace-readiness.md`](docs/notes/action-marketplace-readiness.md). diff --git a/action.yml b/action.yml index 0cb780f8..10623b3c 100644 --- a/action.yml +++ b/action.yml @@ -33,9 +33,15 @@ inputs: required: false default: "error" fail-on-finding: - description: "Fail the step when any leak is found." + description: >- + Whether a FINDING fails the step. Default false: findings are published + as annotations/SARIF and the step succeeds, so adding Owen to a repository + cannot turn its CI red on day one. Set true once you are ready to gate on + it. This input governs findings ONLY — an operational failure (the + analyser crashed, the input could not be read, no SARIF could be written) + always fails the step, in either mode. required: false - default: "true" + default: "false" python-version: description: "Python version for the Owen core." required: false @@ -130,8 +136,25 @@ runs: fi exit 0 fi - args=(--root "${{ github.action_path }}" --format "$OWN_FORMAT" --severity "$OWN_SEVERITY" "${config_args[@]}") - if [ "$OWN_FAIL_ON_FINDING" = "true" ]; then - args+=(--fail-on-finding) + # Always ask own-check for its TRUE tier (0 clean / 1 findings / >=2 + # operational failure) and decide the step's status here. Run WITHOUT + # --fail-on-finding and the script folds findings into 0, making tier 1 + # indistinguishable from tier 0 — and tier 1 is the only one this + # action is allowed to negotiate about. + set +e + "$check" --root "${{ github.action_path }}" --format "$OWN_FORMAT" \ + --severity "$OWN_SEVERITY" "${config_args[@]}" --fail-on-finding -- "$OWN_PATH" + rc=$? + set -e + if [ "$rc" -ge 2 ]; then + # NOT a finding: the analyser crashed, the input could not be read, or + # the contract drifted. "Annotations instead of failure" is a policy + # about defects found in your code, never about the tool failing to + # look — that must not reach anyone as a green check. + echo "::error::Owen could not complete the analysis (exit $rc). This is an operational failure, not a finding — fail-on-finding does not apply to it. The diagnostic is above." + exit "$rc" + fi + if [ "$rc" -eq 1 ] && [ "$OWN_FAIL_ON_FINDING" = "true" ]; then + exit 1 fi - "$check" "${args[@]}" -- "$OWN_PATH" + exit 0 diff --git a/audit/runtime/RetentionPath/Program.cs b/audit/runtime/RetentionPath/Program.cs index 502630e4..146b9293 100644 --- a/audit/runtime/RetentionPath/Program.cs +++ b/audit/runtime/RetentionPath/Program.cs @@ -75,10 +75,79 @@ private static int Main(string[] args) // A failed read must not read as "clean" — exit 2, distinct from // 0 (analysed, nothing retained) and 1 (analysed, retention found). Console.Error.WriteLine($"retention-path: {ex.GetType().Name}: {ex.Message}"); + foreach (var line in AttachAdvice(pid, live: dump == null)) + { + Console.Error.WriteLine(line); + } return 2; } } + /// + /// Turn a bare ClrMD exception into something a person can act on when + /// the kernel — not the tool — refused the attach. On Linux, Yama's + /// ptrace_scope decides whether one process may trace another; the + /// default on most distributions and on CI runners forbids attaching to + /// a process that is not a descendant, and the resulting exception says + /// nothing about why. + /// + /// Deliberately narrow: advice only when this really could be the cause + /// — a LIVE attach, on Linux, where the target exists (a missing pid is + /// a different failure and deserves no lecture about ptrace) and Yama + /// is actually restricting. Otherwise the exception stands alone. + /// + private static IEnumerable AttachAdvice(int pid, bool live) + { + if (!live || !OperatingSystem.IsLinux()) yield break; + + try { using var _ = System.Diagnostics.Process.GetProcessById(pid); } + catch { yield break; } // no such process: not a permission story + + string scope; + try { scope = File.ReadAllText("/proc/sys/kernel/yama/ptrace_scope").Trim(); } + catch { yield break; } // no Yama on this kernel + if (scope == "0") yield break; + + yield return " the target is alive, so this is a PERMISSION failure: the kernel's"; + yield return $" Yama policy (/proc/sys/kernel/yama/ptrace_scope = {scope}) refused it."; + yield return " Owen did not look — this is NOT a verdict about the target's heap."; + + // Each mode restricts something different, and the remedies do not + // carry over: telling a scope-3 user to relaunch the target as a + // descendant would be confident, actionable, and wrong. + switch (scope) + { + case "1": + yield return " Mode 1: only a DESCENDANT of the tracer may be attached to. Options:"; + yield return " * take a dump and read that instead: retention-path roots --dump …"; + yield return " * start the target FROM the witness, so it is a descendant;"; + yield return " * have the target opt in: prctl(PR_SET_PTRACER, );"; + yield return " * or relax the policy deliberately and temporarily:"; + yield return " sudo sysctl -w kernel.yama.ptrace_scope=0"; + break; + case "2": + yield return " Mode 2: attaching requires CAP_SYS_PTRACE — descendant or not, and"; + yield return " PR_SET_PTRACER does not help here. Options:"; + yield return " * take a dump and read that instead: retention-path roots --dump …"; + yield return " * run the witness with CAP_SYS_PTRACE (e.g. under sudo);"; + yield return " * or relax the policy deliberately and temporarily:"; + yield return " sudo sysctl -w kernel.yama.ptrace_scope=0"; + break; + case "3": + yield return " Mode 3: attaching is disabled outright and CANNOT be re-enabled at"; + yield return " runtime — the value is locked until reboot, so no sysctl, capability,"; + yield return " or opt-in will help on this boot. Options:"; + yield return " * take a dump and read that instead: retention-path roots --dump …"; + yield return " * or change the policy in config and reboot."; + break; + default: + yield return $" Mode {scope} is not one this build knows (0-3 are documented). Options:"; + yield return " * take a dump and read that instead: retention-path roots --dump …"; + yield return " * or consult your kernel's Yama documentation for this value."; + break; + } + } + private static int Census(RetentionWalker walker, string[] args) { var c = walker.Census(); diff --git a/docs/how-owen-proves-retention.md b/docs/how-owen-proves-retention.md new file mode 100644 index 00000000..ff13182a --- /dev/null +++ b/docs/how-owen-proves-retention.md @@ -0,0 +1,251 @@ +# How Owen proves retention + +A lifetime-bug report asks two different questions: can the program prove that +an object's release runs, and is a live process actually holding that object +through a durable reference path? + +Static analysis answers the first question. A runtime witness answers the +second for a particular heap snapshot. Owen keeps those claims separate: +source establishes the missing lifetime guarantee; runtime evidence shows what +is holding the object at the moment of observation and where that path leads. + +Everything below is either **proven by CI** on every commit, **observed in a +specific run** and named as such, or an **architectural rule** the +implementation follows. Where something is not yet true, it says so. None of +it is a roadmap entry wearing the present tense. + +--- + +## 1. The bug: an unsubscribe that exists but never runs + +A settings hub lives for the whole process. Every document window subscribes +to it in its constructor so it can restyle itself. There *is* an unsubscribe — +right there in the class: + +```csharp +public void Cleanup(bool keepAlive) +{ + if (!keepAlive) + { + _settings.PropertyChanged -= OnSettingsChanged; + } +} +``` + +And the close path calls `Cleanup(keepAlive: true)`. + +Every closed window stays in the hub's delegate list, with its whole visual +tree behind it. The code review that would catch this has to notice that a +`-=` exists, that it is guarded by a parameter, and that the one caller passes +the value that skips it. Reviewers do not reliably do this. Neither do +checkers that pair each `+=` with any `-=` in the same class. + +This is not a hypothetical. It is issue #278 reduced to its bones — a real +production leak where a formally present unsubscribe sat behind a guard nobody +ever passed `false` to, and 66% of the process heap was retained. + +## 2. What the static half establishes + +`owen check` reads the subscription, the release, and the paths between them. +It flags the subscription when it cannot *prove* the release runs: + +```console +$ owen check examples/flagship/wpf/bad --fail-on-finding +DocumentWindow.xaml.cs:28: warning: [OWN001] event '_settings.PropertyChanged' + is subscribed (handler 'OnSettingsChanged') but never unsubscribed … +``` + +The word doing the work is *provably*. A release counts when it sits somewhere +the platform itself invokes at end of life — `Dispose`, `DisposeAsync`, +`OnClosed`, `OnClosing`, `OnUnloaded`, `OnFormClosed`, `OnFormClosing` — or in +a handler wired to this object's own lifecycle event (`Closed`, `Unloaded`, +`Disposed`, …), plus anything such a method provably calls, resolved by +symbol rather than by name. A `-=` inside `Cleanup(bool)` reached only through +a parameter that switches it off is not evidence, and neither is a same-named +overload nobody calls. + +The fix moves the release into `OnClosed`, unconditionally, and the finding +disappears. Same subscription, same handler — the only change is that teardown +became a teardown. + +**Proven by CI:** the `bad`/`ok` pair is checked on Linux *and* Windows on +every run; `bad` must exit 1 with OWN001, `ok` must exit 0. + +## 3. Why a static finding is not yet proof + +Static analysis answers "is this release provable?" It cannot answer "is this +object actually still held right now, and by what?" Those are different +questions, and the second one is what a developer is really being asked to +believe. + +So Owen has a second half: a runtime witness that attaches to a live process +(or reads a dump), finds the instances of a type, and reports the reference +path from a GC root to them: + +```text +verdict: RETAINED — DocumentWindow: 200 on the heap, 200 durably retained + +AppSettings → PropertyChanged → _invocationList → handler → DocumentWindow +``` + +That is no longer a claim about the future. It is a path you can read, walk +back to a field, and delete. + +Two honest notes. The witness is a **separate step** — it is not part of +`owen check` and does not run on your builds. And it currently lives in +[`audit/runtime/RetentionPath`](../audit/runtime/RetentionPath) as a +standalone tool, not inside the published `Owen.Cli` package. + +## 4. Why "reachable from a GC root" is the wrong question + +The naive version of a witness marks from every GC root and reports whatever +it reaches. That tool will call almost anything a leak, because *reachable +right now* and *durably retained* are not the same claim. + +A local variable in a frame that has not returned yet is a GC root. So is an +object sitting on the finalizer queue. Both mean **"visible at this instant"** +— neither means anything holds the object past this instant. Report them as +retention and every leak hunt drowns in noise; worse, the one real path is +buried among them. + +So roots are classified, not counted: + +| Kind | Means | +| --- | --- | +| `static-event`, `static-field`, `gc-handle` | **durable** — something outside this moment holds it | +| `stack`, `finalizer` | **transient** — alive right now, that is all | +| `unsupported-root:` | an honest refusal: this root kind has no mapping yet | + +The verdict follows from the classification, not from reachability: + +- **RETAINED** — at least one durable retainer. Exit 1. +- **OBSERVED_ONLY** — instances exist and are reachable, but only from + transient roots. Exit 0. Live right now; not established retention. +- **ABSENT** — no instance on the heap. Exit 0. + +An unknown root kind counts as *durable* on purpose. If the mapping is +incomplete, the failure should be loud and visible, never a quietly demoted +verdict. + +## 5. Durable-first, because a stack frame can steal the answer + +An object can be reachable from a durable root *and* be sitting in a register +at the same instant. A single-pass mark credits whichever root it happens to +reach first, and "first" is an implementation detail of iteration order. + +**Observed in a live run on .NET 8:** a `Main` local holding the static +publisher caused the whole retention chain to be attributed to `[stack]` — the +static-event path, the real answer, was invisible. + +The rule the traversal is built around: **durable roots are seeded and walked +to exhaustion before any transient root enters the graph.** Transient paths +then explain only what nothing durable can. The invariant that makes this +sound — a target reachable through a durably-claimed node is itself durably +claimed, so transient traversal can never bury a durable path — is written out +in the code next to the loop that depends on it. + +## 6. What the first implementation got wrong + +Four defects, all found in review, all of the same family: something meant to +bound *cost* silently changed the *answer*. + +1. **Sampling decided the verdict.** The walk took the first N instances in + heap-enumeration order. A type with older garbage ahead of one durably-held + instance read as clean. Fixed: the root-kind census covers *every* reachable + instance; `--sample` bounds only how many paths are resolved for display. +2. **Grouping merged unlike paths.** The signature was hop type names only, so + a stack-rooted and a durably-rooted instance sharing a type sequence + collapsed into one group and inherited whichever classification arrived + first — hiding a real retainer or inventing one. Fixed: the signature + carries the classification and the traversed fields. +3. **`--max-hops` erased the root.** Truncating a long path stopped the unwind + at an intermediate object, whose root kind was `None` → `unsupported-root` + → counted as durable → a long stack-only path reported as RETAINED. Fixed: + the parent chain is walked to the true root for the verdict even when the + rendered path stays short. +4. **Any reachability was a leak.** Before the classification was consulted, a + loop local still in a register read as RETAINED. + +The principle these converge on, and the one worth taking away: + +> Sampling and display limits affect evidence **presentation** — never +> discovery, classification, aggregation, or the exit code. + +## 7. The WPF experiment that lied, and what it taught + +**Observed in a specific CI round.** The fixed WPF sample reported `0 still +subscribed` — our release had run — and the witness simultaneously reported +200 windows retained through a `[gc-handle]` path at 41 hops. Two credible +readings: WPF retains closed windows, or the measurement was wrong. + +It was the measurement. `Window.Close()` finishes through the dispatcher, and +the sample was holding itself open for the witness by parking its UI thread in +a sleep. The application was frozen halfway through tearing the windows down, +and the witness faithfully photographed framework book-keeping mid-teardown. + +With the hold changed to keep the message loop running and the count taken +only once the dispatcher goes idle, the same sample reports `ABSENT` — every +window collected, not one root of any kind. + +Two lessons, both cheap to state and expensive to learn: + +- **A runtime witness is only as honest as the moment you take the picture.** + Whatever suspends the process for measurement must not also change what the + process is holding. +- **The assertion that survived this bug was too narrow to catch it.** The + check asserted "no `static-event` root" — true, specific, and green while + the sample was misbehaving. An assertion scoped tightly enough to survive + anything tests nothing. It now asserts the whole contract. + +## 8. What CI actually proves + +Not "we ran some tests". On every commit, through the **public CLI and its +JSON artifact** — never internal APIs: + +| Claim | How it is checked | +| --- | --- | +| The static verdict is platform-independent | `owen check` runs on both Linux and Windows legs and must agree: `bad` → exit 1 + OWN001, `ok` → exit 0 | +| The samples build anywhere | both WPF projects compile on Linux (`EnableWindowsTargeting`); only *running* them needs Windows | +| The leak is real at runtime | on Windows, the sample is launched and held live, the witness attaches by pid, and must report `RETAINED`, exit 1, a `static-event` durable root, and the path anchors `AppSettings`, `PropertyChanged`, `_invocationList`, `DocumentWindow` | +| The fix is real at runtime | the fixed sample must report exit 0, `ABSENT` or `OBSERVED_ONLY`, and **zero** durable roots | +| The classifier's doctrine holds | a heap-free selftest (16 checks) pins the known root kinds and an unknown one, the verdict rules, and the agreement between the traversal-level and verdict-level definitions of "transient" | +| The demo is reproducible | one script builds, holds, attaches, and machine-validates the JSON against the human-readable verdict; a disagreement between them fails the run | + +The path anchors are checked as **semantics, not text**: type and field names +that must appear, never a verbatim path with addresses and hop counts, which +would break on any harmless change and prove nothing when it passed. + +## 9. Limits, and the operational contract + +The honest boundary of all of the above. + +**The snapshot is a moment.** The witness reports what is true when it reads +the heap. If the process is mid-teardown, mid-GC, or mid-anything, that is +what it reports (§7). Reading it as "always" is your inference, not its claim. + +**`ABSENT` and `OBSERVED_ONLY` are not interchangeable.** `ABSENT` means no +instance was on the heap; `OBSERVED_ONLY` means instances existed and were +reachable, but nothing durable held them. Both mean "no established +retention", which is why the acceptance contract accepts either — pinning one +would be pinning GC timing, which is not a public contract. + +**Unknown roots surface, they do not vanish.** A root kind with no mapping is +reported as `unsupported-root:` and counted as durable. You will see a +verdict you may not like rather than a clean bill of health the tool did not +earn. + +**Linux attach is governed by kernel policy, not by Owen.** Live attach needs +permission to trace the target. Where policy allows it, the analysis runs; +where policy denies it — the common default on modern distributions and on CI +runners — the witness exits **2**, names the policy that refused, and writes no +artifact. It does not retry silently, does not escalate, and never converts "I +could not look" into "I looked and found nothing". Running against a dump +avoids the question entirely. The full operational contract, including the +choice between a dump, a descendant launch, `PR_SET_PTRACER`, and relaxing the +policy, is in +[`docs/runtime-witness-operations.md`](runtime-witness-operations.md). + +**What is not claimed.** The witness reports retention, not causation: it +shows the reference that holds the object, not the commit that introduced it. +It does not repair anything. And it is not, today, part of the published CLI +package — it is a separate tool in this repository. diff --git a/docs/runtime-witness-operations.md b/docs/runtime-witness-operations.md new file mode 100644 index 00000000..74368656 --- /dev/null +++ b/docs/runtime-witness-operations.md @@ -0,0 +1,122 @@ +# Running the retention witness: permissions and failure modes + +The static half of Owen reads files. The runtime witness reads **another +process's memory**, which is a privileged operation on every operating system +that takes security seriously. This page is the operational contract: what it +needs, what happens when it is refused, and why a refusal is never reported as +a clean result. + +The witness lives in [`audit/runtime/RetentionPath`](../audit/runtime/RetentionPath) +and is a standalone tool — it is not part of the published `Owen.Cli` package +today. + +## Two ways to read a heap + +```console +$ retention-path roots --pid 4213 --type DocumentView # attach to a live process +$ retention-path roots --dump core.4213 --type DocumentView # read a dump file +``` + +**Live attach** suspends the target while it reads, then releases it. It needs +permission to trace that process. **Dump** mode needs only a readable file, so +it sidesteps the entire permission question — if attaching is inconvenient or +forbidden, this is the answer, not a workaround. + +## The exit-code contract + +| Exit | Meaning | +| --- | --- | +| 0 | The heap was read. `ABSENT` or `OBSERVED_ONLY` — nothing durably retains the type. | +| 1 | The heap was read. `RETAINED` — a durable retention path exists, and it is printed. | +| 2 | **The heap was not read.** Usage error, unreadable target, refused attach. | + +Exit 2 is the one that matters here. *Not looking* and *looking and finding +nothing* are different outcomes, and collapsing them is how a monitoring +pipeline learns to report health it never measured. A refused attach also +writes **no** `runtime.json` artifact — there is no verdict to record. + +**Proven by CI:** a denied attach exits 2, names the policy that refused, and +leaves no artifact behind. + +## Linux: Yama's `ptrace_scope` + +On Linux the decision belongs to the kernel, not to Owen. The Yama LSM +publishes its policy at `/proc/sys/kernel/yama/ptrace_scope`: + +| Value | Who may attach | +| --- | --- | +| `0` | any process of the same user (classic behaviour) | +| `1` | **only a descendant** — the default on Ubuntu, Debian, and GitHub-hosted runners | +| `2` | admin only | +| `3` | nobody; attach is disabled until reboot | + +Under the common default (`1`), attaching to a service you did not start from +this shell is refused *even though you own it*. The raw ClrMD exception does +not explain that, so the witness adds the reason and the ways out: + +```console +$ retention-path roots --pid 4213 --type DocumentView +retention-path: ClrDiagnosticsException: Could not attach to process 4213 + the target is alive, so this is a PERMISSION failure: the kernel's + Yama policy (/proc/sys/kernel/yama/ptrace_scope = 1) forbids attaching + to a process that is not a descendant of this one. Owen did not look — + this is NOT a verdict about the target's heap. Options: + * take a dump and read that instead: retention-path roots --dump … + * start the target FROM the witness, so it is a descendant; + * have the target opt in: prctl(PR_SET_PTRACER, ); + * or relax the policy deliberately and temporarily: + sudo sysctl -w kernel.yama.ptrace_scope=0 +``` + +The advice is deliberately narrow. It appears only when it could actually be +the cause — a live attach, on Linux, where the target process exists and Yama +is restricting. A typo'd pid gets the plain "process is not running" and no +lecture about kernel policy. + +### Choosing among the options + +- **A dump** is the right default for anything you did not launch yourself — + production services especially. No policy change, no privileges, and the + file can be read somewhere else entirely. +- **Launching the target from the witness** suits reproductions and demos: + descendants are always traceable. +- **`PR_SET_PTRACER`** is for programs that expect to be inspected and opt in + themselves. It requires changing the target. +- **Relaxing `ptrace_scope`** weakens a system-wide protection for every + process on the machine. Reasonable on a disposable CI runner or a developer + box; think twice anywhere else, and set it back. + +### What Owen does *not* do + +It does not attempt to escalate, does not retry with `sudo`, and does not +suggest running the whole tool as root. The witness stays inside whatever +permission it was given, reports honestly when that is not enough, and leaves +the policy decision to the operator. + +`scripts/flagship-demo.sh` is deliberately **sudo-free** for the same reason: a +demo script that quietly rewrites a kernel security setting is a bad neighbour. +The one place the project relaxes `ptrace_scope` is inside its own CI workflow, +on a throwaway runner, as one visible line. + +## Windows + +There is no Yama equivalent. Attaching works when the caller has sufficient +rights over the target — same user, or an elevated process for anything else. +Denials surface the same way: exit 2 with the underlying diagnostic, never a +verdict. CI attaches to its own child process, which needs no elevation. + +## In CI + +Two rules, both worth copying: + +1. **Read the exit code, not the output.** Exit 2 means the step failed to + measure; treat it as a broken step, not as a passing check with an empty + result. +2. **Relax policy in the workflow, never in the tool.** If a job needs a live + attach on a runner, the `sysctl` belongs in the job — visible in the log, + scoped to that machine — not hidden inside a script that people also run on + their laptops. + +Own.NET's own gate does exactly this, and asserts both halves: one step sets +`ptrace_scope=1` and requires the honest refusal; the next sets it to `0` and +requires the full end-to-end demo to produce its retention path. diff --git a/docs/suppression-and-fp-policy.md b/docs/suppression-and-fp-policy.md index b9c5ecbf..5b963e86 100644 --- a/docs/suppression-and-fp-policy.md +++ b/docs/suppression-and-fp-policy.md @@ -44,7 +44,7 @@ fire on unprovable input. | Lever | Status | Scope | |---|---|---| | `--severity warning` | **works today** (P-013) | Global: downgrades every error-tier finding for that run to advisory. Per-run, not per-finding — an escape hatch for "show me everything, but don't fail the build yet," not a way to silence one specific site. | -| `--fail-on-finding` set to off | **works today** (P-013) | Global: findings still print/annotate, but the process/step exit code stays 0. The CLI (`own-check.sh`) is off by default — you must pass the flag to make findings fail the shell. The GitHub Action inverts that for safety: its `fail-on-finding` input defaults to `"true"` (fails the step on a finding), so to get the "annotate but don't fail" behavior in CI you must explicitly set `fail-on-finding: "false"`. | +| `--fail-on-finding` set to off | **works today** (P-013) | Global: findings still print/annotate, but the process/step exit code stays 0. Both surfaces are off by default: the CLI (`own-check.sh`) needs the flag to make findings fail the shell, and the Action's `fail-on-finding` input defaults to `"false"` — findings annotate the PR (or land in code scanning) and the step stays green, so adding Owen to a repository cannot turn its CI red on day one. Set `fail-on-finding: "true"` when you are ready to gate on it. **This lever governs findings only**: an operational failure (the analyser crashed, the input could not be read, no SARIF could be written — own-check's `>= 2` tier) fails the step in either mode. | | `[OwnIgnore("reason")]` | **works today** on `IDisposable` fields (P-004, #209) | Inline, per-site suppression attribute — the fine-grained escape hatch for a specific site the checker can't see enough context to clear. Put `[OwnIgnore("reason")]` on the field; the finding is then **silent-but-counted** — kept out of the exit code and the human findings stream, but tallied in the run summary and carried in SARIF `suppressions` (`kind: "inSource"`, your reason as the `justification`) so nothing is lost and a consumer can audit it. The **reason is mandatory**: a reason-less `[OwnIgnore]` (or an empty `[OwnIgnore("")]`) does **not** suppress — a suppression is a documented decision, never a silent accept. The attribute is matched by simple name, so you can declare your own `OwnIgnoreAttribute`. Currently reads on `IDisposable` **field** declarations (the clearest attribute site); other sites (subscriptions, timers) are follow-up increments. | | Project-wide config (`.ownrc`/`own.toml`) | **draft, not implemented** (P-015) | Per-check-category enable/disable + severity + per-path overrides (e.g. relax a category under `tests/`). Stub status — format (TOML vs INI vs JSON) and enforcement point are still open questions in the proposal. | | `corpus/oracle-fp-baseline.txt` | **exists, but not a user-facing suppression tool** | An allowlist the *oracle comparator* (`scripts/oracle_compare.py`, a dev/maintainer tool) uses to keep already-triaged false positives out of the `own-only` bucket on re-runs. It doesn't change what `own-check`/the Action reports — it only keeps the oracle's own triage queue from re-showing confirmed noise. | diff --git a/examples/flagship/README.md b/examples/flagship/README.md index 4f265f40..1d4f4864 100644 --- a/examples/flagship/README.md +++ b/examples/flagship/README.md @@ -112,6 +112,11 @@ three release paths: | `OWEN_FLAGSHIP_STOP=`, then create that file | callers whose stdin is not a console — every CI runner | | `OWEN_FLAGSHIP_HOLD_SECONDS` (default 300) | the backstop: it applies to *every* path, so a forgotten sample cannot outlive its job | +The backstop is measured with a `Stopwatch`, not `DateTime.UtcNow` arithmetic. +A wall-clock deadline moves when the system clock does, and a bound a clock +adjustment can extend is not a bound — a small thing to get right, but "cannot +outlive its job" is either true or it is decoration. + Two details are load-bearing rather than incidental. Stdin is read on a **background** thread — a blocking read would ignore the deadline it claims to honour, and in the WPF samples it would also starve the dispatcher the hold diff --git a/examples/flagship/console/bad/Hold.cs b/examples/flagship/console/bad/Hold.cs index 7b65400d..9bc02872 100644 --- a/examples/flagship/console/bad/Hold.cs +++ b/examples/flagship/console/bad/Hold.cs @@ -10,13 +10,17 @@ // * THE STOP FILE (OWEN_FLAGSHIP_STOP) — for callers whose stdin is not a // console. Every CI runner is one of those. // * THE DEADLINE (OWEN_FLAGSHIP_HOLD_SECONDS, default 300) — applies to -// every path, so a stray sample can never outlive its job. +// every path, so a stray sample can never outlive its job. Measured with a +// Stopwatch, not wall-clock arithmetic: `DateTime.UtcNow + seconds` is a +// deadline the system clock can move, and a bound that a clock adjustment +// can extend is not a bound. // // A NULL read is deliberately not a release: with stdin closed or redirected // from nothing, `Console.ReadLine()` returns null immediately, and treating // that as "the user pressed Enter" would end the hold before a witness could // attach. using System; +using System.Diagnostics; using System.IO; using System.Threading; @@ -27,6 +31,7 @@ internal static class Hold private const int DefaultSeconds = 300; private static volatile bool _lineReceived; + private static readonly Stopwatch Elapsed = new(); public static bool Requested => Environment.GetEnvironmentVariable("OWEN_FLAGSHIP_HOLD") == "1"; @@ -45,24 +50,26 @@ public static void IfAsked() if (!Requested) return; Announce(); - DateTime deadline = DateTime.UtcNow.AddSeconds(Seconds); - while (!ShouldRelease(deadline)) Thread.Sleep(200); + while (!ShouldRelease()) Thread.Sleep(200); } /// The pid line is the orchestration contract: whoever launched - /// this process waits for it before attaching. + /// this process waits for it before attaching. Starts the deadline clock. public static void Announce() { string? stop = StopFile; Console.WriteLine($"holding (pid {Environment.ProcessId}) — send a line to exit" + (stop is null ? $", or wait {Seconds}s." : $", create {stop}, or wait {Seconds}s.")); Console.Out.Flush(); + Elapsed.Restart(); WatchStdin(); } - public static bool ShouldRelease(DateTime deadlineUtc) + /// True once ANY release path has fired: a line on stdin, the stop + /// file, or the deadline. + public static bool ShouldRelease() { - if (_lineReceived || DateTime.UtcNow >= deadlineUtc) return true; + if (_lineReceived || Elapsed.Elapsed >= TimeSpan.FromSeconds(Seconds)) return true; string? stop = StopFile; return stop != null && File.Exists(stop); } diff --git a/examples/flagship/console/ok/Hold.cs b/examples/flagship/console/ok/Hold.cs index 7b65400d..9bc02872 100644 --- a/examples/flagship/console/ok/Hold.cs +++ b/examples/flagship/console/ok/Hold.cs @@ -10,13 +10,17 @@ // * THE STOP FILE (OWEN_FLAGSHIP_STOP) — for callers whose stdin is not a // console. Every CI runner is one of those. // * THE DEADLINE (OWEN_FLAGSHIP_HOLD_SECONDS, default 300) — applies to -// every path, so a stray sample can never outlive its job. +// every path, so a stray sample can never outlive its job. Measured with a +// Stopwatch, not wall-clock arithmetic: `DateTime.UtcNow + seconds` is a +// deadline the system clock can move, and a bound that a clock adjustment +// can extend is not a bound. // // A NULL read is deliberately not a release: with stdin closed or redirected // from nothing, `Console.ReadLine()` returns null immediately, and treating // that as "the user pressed Enter" would end the hold before a witness could // attach. using System; +using System.Diagnostics; using System.IO; using System.Threading; @@ -27,6 +31,7 @@ internal static class Hold private const int DefaultSeconds = 300; private static volatile bool _lineReceived; + private static readonly Stopwatch Elapsed = new(); public static bool Requested => Environment.GetEnvironmentVariable("OWEN_FLAGSHIP_HOLD") == "1"; @@ -45,24 +50,26 @@ public static void IfAsked() if (!Requested) return; Announce(); - DateTime deadline = DateTime.UtcNow.AddSeconds(Seconds); - while (!ShouldRelease(deadline)) Thread.Sleep(200); + while (!ShouldRelease()) Thread.Sleep(200); } /// The pid line is the orchestration contract: whoever launched - /// this process waits for it before attaching. + /// this process waits for it before attaching. Starts the deadline clock. public static void Announce() { string? stop = StopFile; Console.WriteLine($"holding (pid {Environment.ProcessId}) — send a line to exit" + (stop is null ? $", or wait {Seconds}s." : $", create {stop}, or wait {Seconds}s.")); Console.Out.Flush(); + Elapsed.Restart(); WatchStdin(); } - public static bool ShouldRelease(DateTime deadlineUtc) + /// True once ANY release path has fired: a line on stdin, the stop + /// file, or the deadline. + public static bool ShouldRelease() { - if (_lineReceived || DateTime.UtcNow >= deadlineUtc) return true; + if (_lineReceived || Elapsed.Elapsed >= TimeSpan.FromSeconds(Seconds)) return true; string? stop = StopFile; return stop != null && File.Exists(stop); } diff --git a/examples/flagship/wpf/bad/App.xaml.cs b/examples/flagship/wpf/bad/App.xaml.cs index 41dc58c1..8d594a3c 100644 --- a/examples/flagship/wpf/bad/App.xaml.cs +++ b/examples/flagship/wpf/bad/App.xaml.cs @@ -63,14 +63,13 @@ private void ReportAndHold() // Hold with the message loop STILL RUNNING (see Hold.cs): a parked UI // thread is indistinguishable, to a witness, from a leak. Hold.Announce(); - DateTime deadline = DateTime.UtcNow.AddSeconds(Hold.Seconds); var timer = new DispatcherTimer(DispatcherPriority.Background) { Interval = TimeSpan.FromMilliseconds(200), }; timer.Tick += (_, _) => { - if (!Hold.ShouldRelease(deadline)) return; + if (!Hold.ShouldRelease()) return; timer.Stop(); Shutdown(); }; diff --git a/examples/flagship/wpf/bad/Hold.cs b/examples/flagship/wpf/bad/Hold.cs index 37599aec..d07a0de2 100644 --- a/examples/flagship/wpf/bad/Hold.cs +++ b/examples/flagship/wpf/bad/Hold.cs @@ -14,8 +14,11 @@ // otherwise the sample would exit before the witness could attach. Only an // actual line counts. // * Any hold can be forgotten. The deadline applies to every release path, -// so a stray sample can never outlive its job. +// so a stray sample can never outlive its job — and it is measured with a +// Stopwatch, not wall-clock arithmetic: a bound that a system-clock +// adjustment can extend is not a bound. using System; +using System.Diagnostics; using System.IO; using System.Threading; @@ -26,6 +29,7 @@ internal static class Hold private const int DefaultSeconds = 300; private static volatile bool _lineReceived; + private static readonly Stopwatch Elapsed = new(); public static bool Requested => Environment.GetEnvironmentVariable("OWEN_FLAGSHIP_HOLD") == "1"; @@ -38,22 +42,24 @@ internal static class Hold out int s) && s > 0 ? s : DefaultSeconds; /// Print the pid line — the orchestration contract: whoever - /// launched this process waits for it before attaching — and start - /// watching stdin for the interactive release. + /// launched this process waits for it before attaching — start the + /// deadline clock, and start watching stdin for the interactive + /// release. public static void Announce() { string? stop = StopFile; Console.WriteLine($"holding (pid {Environment.ProcessId}) — send a line to exit" + (stop is null ? $", or wait {Seconds}s." : $", create {stop}, or wait {Seconds}s.")); Console.Out.Flush(); + Elapsed.Restart(); WatchStdin(); } /// True once ANY release path has fired: a line on stdin, the stop /// file, or the deadline. - public static bool ShouldRelease(DateTime deadlineUtc) + public static bool ShouldRelease() { - if (_lineReceived || DateTime.UtcNow >= deadlineUtc) return true; + if (_lineReceived || Elapsed.Elapsed >= TimeSpan.FromSeconds(Seconds)) return true; string? stop = StopFile; return stop != null && File.Exists(stop); } diff --git a/examples/flagship/wpf/ok/App.xaml.cs b/examples/flagship/wpf/ok/App.xaml.cs index 8b350a79..1fffdd28 100644 --- a/examples/flagship/wpf/ok/App.xaml.cs +++ b/examples/flagship/wpf/ok/App.xaml.cs @@ -62,14 +62,13 @@ private void ReportAndHold() // Hold with the message loop STILL RUNNING (see Hold.cs): a parked UI // thread is indistinguishable, to a witness, from a leak. Hold.Announce(); - DateTime deadline = DateTime.UtcNow.AddSeconds(Hold.Seconds); var timer = new DispatcherTimer(DispatcherPriority.Background) { Interval = TimeSpan.FromMilliseconds(200), }; timer.Tick += (_, _) => { - if (!Hold.ShouldRelease(deadline)) return; + if (!Hold.ShouldRelease()) return; timer.Stop(); Shutdown(); }; diff --git a/examples/flagship/wpf/ok/Hold.cs b/examples/flagship/wpf/ok/Hold.cs index 37599aec..d07a0de2 100644 --- a/examples/flagship/wpf/ok/Hold.cs +++ b/examples/flagship/wpf/ok/Hold.cs @@ -14,8 +14,11 @@ // otherwise the sample would exit before the witness could attach. Only an // actual line counts. // * Any hold can be forgotten. The deadline applies to every release path, -// so a stray sample can never outlive its job. +// so a stray sample can never outlive its job — and it is measured with a +// Stopwatch, not wall-clock arithmetic: a bound that a system-clock +// adjustment can extend is not a bound. using System; +using System.Diagnostics; using System.IO; using System.Threading; @@ -26,6 +29,7 @@ internal static class Hold private const int DefaultSeconds = 300; private static volatile bool _lineReceived; + private static readonly Stopwatch Elapsed = new(); public static bool Requested => Environment.GetEnvironmentVariable("OWEN_FLAGSHIP_HOLD") == "1"; @@ -38,22 +42,24 @@ internal static class Hold out int s) && s > 0 ? s : DefaultSeconds; /// Print the pid line — the orchestration contract: whoever - /// launched this process waits for it before attaching — and start - /// watching stdin for the interactive release. + /// launched this process waits for it before attaching — start the + /// deadline clock, and start watching stdin for the interactive + /// release. public static void Announce() { string? stop = StopFile; Console.WriteLine($"holding (pid {Environment.ProcessId}) — send a line to exit" + (stop is null ? $", or wait {Seconds}s." : $", create {stop}, or wait {Seconds}s.")); Console.Out.Flush(); + Elapsed.Restart(); WatchStdin(); } /// True once ANY release path has fired: a line on stdin, the stop /// file, or the deadline. - public static bool ShouldRelease(DateTime deadlineUtc) + public static bool ShouldRelease() { - if (_lineReceived || DateTime.UtcNow >= deadlineUtc) return true; + if (_lineReceived || Elapsed.Elapsed >= TimeSpan.FromSeconds(Seconds)) return true; string? stop = StopFile; return stop != null && File.Exists(stop); } diff --git a/scripts/own-check.ps1 b/scripts/own-check.ps1 index 544674e2..8ecd0996 100644 --- a/scripts/own-check.ps1 +++ b/scripts/own-check.ps1 @@ -78,7 +78,15 @@ try { $exArgs = @($Paths) + @("-o", $facts.FullName) if (-not $Legacy) { $exArgs += "--flow-locals" } & dotnet run --project $extractor -- @exArgs 1>$null - if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + if ($LASTEXITCODE -ne 0) { + # Stage 1 failed: no verdict was produced. Exit 1 is reserved for + # "analysed, findings present", so a broken build must not borrow it — + # a caller that does not gate on findings would read it as clean. Map + # it into the hard-error tier (the extractor's own 2/4 pass through). + $stage1 = $LASTEXITCODE + if ($stage1 -eq 1) { $stage1 = 2 } + exit $stage1 + } # Stage 2: the one checker produces the verdict at the C# location. $env:PYTHONPATH = $Root diff --git a/scripts/own-check.sh b/scripts/own-check.sh index bb98fee0..8f937e42 100755 --- a/scripts/own-check.sh +++ b/scripts/own-check.sh @@ -111,7 +111,20 @@ if [[ -n "$config" ]]; then [[ -n "$pair" ]] && extractor_args+=(--weak-subscribe "$pair") done <<< "$weak_pairs" fi +set +e dotnet run --project "$extractor" -- "${extractor_args[@]}" 1>&2 +extract_rc=$? +set -e +if [[ "$extract_rc" -ne 0 ]]; then + # Stage 1 failed: the build broke, the extractor crashed, or it refused the + # input. NO VERDICT WAS PRODUCED, so this must not land on exit 1 — that code + # is reserved for "analysed, and there are findings", and a caller that + # chooses not to gate on findings (the Action's default) would read it as a + # clean run. Map it into the hard-error tier; the extractor's own contract + # codes (2 = usage, 4 = no input) already live there and pass through. + [[ "$extract_rc" -eq 1 ]] && extract_rc=2 + exit "$extract_rc" +fi # Optional: persist the OwnIR facts (the audit's XAML Phase-2 join consumes them # alongside xaml-facts.json). The verdict still comes from stage 2; this is just a