Skip to content

fix(aiupdate): Fix XFER and CRC of AIUpdateInterface::m_guardTargetType#2662

Merged
xezon merged 4 commits intoTheSuperHackers:mainfrom
Caball009:fix_xfer_aiupdateinterface_guardtargettype
May 1, 2026
Merged

fix(aiupdate): Fix XFER and CRC of AIUpdateInterface::m_guardTargetType#2662
xezon merged 4 commits intoTheSuperHackers:mainfrom
Caball009:fix_xfer_aiupdateinterface_guardtargettype

Conversation

@Caball009
Copy link
Copy Markdown

@Caball009 Caball009 commented Apr 29, 2026

Original EA code links

https://github.com/electronicarts/CnC_Generals_Zero_Hour/blob/0a05454d8574207440a5fb15241b98ad0b435590/Generals/Code/GameEngine/Include/GameLogic/Module/AIUpdate.h#L679-L680

https://github.com/electronicarts/CnC_Generals_Zero_Hour/blob/0a05454d8574207440a5fb15241b98ad0b435590/Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp#L4760-L4761

https://github.com/electronicarts/CnC_Generals_Zero_Hour/blob/0a05454d8574207440a5fb15241b98ad0b435590/GeneralsMD/Code/GameEngine/Include/GameLogic/Module/AIUpdate.h#L697-L698

https://github.com/electronicarts/CnC_Generals_Zero_Hour/blob/0a05454d8574207440a5fb15241b98ad0b435590/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp#L5039-L5040

GuardTargetType m_guardTargetType[2];
Coord3D m_locationToGuard;

xfer->xferUser(&m_guardTargetType[0], sizeof(m_guardTargetType));
xfer->xferUser(&m_guardTargetType[1], sizeof(m_guardTargetType));

The above code xfers m_guardTargetType indices [0], [1], [1], [2], which is out-of-bounds. This PR fixes that.

Edit: FWIW this equals true:

static_assert(offsetof(AIUpdateInterface, m_guardTargetType) 
  + sizeof(m_guardTargetType) == offsetof(AIUpdateInterface, m_locationToGuard))

TODO:

  • Replicate in Generals.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Fix Is fixing something, but is not user facing NoRetail This fix or change is not applicable with Retail game compatibility labels Apr 29, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR fixes an out-of-bounds array access bug in AIUpdateInterface::xfer() where m_guardTargetType (a 2-element array) was xfer'd with sizeof(m_guardTargetType) starting at index [1], effectively reading into index [2] which is OOB and aliased to the first bytes of m_locationToGuard. The fix bumps the xfer version to 5 (unless RETAIL_COMPATIBLE_CRC/RETAIL_COMPATIBLE_XFER_SAVE is set) and adds a backward-compatibility path that faithfully reproduces the original byte layout when loading version < 5 saves.

Confidence Score: 5/5

Safe to merge — the fix is correct, the backward-compat path faithfully reproduces the original byte layout, and a static_assert guards the size assumption.

No P0 or P1 issues found. The out-of-bounds fix and version bump are correctly implemented in both the Generals and GeneralsMD codebases. The backward-compatibility path accurately accounts for all four element-widths written by the original buggy code, and xferCoord3D below the compat block correctly overwrites the partial m_locationToGuard data on load.

No files require special attention.

Important Files Changed

Filename Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Bumps xfer version to 5 and replaces two buggy xferUser calls with a correct single call; adds backward-compat path that reproduces the original [0],[1],[1],[2]-equivalent byte layout for version < 5 saves.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Identical fix applied to the Generals (non-Zero Hour) codebase; same version bump and backward-compat logic.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["AIUpdateInterface::xfer()"] --> B{RETAIL_COMPATIBLE_CRC\nor RETAIL_COMPATIBLE_XFER_SAVE?}
    B -- Yes --> C["currentVersion = 4"]
    B -- No --> D["currentVersion = 5"]
    C & D --> E["xferVersion(&version, currentVersion)"]
    E --> F{version < 5?}
    F -- Yes\nbackward-compat path --> G["xferUser(&m_guardTargetType[0], sizeof array)\n→ reads [0] and [1]"]
    G --> H["xferUser(&m_guardTargetType[1], sizeof element)\n→ reads [1]"]
    H --> I["xferUser(&m_locationToGuard, sizeof element)\n→ consumes OOB bytes from old saves"]
    F -- No\nfixed path --> J["xferUser(m_guardTargetType, sizeof array)\n→ correctly reads [0] and [1] only"]
    I & J --> K["xferCoord3D(&m_locationToGuard)\n→ reads/writes full Coord3D"]
Loading

Reviews (4): Last reviewed commit: "Replicated in Generals." | Re-trigger Greptile

Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile has a P2 complaint.

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Outdated
@Caball009
Copy link
Copy Markdown
Author

The sole concern is a missing offsetof-based static_assert to guard against padding assumptions.

Greptile has a P2 complaint.

static_assert(offsetof(AIUpdateInterface, m_guardTargetType) 
  + sizeof(m_guardTargetType) == offsetof(AIUpdateInterface, m_locationToGuard))

This is currently the case which the makes change correct, but the point of this PR is to make it work even if we were to break this assertion in the future.

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate.cpp Outdated
@Caball009
Copy link
Copy Markdown
Author

Replicated in Generals.

@xezon xezon merged commit 3ca03de into TheSuperHackers:main May 1, 2026
17 checks passed
@Caball009 Caball009 deleted the fix_xfer_aiupdateinterface_guardtargettype branch May 1, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Minor Severity: Minor < Major < Critical < Blocker NoRetail This fix or change is not applicable with Retail game compatibility ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants