SIGSEGV in closure iterator with try/except not at top level

iffy commented 1 year ago

Description

UPDATED: The following code produces a SIGSEGV:

try:
  proc myFunc() =
    iterator myFuncIter(): int {.closure.} =
      if false:
        try:
          yield 5
        except:
          discard
    var nameIterVar = myFuncIter
    discard nameIterVar()
  try:
    raise ValueError.newException("foo")
  finally:
    myFunc()
except ValueError as exc:
  if exc.msg == "foo":
    echo "21235 ok"
  else:
    echo "21235 fail"

I now believe it happens because the try/except block is not the top block in the closure iterator.

Original report:

import std/asyncdispatch

proc myFunc*() {.async.} =
  if false:
    try:
      await newFuture[void]("Thing.myFunc")
    except:
      discard

try:
  raise ValueError.newException("foo")
finally:
  waitFor myFunc()

Traceback (most recent call last)
/Users/matt/tmp/t21235.nim(13) t21235
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

On my macOS machine, this minimal code produces a SIGSEGV.

import chronos

type
  Obj = ref object
    queue: AsyncQueue[string]

proc newQueue(o: Obj): AsyncQueue[string] =
  o.queue = newAsyncQueue[string]()
  return o.queue

block:
  var o = Obj()
  var oq = o.newQueue()
  defer: waitFor o.queue.addLast("foo")
  raise ValueError.newException("foo")

Taking any line out prevents the segfault.

Nim Version

active boot switches: -d:release

Current Output

Traceback (most recent call last)
/tmp/test_tmp.nim(14) test_tmp
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Expected Output

/tmp/test_tmp.nim(15) test_tmp
Error: unhandled exception: foo [ValueError]

Possible Solution

No response

Additional Information

If it's helpful, here's a portion of the core dump:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   test_tmp                        0x0000000108c0af3e reportUnhandledErrorAux__system_3392 + 30
1   test_tmp                        0x0000000108c0b2d5 reportUnhandledError__system_3409 + 53
2   test_tmp                        0x0000000108c103fd nimLeaveFinally + 77
3   test_tmp                        0x0000000108c25fbc NimMainModule + 636
4   test_tmp                        0x0000000108c25d39 NimMainInner + 9
5   test_tmp                        0x0000000108c2600a NimMain + 42
6   test_tmp                        0x0000000108c2604e main + 62
7   libdyld.dylib                   0x00007fff647d03d5 start + 1

Original issue I filed with chronos is here: https://github.com/status-im/nim-chronos/issues/342 which includes this other version of the issue:

import chronos

type
  Obj = ref object
    queue: AsyncQueue[string]

proc newQueue(o: Obj): AsyncQueue[string] =
  o.queue = newAsyncQueue[string]()
  return o.queue

try:
  var o = Obj()
  var oq = o.newQueue()
  defer: waitFor o.queue.addLast("foo")
  raise (ref ValueError)(msg: "test")
except ValueError:
  echo "test"

iffy commented 1 year ago

I've done a little poking.

I believe it is not a bug with defer. I tested this by writing my own defer macro and it had the same error.
After adding debugging statements throughout lib/system/excpt.nim I believe it is a bug in https://github.com/nim-lang/Nim/blob/devel/compiler/closureiters.nim#L809

If I can figure out how to debug that code, I will, but if someone knows off-hand what's wrong, I'd gladly defer.

iffy commented 1 year ago

Here's the result of enabling some of the debug statements in closureiters.nim. There's a closureIterSetupExc(nil) call within this that I think might be responsible. But I don't really know what closureIterSetupExc is supposed to do.

TRANSFORM TO STATES: 
while true:
  block :stateLoop:
    try:
      closureIterSetupExc(:envP.`:curExc1`)
      goto :envP.`:state`8
      state 0:
      block :tmp:
        template result(): auto {.used.} =
          {.fatal: "You should not reference the `result` variable inside" &
              " a void async proc".}

        ## Put an ``item`` to the end of the queue ``aq``. If the queue is full,
        ## wait until a free slot is available before adding item.
        block :tmp:
          :envP.`:state` = 1
          break :stateLoop
      state 1:
      if full(aq):
        var putter =
          newFutureImpl(srcLocImpl("AsyncQueue.addLast", "asyncsync.nim", 351))
        add(aq.putters, putter)
        :envP.`:state` = 2
        break :stateLoop
      else:
        :envP.`:state` = 6
        break :stateLoop
      state 2:
      var chronosInternalTmpFuture: FutureBase = FutureBase(putter)
      chronosInternalRetFuture.child = chronosInternalTmpFuture
      :envP.`:state` = 5
      return chronosInternalTmpFuture
      state 3:
      :envP.`:curExc1` = nil
      if of(getCurrentException(), CatchableError):
        let exc = getCurrentException()
        if not full(aq) and not cancelled(FutureBase(putter)):
          wakeupNext(aq.putters)
        raise exc
      else:
        :envP.`:unrollFinally3` = true
        :envP.`:curExc1` = getCurrentException()
        :envP.`:state` = 4
        break :stateLoop
      :envP.`:state` = 4
      break :stateLoop
      state 4:
      if :envP.`:unrollFinally3`:
        if `==`(:envP.`:curExc1`, nil):
          :envP.`:state` = -1
          return result = :envP.`:tmpResult2`
        else:
          closureIterSetupExc(nil)
          raise :envP.`:curExc1`
      :envP.`:state` = 1
      break :stateLoop
      state 5:
      chronosInternalRetFuture.child = nil
      if chronosInternalRetFuture.mustCancel:
        raise (ref CancelledError)(msg: "Future operation cancelled!")
      internalCheckComplete(chronosInternalTmpFuture)
      :envP.`:state` = 4
      break :stateLoop
      state 6:
      addLastNoWait(aq, item)
      :envP.`:state` = 7
      break :stateLoop
      state 7:
      complete(chronosInternalRetFuture, srcLocImpl("", "asyncsync.nim", 348))
      :envP.`:state` = 8
      break :stateLoop
      state 8:
      :envP.`:state` = -1
      break :stateLoop
    except:
      :envP.`:state` = [0, 0, -3, 4, 0, -3, 0, 0, 0][:envP.`:state`]
      if `==`(:envP.`:state`, 0):
        raise
      :envP.`:unrollFinally3` = `<`(0, :envP.`:state`)
      if `<`(:envP.`:state`, 0):
        :envP.`:state` = `-`(:envP.`:state`)
      :envP.`:curExc1` = getCurrentException()
exception table:
0 -> 0
1 -> 0
2 -> -3
3 -> 4
4 -> 0
5 -> -3
6 -> 0
7 -> 0
8 -> 0

iffy commented 1 year ago

Here's an even smaller demonstration:

import chronos

var oq = newAsyncQueue[string]()
try:
  raise ValueError.newException("foo")
finally:
  waitFor oq.addLast("foo")

I now suspect AsyncQueue.addLast. Let me investigate that. I hope no one's getting an email for each of these comments -- if so, let me know and I'll do my debugging more quietly.

iffy commented 1 year ago

Here's an example that doesn't use AsyncQueue or chronos:

import std/asyncdispatch

proc myFunc*() {.async.} =
  if false:
    try:
      await newFuture[void]("Thing.myFunc")
    except:
      discard

try:
  raise ValueError.newException("foo")
finally:
  waitFor myFunc()

Traceback (most recent call last)
/Users/matt/tmp/t21235.nim(13) t21235
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

iffy commented 1 year ago

More minimal, no imports, no macros. I think it has something to do with yielding inside an if branch that is never taken maybe?

proc myFunc() =
  iterator myFuncIter(): int {.closure.} =
    if false:
      try:
        yield 5
      except:
        discard
  var nameIterVar = myFuncIter
  discard nameIterVar()

try:
  raise ValueError.newException("foo")
finally:
  myFunc()

iffy commented 1 year ago

Alright, I need to sleep now, but I think the issue is that when a closure iterator has a try/except within it, the whole closure is wrapped in special try/except code here:

https://github.com/nim-lang/Nim/blob/b34412ff0fb450278d8c93f6e5d7bf0e8d3f0a0b/compiler/closureiters.nim#L1283

I think one of these would be a good fix:

Make the try/except wrapping more narrow (only wrapping the original actual try/except code)
Keep wrapping the whole thing, but instead of calling closureIterSetupExc at the top, wait to call it until the point corresponding to where each try is.
Call closureIterSetupExc(nil) at the beginning of any state that happens after a finally block.

I've made a failing test here: https://github.com/iffy/Nim/commit/d6dd8f822050bfed972c0b94e2b5664cc8956ca2 If I get around to making a fix, I'll submit both the test and fix as a PR.

iffy commented 1 year ago

For instance, this fixes my issue, but since there's not much test coverage for closure iterators with try/except in tests/iter/tclosureiters.nim, I imagine this change would break a lot of code:

