From b8a41fa5093002d5b59feb360d00fbee5022e818 Mon Sep 17 00:00:00 2001 From: James R Date: Tue, 14 Feb 2023 00:51:12 -0800 Subject: [PATCH] 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;