From eb91cf991aa9840639cc358c21d503e4cf27e268 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Tue, 5 Sep 2023 16:31:28 +0800
Subject: [PATCH] fixes #22619; don't lift cursor fields in the hook calls
 (#22638)

fixes https://github.com/nim-lang/Nim/issues/22619

It causes double free for closure iterators because cursor fields are
destroyed in the lifted destructors of `Env`.

Besides, according to the Nim manual

> In fact, cursor more generally prevents object
construction/destruction pairs and so can also be useful in other
contexts.

At least, destruction of cursor fields might cause troubles.


todo
- [x] tests
- [x] revert a certain old PR

---------

Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
---
 compiler/liftdestructors.nim |  3 +-
 tests/iter/t22619.nim        | 79 ++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 tests/iter/t22619.nim

diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim
index f28586addb..387a22555e 100644
--- a/compiler/liftdestructors.nim
+++ b/compiler/liftdestructors.nim
@@ -162,8 +162,7 @@ proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode; enforceDefaultOp: bool)
     if c.filterDiscriminator != nil: return
     let f = n.sym
     let b = if c.kind == attachedTrace: y else: y.dotField(f)
-    if (sfCursor in f.flags and f.typ.skipTypes(abstractInst).kind in {tyRef, tyProc} and
-        c.g.config.selectedGC in {gcArc, gcAtomicArc, gcOrc, gcHooks}) or
+    if (sfCursor in f.flags and c.g.config.selectedGC in {gcArc, gcAtomicArc, gcOrc, gcHooks}) or
         enforceDefaultOp:
       defaultOp(c, f.typ, body, x.dotField(f), b)
     else:
diff --git a/tests/iter/t22619.nim b/tests/iter/t22619.nim
new file mode 100644
index 0000000000..04e707633c
--- /dev/null
+++ b/tests/iter/t22619.nim
@@ -0,0 +1,79 @@
+# bug #22619
+
+when false: # todo fixme
+  block:
+    type
+      Resource = object
+        value: int
+  
+      Object = object
+        r {.cursor.}: Resource
+        s {.cursor.}: seq[Resource]
+  
+    var numDestroy = 0
+  
+    proc `=copy`(x: var Resource, y: Resource) {.error.} # disallow full copies
+    proc `=destroy`(x: Resource) =
+      inc numDestroy
+  
+    proc test() =
+      # perform the test in procedure so that globals aren't used (their different
+      # semantics with regards to destruction would interfere)
+      var
+        r = Resource(value: 1) # initialize a resource
+        s = @[Resource(value: 2)]
+  
+      # make sure no copy is required in the initializer expression:
+      var o = Object(r: r, s: s)
+  
+      # copying the object doesn't perform a full copy of the cursor fields:
+      var o2 = o
+      discard addr(o2) # prevent `o2` from being turned into a cursor
+  
+      # check that the fields were shallow-copied:
+      doAssert o2.r.value == 1
+      doAssert o2.s[0].value == 2
+  
+      # make sure no copy is required with normal field assignments:
+      o.r = r
+      o.s = s
+  
+  
+      # when `o` and `o2` are destroyed, their destructor must not be called on
+      # their fields
+  
+    test()
+  
+    # one call for the `r` local and one for the object in `s`
+    doAssert numDestroy == 2
+
+block:
+  type Value = distinct int
+
+  var numDestroy = 0
+
+  proc `=destroy`(x: Value) =
+    inc numDestroy
+
+  iterator iter(s: seq[Value]): int {.closure.} =
+    # because it is used across yields, `s2` is lifted into the iterator's
+    # environment. Since non-ref cursors in object didn't have their hooks
+    # disabled inside the environments lifted hooks, this led to double
+    # frees
+    var s2 {.cursor.} = s
+    var i = 0
+    let L = s2.len
+    while i < L:
+      yield s2[i].int
+      inc i
+
+  proc test() =
+    var s = @[Value(1), Value(2)]
+    let cl = iter
+    # make sure resuming the iterator works:
+    doAssert cl(s) == 1
+    doAssert cl(s) == 2
+    doAssert cl(s) == 0
+
+  test()
+  doAssert numDestroy == 2