Skip to content

Conversation

@Caball009
Copy link

@Caball009 Caball009 commented Jan 22, 2026

DEBUG_ASSERTCRASH(!nextT || t->getTemplateID() == nextT->getTemplateID() + 1, ("Next template ID is unexpected"));
t can potentially point to deallocated / freed memory, so the assertion needs to happen earlier in the function.

Not sure whether it's a 'fix' or 'bugfix', but this is not an issue for standard release builds.

@Caball009 Caball009 added Bug Something is not working right, typically is user facing Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour ThisProject The issue was introduced by this project, or this task is specific to this project labels Jan 22, 2026
@greptile-apps
Copy link

greptile-apps bot commented Jan 22, 2026

Greptile Summary

Moved the DEBUG_ASSERTCRASH validation to execute before calling t->deleteOverrides() instead of after. This prevents a use-after-free bug where the assertion would access t->getTemplateID() after t has potentially been deleted by deleteOverrides().

  • The assertion now executes at line 218, immediately after getting nextT but before any operations that could delete t
  • When deleteOverrides() is called on a template marked as an override, it calls deleteInstance(this) (line 108 in Overridable.h), freeing the memory
  • The previous assertion location (after the deletion) would trigger undefined behavior in debug builds when accessing the freed pointer
  • This fix only affects debug builds with assertions enabled; release builds are unaffected

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The fix correctly addresses a use-after-free bug in debug assertion code by moving the assertion to execute before the pointer can be freed. The logic is sound, changes are minimal and focused, and identical fixes are applied to both Generals and GeneralsMD codebases. No behavioral changes to release builds.
  • No files require special attention

Important Files Changed

Filename Overview
Generals/Code/GameEngine/Source/Common/Thing/ThingFactory.cpp Moved assertion before deleteOverrides() call to prevent use-after-free when t is deleted
GeneralsMD/Code/GameEngine/Source/Common/Thing/ThingFactory.cpp Moved assertion before deleteOverrides() call to prevent use-after-free when t is deleted

Sequence Diagram

sequenceDiagram
    participant TF as ThingFactory::reset()
    participant T as ThingTemplate t
    participant NT as ThingTemplate nextT
    participant OV as Overridable::deleteOverrides()
    
    TF->>T: Get current template
    T->>NT: friend_getNextTemplate()
    NT-->>TF: Return nextT pointer
    
    Note over TF,T: BEFORE FIX: Assertion was here (after deleteOverrides)
    Note over TF,T: AFTER FIX: Assertion moved here (before deleteOverrides)
    TF->>T: Check assertion: t->getTemplateID() == nextT->getTemplateID() + 1
    
    TF->>T: getName()
    T-->>TF: Return templateName
    
    TF->>OV: t->deleteOverrides()
    alt t is marked as override
        OV->>T: deleteInstance(this)
        Note over T: t is FREED - pointer invalid
        OV-->>TF: Return nullptr
    else t is not override
        OV-->>TF: Return this
    end
    
    Note over TF: BEFORE FIX: Accessing t->getTemplateID() here would be use-after-free
    
    TF->>TF: Update m_firstTemplate if needed
    TF->>TF: Erase from hash map if needed
    TF->>TF: t = nextT (advance to next template)
Loading

@xezon xezon added Debug Is mostly debug functionality Fix Is fixing something, but is not user facing and removed Bug Something is not working right, typically is user facing labels Jan 23, 2026
@xezon xezon changed the title bugfix: Prevent use-after-free in assertion in ThingFactory::reset fix(thingfactory): Prevent use-after-free in assertion in ThingFactory::reset() Jan 23, 2026
@xezon xezon changed the title fix(thingfactory): Prevent use-after-free in assertion in ThingFactory::reset() fix(thingfactory): Prevent use-after-free from assert in ThingFactory::reset() Jan 24, 2026
@xezon xezon merged commit 669e36f into TheSuperHackers:main Jan 24, 2026
27 checks passed
@Caball009 Caball009 deleted the fix_assertion_thingfactory_reset branch January 24, 2026 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Debug Is mostly debug functionality Fix Is fixing something, but is not user facing Gen Relates to Generals Minor Severity: Minor < Major < Critical < Blocker ThisProject The issue was introduced by this project, or this task is specific to this project ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants