fixes #26094; memory leak on exception unwinding — raising proc's result is never destroyed

compiler/ccgcalls.nim

@@ -76,25 +76,6 @@ proc isHarmlessStore(p: BProc; canRaise: bool; d: TLoc): bool =
   else:
     result = false
 
-proc cleanupTemp(p: BProc; returnType: PType, tmp: TLoc): bool =
-  if returnType.kind in {tyVar, tyLent}:
-    # we don't need to worry about var/lent return types
-    result = false
-  elif hasDestructor(returnType) and getAttachedOp(p.module.g.graph, returnType, attachedDestructor) != nil:
-    let dtor = getAttachedOp(p.module.g.graph, returnType, attachedDestructor)
-    var op = initLocExpr(p, newSymNode(dtor))
-    var callee = rdLoc(op)
-    let destroyArg =
-      if dtor.typ.firstParamType.kind == tyVar:
-        cAddr(rdLoc(tmp))
-      else:
-        rdLoc(tmp)
-    let destroy = cCall(callee, destroyArg)
-    raiseExitCleanup(p, destroy)
-    result = true
-  else:
-    result = false
-
 proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
                result: var Builder, call: var CallBuilder) =
   let canRaise = p.config.exc == excGoto and canRaiseDisp(p, ri.firstSon)
@@ -151,25 +132,18 @@ proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
             if canRaise: raiseExit(p)
 
       elif isHarmlessStore(p, canRaise, d):
-        var useTemp = false
-        if d.k == locNone:
-          useTemp = true
-          d = getTemp(p, typ.returnType)
+        if d.k == locNone: d = getTemp(p, typ.returnType)
         assert(d.t != nil)        # generate an assignment to d:
         var list = initLoc(locCall, d.lode, OnUnknown)
         list.snippet = extract(result)
         genAssignment(p, d, list, flags+{needAssignCall}) # no need for deep copying
-        if canRaise:
-          if not (useTemp and cleanupTemp(p, typ.returnType, d)):
-            raiseExit(p)
+        if canRaise: raiseExit(p)
       else:
         var tmp: TLoc = getTemp(p, typ.returnType, needsInit=true)
         var list = initLoc(locCall, d.lode, OnUnknown)
         list.snippet = extract(result)
         genAssignment(p, tmp, list, flags+{needAssignCall}) # no need for deep copying
-        if canRaise:
-          if not cleanupTemp(p, typ.returnType, tmp):
-            raiseExit(p)
+        if canRaise: raiseExit(p)
         genAssignment(p, d, tmp, {})
   else:
     finishCallBuilder(result, call)

compiler/ccgstmts.nim

@@ -820,20 +820,6 @@ proc raiseExit(p: BProc) =
       else:
         p.s(cpsStmts).addGoto("LA" & $p.nestedTryStmts[^1].label & "_")
 
-proc raiseExitCleanup(p: BProc, destroy: string) =
-  assert p.config.exc == excGoto
-  if nimErrorFlagDisabled notin p.flags:
-    p.flags.incl nimErrorFlagAccessed
-    p.s(cpsStmts).addSingleIfStmt(cUnlikely(cDeref("nimErr_"))):
-      p.s(cpsStmts).addStmt():
-        p.s(cpsStmts).add(destroy)
-      if p.nestedTryStmts.len == 0:
-        p.flags.incl beforeRetNeeded
-        # easy case, simply goto 'ret':
-        p.s(cpsStmts).addGoto("BeforeRet_")
-      else:
-        p.s(cpsStmts).addGoto("LA" & $p.nestedTryStmts[^1].label & "_")
-
 proc finallyActions(p: BProc) =
   if p.config.exc != excGoto:
     # Walk past compiler-injected `nkHiddenTryStmt` wrappers (e.g. ARC's

compiler/cgen.nim

@@ -947,7 +947,6 @@ proc putLocIntoDest(p: BProc, d: var TLoc, s: TLoc)
 proc genLiteral(p: BProc, n: PNode; result: var Builder)
 proc genOtherArg(p: BProc; ri: PNode; i: int; typ: PType; result: var Builder; argBuilder: var CallBuilder)
 proc raiseExit(p: BProc)
