fix(vm): restore caller's open_upvalues after nested execution#245
Merged
Conversation
`Lua.VM.Executor.execute/5` reset `state.open_upvalues` to `%{}` at
entry but never restored the caller's map on return. When a Lua chunk
called `require`, the inner module's body would populate
`open_upvalues` with cells keyed by the inner's register indices, and
those entries would leak back to the outer caller. The outer caller's
later closures would then reuse the stale cells by register index,
aliasing the outer's locals to unrelated inner values.
This broke real-world Lua libraries (luassert.assertions, luassert.array,
luassert.spy) that follow the pattern `local x = require(...)` →
many `local function` defs that close over `x` →
`x:method(...)`: by the time `x:method` ran, reads of `x` went
through a stale upvalue cell and saw an inner module's local instead.
As a side effect, `Lua.call_function/3` (public API) now preserves the
caller's `open_upvalues` across calls — previously the same leak
applied there too, but no caller of the public API depended on the
buggy behaviour.
Two layers of test coverage:
- Unit regression in `test/lua/vm/require_open_upvalue_test.exs` with
a minimal two-file pure-Lua repro.
- Integration test that vendors luassert v1.9.0 + say v1.4.1 under
`test/integration/luassert/` and asserts every interior luassert
module loads via `require` without raising. luassert is the
dominant testing framework in the Lua ecosystem and was the original
reproducer in the bug report.
Plan: A39
Closes #244
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
require() leaks inner module's open_upvalues into outer caller
Plan:
.agents/plans/A39-require-leaks-open-upvalues.mdCloses #244
Goal
Fix
Lua.VM.Executor.execute/5so that nested executions (mostimportantly
require) no longer leak the inner module'sstate.open_upvaluesmap back to the outer caller. This unblocksloading real-world Lua libraries that mix nested
requirechains withmany top-level
local functiondefinitions (luassert, busted, etc.).Root cause
Lua.VM.Executor.execute/5resetsstate.open_upvaluesto%{}atentry but never restores the caller's
open_upvalueson return. Everyother call site that descends into a nested execution
(
call_function/3for:lua_closure,call_value/5, the dispatcherentry, the dispatcher's frame returns, the interpreter's
:callop forLua closures) carefully saves the caller's map, resets to
%{}, runsthe callee, and restores on return.
Executor.execute/5is the oneoutlier.
When
requireis called as anative_funcfrom a Lua execution, theinner module's
Lua.VM.execute(proto, state)populates its ownopen_upvaluesas closures are created over the inner module'stop-level locals. When the inner returns, those entries leak back to
the outer caller. If the outer then creates a closure that captures a
top-level local at a register number that collides with one of the
inner's leftover entries, the outer's closure reuses the inner's
stale cell, aliasing the outer's local to whatever value the inner
had at that register.
For
luassert.assertionsspecifically, the outer'sassert(reg 0)ends up aliased to the inner
luassert.assertmodule'ss(reg 0,the
saymodule). At line 307,assert:register(...)readsassertthrough the stale upvalue cell and sees
say, not the obj table —hence the bug report's "attempt to call a nil value (method 'register'
on local 'assert')".
Fix
In
lib/lua/vm/executor.exexecute/5, snapshotstate.open_upvaluesbefore resetting and restore it on the way out:
This mirrors the save/restore pattern already used by
call_function/3for
:lua_closure,call_value/5, and the dispatcher entry(
do_execute_top). The fix is two-call-site:Lua.VM.execute/2— used byparse_and_execute_module(the bugpath) and by top-level
Lua.eval!. Save/restore is correct in bothcases.
Lua.do_call_function/3for:lua_closure— called from the publicLua.call_function/3. Save/restore makes the public API safer:callers don't lose
open_upvaluesacrosscall_functioninvocations.Success criteria
mix testpasses (1772 → 1792, +20 new tests, 0 failures).test/lua/vm/require_open_upvalue_test.exsreproduces the bug with a minimal two-file pure-Lua repro.
Without the fix, both tests fail with the exact error from require: cached module result lost after deep require chain (assert:register nil) #244.
test/integration/luassert_test.exsvendors luassert v1.9.0 + say v1.4.1 and asserts every interior
luassert module loads via
require. Without the fix, 8 of 18integration tests fail (
assertions,array,spy,match,matchers,mock, plus the explicit issue-require: cached module result lost after deep require chain (assert:register nil) #244 repro tests).mix test --only lua53suite count unchanged (29 tests, 0failures, 23 skipped).
Changes
Verification
Negative verification — temporarily reverted the fix to confirm the
tests actually catch the bug:
definitions" test fails with the exact error string from require: cached module result lost after deep require chain (assert:register nil) #244:
attempt to call a nil value (method 'register' on local 'assert_obj').luassert.assertions,luassert.array,luassert.spy,luassert.match,luassert.matchers,luassert.mock, plus the two named API tests).These are the same modules called out as broken in require: cached module result lost after deep require chain (assert:register nil) #244.
Out of scope (intentional)
save/restore is in place.
package.searchersmechanism.the interpreter is what surfaced this bug, but the interpreter path
should be correct on its own).
tv-labs/platform/sidecarLua bump.assert.are.equal(1, 1)passes). Loading the module graph is thebug surface; behavioural coverage is a separate follow-up.
luassert.formattersand top-levelluassert. Both dependon
io.type(io.stdout)for TTY detection at module-load time, whichthis VM intentionally does not expose. Tracked separately from
issue require: cached module result lost after deep require chain (assert:register nil) #244.