fix stack-slot leaks in get-string-global, val, eval-file, and do-in - #10
Conversation
Each of these read a value or an error object off the Lua stack and never popped it, so every call permanently grew the stack of a long-lived state. Copy the value out and pop, mirroring global-exists? and call-fn. val no longer leaves the Lua error object on the stack on failure; use Luax.do-in when the message is needed.
There was a problem hiding this comment.
Build & Tests
All five suites pass locally (Linux, system Lua 5.4), matching your numbers exactly: lua 68, midlevel 53, cfunction 10, metatable 22, coroutine 25 — 178 assertions, 0 failures. CI green on both OSes.
Rather than take the A/B table on trust I re-derived it with my own probes, run against master and then against this branch. The differential is stark and confirms every claim:
master this branch
sentinel pushed, then 10x get-string-global
final get-top 11 1
get-int at -1 (should be the sentinel) 0 999
sentinel pushed, then 10x failed do-in
final get-top 11 1
get-int at -1 (should be the sentinel) 0 777
20x get-string-global on a table global
final get-top 20 0
20x Lua.val with a runtime (not syntax) error
final get-top 20 0
The get-int row is the interesting one: on master the caller's own value isn't just buried, it's unreadable at -1 because a leaked string sits on top of it. That's the concrete harm behind "a leaked slot is permanent" and it's gone here. The same probes also confirm there's no over-popping — the sentinel survives with its value intact, which is the failure mode a fix like this most easily introduces.
I also checked the two functions you deliberately left alone and agree with both calls, for the reasons you give.
Findings
Two residuals, both in Lua.val. I want to be clear up front that neither is a regression — I measured both on master and got identical results, and both sit outside the "read a value off the stack and never popped it" class this PR is about. Raising them only because this PR touches val and rewrites its docstring to describe stack behaviour, so they're natural neighbours.
1. Lua.val still leaks on a multi-value expression
return %s pushes one value per returned expression, but the success path calls set-global exactly once, and lua_setglobal pops one:
20x (Lua.val lua "x" "1, 2, 3") -> get-top = 40 (both master and this branch)
20x (Lua.val lua "x" "42") -> get-top = 0 (control)
Two slots per call, permanent, same mechanism and same consequence as the leaks you did fix. The new "val leaves the stack unchanged" test uses "42", so the suite reads as covering this when it doesn't. A settop-to-baseline after set-global, or a test pinning the multi-value case as known-unfixed, would close the gap.
2. (Lua.val lua "x" "") drives the stack pointer negative
An empty expression compiles to a bare return, which pushes nothing — and set-global then pops from an empty stack:
(Lua.val lua "x" "") -> get-top = -1 (both master and this branch)
A negative gettop means lua_setglobal popped below the stack base, which is UB rather than a leak. Reachable from any caller that passes a user-supplied expression through, and cheap to guard with a get-top check before set-global.
Verdict: merge
The diagnosis is right, the fix is the pattern already used elsewhere in the module, the tests fail on the unfixed code for the stated reasons, and my own before/after probes reproduce all of it independently — including the no-over-popping property. val's rewrite preserves the return value while adding the pop, and the docstring changes are honest about the one user-visible behaviour change. The two residuals above are pre-existing and out of this PR's stated scope; they'd make a good small follow-up rather than a reason to hold this.
Four functions in the low-level
Luamodule read a value or an error object offthe Lua stack and never pop it. A leaked slot is permanent for the life of the
state, so a long-running embedder — a state kept open across many calls, which is
the case this library exists for — drifts upward until it hits
LUAI_MAXSTACK.Measured with a
Lua.get-topA/B probe against a single state (baseline top = 0):Lua.get-string-globalLua.valLua.eval-fileLuax.do-inThe success paths of
val,eval-file, anddo-inwere already balanced; onlythe error paths leaked.
Lua.get-string-globalleaked unconditionally.The fix
No new style — this is the pattern already used by
Lua.global-exists?,Luax.get-string-global, the macro-generatedluax--def-get-globalgetters, andthe error branch of
Luax.call-fn: bind the copied-out value in alet-do,Lua.pop, then return it.String.from-cstr-orcopies the C string before thepop, so the returned message is unaffected.
One user-visible behaviour change: on failure
Lua.valnow discards the Luaerror object instead of leaving it on the stack. Its docstring never promised
that slot and nothing in the repo read it; the docstring now points at
Luax.do-infor callers who want the message.Left alone deliberately:
Lua.evaluatereads at -1 without popping, but it is wrapped inwith-lua-do,which closes the whole state immediately afterwards.
Luax.resume-coroutineleaves the error on the coroutine's stack, which itsdocstring documents as intentional ("Results or yielded values are left on
co's stack").Tests
Eight assertions added to
test/lua.carp, each looping N calls in one state andasserting
Lua.get-topis back to 0. The five that cover the leaking paths wereconfirmed to fail on the unfixed code with exactly the numbers above:
The other three cover the success paths, which pass both before and after, so
they guard against over-popping.
Verified locally (Linux, system Lua 5.4): all five suites green —
lua 68, midlevel 53, cfunction 10, metatable 22, coroutine 25, 0 failures.
carp-fmt --checkandangler --disable lonely-doare clean on both changedfiles. A separate scratch probe confirmed error messages are still intact after
the pop and that caller values sitting below on the stack are untouched (no
over-popping).
Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.