Follow-up from #599 (cursor LOW finding on commit 2f76de72, bug ID df4551ec-21e6-4808-9005-c8b2dffe3314).
The issue
applyFileMetaDirectives (src/jsonc/scene_loader.zig around L443-L453) does:
game.setState(new_owned);
if (game.game_state.ptr != new_owned.ptr) {
game.game_state = new_owned; // refresh pointer if setState short-circuited
}
if (old) |s| game.allocator.free(s);
The pointer-mismatch guard assumes the only reason game.game_state.ptr differs from new_owned.ptr post-setState is that setState short-circuited on its eql-check. But setState also fires state_after_change hooks after assigning game.game_state. If a hook handler calls setState(\"other\"), it legitimately re-changes game_state — and our guard then silently overwrites the hook's change with new_owned, without firing change hooks or incrementing state_change_count.
Fix shape
Narrow the guard to only reassign when game.game_state is actually aliasing the about-to-be-freed slot:
if (old) |old_slot| {
if (game.game_state.ptr == old_slot.ptr) {
game.game_state = new_owned;
}
}
This leaves a hook-installed state untouched, and only refreshes the pointer when we're about to free the slot it's pointing at.
Why deferred
This only fires for callers whose state_after_change hook re-calls setState, which is uncommon today. The fix is mechanical and adds a regression test (hook that calls setState mid-load); deferred to keep #599 from spinning another fix iteration.
Context
#599 was the implementation of RFC #596's file-header meta directives. Iterations 1-3 of cursor's review surfaced two real UAFs and one semantic-skip bug in the same function; this LOW is iteration 4. Each prior iteration was fixed inline; this one is being shipped with the limitation documented.
Follow-up from #599 (cursor LOW finding on commit
2f76de72, bug IDdf4551ec-21e6-4808-9005-c8b2dffe3314).The issue
applyFileMetaDirectives(src/jsonc/scene_loader.zig around L443-L453) does:The pointer-mismatch guard assumes the only reason
game.game_state.ptrdiffers fromnew_owned.ptrpost-setStateis thatsetStateshort-circuited on its eql-check. ButsetStatealso firesstate_after_changehooks after assigninggame.game_state. If a hook handler callssetState(\"other\"), it legitimately re-changesgame_state— and our guard then silently overwrites the hook's change withnew_owned, without firing change hooks or incrementingstate_change_count.Fix shape
Narrow the guard to only reassign when
game.game_stateis actually aliasing the about-to-be-freed slot:This leaves a hook-installed state untouched, and only refreshes the pointer when we're about to free the slot it's pointing at.
Why deferred
This only fires for callers whose
state_after_changehook re-callssetState, which is uncommon today. The fix is mechanical and adds a regression test (hook that calls setState mid-load); deferred to keep #599 from spinning another fix iteration.Context
#599 was the implementation of RFC #596's file-header meta directives. Iterations 1-3 of cursor's review surfaced two real UAFs and one semantic-skip bug in the same function; this LOW is iteration 4. Each prior iteration was fixed inline; this one is being shipped with the limitation documented.