-proc raiseExitCleanup(p: BProc, destroy: string)
 
 proc initLocExpr(p: BProc, e: PNode, flags: TLocFlags = {}): TLoc =
   result = initLoc(locNone, e, OnUnknown, flags)

compiler/injectdestructors.nim

@@ -527,16 +527,17 @@ proc containsConstSeq(n: PNode): bool =
       if containsConstSeq(son): return true
   else: discard
 
-proc ensureDestruction(arg, orig: PNode; c: var Con; s: var Scope): PNode =
-  # it can happen that we need to destroy expression contructors
-  # like [], (), closures explicitly in order to not leak them.
+proc ensureDestruction(arg, orig: PNode; c: var Con; s: var Scope;
+                       consume = false): PNode =
+  # Give destructible expressions a scoped owner. If the expression is
+  # consumed, move it out and clear the temporary before its finalizer runs.
   if arg.typ != nil and hasDestructor(c, arg.typ):
     # produce temp creation for (fn, env). But we need to move 'env'?
     # This was already done in the sink parameter handling logic.
     result = newNodeIT(nkStmtListExpr, arg.info, arg.typ)
     let tmp = c.getTemp(s, arg.typ, arg.info, true)
     result.add c.genSink(s, tmp, arg, {IsDecl})
-    result.add tmp
+    result.add if consume: destructiveMoveVar(tmp, c, s) else: tmp
     s.final.add c.genDestroy(tmp)
   else:
     result = arg
@@ -953,11 +954,13 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode; tmpFlags = {sfSing
       else:
         result[0] = p(n[0], c, s, normal)
       if canRaise(n[0]): s.needsTry = true
-      if mode == normal:
+      # A raising call needs owned storage even when its value is consumed: the
+      # callee can partially initialize the result before control unwinds.
+      if mode == normal or canRaise(n[0]):
         if result.typ != nil and result.typ.kind notin {tyOpenArray, tyVarargs}:
           # Returns of openarray types shouldn't be destroyed
           # bug #19435; # bug #23247
-          result = ensureDestruction(result, n, c, s)
+          result = ensureDestruction(result, n, c, s, consume = mode != normal)
     of nkDiscardStmt: # Small optimization
       result = shallowCopy(n)
       if n[0].kind != nkEmpty:

tests/arc/t23267_result.nim

@@ -0,0 +1,67 @@
+discard """
+  valgrind: true
+  cmd: "nim c -d:useMalloc $file"
+  matrix: "--mm:arc; --mm:orc"
+  disabled: "freebsd"
+  disabled: "osx"
+  disabled: "openbsd"
+  disabled: "windows"
+  disabled: "32bit"
+"""
+
+import std/options
+
+type Foo = object
+  id: string
+  items: seq[string]
+
+proc build(x: int): Foo =
+  result = Foo(id: "padding-padding", items: @["a", "b", "c"])
+  if x < 0:
+    raise newException(ValueError, "boom")
+
+proc parseResult(x: int): Foo =
+  try:
+    result = build(x)
+  except CatchableError:
+    result = default(Foo)
+
+proc parseVar(x: int; dst: var Foo): bool =
+  dst = build(x)
+  result = true
+
+proc parseOption(x: int): Option[Foo] =
+  result = some(build(x))
+
+const iterations = 10_000
+
+doAssert parseResult(1).items == @["a", "b", "c"]
+
+var successfulDst: Foo
+doAssert parseVar(1, successfulDst)
+doAssert successfulDst.id == "padding-padding"
+
+doAssert parseOption(1).get.items == @["a", "b", "c"]
+
+var unchangedDst = Foo(id: "old", items: @["old"])
+try:
+  discard parseVar(-1, unchangedDst)
+except CatchableError:
+  discard
+doAssert unchangedDst == Foo(id: "old", items: @["old"])
+
+for _ in 0 ..< iterations:
+  discard parseResult(-1)
+
+for _ in 0 ..< iterations:
+  var dst: Foo
+  try:
+    discard parseVar(-1, dst)
+  except CatchableError:
+    discard
+
+for _ in 0 ..< iterations:
+  try:
+    discard parseOption(-1)
+  except CatchableError:
+    discard