From 36425e91fdfd92366b2867b2fb732240d502d774 Mon Sep 17 00:00:00 2001 From: ringabout <43030857+ringabout@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:42:35 +0800 Subject: [PATCH] =?UTF-8?q?fixes=20#26094;=20memory=20leak=20on=20exceptio?= =?UTF-8?q?n=20unwinding=20=E2=80=94=20raising=20proc's=20result=20is=20ne?= =?UTF-8?q?ver=20destroyed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- compiler/ccgcalls.nim | 32 ++-------------- compiler/ccgstmts.nim | 14 ------- compiler/cgen.nim | 1 - compiler/injectdestructors.nim | 15 +++++--- tests/arc/t23267_result.nim | 67 ++++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 50 deletions(-) create mode 100644 tests/arc/t23267_result.nim diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim index eac004a3b0..708f88f972 100644 --- a/compiler/ccgcalls.nim +++ b/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) diff --git a/compiler/ccgstmts.nim b/compiler/ccgstmts.nim index 928caeacd0..0742a6d69f 100644 --- a/compiler/ccgstmts.nim +++ b/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 diff --git a/compiler/cgen.nim b/compiler/cgen.nim index 9f84920e9e..a8ce5667ae 100644 --- a/compiler/cgen.nim +++ b/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) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 37969dfd31..62678e8120 100644 --- a/compiler/injectdestructors.nim +++ b/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: diff --git a/tests/arc/t23267_result.nim b/tests/arc/t23267_result.nim new file mode 100644 index 0000000000..a9060d6147 --- /dev/null +++ b/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