From c8e805a2fae2291c306943cbe3eb2fa7a0410c7c Mon Sep 17 00:00:00 2001 From: Corey Leavitt Date: Tue, 2 Jun 2026 23:25:33 -0600 Subject: [PATCH] fixes #25595; cursor inference: a recorded mutation extends the variable's liveness (#25864) fixes #25595 ## Bug A `let` bound to a field of a value-type **case object** with a `ref` field is inferred as a non-owning cursor, but the cursor's source can be mutated through the cursor's own ref during a call, freeing the ref while the borrow still reads it. Use-after-free under arc/orc (refc is unaffected, it has no cursor inference): ```nim var destroyed = false type O = ref object value: int home: H W = object case k: bool of true: r: O of false: discard H = ref object w: W proc `=destroy`(o: var typeof(O()[])) = destroyed = true proc clear(o: O): int = o.home.w = W() # overwrites h.w via the back-reference -> frees the ref doAssert not destroyed # fails: the element was destroyed during the call result = o.value proc go(h: H): int = let c = h.w # inferred cursor (borrow of h.w) result = clear(c.r) proc main = let h = H() let o = O(value: 42) o.home = h h.w = W(k: true, r: o) doAssert go(h) == 42 main() ``` The `not destroyed` assert fails: the element is destroyed during the call, so the following `o.value` read is a use-after-free. The same code with the `=destroy` guard removed (so the freed `o.value` is actually read) is reported as `heap-use-after-free` by ASan under `-d:useMalloc -fsanitize=address`. Longstanding (reproduces back to 2.2.0). `--cursorInference:off` is a workaround. ## Root cause Cursor inference (`varpartitions.computeCursors`) cursors `let c = h.w` unless `dangerousMutation` finds a mutation of `c`'s graph within `c`'s alive range `aliveStart..aliveEnd`. Here the mutation (the `clear(c.r)` call) *is* connected to `c`'s graph and *is* recorded with `isMutated`, but it is recorded at an `abstractTime` just past `c.aliveEnd`, so the range check misses it. The gap is timing. `aliveEnd` is set from the last `nkSym` use of `c`. A call records its argument's mutation *after* traversing the whole argument subtree (`potentialMutationViaArg`). When the argument is `c.r` on a case object it is an `nkCheckedFieldExpr` (the discriminant check), whose extra nodes advance `abstractTime` past `c`'s last `nkSym`. A plain `nkDotExpr` has no such gap, so the bug needs a case object. ## Fix In `potentialMutation`, extend the mutated variable's liveness to the mutation time: ```nim v.s[id].aliveEnd = max(v.s[id].aliveEnd, v.abstractTime) ``` A variable mutated at time T is provably alive at T, so this only completes the liveness computation that `dangerousMutation` relies on. The worst case is an extra copy, never an unsound cursor. ## Note on the locus The fix is conservative by mechanism (it runs at every recorded mutation) but perf-neutral in practice: it only suppresses a cursor where the corrected liveness proves the borrow unsafe (cursor counts are unchanged on the suites). I can scope it to call arguments if you'd prefer it narrower. ## Test `tests/arc/t25595.nim`, matrix `--mm:orc; --mm:arc; --mm:refc`: the repro above as a `doAssert`. Fails (UAF) on arc/orc before the fix and passes after. refc passes throughout. ## Checks - repro passes on orc/arc after the fix. The guard-removed variant (which reads the freed value) is ASan-clean after the fix and was heap-use-after-free before. refc unaffected. - testament `destructor` 90/90, `arc` 120/120. `views` 5/6, same as stock (the one failure is environmental and pre-exists this change). - perf-neutral: inferred-cursor count is identical stock vs fix across the `arc` and `destructor` test files under `--mm:orc` (322 vs 322). --- compiler/varpartitions.nim | 3 +++ tests/arc/t25595.nim | 43 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 tests/arc/t25595.nim diff --git a/compiler/varpartitions.nim b/compiler/varpartitions.nim index ddba3d4bcb..fea6cc540b 100644 --- a/compiler/varpartitions.nim +++ b/compiler/varpartitions.nim @@ -185,6 +185,9 @@ proc root(v: var Partitions; start: int): int = proc potentialMutation(v: var Partitions; s: PSym; level: int; info: TLineInfo) = let id = variableId(v, s) if id >= 0: + # mutated here => alive here: keep aliveEnd in sync so dangerousMutation catches + # mutations recorded after the var's last use (e.g. via a call arg). See #25595. + v.s[id].aliveEnd = max(v.s[id].aliveEnd, v.abstractTime) let r = root(v, id) let flags = if s.kind == skParam: if isConstParam(s): diff --git a/tests/arc/t25595.nim b/tests/arc/t25595.nim new file mode 100644 index 0000000000..6cfaa7673a --- /dev/null +++ b/tests/arc/t25595.nim @@ -0,0 +1,43 @@ +discard """ + matrix: "--mm:orc; --mm:arc; --mm:refc" +""" + +# bug #25595: cursor inference must not borrow a case object whose source can be +# mutated through the cursor's own ref across a call. `let c = h.w` was inferred as a +# non-owning cursor; `clear(c.r)` overwrites `h.w` via the cursor's back-reference, +# freeing the ref while the borrow still uses it -> use-after-free. Detected here +# deterministically: the element's destructor must not run during the call. + +var destroyed = false + +type + O = ref object + value: int + home: H + W = object + case k: bool + of true: r: O + of false: discard + H = ref object + w: W + +proc `=destroy`(o: var typeof(O()[])) = + destroyed = true + +proc clear(o: O): int = + o.home.w = W() + doAssert not destroyed, "use-after-free: element destroyed during the call" + result = o.value + +proc go(h: H): int = + let c = h.w + result = clear(c.r) + +proc main = + let h = H() + let o = O(value: 42) + o.home = h + h.w = W(k: true, r: o) + doAssert go(h) == 42 + +main()