From b8a41fa5093002d5b59feb360d00fbee5022e818 Mon Sep 17 00:00:00 2001 From: James R Date: Tue, 14 Feb 2023 00:51:12 -0800 Subject: [PATCH 1/4] Fix negative reference counting in P_RestoreTMStruct This fixes some thinkers never being removed due to having negative reference counts. And here's a breakdown of why the old code could produce negative reference counts: Consider P_CheckPosition. This function calls P_SetTarget on tm.thing but does not call P_RestoreTMStruct. This means that tm.thing will not be NULL the next P_SetTarget is called on it. What are the implications of this? Consider the following series of events: - P_CheckPosition is called, tm.thing != NULL afterward - Another function saves the tm struct and sets tm.thing to a different mobj - - the old tm.thing will have its references decremented - - the new tm.thing will have its references incremented - This function calls P_RestoreTMStruct What should happen when P_RestoreTMStruct is called? The *new* tm.thing should have its references *decremented* and the *old* tm.thing should its references *incremented*, of course, for symmetry with P_SetTarget. The old code correctly decremented new tm.thing's references but did not increment old tm.thing's references. --- src/p_map.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/p_map.c b/src/p_map.c index ebb9a1786..1c495f3b0 100644 --- a/src/p_map.c +++ b/src/p_map.c @@ -49,20 +49,10 @@ tm_t tm = {0}; void P_RestoreTMStruct(tm_t tmrestore) { // Reference count management - if (tm.thing != tmrestore.thing) - { - P_SetTarget(&tm.thing, NULL); - } - - if (tm.floorthing != tmrestore.floorthing) - { - P_SetTarget(&tm.floorthing, NULL); - } - - if (tm.hitthing != tmrestore.hitthing) - { - P_SetTarget(&tm.hitthing, NULL); - } + // These are effectively a no-op if mobj remains the same + P_SetTarget(&tm.thing, tmrestore.thing); + P_SetTarget(&tm.floorthing, tmrestore.floorthing); + P_SetTarget(&tm.hitthing, tmrestore.hitthing); // Restore state tm = tmrestore; From 33145ab2ae45737c517cb65149bc4d91106a97e3 Mon Sep 17 00:00:00 2001 From: James R Date: Tue, 14 Feb 2023 02:48:09 -0800 Subject: [PATCH 2/4] p_mobj.c: reference count kitemcap and overlaycap With the old code, if the object at the head of the list was removed, it would leave the reference behind, extending the lifetime of the thinker until P_RunKartItems or P_RunOverlays was run. --- src/p_mobj.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/p_mobj.c b/src/p_mobj.c index b773134b9..07e6d10d6 100644 --- a/src/p_mobj.c +++ b/src/p_mobj.c @@ -5280,13 +5280,13 @@ void P_AddKartItem(mobj_t *thing) // Keeps the hnext list from corrupting. static void P_RemoveKartItem(mobj_t *thing) { - mobj_t *mo; - for (mo = kitemcap; mo; mo = mo->itnext) + mobj_t *mo, **p; + for (mo = *(p = &kitemcap); mo; mo = *(p = &mo->itnext)) { - if (mo->itnext != thing) + if (mo != thing) continue; - P_SetTarget(&mo->itnext, thing->itnext); + P_SetTarget(p, thing->itnext); P_SetTarget(&thing->itnext, NULL); return; } @@ -5433,13 +5433,13 @@ static void P_AddOverlay(mobj_t *thing) // Keeps the hnext list from corrupting. static void P_RemoveOverlay(mobj_t *thing) { - mobj_t *mo; - for (mo = overlaycap; mo; mo = mo->hnext) + mobj_t *mo, **p; + for (mo = *(p = &overlaycap); mo; mo = *(p = &mo->hnext)) { - if (mo->hnext != thing) + if (mo != thing) continue; - P_SetTarget(&mo->hnext, thing->hnext); + P_SetTarget(p, thing->hnext); P_SetTarget(&thing->hnext, NULL); return; } From e8ab92fa6451a8e3d26bc5a5a4989711a15a602f Mon Sep 17 00:00:00 2001 From: James R Date: Tue, 14 Feb 2023 02:52:33 -0800 Subject: [PATCH 3/4] p_mobj.c: guard some cases where a mobj could be removed --- src/p_mobj.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/p_mobj.c b/src/p_mobj.c index 07e6d10d6..f8c6ec400 100644 --- a/src/p_mobj.c +++ b/src/p_mobj.c @@ -6469,6 +6469,9 @@ static void P_MobjSceneryThink(mobj_t *mobj) break; case MT_ITEMCAPSULE_PART: P_ItemCapsulePartThinker(mobj); + + if (P_MobjWasRemoved(mobj)) + return; break; case MT_BATTLECAPSULE_PIECE: if (mobj->extravalue2) @@ -9850,9 +9853,16 @@ void P_MobjThinker(mobj_t *mobj) P_CheckMobjTrigger(mobj, false); } + I_Assert(!P_MobjWasRemoved(mobj)); + if (mobj->scale != mobj->destscale) + { P_MobjScaleThink(mobj); // Slowly scale up/down to reach your destscale. + if (P_MobjWasRemoved(mobj)) + return; + } + if (mobj->type == MT_GHOST && mobj->fuse > 0) // Not guaranteed to be MF_SCENERY or not MF_SCENERY! { if (mobj->extravalue1 > 0) // Sonic Advance 2 mode From 701324fb42e1ffcd0eba5f4c9e506e84b4f7c6ce Mon Sep 17 00:00:00 2001 From: James R Date: Tue, 14 Feb 2023 03:38:29 -0800 Subject: [PATCH 4/4] k_kart.c: reference count HOLD bubble --- src/k_kart.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/k_kart.c b/src/k_kart.c index 081b1b124..e99ee3863 100644 --- a/src/k_kart.c +++ b/src/k_kart.c @@ -9529,7 +9529,7 @@ void K_KartEbrakeVisuals(player_t *p) P_RemoveMobj(p->mo->hprev); } - p->mo->hprev = P_SpawnMobj(p->mo->x, p->mo->y, p->mo->z, MT_HOLDBUBBLE); + P_SetTarget(&p->mo->hprev, P_SpawnMobj(p->mo->x, p->mo->y, p->mo->z, MT_HOLDBUBBLE)); p->mo->hprev->renderflags |= (RF_DONTDRAW & ~K_GetPlayerDontDrawFlag(p)); } @@ -9617,7 +9617,7 @@ void K_KartEbrakeVisuals(player_t *p) if (p->mo->hprev && !P_MobjWasRemoved(p->mo->hprev) && (p->mo->hprev->frame & FF_FRAMEMASK) != 5) { P_RemoveMobj(p->mo->hprev); - p->mo->hprev = NULL; + P_SetTarget(&p->mo->hprev, NULL); } p->ebrakefor = 0;