diff --git a/compiler/closureiters.nim b/compiler/closureiters.nim
index 5c048db2b..bc5832143 100644
--- a/compiler/closureiters.nim
+++ b/compiler/closureiters.nim
@@ -1243,11 +1243,11 @@ proc newCatchBody(ctx: var Ctx, info: TLineInfo): PNode {.inline.} =
       ctx.g.callCodegenProc("getCurrentException")))

 proc wrapIntoTryExcept(ctx: var Ctx, n: PNode): PNode {.inline.} =
-  let setupExc = newTree(nkCall,
-    newSymNode(ctx.g.getCompilerProc("closureIterSetupExc")),
-    ctx.newCurExcAccess())
+  # let setupExc = newTree(nkCall,
+  #   newSymNode(ctx.g.getCompilerProc("closureIterSetupExc")),
+  #   ctx.newCurExcAccess())

-  let tryBody = newTree(nkStmtList, setupExc, n)
+  let tryBody = newTree(nkStmtList, n)
   let exceptBranch = newTree(nkExceptBranch, ctx.newCatchBody(ctx.fn.info))

   result = newTree(nkTryStmt, tryBody, exceptBranch)

iffy commented 1 year ago

After looking it over more, I don't think I'm the person to implement this fix :) But hopefully the failing testcases are helpful?

bung87 commented 1 year ago

compile with --passc:-fsanitize=address --passl:-fsanitize=address -d:nosignalhandler I get

AddressSanitizer:DEADLYSIGNAL
=================================================================
==47254==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000100808af0 bp 0x00016f63efb0 sp 0x00016f63ecc0 T0)
==47254==The signal is caused by a READ memory access.
==47254==Hint: address points to the zero page.
    #0 0x100808af0 in NimMainModule+0x704 (a:arm64+0x100048af0) (BuildId: ca7428a66c6e38a6b4414fe6021ab2b532000000200000000100000000000d00)
    #1 0x1008083e0 in NimMainInner+0x8 (a:arm64+0x1000483e0) (BuildId: ca7428a66c6e38a6b4414fe6021ab2b532000000200000000100000000000d00)
    #2 0x100808fd4 in NimMain+0xc (a:arm64+0x100048fd4) (BuildId: ca7428a66c6e38a6b4414fe6021ab2b532000000200000000100000000000d00)
    #3 0x100809120 in main+0x140 (a:arm64+0x100049120) (BuildId: ca7428a66c6e38a6b4414fe6021ab2b532000000200000000100000000000d00)
    #4 0x1a8c43f24  (<unknown module>)

==47254==Register values:
 x[0] = 0x0000000000000000   x[1] = 0x0000000103900900   x[2] = 0x0000000000000000   x[3] = 0x000000016f63da10  
 x[4] = 0x00000001009760c0   x[5] = 0x0000000000000000   x[6] = 0x000000000000001f   x[7] = 0x0000000000000000  
 x[8] = 0x0000000000000000   x[9] = 0x9da3dca2563a0017  x[10] = 0x000000016f63eb40  x[11] = 0x000000702dee7d70  
x[12] = 0x000000702dee7d6c  x[13] = 0x000000016f63eb48  x[14] = 0x000000016f63eb40  x[15] = 0x0000000000000000  
x[16] = 0x0000000000002aa0  x[17] = 0x0000000103900100  x[18] = 0x0000000000000000  x[19] = 0x000000016f63ed40  
x[20] = 0x0000000100828000  x[21] = 0x0000000100829910  x[22] = 0x000000016f63f160  x[23] = 0x00000001a8cbe396  
x[24] = 0x000000016f63f0e0  x[25] = 0x0000000000000001  x[26] = 0x0000000000000000  x[27] = 0x0000000000000000  
x[28] = 0x0000000000000000     fp = 0x000000016f63efb0     lr = 0x0000000100808abc     sp = 0x000000016f63ecc0  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (a:arm64+0x100048af0) (BuildId: ca7428a66c6e38a6b4414fe6021ab2b532000000200000000100000000000d00) in NimMainModule+0x704
==47254==ABORTING

bung87 commented 1 year ago

with --exceptions:quirky it compile and run but print out nothing.

bung87 commented 1 year ago

change back proc closureIterSetupExc from https://github.com/nim-lang/Nim/pull/11964/files will make this work.

- if not e.isNil:
-   currException = e

dxxb commented 4 days ago

Hi @yglukhov, is there any reason why the fix suggested in the previous comment (thank you @bung87) should not be applied?

nim-lang / Nim