From 5e7cce9047e774e2b3ec93c7fce329b6f1528ef1 Mon Sep 17 00:00:00 2001
From: toaster <rollerorbital@gmail.com>
Date: Mon, 26 May 2025 15:22:30 +0100
Subject: [PATCH] CallFunc_SetThingProperty: Get next mobj in TID chain at
 start of loop

In cases where state/property set can cause instant deletion, definitely interrupts FindMobjFromTID iteration after one step and potentially uses after free

Also adds comment warnings to this effect near ways to find P_FindMobjFromTID, and updates P_ProcessSpecial even though we could probably stand to rip it out now
---
 src/acs/call-funcs.cpp | 8 ++++----
 src/p_local.h          | 3 +++
 src/p_mobj.c           | 2 ++
 src/p_spec.c           | 7 ++++++-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/acs/call-funcs.cpp b/src/acs/call-funcs.cpp
index 47502cb0f..8e598bef8 100644
--- a/src/acs/call-funcs.cpp
+++ b/src/acs/call-funcs.cpp
@@ -3589,13 +3589,15 @@ bool CallFunc_SetThingProperty(ACSVM::Thread *thread, const ACSVM::Word *argV, A
 	INT32 value = 0;
 
 	tag = argV[0];
-	mobj = P_FindMobjFromTID(tag, mobj, info->mo);
+	mobj_t *next = P_FindMobjFromTID(tag, mobj, info->mo);
 
 	property = argV[1];
 	value = argV[2];
 
-	while (mobj != NULL)
+	while ((mobj = next) != NULL)
 	{
+		// First in case of deletion. (Can't check for value == S_NULL because of A_ calls, etc)
+		next = P_FindMobjFromTID(tag, mobj, info->mo);
 
 #define PROP_READONLY(x, y) \
 	case x: \
@@ -3830,8 +3832,6 @@ bool CallFunc_SetThingProperty(ACSVM::Thread *thread, const ACSVM::Word *argV, A
 			}
 		}
 
-		mobj = P_FindMobjFromTID(tag, mobj, info->mo);
-
 #undef PROP_FLAGS
 #undef PROP_SCALE
 #undef PROP_MOBJ
diff --git a/src/p_local.h b/src/p_local.h
index 505ccd8e0..af4f722bf 100644
--- a/src/p_local.h
+++ b/src/p_local.h
@@ -612,6 +612,9 @@ void P_InitTIDHash(void);
 void P_AddThingTID(mobj_t *mo);
 void P_RemoveThingTID(mobj_t *mo);
 void P_SetThingTID(mobj_t *mo, mtag_t tid);
+
+// This function cannot be safely called after *i is removed!
+// Please call at start of loops if *i is to be mutated
 mobj_t *P_FindMobjFromTID(mtag_t tid, mobj_t *i, mobj_t *activator);
 
 void P_DeleteMobjStringArgs(mobj_t *mobj);
diff --git a/src/p_mobj.c b/src/p_mobj.c
index 717cc6cca..98d377637 100644
--- a/src/p_mobj.c
+++ b/src/p_mobj.c
@@ -15417,6 +15417,8 @@ void P_SetThingTID(mobj_t *mo, mtag_t tid)
 //
 // P_FindMobjFromTID
 // Mobj tag search function.
+// This function cannot be safely called after *i is removed!
+// Please call at start of loops if *i is to be mutated
 //
 mobj_t *P_FindMobjFromTID(mtag_t tid, mobj_t *i, mobj_t *activator)
 {
diff --git a/src/p_spec.c b/src/p_spec.c
index fc91dfad4..eb30cc265 100644
--- a/src/p_spec.c
+++ b/src/p_spec.c
@@ -3275,8 +3275,13 @@ boolean P_ProcessSpecial(activator_t *activator, INT16 special, INT32 *args, cha
 					return false;
 				}
 
-				while ((targetThing = P_FindMobjFromTID(args[1], targetThing, mo)) != NULL)
+				mobj_t *next = P_FindMobjFromTID(args[1], targetThing, mo);
+
+				while ((targetThing = next) != NULL)
 				{
+					// First in case of deletion. (Can't check for state == S_NULL because of A_ calls, etc)
+					next = P_FindMobjFromTID(args[1], targetThing, mo);
+
 					if (targetThing->player != NULL)
 					{
 						continue;