From 942d3a69b5822bc6a44ebfe66e94a902e6b03995 Mon Sep 17 00:00:00 2001 From: xezon <4720891+xezon@users.noreply.github.com> Date: Tue, 20 May 2025 23:54:37 +0200 Subject: [PATCH 1/2] [GEN][ZH] Fix crash in StateMachine when a chained StateMachine update deletes the executor StateMachine --- Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h | 6 ++++-- .../GameEngine/Include/Common/StateMachine.h | 19 ++++++++++++++++++- .../GameEngine/Source/Common/StateMachine.cpp | 10 +++++++++- .../GameEngine/Include/Common/StateMachine.h | 19 ++++++++++++++++++- .../GameEngine/Source/Common/StateMachine.cpp | 10 +++++++++- 5 files changed, 58 insertions(+), 6 deletions(-) diff --git a/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h b/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h index c21d44e4a5d..1c88e91a988 100644 --- a/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h +++ b/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h @@ -216,6 +216,8 @@ }; */ +#define ALLOW_AUTOMATIC_REF_COUNT_PTR_CONSTRUCTION + class DummyPtrType; template @@ -416,7 +418,7 @@ bool operator <(const RefCountPtr & lhs, const RefCountPtr & rhs) template bool operator ==(DummyPtrType * dummy, const RefCountPtr & rhs) { - FAIL_IF(0 != dummy) { + if (0 != dummy) { return false; } @@ -428,7 +430,7 @@ bool operator ==(DummyPtrType * dummy, const RefCountPtr & rhs) template bool operator !=(DummyPtrType * dummy, const RefCountPtr & rhs) { - FAIL_IF(0 != dummy) { + if (0 != dummy) { return true; } diff --git a/Generals/Code/GameEngine/Include/Common/StateMachine.h b/Generals/Code/GameEngine/Include/Common/StateMachine.h index 49b561c65c2..a6a6ef2054c 100644 --- a/Generals/Code/GameEngine/Include/Common/StateMachine.h +++ b/Generals/Code/GameEngine/Include/Common/StateMachine.h @@ -37,6 +37,8 @@ #include "Common/Snapshot.h" #include "Common/Xfer.h" +#include "refcount.h" + //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- @@ -236,7 +238,7 @@ inline State::~State() { } /** * A finite state machine. */ -class StateMachine : public MemoryPoolObject, public Snapshot +class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass { MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" ); @@ -340,6 +342,12 @@ class StateMachine : public MemoryPoolObject, public Snapshot virtual void xfer( Xfer *xfer ); virtual void loadPostProcess(); + // RefCountClass interface + virtual void Delete_This() + { + MemoryPoolObject::deleteInstanceInternal(this); + } + protected: /** @@ -473,4 +481,13 @@ class SleepState : public State EMPTY_DTOR(SleepState) +//----------------------------------------------------------------------------------------------------------- +// TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small. +// @todo Replace calls to deleteInstance with RefCountPtr when so appropriate. +inline void deleteInstance(StateMachine* state) +{ + state->Release_Ref(); +} + + #endif // _STATE_MACHINE_H_ diff --git a/Generals/Code/GameEngine/Source/Common/StateMachine.cpp b/Generals/Code/GameEngine/Source/Common/StateMachine.cpp index 8c45edf7529..2bc58351b11 100644 --- a/Generals/Code/GameEngine/Source/Common/StateMachine.cpp +++ b/Generals/Code/GameEngine/Source/Common/StateMachine.cpp @@ -37,6 +37,8 @@ #include "GameLogic/GameLogic.h" #include "GameLogic/Object.h" +#include "ref_ptr.h" + #ifdef RTS_INTERNAL // for occasional debugging... @@ -433,13 +435,19 @@ StateReturnType StateMachine::updateStateMachine() if (m_currentState) { + // TheSuperHackers @bugfix xezon 20/05/2025 Defer the deletion of this state machine. + // Calling m_currentState->update() can release this state machine in certain circumstances, + // for example if something kills the entity of this state machine as a result of this state update. + // See https://github.com/TheSuperHackers/GeneralsGameCode/issues/212 + RefCountPtr refThis(this); + // update() can change m_currentState, so save it for a moment... State* stateBeforeUpdate = m_currentState; // execute this state StateReturnType status = m_currentState->update(); - // it is possible that the state's update() method may cause the state to be destroyed + // it is possible that the state's update() method clears the state machine. if (m_currentState == NULL) { return STATE_FAILURE; diff --git a/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h b/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h index 4cd35c9d4f8..adc09425df6 100644 --- a/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h +++ b/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h @@ -37,6 +37,8 @@ #include "Common/Snapshot.h" #include "Common/Xfer.h" +#include "refcount.h" + //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- @@ -237,7 +239,7 @@ inline State::~State() { } /** * A finite state machine. */ -class StateMachine : public MemoryPoolObject, public Snapshot +class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass { MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" ); @@ -342,6 +344,12 @@ class StateMachine : public MemoryPoolObject, public Snapshot virtual void xfer( Xfer *xfer ); virtual void loadPostProcess(); + // RefCountClass interface + virtual void Delete_This() + { + MemoryPoolObject::deleteInstanceInternal(this); + } + protected: /** @@ -475,4 +483,13 @@ class SleepState : public State EMPTY_DTOR(SleepState) +//----------------------------------------------------------------------------------------------------------- +// TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small. +// @todo Replace calls to deleteInstance with RefCountPtr when so appropriate. +inline void deleteInstance(StateMachine* state) +{ + state->Release_Ref(); +} + + #endif // _STATE_MACHINE_H_ diff --git a/GeneralsMD/Code/GameEngine/Source/Common/StateMachine.cpp b/GeneralsMD/Code/GameEngine/Source/Common/StateMachine.cpp index 736bb586f30..3a453729888 100644 --- a/GeneralsMD/Code/GameEngine/Source/Common/StateMachine.cpp +++ b/GeneralsMD/Code/GameEngine/Source/Common/StateMachine.cpp @@ -37,6 +37,8 @@ #include "GameLogic/GameLogic.h" #include "GameLogic/Object.h" +#include "ref_ptr.h" + #ifdef RTS_INTERNAL // for occasional debugging... @@ -433,13 +435,19 @@ StateReturnType StateMachine::updateStateMachine() if (m_currentState) { + // TheSuperHackers @bugfix xezon 20/05/2025 Defer the deletion of this state machine. + // Calling m_currentState->update() can release this state machine in certain circumstances, + // for example if something kills the entity of this state machine as a result of this state update. + // See https://github.com/TheSuperHackers/GeneralsGameCode/issues/212 + RefCountPtr refThis(this); + // update() can change m_currentState, so save it for a moment... State* stateBeforeUpdate = m_currentState; // execute this state StateReturnType status = m_currentState->update(); - // it is possible that the state's update() method may cause the state to be destroyed + // it is possible that the state's update() method clears the state machine. if (m_currentState == NULL) { return STATE_FAILURE; From 44ae55449eff1862f1f693ef19b6ac77aa22631c Mon Sep 17 00:00:00 2001 From: xezon <4720891+xezon@users.noreply.github.com> Date: Wed, 28 May 2025 19:41:37 +0200 Subject: [PATCH 2/2] Fix review comments --- Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h | 2 ++ Generals/Code/GameEngine/Include/Common/StateMachine.h | 4 ++-- GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h b/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h index 1c88e91a988..0e66d434cd6 100644 --- a/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h +++ b/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h @@ -419,6 +419,7 @@ template bool operator ==(DummyPtrType * dummy, const RefCountPtr & rhs) { if (0 != dummy) { + WWASSERT(0); return false; } @@ -431,6 +432,7 @@ template bool operator !=(DummyPtrType * dummy, const RefCountPtr & rhs) { if (0 != dummy) { + WWASSERT(0); return true; } diff --git a/Generals/Code/GameEngine/Include/Common/StateMachine.h b/Generals/Code/GameEngine/Include/Common/StateMachine.h index a6a6ef2054c..3d1dd9e5521 100644 --- a/Generals/Code/GameEngine/Include/Common/StateMachine.h +++ b/Generals/Code/GameEngine/Include/Common/StateMachine.h @@ -484,9 +484,9 @@ EMPTY_DTOR(SleepState) //----------------------------------------------------------------------------------------------------------- // TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small. // @todo Replace calls to deleteInstance with RefCountPtr when so appropriate. -inline void deleteInstance(StateMachine* state) +inline void deleteInstance(StateMachine* machine) { - state->Release_Ref(); + machine->Release_Ref(); } diff --git a/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h b/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h index adc09425df6..7478a97dd62 100644 --- a/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h +++ b/GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h @@ -486,9 +486,9 @@ EMPTY_DTOR(SleepState) //----------------------------------------------------------------------------------------------------------- // TheSuperHackers @info Misappropriates deleteInstance to call Release_Ref to keep the StateMachine fix small. // @todo Replace calls to deleteInstance with RefCountPtr when so appropriate. -inline void deleteInstance(StateMachine* state) +inline void deleteInstance(StateMachine* machine) { - state->Release_Ref(); + machine->Release_Ref(); }