From 7604061e1ca1271ced60e5cec60ff4e2b1855ac0 Mon Sep 17 00:00:00 2001 From: Mike Klaas Date: Fri, 24 Jul 2026 08:02:15 -0700 Subject: [PATCH] Fix script use-after-free when scripts remove themselves (#573) * Fix script use-after-free when scripts remove themselves --- src/proto_instance.cc | 124 ++++++++++++++++++------------------------ src/scripts.cc | 64 +++++++++++++--------- 2 files changed, 92 insertions(+), 96 deletions(-) diff --git a/src/proto_instance.cc b/src/proto_instance.cc index 99292a2c..cc07be05 100644 --- a/src/proto_instance.cc +++ b/src/proto_instance.cc @@ -45,6 +45,7 @@ static UseItemResultCode _obj_use_power_on_car(Object* ammo); static UseItemResultCode _obj_use_misc_item(Object* item); static int _protinstTestDroppedExplosive(Object* explosiveItem); static UseItemResultCode _protinst_default_use_item(Object* user, Object* targetObj, Object* item); +static int scriptExecProcWithObjects(int sid, Object* source, Object* target, ScriptProc proc); static int useLadderDown(Object* user, Object* ladder); static int useLadderUp(Object* user, Object* ladder); static int useStairs(Object* user, Object* stairs); @@ -61,6 +62,22 @@ static bool objectIsJammed(Object* obj); // 0x49A990 static MessageListItem stru_49A990; +// set script objects and return scriptOverrides value (if the script still exists) +static int scriptExecProcWithObjects(int sid, Object* source, Object* target, ScriptProc proc) +{ + scriptSetObjects(sid, source, target); + scriptExecProc(sid, proc); + + // subtle: some scripts delete themselves (and the object) during execution, so we handle them carefully afterward + + Script* script; + if (scriptGetScript(sid, &script) == -1) { + return -1; + } + + return script->scriptOverrides; +} + // 0x49A9A0 int objectGetSid(Object* object, int* sidPtr) { @@ -195,15 +212,12 @@ int objectLookAtFunc(Object* critter, Object* target, void (*fn)(const char* str bool scriptOverrides = false; if (target->sid != -1) { - scriptSetObjects(target->sid, critter, target); - scriptExecProc(target->sid, SCRIPT_PROC_LOOK_AT); - - Script* script; - if (scriptGetScript(target->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(target->sid, critter, target, SCRIPT_PROC_LOOK_AT); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = script->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (!scriptOverrides) { @@ -253,15 +267,12 @@ int objectExamineFunc(Object* critter, Object* target, void (*fn)(const char* st bool scriptOverrides = false; if (target->sid != -1) { - scriptSetObjects(target->sid, critter, target); - scriptExecProc(target->sid, SCRIPT_PROC_DESCRIPTION); - - Script* script; - if (scriptGetScript(target->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(target->sid, critter, target, SCRIPT_PROC_DESCRIPTION); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = script->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (!scriptOverrides) { @@ -575,15 +586,12 @@ int objectPickup(Object* critter, Object* item) } if (item->sid != -1) { - scriptSetObjects(item->sid, critter, item); - scriptExecProc(item->sid, SCRIPT_PROC_PICKUP); - - Script* script; - if (scriptGetScript(item->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(item->sid, critter, item, SCRIPT_PROC_PICKUP); + if (scriptProcResult == -1) { return -1; } - overriden = script->scriptOverrides; + overriden = scriptProcResult != 0; } if (!overriden) { @@ -695,15 +703,12 @@ int objectDrop(Object* invenObj, Object* itemObj) bool scriptOverrides = false; if (invenObj->sid != -1) { - scriptSetObjects(invenObj->sid, itemObj, nullptr); - scriptExecProc(invenObj->sid, SCRIPT_PROC_IS_DROPPING); - - Script* scr; - if (scriptGetScript(invenObj->sid, &scr) == -1) { + int scriptProcResult = scriptExecProcWithObjects(invenObj->sid, itemObj, nullptr, SCRIPT_PROC_IS_DROPPING); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = scr->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (scriptOverrides) { @@ -711,15 +716,12 @@ int objectDrop(Object* invenObj, Object* itemObj) } if (itemObj->sid != -1) { - scriptSetObjects(itemObj->sid, invenObj, itemObj); - scriptExecProc(itemObj->sid, SCRIPT_PROC_DROP); - - Script* scr; - if (scriptGetScript(itemObj->sid, &scr) == -1) { + int scriptProcResult = scriptExecProcWithObjects(itemObj->sid, invenObj, itemObj, SCRIPT_PROC_DROP); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = scr->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (scriptOverrides) { @@ -868,16 +870,11 @@ static UseItemResultCode _obj_use_flare(Object* critter, Object* flare) // 0x49BC60 static UseItemResultCode _obj_use_radio(Object* item) { - Script* scr; - if (item->sid == -1) { return USE_ITEM_RESULT_ERROR; } - scriptSetObjects(item->sid, gDude, item); - scriptExecProc(item->sid, SCRIPT_PROC_USE); - - if (scriptGetScript(item->sid, &scr) == -1) { + if (scriptExecProcWithObjects(item->sid, gDude, item, SCRIPT_PROC_USE) == -1) { return USE_ITEM_RESULT_ERROR; } @@ -1021,11 +1018,7 @@ static UseItemResultCode _obj_use_misc_item(Object* item) return USE_ITEM_RESULT_REMOVE; } - scriptSetObjects(item->sid, gDude, item); - scriptExecProc(item->sid, SCRIPT_PROC_USE); - - Script* scr; - if (scriptGetScript(item->sid, &scr) == -1) { + if (scriptExecProcWithObjects(item->sid, gDude, item, SCRIPT_PROC_USE) == -1) { return USE_ITEM_RESULT_ERROR; } @@ -1308,9 +1301,7 @@ UseItemResultCode objectUseItemOnInternal(Object* critter, Object* targetObj, Ob } if (skill == -1) { - // store the item script id as there's no guarantee item is not deallocated within the script const int itemSid = item->sid; - if (itemSid != -1) { Script* itemScript; @@ -1327,18 +1318,17 @@ UseItemResultCode objectUseItemOnInternal(Object* critter, Object* targetObj, Ob } } - // store the target object script id as there's no guarantee target object is not deallocated within the script - const int targetObjectSid = targetObj->sid; - if (targetObjectSid == -1) { + const int targetSid = targetObj->sid; + if (targetSid == -1) { return _protinst_default_use_item(critter, targetObj, item); } Script* targetScript; - scriptSetObjects(targetObjectSid, critter, item); - scriptExecProc(targetObjectSid, SCRIPT_PROC_USE_OBJ_ON); + scriptSetObjects(targetSid, critter, item); + scriptExecProc(targetSid, SCRIPT_PROC_USE_OBJ_ON); - if (scriptGetScript(targetObjectSid, &targetScript) == -1) { + if (scriptGetScript(targetSid, &targetScript) == -1) { return USE_ITEM_RESULT_ERROR; } @@ -1485,15 +1475,12 @@ int objectUse(Object* user, Object* targetObj) bool scriptOverrides = false; if (targetObj->sid != -1) { - scriptSetObjects(targetObj->sid, user, targetObj); - scriptExecProc(targetObj->sid, SCRIPT_PROC_USE); - - Script* script; - if (scriptGetScript(targetObj->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(targetObj->sid, user, targetObj, SCRIPT_PROC_USE); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = script->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (!scriptOverrides) { @@ -1745,15 +1732,12 @@ int objectUseDoor(Object* user, Object* door, bool animateOnly) bool scriptOverrides = false; if (door->sid != -1) { - scriptSetObjects(door->sid, user, door); - scriptExecProc(door->sid, SCRIPT_PROC_USE); - - Script* script; - if (scriptGetScript(door->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(door->sid, user, door, SCRIPT_PROC_USE); + if (scriptProcResult == -1) { return -1; } - scriptOverrides = script->scriptOverrides; + scriptOverrides = scriptProcResult != 0; } if (!scriptOverrides) { @@ -1850,15 +1834,12 @@ int objectUseContainer(Object* critter, Object* item) bool overriden = false; if (item->sid != -1) { - scriptSetObjects(item->sid, critter, item); - scriptExecProc(item->sid, SCRIPT_PROC_USE); - - Script* script; - if (scriptGetScript(item->sid, &script) == -1) { + int scriptProcResult = scriptExecProcWithObjects(item->sid, critter, item, SCRIPT_PROC_USE); + if (scriptProcResult == -1) { return -1; } - overriden = script->scriptOverrides; + overriden = scriptProcResult != 0; } if (overriden) { @@ -1918,12 +1899,13 @@ int objectUseSkillOn(Object* source, Object* target, int skill) bool scriptOverrides = false; if (target->sid != -1) { - scriptSetObjects(target->sid, source, target); - scriptSetActionBeingUsed(target->sid, skill); - scriptExecProc(target->sid, SCRIPT_PROC_USE_SKILL_ON); + int targetSid = target->sid; + scriptSetObjects(targetSid, source, target); + scriptSetActionBeingUsed(targetSid, skill); + scriptExecProc(targetSid, SCRIPT_PROC_USE_SKILL_ON); Script* script; - if (scriptGetScript(target->sid, &script) == -1) { + if (scriptGetScript(targetSid, &script) == -1) { return -1; } diff --git a/src/scripts.cc b/src/scripts.cc index 09b640f2..05cd21f8 100644 --- a/src/scripts.cc +++ b/src/scripts.cc @@ -5,6 +5,7 @@ #include #include #include +#include #include "actions.h" #include "animation.h" @@ -291,7 +292,7 @@ static int gMovieTimerArtimer4; // Returns game time in ticks (1/10 second). // -// 0x4A3330 +// 0x4A3330 game_time unsigned int gameTimeGetTime() { return gGameTime; @@ -339,8 +340,7 @@ void gameTimeGetDate(int* monthPtr, int* dayPtr, int* yearPtr) // - 3:00 P.M. -> 1500 // - 11:59 P.M. -> 2359 // -// game_time_hour -// 0x4A33C8 +// 0x4A33C8 game_time_hour int gameTimeGetHour() { return 100 * ((gGameTime / 600) / 60 % 24) + (gGameTime / 600) % 60; @@ -370,7 +370,7 @@ void gameTimeSetTime(unsigned int time) gGameTime = time; } -// 0x4A34CC +// 0x4A34CC inc_game_time void gameTimeAddTicks(int ticks) { gGameTime += ticks; @@ -389,14 +389,14 @@ void gameTimeAddTicks(int ticks) } } -// 0x4A3518 +// 0x4A3518 inc_game_time_in_seconds void gameTimeAddSeconds(int seconds) { // NOTE: Uninline. gameTimeAddTicks(seconds * 10); } -// 0x4A3570 +// 0x4A3570 gtime_q_add int gameTimeScheduleUpdateEvent() { // ticks until midnight @@ -414,7 +414,7 @@ int gameTimeScheduleUpdateEvent() return 0; } -// 0x4A3620 +// 0x4A3620 gtime_q_process int gameTimeEventProcess(Object* obj, void* data) { int movie_index; @@ -516,7 +516,7 @@ int _scriptsCheckGameEvents(int* moviePtr, int window) return 0; } -// 0x4A382C +// 0x4A382C src_map_q_process int mapUpdateEventProcess(Object* obj, void* data) { scriptsExecMapUpdateScripts(SCRIPT_PROC_MAP_UPDATE); @@ -534,8 +534,7 @@ int mapUpdateEventProcess(Object* obj, void* data) return -1; } -// new_obj_id -// 0x4A386C +// 0x4A386C new_obj_id int scriptsNewObjectId() { Object* ptr; @@ -624,7 +623,7 @@ void scriptsSyncObjectId(Object* object) } } -// 0x4A390C +// 0x4A390C src_find_sid_from_program int scriptGetSid(Program* program) { for (int type = 0; type < SCRIPT_TYPE_COUNT; type++) { @@ -695,7 +694,7 @@ Object* scriptGetSelf(Program* program) return object; } -// 0x4A3B0C +// 0x4A3B0C scr_set_objs int scriptSetObjects(int sid, Object* source, Object* target) { Script* script; @@ -709,7 +708,7 @@ int scriptSetObjects(int sid, Object* source, Object* target) return 0; } -// 0x4A3B34 +// 0x4A3B34 src_set_ext_param void scriptSetFixedParam(int sid, int value) { Script* script; @@ -718,7 +717,7 @@ void scriptSetFixedParam(int sid, int value) } } -// 0x4A3B54 +// 0x4A3B54 scr_set_action_param int scriptSetActionBeingUsed(int sid, int value) { Script* scr; @@ -732,7 +731,7 @@ int scriptSetActionBeingUsed(int sid, int value) return 0; } -// 0x4A3B74 +// 0x4A3B74 loadProgram static Program* scriptsCreateProgramByName(const char* name) { char path[COMPAT_MAX_PATH]; @@ -1375,11 +1374,22 @@ int scriptExecProc(int sid, int proc) programExecuteProcedure(program, procedureIndex); + Script* executedScript; + if (scriptGetScript(sid, &executedScript) == -1) { + // if the script was removed during execution, it (and the object) might be gone, so we shouldn't try to clean up + // or call HOOK_STDPROCEDURE_END + return 0; + } + // HOOK_STDPROCEDURE_END scriptHooks_StdProcedure(proc, self, source, target, fixedParam, true); - script->source = nullptr; - script->action = 0; + if (scriptGetScript(sid, &executedScript) == -1) { + return 0; + } + + executedScript->source = nullptr; + executedScript->action = 0; return 0; } @@ -2598,11 +2608,9 @@ bool scriptsExecSpatialProc(Object* object, int tile, int elevation) int builtTile = builtTileCreate(tile, elevation); + std::vector spatialScriptIds; for (Script* script = scriptGetFirstSpatialScript(elevation); script != nullptr; script = scriptGetNextSpatialScript()) { - if (builtTile == script->sp.built_tile) { - // NOTE: Uninline. - scriptSetObjects(script->sid, object, nullptr); - } else { + if (builtTile != script->sp.built_tile) { if (script->sp.radius == 0) { continue; } @@ -2611,12 +2619,17 @@ bool scriptsExecSpatialProc(Object* object, int tile, int elevation) if (distance > script->sp.radius) { continue; } - - // NOTE: Uninline. - scriptSetObjects(script->sid, object, nullptr); } - scriptExecProc(script->sid, SCRIPT_PROC_SPATIAL); + spatialScriptIds.push_back(script->sid); + } + + for (int sid : spatialScriptIds) { + // NOTE: Uninline. + if (scriptSetObjects(sid, object, nullptr) == -1) { + continue; + } + scriptExecProc(sid, SCRIPT_PROC_SPATIAL); } gSpatialsEnabled = true; @@ -2628,6 +2641,7 @@ bool scriptsExecSpatialProc(Object* object, int tile, int elevation) // 0x4A677C int scriptsExecStartProc() { + // note: this could do weird things if scripts/object are deleted while running these procs for (int scriptListIndex = 0; scriptListIndex < SCRIPT_TYPE_COUNT; scriptListIndex++) { ScriptList* scriptList = &(gScriptLists[scriptListIndex]); ScriptListExtent* extent = scriptList->head;