Wargus / stratagus

The Stratagus strategy game engine
GNU General Public License v2.0
619 stars 119 forks source link

Trigger: LoadGame memory issue by adding more triggers #607

Closed zzam closed 5 months ago

zzam commented 6 months ago

When loading a saved (campaign) game, the trigger code shows this valgrind warning:

==26988== Conditional jump or move depends on uninitialised value(s)
==26988==    at 0x224323: CclAddTrigger(lua_State*) (trigger.cpp:431)
==26988==    by 0x48A9F4D: luaD_precall (ldo.c:320)
==26988==    by 0x48BBBB3: luaV_execute (lvm.c:591)
==26988==    by 0x48AA5FC: luaD_call (ldo.c:378)
==26988==    by 0x48A98EA: luaD_rawrunprotected (ldo.c:116)
==26988==    by 0x48AA79C: luaD_pcall (ldo.c:464)
==26988==    by 0x48A1E67: lua_pcall (lapi.c:821)
==26988==    by 0x29D5FE: LuaCall(lua_State*, int, int, int, bool) (script.cpp:200)
==26988==    by 0x2A22C0: LuaLoadFile(std::filesystem::__cxx11::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) (script.cpp:271)
==26988==    by 0x21CA68: LoadGame(std::filesystem::__cxx11::path const&) (loadgame.cpp:204)
==26988==    by 0x223EBB: StartSavedGame(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (savegame.cpp:201)
==26988==    by 0x3010C9: tolua_stratagus_StartSavedGame00(lua_State*) (tolua.cpp:1165)
==26988==  Uninitialised value was created by a heap allocation
==26988==    at 0x483EEDD: operator new(unsigned long) (vg_replace_malloc.c:485)
==26988==    by 0x195EBF: allocate (new_allocator.h:151)
==26988==    by 0x195EBF: allocate (alloc_traits.h:482)
==26988==    by 0x195EBF: _M_allocate (stl_bvector.h:676)
==26988==    by 0x195EBF: std::vector<bool, std::allocator<bool> >::_M_fill_insert(std::_Bit_iterator, unsigned long, bool) (vector.tcc:877)
==26988==    by 0x225D0D: insert (stl_bvector.h:1239)
==26988==    by 0x225D0D: resize (stl_bvector.h:1285)
==26988==    by 0x225D0D: CclSetActiveTriggers(lua_State*) (trigger.cpp:464)
==26988==    by 0x48A9F4D: luaD_precall (ldo.c:320)
==26988==    by 0x48BBBB3: luaV_execute (lvm.c:591)
==26988==    by 0x48AA5FC: luaD_call (ldo.c:378)
==26988==    by 0x48A98EA: luaD_rawrunprotected (ldo.c:116)
==26988==    by 0x48AA79C: luaD_pcall (ldo.c:464)
==26988==    by 0x48A1E67: lua_pcall (lapi.c:821)
==26988==    by 0x29D5FE: LuaCall(lua_State*, int, int, int, bool) (script.cpp:200)
==26988==    by 0x2A22C0: LuaLoadFile(std::filesystem::__cxx11::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) (script.cpp:271)
==26988==    by 0x21CA68: LoadGame(std::filesystem::__cxx11::path const&) (loadgame.cpp:204)

Debugging shows strange behaviour: The saved game seem to Load the original campain file:

[...]
Load("./campaigns/human-exp/levelx01h_c.sms")
[...]
--- -----------------------------------------
--- MODULE: triggers

SetActiveTriggers(true, true, true, true)
SetTrigger(2)

if (Triggers ~= nil) then assert(loadstring(Triggers))() end

So all triggers are added twice. First in the campaign definition and then at the end after SetActiveTriggers and SetTrigger. This second block of AddTrigger causes CclAddTrigger to read from after the defined bits in ActiveTriggers. https://github.com/Wargus/stratagus/blob/6cce6e5bd4cab7264d1d013c4f81014a26567153/src/game/trigger.cpp#L431 For me there are 4 triggers already defined in the campaign. and SetActiveTriggers also put 4 bools into ActiveTriggers.

I think this does not lead to crashes, but it is ugly. How is the logic of Triggers regarding save/load?