Fix script use-after-free when scripts remove themselves (#573)

* Fix script use-after-free when scripts remove themselves

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;
         }
 

src/scripts.cc

@@ -5,6 +5,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
+#include <vector>
 
 #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<int> 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;