bugfix(particlesystem): Prevent double-destroy and use-after-free in ParticleSystem::destroy

[bobtista]

Summary

• Add early return in ParticleSystem::destroy() if already destroyed
• Clear m_slaveSystem pointer after calling destroy on slave

Problem

During shutdown, W3DTankDraw::tossEmitters() can call destroy() on particle systems that have already been freed by ParticleSystemManager::update(). This causes a crash in debug builds where freed memory is filled with 0xDEADBEEF.

The crash occurs because:

1. W3DTankDraw holds raw pointers to particle systems (m_treadDebrisLeft, m_treadDebrisRight)
2. ParticleSystemManager::update() deletes systems after destroy() is called and particles finish
3. W3DTankDraw's destructor later calls tossEmitters() on the now-freed memory

This fix is just to return if m_isDestroyed is truthy, which prevents the crash for me.

Notes

W3DTankDraw's pointers can become dangling when ParticleSystemManager deletes the systems. Are there any better ways to handle this? eg

1. Should callers use ParticleSystemID and findParticleSystem() instead of raw pointers?
2. Should particle systems notify attached drawables when deleted?

Testing

• Debug build no longer crashes during headless replay shutdown
• Verify normal gameplay particle effects still work correctly

[greptile-apps]

Greptile Summary

Adds defensive checks to prevent double-destroy crashes when ParticleSystem::destroy() is called on already-freed memory. The fix adds an early return guard if the system is already destroyed and nulls out the slave system pointer after destroying it to prevent use-after-free.

• Added idempotency check at start of destroy() method to prevent double-destroy
• Nulled out m_slaveSystem pointer after calling destroy on it (prevents dangling pointer dereference)
• Fixes crash during headless replay shutdown when W3DTankDraw::tossEmitters() calls destroy on particle systems already freed by ParticleSystemManager::update()

The root cause is that W3DTankDraw holds raw pointers to particle systems that can become dangling when ParticleSystemManager deletes them. This fix makes destroy() safe to call multiple times, which is a reasonable defensive measure for cleanup code. However, the underlying lifetime management issue (raw pointers vs IDs) remains but is appropriately noted in the PR description for future consideration.

Confidence Score: 4/5

• Safe to merge with minimal risk - defensive fix prevents crashes without changing normal behavior
• The changes are minimal, defensive, and directly address a reproducible crash. The early return check makes destroy() idempotent which is a safe pattern for cleanup methods. Nulling out the slave pointer prevents use-after-free. The fix doesn't change the normal flow - it only adds safety guards for edge cases during shutdown. Score is 4 instead of 5 because: (1) the underlying lifetime management issue with raw pointers still exists (acknowledged by author), and (2) more comprehensive testing across different shutdown scenarios would be ideal, though the fix is inherently safe.
• No files require special attention - the single-file change is straightforward and defensive

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameClient/System/ParticleSys.cpp	Added double-destroy guard and null pointer safety to prevent use-after-free crashes during shutdown

Sequence Diagram

sequenceDiagram
    participant W3DTankDraw
    participant ParticleSystem
    participant SlaveSystem
    participant ParticleSystemManager

    Note over W3DTankDraw: Holds raw pointers:<br/>m_treadDebrisLeft,<br/>m_treadDebrisRight

    rect rgb(255, 240, 240)
        Note over ParticleSystemManager,SlaveSystem: Problem Scenario (Before Fix)
        ParticleSystem->>ParticleSystem: destroy() called
        ParticleSystem->>ParticleSystem: m_isDestroyed = true
        ParticleSystem->>SlaveSystem: destroy()
        ParticleSystem->>ParticleSystem: update() returns false
        ParticleSystemManager->>ParticleSystem: deleteInstance()
        Note over ParticleSystem: Memory freed (0xDEADBEEF)
        
        W3DTankDraw->>W3DTankDraw: ~W3DTankDraw()
        W3DTankDraw->>W3DTankDraw: tossEmitters()
        W3DTankDraw->>ParticleSystem: destroy() on freed memory
        Note over ParticleSystem: CRASH! Use-after-free
    end

    rect rgb(240, 255, 240)
        Note over ParticleSystemManager,SlaveSystem: Fixed Scenario (After Fix)
        ParticleSystem->>ParticleSystem: destroy() called
        ParticleSystem->>ParticleSystem: m_isDestroyed = true
        ParticleSystem->>SlaveSystem: destroy()
        ParticleSystem->>ParticleSystem: m_slaveSystem = nullptr
        ParticleSystem->>ParticleSystem: update() returns false
        ParticleSystemManager->>ParticleSystem: deleteInstance()
        Note over ParticleSystem: Memory freed
        
        W3DTankDraw->>W3DTankDraw: ~W3DTankDraw()
        W3DTankDraw->>W3DTankDraw: tossEmitters()
        W3DTankDraw->>ParticleSystem: destroy() on freed memory
        ParticleSystem->>ParticleSystem: Early return (m_isDestroyed check)
        Note over ParticleSystem: Safe! No crash
    end

Loading

[xezon]

Hmm yeah this looks a bit risky. I will look into it.

[Caball009]

Is there a way to reproduce this issue?

[bobtista]

Is there a way to reproduce this issue?

Reproduction Steps:

1. Build a Debug version with RTS_DEBUG enabled
2. Run headless replay simulation (01-41-42_2v2v2_Dad_blakeybo_MediAI_HardAI_MediAI_MediAI.rep)
3. Wait for shutdown - the crash occurs during cleanup after the replay ends

Should see something like Access violation writing to 0xDEADBEEF + offset
Call stack includes: W3DTankDraw::~W3DTankDraw() → tossEmitters() → ParticleSystem::destroy()