From c8e805a2fae2291c306943cbe3eb2fa7a0410c7c Mon Sep 17 00:00:00 2001 From: Corey Leavitt Date: Tue, 2 Jun 2026 23:25:33 -0600 Subject: [PATCH 1/2] 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() From 4b374eb0a615314f7a1ffac51f2483a1fcea1385 Mon Sep 17 00:00:00 2001 From: ringabout <43030857+ringabout@users.noreply.github.com> Date: Thu, 4 Jun 2026 19:29:48 +0800 Subject: [PATCH 2/2] stop a temp register from being freed if addressed for `lent` (#25861) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://github.com/nim-lang/Nim/issues/25849 The important part is in compiler/vmgen.nim:1838: when the VM lowers a[i] or a.b as an address-producing operation, it emits opcLdArrAddr / opcLdObjAddr. That returns an alias into the storage owned by the source register. Before the patch, that source register could still betreated as a normal temporary and later reclaimed or reused by the allocator. Once that happened, the address result was still live, but the backing temp was no longer guaranteed to exist, which is what led to the nil/illegal-storage crash. The fix is to pin that source temp by changing its slot kind to slotTempPerm right after emitting the address load. You can see the same lifetime rule already existed for the generic addr(...) path around compiler/vmgen.nim:1551: if the source is a temporary and we take its address, the compiler marks it permanent so freeTemp won’t recycle it. The patch extends that exact rule to array and object address loads: - compiler/vmgen.nim:1843 - compiler/vmgen.nim:1861 slotTempPerm is outside the normal freeTemp range in compiler/vmgen.nim:248, so once a temp is upgraded to permanent, the VM allocator stops treating it as reusable. That is the actual root-cause fix: it preserves the backing storage for the address result until the surrounding evaluation is done. The regression test in tests/vm/t25849.nim:8 forces exactly that path with a local lent iterator over an array and a static VM evaluation. --- compiler/vmgen.nim | 4 ++++ tests/vm/t25849.nim | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 tests/vm/t25849.nim diff --git a/compiler/vmgen.nim b/compiler/vmgen.nim index 898b5b5def..dd8b8365ca 100644 --- a/compiler/vmgen.nim +++ b/compiler/vmgen.nim @@ -1842,6 +1842,8 @@ proc genArrAccessOpcode(c: PCtx; n: PNode; dest: var TDest; opc: TOpcode; if dest < 0: dest = c.getTemp(n.typ) if opc in {opcLdArrAddr, opcLdStrIdxAddr} and gfNodeAddr in flags: c.gABC(n, opc, dest, a, b) + if c.prc.regInfo[a].kind >= slotTempUnknown: + c.prc.regInfo[a].kind = slotTempPerm elif needsRegLoad(): var cc = c.getTemp(n.typ) c.gABC(n, opc, cc, a, b) @@ -1858,6 +1860,8 @@ proc genObjAccessAux(c: PCtx; n: PNode; a, b: int, dest: var TDest; flags: TGenF if dest < 0: dest = c.getTemp(n.typ) if {gfNodeAddr} * flags != {}: c.gABC(n, opcLdObjAddr, dest, a, b) + if a < c.prc.regInfo.len and c.prc.regInfo[a].kind >= slotTempUnknown: + c.prc.regInfo[a].kind = slotTempPerm elif needsRegLoad(): var cc = c.getTemp(n.typ) c.gABC(n, opcLdObj, cc, a, b) diff --git a/tests/vm/t25849.nim b/tests/vm/t25849.nim new file mode 100644 index 0000000000..a503dd0479 --- /dev/null +++ b/tests/vm/t25849.nim @@ -0,0 +1,16 @@ +discard """ + targets: "c cpp js" +""" + +import std/os +from std/sequtils import toSeq + +iterator items(a: array[3, string]): lent string {.inline.} = + for i in 0..2: + yield a[i] + +static: + const key = "NIM_TESTS_TOSENV_KEY" + for val in items(["a", "b", "c"]): + putEnv(key, val) + doAssert (key, val) in toSeq(envPairs())