`reset` causes crash with `cursor` and `mm:arc`

[arnetheduck]

Nim Version

2.2.10, 0bfb59f

Description

type Callback = proc() {.closure.}
proc testit(v: ref seq[Callback]) =
  var cbc {.cursor.}: Callback

  proc cb() =
    for c in v[].mitems():
      if c == cbc:
        reset(c)
    reset(cbc) # Commenting this fixes the crash

  cbc = cb

  v[].add cb

proc run() =
  let callbacks = new seq[Callback]
  for i in 0..10:
    testit(callbacks)

  for c in callbacks[]:
    c()

run()

Current Output

/Nim/lib/system/arc.nim(221) nimDestroyAndDispose
/Nim/lib/system/arc.nim(221) nimDestroyAndDispose
/Nim/lib/system/arc.nim(221) nimDestroyAndDispose
/Nim/lib/system/arc.nim nimDecRefIsLast
Error: call depth limit reached in a debug build (2000 function calls). You can change it with -d:nimCallDepthLimit=<int> but really try to avoid deep recursions instead.
Error: execution of an external program failed: 'testit'

Segmentation fault         (core dumped)

Expected Output

Known Workarounds

No response

Additional Information

No response

[Graveflo]

reset(c) is where the closure is destroyed and then you mutate its environment with reset(cbc). That is UB.

[arnetheduck]

is where the closure is destroyed

Right, good point, thanks!

[arnetheduck]

reset(c) is where the closure is destroyed and then you mutate its environment with reset(cbc). That is UB.

Actually, I think this looks wrong - environment should be kept alive for the duration of the call in c() - it is after all a reference to the environment as well.

Here's a simpler example that similarly fails:

type Callback = proc() {.closure.}
proc testit(v: ref seq[Callback]) =
  var cbc {.cursor.}: Callback

  proc cb() =
    for c in v[].mitems():
      if c == cbc:
        reset(c)

  cbc = cb

  v[].add cb

proc run() =
  let callbacks = new seq[Callback]
  for i in 0..10:
    testit(callbacks)

  for c in callbacks[]:
    c()

run()

nim c --mm:arc -d:useMalloc testit.nim
valgrind ./testit
==188055== Invalid read of size 8
==188055==    at 0x223820: testit::cb (/lib/system/iterators.nim:263)
==188055==    by 0x223B5B: testit::run (_exper/testit.nim:20)
==188055==    by 0x223C9B: NimMainModule (_exper/testit.nim:22)
==188055==    by 0x223C9B: NimMainInner (_exper/testit.nim:27)
==188055==    by 0x223C9B: NimMain (_exper/testit.nim:38)
==188055==    by 0x223C9B: main (_exper/testit.nim:46)
==188055==  Address 0x4e860f0 is 16 bytes inside a block of size 40 free'd
==188055==    at 0x4843D73: free (vg_replace_malloc.c:950)
==188055==    by 0x2229C6: _ZN6system11deallocImplE7pointer (/lib/system/mm/malloc.nim:28)
==188055==    by 0x2229C6: _ZN6system17deallocSharedImplE7pointer (/lib/system/mm/malloc.nim:45)
==188055==    by 0x2229C6: deallocShared (/lib/system/memalloc.nim:308)
==188055==    by 0x2229C6: alignedDealloc (/lib/system/memalloc.nim:0)
==188055==    by 0x2229C6: nimRawDispose (/lib/system/arc.nim:213)
==188055==    by 0x2229C6: nimDestroyAndDispose (/lib/system/arc.nim:229)
==188055==    by 0x2229C6: _ZN6system10eqdestroy_E3varI4procIEE (/lib/system/threadimpl.nim:39)
==188055==    by 0x2229C6: system::reset(var<proc<> >) (/lib/system.nim:938)
==188055==    by 0x22381F: testit::cb (_exper/testit.nim:8)
==188055==    by 0x223B5B: testit::run (_exper/testit.nim:20)
==188055==    by 0x223C9B: NimMainModule (_exper/testit.nim:22)
==188055==    by 0x223C9B: NimMainInner (_exper/testit.nim:27)
==188055==    by 0x223C9B: NimMain (_exper/testit.nim:38)
==188055==    by 0x223C9B: main (_exper/testit.nim:46)
==188055==  Block was alloc'd at
==188055==    at 0x4840AE6: malloc (vg_replace_malloc.c:447)
==188055==    by 0x22020B: _ZN6system9allocImplE25range09223372036854775807 (/lib/system/mm/malloc.nim:5)
==188055==    by 0x22020B: allocSharedImpl (/lib/system/mm/malloc.nim:34)
==188055==    by 0x22020B: _ZN6system12alignedAllocE25range0922337203685477580725range09223372036854775807 (/lib/system/memalloc.nim:325)
==188055==    by 0x22020B: _ZN6system13alignedAlloc0E25range0922337203685477580725range09223372036854775807 (/lib/system/memalloc.nim:344)
==188055==    by 0x22020B: nimNewObj (/lib/system/arc.nim:116)
==188055==    by 0x223948: testit::testit(ref<seq<proc<> > >) (_exper/testit.nim:3)
==188055==    by 0x223A44: testit::run (_exper/testit.nim:17)
==188055==    by 0x223C9B: NimMainModule (_exper/testit.nim:22)
==188055==    by 0x223C9B: NimMainInner (_exper/testit.nim:27)
==188055==    by 0x223C9B: NimMain (_exper/testit.nim:38)
==188055==    by 0x223C9B: main (_exper/testit.nim:46)

The underlying issue I guess is that the for loop lends the reference to the environment during iteration which the reset rugpulls - the open question is whether c(), when it calls an environment, should in fact refcount that environment / de-lend c.

[nitely]

workaround:

type Callback = proc() {.closure.}
proc testit(v: ref seq[Callback]) =
  var cbc {.cursor.}: Callback

  proc cb() =
    for c in v[].mitems():
      if c == cbc:
        reset(c)
        return  # <--- this

  cbc = cb

  v[].add cb

proc run() =
  let callbacks = new seq[Callback]
  for i in 0..10:
    testit(callbacks)

  for c in callbacks[]:
    c()

run()

IIUC, if you don't return right away, the loops continues and access the env, which has been destroyed:

--expandArc: cb

block :tmp:
  var c
  var i = 0
  let L = len(:envP.v0[])
  block :tmp_1:
    while i < L:
      c = :envP.v0[][i]
      if c == :envP.cbc1_cursor:
        reset(c)
        return
      {.push, overflowChecks: false.}
      inc(i, 1)
      {.pop.}
-- end of expandArc ------------------------

[arnetheduck]

the loops continues and access the env, which has been destroyed:

indeed - the question is though, should the env be destroyed in this case? the caller is after all still in scope..

[nitely]

variant:

type Callback = proc() {.closure.}
proc testit(v: ref seq[Callback]) =
  var cbc {.cursor.}: Callback

  proc cb() =
    for c in v[].mitems():
      if c == cbc:
        reset(c)
        #return  # <--- this

  cbc = cb

  v[].add cb

proc run() =
  let callbacks = new seq[Callback]
  for i in 0..10:
    testit(callbacks)

  let L = callbacks[].len
  var i = 0
  while i < L:
    callbacks[][i]()
    inc i

run()