From 7a3773a4058a7676a03b5030fb96b0d4117966fc Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Thu, 14 May 2026 11:39:54 +0200 Subject: [PATCH] fix(lua): install game.* before script eval so handle_input closures capture it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lua function closures capture `_ENV` at compile time. `asobi_lua_api:install/2` ran AFTER `asobi_lua_loader:new/1` evaluated the script chunk, so functions the script defined captured a `_G` without the `game` namespace. Every callback except `handle_input` hid the bug behind `bounded_eval`'s spawn round-trip (`call/4`). `handle_input` uses `call/3` per ADR 0002, so its closure-bound `_ENV` was the bare `_G` from before install — making `game.*` (notify, storage, broadcast, zone.spawn, spatial.*, log) silently nil for the most common server-auth callback. Fix: add `asobi_lua_loader:new/3` with a `PreInstall :: fun((St) -> St)` hook that runs between sandbox setup and `luerl:do(scriptCode, St)`. `asobi_lua_world:init/1`, `asobi_lua_world:generate_world/2`, `asobi_lua_world:inject_per_zone_lua/3`, and `asobi_lua_match:init/1` pass `fun(St) -> asobi_lua_api:install(Ctx, St) end` so `game.*` is populated into `_G` before any script-defined function captures its env. `new/1` and `new/2` are kept (delegate to `new/3` with identity PreInstall) so existing callers are unaffected. Behaviour change: a script's `generate_world` callback can now use `game.*`, which previously also silently saw nil. Same Ctx is installed, so all existing scripts continue to work — the fix only extends visibility to every closure rather than just the bounded_eval ones. Regression coverage: - world: init, join, leave, post_tick, zone_tick + handle_input each assert `_G.game` is a table and `game.id()` is callable - match: one consolidated test across init, join, leave, handle_input, tick, get_state - loader: new/3 PreInstall-runs-before-script + new/2 control pair --- src/lua/asobi_lua_loader.erl | 23 ++++++++-- src/lua/asobi_lua_match.erl | 14 +++--- src/lua/asobi_lua_world.erl | 34 +++++++++----- test/asobi_lua_loader_tests.erl | 29 +++++++++++- test/asobi_lua_match_tests.erl | 32 +++++++++++++ test/asobi_lua_world_tests.erl | 56 +++++++++++++++++++++++ test/fixtures/lua/game_api_match.lua | 40 +++++++++++++++++ test/fixtures/lua/game_api_world.lua | 60 +++++++++++++++++++++++++ test/fixtures/lua/pre_install_probe.lua | 7 +++ 9 files changed, 272 insertions(+), 23 deletions(-) create mode 100644 test/fixtures/lua/game_api_match.lua create mode 100644 test/fixtures/lua/game_api_world.lua create mode 100644 test/fixtures/lua/pre_install_probe.lua diff --git a/src/lua/asobi_lua_loader.erl b/src/lua/asobi_lua_loader.erl index a7343aa..bac5545 100644 --- a/src/lua/asobi_lua_loader.erl +++ b/src/lua/asobi_lua_loader.erl @@ -29,7 +29,11 @@ attached (e.g. for evaluating a `config.lua` manifest); use `new/1` to load a specific script and pin its base directory for `require`. """. --export([new/1, new/2, init_sandboxed/0, call/3, call/4, do_with_timeout/3]). +-export([new/1, new/2, new/3, init_sandboxed/0, call/3, call/4, do_with_timeout/3]). + +-export_type([pre_install/0]). + +-type pre_install() :: fun((dynamic()) -> dynamic()). -include_lib("kernel/include/file.hrl"). @@ -54,18 +58,31 @@ load a specific script and pin its base directory for `require`. -spec new(binary() | string()) -> {ok, dynamic()} | {error, term()}. new(ScriptPath) -> - new(ScriptPath, ?DEFAULT_INIT_TIMEOUT_MS). + new(ScriptPath, ?DEFAULT_INIT_TIMEOUT_MS, fun(St) -> St end). -spec new(binary() | string(), non_neg_integer()) -> {ok, dynamic()} | {error, term()}. new(ScriptPath, TimeoutMs) -> + new(ScriptPath, TimeoutMs, fun(St) -> St end). + +%% Lua closures capture `_ENV` at compile time, so any global installed +%% AFTER the script chunk is evaluated is invisible to functions the +%% script defined. The `PreInstall` hook runs between sandbox setup and +%% script eval — that is the only window in which adding tables to `_G` +%% (e.g. the `game.*` API) makes them reachable from every callback the +%% script defines, including `handle_input` which doesn't go through a +%% spawn round-trip (see ADR 0002). +-spec new(binary() | string(), non_neg_integer(), pre_install()) -> + {ok, dynamic()} | {error, term()}. +new(ScriptPath, TimeoutMs, PreInstall) when is_function(PreInstall, 1) -> BaseDir = filename:dirname(to_string(ScriptPath)), FileName = filename:basename(to_string(ScriptPath)), St0 = sandboxed_state(BaseDir), + St1 = PreInstall(St0), FullPath = filename:join(BaseDir, FileName), case file:read_file(FullPath) of {ok, Code} -> CodeStr = binary_to_list(Code), - do_with_timeout(CodeStr, St0, TimeoutMs); + do_with_timeout(CodeStr, St1, TimeoutMs); {error, Reason} -> {error, {file_error, FullPath, Reason}} end. diff --git a/src/lua/asobi_lua_match.erl b/src/lua/asobi_lua_match.erl index b0b025d..d549390 100644 --- a/src/lua/asobi_lua_match.erl +++ b/src/lua/asobi_lua_match.erl @@ -61,14 +61,14 @@ init(Config) -> erlang:error({missing_lua_script, Config}) end, GameConfig = maps:get(game_config, Config, #{}), - case asobi_lua_loader:new(ScriptPath) of + Ctx = #{ + match_id => maps:get(match_id, Config, undefined), + match_pid => self() + }, + PreInstall = fun(St) -> asobi_lua_api:install(Ctx, St) end, + case asobi_lua_loader:new(ScriptPath, ?INIT_TIMEOUT, PreInstall) of {ok, LuaSt0} -> - Ctx = #{ - match_id => maps:get(match_id, Config, undefined), - match_pid => self() - }, - LuaSt0a = asobi_lua_api:install(Ctx, LuaSt0), - {EncConfig, LuaSt1} = luerl:encode(GameConfig, LuaSt0a), + {EncConfig, LuaSt1} = luerl:encode(GameConfig, LuaSt0), case asobi_lua_loader:call(init, [EncConfig], LuaSt1, ?INIT_TIMEOUT) of {ok, [GameState | _], LuaSt2} -> {ok, #{ diff --git a/src/lua/asobi_lua_world.erl b/src/lua/asobi_lua_world.erl index b0c4b91..93f2473 100644 --- a/src/lua/asobi_lua_world.erl +++ b/src/lua/asobi_lua_world.erl @@ -63,14 +63,10 @@ init(Config) -> erlang:error({missing_lua_script, Config}) end, GameConfig = maps:get(game_config, Config, #{}), - case asobi_lua_loader:new(ScriptPath) of + PreInstall = fun(St) -> asobi_lua_api:install(make_ctx(Config), St) end, + case asobi_lua_loader:new(ScriptPath, ?INIT_TIMEOUT, PreInstall) of {ok, LuaSt0} -> - Ctx = #{ - match_id => maps:get(match_id, Config, undefined), - match_pid => self() - }, - LuaSt0a = asobi_lua_api:install(Ctx, LuaSt0), - {EncConfig, LuaSt1} = luerl:encode(GameConfig, LuaSt0a), + {EncConfig, LuaSt1} = luerl:encode(GameConfig, LuaSt0), case asobi_lua_loader:call(init, [EncConfig], LuaSt1, ?INIT_TIMEOUT) of {ok, [GameState | _], LuaSt2} -> {ok, #{ @@ -245,10 +241,15 @@ generate_world(Seed, Config) when is_map(Config) -> undefined -> {ok, #{}}; ScriptPath -> - case asobi_lua_loader:new(ScriptPath) of + %% match_pid in the ctx is the caller of generate_world/2 — typically + %% asobi_world_server, not a match process. game.broadcast emitted + %% from a script's generate_world callback therefore reaches the + %% world server, mirroring how broadcast already routed pre-fix. + PreInstall = fun(St) -> asobi_lua_api:install(make_ctx(Config), St) end, + case asobi_lua_loader:new(ScriptPath, ?GENERATE_TIMEOUT, PreInstall) of {ok, LuaSt} -> {ok, ZoneStates} = generate_world(Seed, #{lua_state => LuaSt}), - {ok, inject_per_zone_lua(ZoneStates, ScriptPath)}; + {ok, inject_per_zone_lua(ZoneStates, ScriptPath, PreInstall)}; {error, Reason} -> logger:error(#{ msg => @@ -260,8 +261,10 @@ generate_world(Seed, Config) when is_map(Config) -> end end. --spec inject_per_zone_lua(map(), file:filename_all()) -> map(). -inject_per_zone_lua(ZoneStates, ScriptPath) -> +-spec inject_per_zone_lua( + map(), file:filename_all(), asobi_lua_loader:pre_install() +) -> map(). +inject_per_zone_lua(ZoneStates, ScriptPath, PreInstall) -> Mtime = filelib:last_modified(ScriptPath), maps:map( fun(_Coords, ZoneState) -> @@ -270,7 +273,7 @@ inject_per_zone_lua(ZoneStates, ScriptPath) -> M when is_map(M) -> M; _ -> #{} end, - case asobi_lua_loader:new(ScriptPath) of + case asobi_lua_loader:new(ScriptPath, ?GENERATE_TIMEOUT, PreInstall) of {ok, LuaSt} -> Base#{ lua_state => LuaSt, @@ -671,3 +674,10 @@ decode_respawn_rule(_) -> decode_max_respawns(nil) -> infinity; decode_max_respawns(N) when is_number(N) -> trunc(N); decode_max_respawns(_) -> infinity. + +-spec make_ctx(map()) -> map(). +make_ctx(Config) -> + #{ + match_id => maps:get(match_id, Config, undefined), + match_pid => self() + }. diff --git a/test/asobi_lua_loader_tests.erl b/test/asobi_lua_loader_tests.erl index 94f67b2..a591e5c 100644 --- a/test/asobi_lua_loader_tests.erl +++ b/test/asobi_lua_loader_tests.erl @@ -32,7 +32,9 @@ loader_test_() -> {"max_heap_words honors application env override", fun max_heap_env_override/0}, {"math.random works", fun math_random_works/0}, {"math.sqrt works", fun math_sqrt_works/0}, - {"math.random no args returns float", fun math_random_no_args/0} + {"math.random no args returns float", fun math_random_no_args/0}, + {"new/3 PreInstall runs before script eval", fun new3_pre_install_before_script/0}, + {"new/2 backwards-compat (no PreInstall)", fun new2_no_pre_install/0} ]. loads_valid_script() -> @@ -134,6 +136,31 @@ math_random_no_args() -> ?assert(is_float(Result)), ?assert(Result >= 0.0 andalso Result < 1.0). +new3_pre_install_before_script() -> + %% Script defines a function that closes over a global the host injects + %% via PreInstall. If PreInstall runs BEFORE script eval, the closure's + %% `_ENV` captures the injected value and `probe()` returns it. If it + %% ran AFTER (the bug fixed by this hook), `probe()` would see nil. + %% This is the same property that makes `game.*` reachable from + %% `handle_input` in the world bridge. + PreInstall = fun(St) -> + {Enc, St1} = luerl:encode(~"injected_value", St), + {ok, St2} = luerl:set_table_keys([~"injected"], Enc, St1), + St2 + end, + {ok, St} = asobi_lua_loader:new( + fixture("pre_install_probe.lua"), 2000, PreInstall + ), + {ok, [Value | _], _} = asobi_lua_loader:call(probe, [], St), + ?assertEqual(~"injected_value", Value). + +new2_no_pre_install() -> + %% Without PreInstall, the same script's `probe()` should see nil for + %% the missing global. Confirms the new/3 hook is opt-in. + {ok, St} = asobi_lua_loader:new(fixture("pre_install_probe.lua"), 2000), + {ok, [Value | _], _} = asobi_lua_loader:call(probe, [], St), + ?assertEqual(nil, Value). + %% --- Helpers --- -spec encode_map(map(), dynamic()) -> dynamic(). diff --git a/test/asobi_lua_match_tests.erl b/test/asobi_lua_match_tests.erl index 30d04a3..bce2346 100644 --- a/test/asobi_lua_match_tests.erl +++ b/test/asobi_lua_match_tests.erl @@ -437,6 +437,38 @@ handle_input_failure() -> file:delete(Path) end. +%% --- Regression: `game.*` API must be reachable from every match callback --- +%% +%% Mirrors the world-side regression suite. handle_input is the headline +%% case: ADR 0002 means it uses call/3 (no bounded_eval), and that path +%% only sees `game.*` if the install ran BEFORE the script chunk was +%% evaluated. + +game_api_visible_in_match_callbacks_test() -> + {ok, State0} = asobi_lua_match:init(#{lua_script => fixture("game_api_match.lua")}), + ?assertEqual(true, lookup_flag(~"init_saw_game", State0)), + {ok, State1} = asobi_lua_match:join(~"p1", State0), + ?assertEqual(true, lookup_player_flag(~"p1", ~"join_saw_game", State1)), + {ok, State2} = asobi_lua_match:handle_input(~"p1", #{}, State1), + ?assertEqual(true, lookup_player_flag(~"p1", ~"handle_input_saw_game", State2)), + ?assertEqual(true, lookup_player_flag(~"p1", ~"game_id_callable", State2)), + {ok, State3} = asobi_lua_match:tick(State2), + ?assertEqual(true, lookup_flag(~"tick_saw_game", State3)), + {ok, State4} = asobi_lua_match:leave(~"p1", State3), + ?assertEqual(true, lookup_player_flag(~"p1", ~"leave_saw_game", State4)). + +-spec lookup_flag(binary(), map()) -> term(). +lookup_flag(Key, #{lua_state := LuaSt, game_state := GS}) -> + GsMap = asobi_lua_api:decode_to_map(GS, LuaSt), + maps:get(Key, GsMap, false). + +-spec lookup_player_flag(binary(), binary(), map()) -> term(). +lookup_player_flag(PlayerId, Key, #{lua_state := LuaSt, game_state := GS}) -> + GsMap = asobi_lua_api:decode_to_map(GS, LuaSt), + Players = maps:get(~"players", GsMap, #{}), + P = maps:get(PlayerId, Players, #{}), + maps:get(Key, P, false). + %% --- Helpers --- -spec init_match() -> {ok, map()}. diff --git a/test/asobi_lua_world_tests.erl b/test/asobi_lua_world_tests.erl index fe05959..2e6dc1f 100644 --- a/test/asobi_lua_world_tests.erl +++ b/test/asobi_lua_world_tests.erl @@ -558,6 +558,62 @@ hot_reload_zone_tick_survives_syntax_error_test() -> %% --- Helpers --- +%% --- Regression: `game.*` API must be reachable from every callback --- +%% +%% Lua closures capture `_ENV` at compile time. If `asobi_lua_api:install/2` +%% runs AFTER the script chunk is evaluated, functions the script defined +%% see a `_G` that doesn't include the `game` namespace. The asymmetry +%% bites `handle_input` hardest because it uses `call/3` (no bounded_eval +%% round-trip) — see ADR 0002. These tests fail loudly if any callback +%% ever loses access to `game.*` again. + +%% game_state is held as a luerl tref; decode it for assertions. +decoded_game_state(#{lua_state := LuaSt, game_state := GS}) -> + asobi_lua_api:decode_to_map(GS, LuaSt). + +game_namespace_visible_in_init_test() -> + {ok, State} = asobi_lua_world:init(#{lua_script => fixture("game_api_world.lua")}), + GS = decoded_game_state(State), + ?assertEqual(true, maps:get(~"init_saw_game", GS, false)). + +game_namespace_visible_in_join_leave_test() -> + {ok, S0} = asobi_lua_world:init(#{lua_script => fixture("game_api_world.lua")}), + {ok, S1} = asobi_lua_world:join(~"p1", S0), + ?assertEqual(true, maps:get(~"join_saw_game", decoded_game_state(S1), false)), + {ok, S2} = asobi_lua_world:leave(~"p1", S1), + ?assertEqual(true, maps:get(~"leave_saw_game", decoded_game_state(S2), false)). + +game_namespace_visible_in_post_tick_test() -> + {ok, S0} = asobi_lua_world:init(#{lua_script => fixture("game_api_world.lua")}), + {ok, S1} = asobi_lua_world:post_tick(1, S0), + ?assertEqual(true, maps:get(~"post_tick_saw_game", decoded_game_state(S1), false)). + +game_namespace_visible_in_zone_tick_and_handle_input_test() -> + %% This is the regression case: install must happen BEFORE the script + %% chunk is evaluated so handle_input's closure can see game.*. zone_tick + %% comes along for the ride because it shares the same per-zone state. + Script = fixture("game_api_world.lua"), + Config = #{game_config => #{lua_script => Script}}, + {ok, ZoneStates} = asobi_lua_world:generate_world(0, Config), + ZoneState = maps:get({0, 0}, ZoneStates), + + erlang:erase({asobi_lua_world, zone_state}), + {_Ents, ZoneState1} = asobi_lua_world:zone_tick(#{}, ZoneState), + %% ZoneState1.game_state holds the script's zone_state luerl tref; + %% decode it to inspect the flag. + ZoneTickGS = asobi_lua_api:decode_to_map( + maps:get(game_state, ZoneState1), maps:get(lua_state, ZoneState1) + ), + ?assertEqual(true, maps:get(~"zone_tick_saw_game", ZoneTickGS, false)), + + {ok, Entities1} = asobi_lua_world:handle_input( + ~"p1", #{~"kind" => ~"probe"}, #{} + ), + PE = maps:get(~"p1", Entities1), + ?assertEqual(true, maps:get(~"handle_input_saw_game", PE, false)), + ?assertEqual(true, maps:get(~"game_id_callable", PE, false)), + erlang:erase({asobi_lua_world, zone_state}). + -spec world_temp_script(binary()) -> file:filename_all(). world_temp_script(Code) -> Name = "world_" ++ integer_to_list(erlang:unique_integer([positive])) ++ ".lua", diff --git a/test/fixtures/lua/game_api_match.lua b/test/fixtures/lua/game_api_match.lua new file mode 100644 index 0000000..c633acd --- /dev/null +++ b/test/fixtures/lua/game_api_match.lua @@ -0,0 +1,40 @@ +-- Regression fixture: probes `game.*` visibility from every match callback. +local function game_visible() + return type(_G.game) == "table" and type(_G.game.id) == "function" +end + +function init(_config) + return { + players = {}, + init_saw_game = game_visible(), + tick_count = 0, + } +end + +function join(player_id, state) + state.players[player_id] = { join_saw_game = game_visible() } + return state +end + +function leave(player_id, state) + state.players[player_id] = { leave_saw_game = game_visible() } + return state +end + +function handle_input(player_id, _input, state) + local p = state.players[player_id] or {} + p.handle_input_saw_game = game_visible() + p.game_id_callable = type(game.id()) == "string" + state.players[player_id] = p + return state +end + +function tick(state) + state.tick_count = state.tick_count + 1 + state.tick_saw_game = game_visible() + return state +end + +function get_state(_player_id, state) + return state +end diff --git a/test/fixtures/lua/game_api_world.lua b/test/fixtures/lua/game_api_world.lua new file mode 100644 index 0000000..c809af8 --- /dev/null +++ b/test/fixtures/lua/game_api_world.lua @@ -0,0 +1,60 @@ +-- Regression fixture: probes `game.*` visibility from every callback the +-- world bridge exposes. Tests assert each callback wrote a truthy flag +-- back into observable state, so a regression where `game.*` is missing +-- in any callback shows up as a failing assertion rather than a silent +-- no-op. +match_size = 1 +max_players = 16 +game_type = "world" +grid_size = 1 +view_radius = 0 + +local function game_visible() + return type(_G.game) == "table" and type(_G.game.id) == "function" +end + +function init(_config) + return { init_saw_game = game_visible() } +end + +function join(_player_id, state) + state.join_saw_game = game_visible() + return state +end + +function leave(_player_id, state) + state.leave_saw_game = game_visible() + return state +end + +function spawn_position(_player_id, _state) + return { x = 0, y = 0 } +end + +function zone_tick(entities, zone_state) + zone_state = zone_state or {} + zone_state.zone_tick_saw_game = game_visible() + return entities, zone_state +end + +function handle_input(player_id, input, entities) + if input and input.kind == "probe" then + entities[player_id] = { + type = "player", + handle_input_saw_game = game_visible(), + -- Also exercise an actual game.* call to catch cases where the + -- table exists but its functions are stubbed. + game_id_callable = type(game.id()) == "string", + } + end + return entities +end + +function post_tick(_tick, state) + state.post_tick_saw_game = game_visible() + return state +end + +function generate_world(_seed, _config) + return { ["0,0"] = { tiles = {}, mobs = {} } } +end diff --git a/test/fixtures/lua/pre_install_probe.lua b/test/fixtures/lua/pre_install_probe.lua new file mode 100644 index 0000000..0fe62a8 --- /dev/null +++ b/test/fixtures/lua/pre_install_probe.lua @@ -0,0 +1,7 @@ +-- Minimal fixture for asobi_lua_loader:new/3 PreInstall coverage. +-- probe() reads a global the host injects via PreInstall; if PreInstall +-- runs before the script is evaluated the closure captures the value, +-- otherwise it captures a stale _ENV and returns nil. +function probe() + return injected +end