fix(logic): Improve validation of MSG_DO_SPECIAL_POWER and variants in GameLogicDispatch

[stephanmeesters]

This PR improves the stability of network games by validating the origin of source objects in MSG_DO_SPECIAL_POWER and related MSG variants in GameLogicDispatch. Tested on ~1k replays.

Todo

• Replicate in Generals

[greptile-apps]

Greptile Summary

This PR adds player-ownership validation to four MSG_DO_SPECIAL_POWER message-handler cases in GameLogicDispatch, preventing a player from issuing special-power commands on objects they don't own. All additions are guarded by #if !RETAIL_COMPATIBLE_CRC, keeping retail/CRC-synced builds unaffected.

• All four validation blocks (MSG_DO_SPECIAL_POWER, MSG_DO_SPECIAL_POWER_AT_LOCATION, MSG_DO_SPECIAL_POWER_AT_OBJECT, MSG_DO_SPECIAL_POWER_OVERRIDE_DESTINATION) follow a consistent pattern and align with existing ownership checks elsewhere in the file (e.g. line 1073).
• Null-pointer dereference risk: In each block, source->getControllingPlayer() is called twice — once in the comparison and once inside the DEBUG_CRASH format string. If the source object has no controlling player (neutral/world objects), the second call returns nullptr and ->getPlayerDisplayName() crashes. This affects all four blocks and will manifest in debug/development builds whenever such an object triggers the validation path.

Confidence Score: 3/5

• Safe to merge for retail builds, but the shared null-dereference bug in DEBUG_CRASH blocks should be fixed before wider use in development builds.
• The logic of the fix is sound and consistent with existing patterns in the file. The #if !RETAIL_COMPATIBLE_CRC guard limits blast radius. However, all four new blocks share an identical latent null-pointer dereference inside the DEBUG_CRASH message — calling getControllingPlayer() a second time without a null guard — which can crash debug builds when processing commands on neutral or world-owned objects.
• GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp — all four new validation blocks (lines 687–690, 744–747, 799–802, 1254–1257).

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Adds ownership validation for the source object in four MSG_DO_SPECIAL_POWER variants, guarded by #if !RETAIL_COMPATIBLE_CRC. All four blocks share a latent null-pointer dereference: getControllingPlayer() is called a second time inside the DEBUG_CRASH format string without a null guard, which crashes in debug builds if the source object has no controlling player.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive MSG_DO_SPECIAL_POWER variant] --> B{source != nullptr?}
    B -- No --> E[Use currentlySelectedGroup]
    B -- Yes --> C{RETAIL_COMPATIBLE_CRC?}
    C -- Yes, retail build --> F[Execute special power command]
    C -- No, dev/debug build --> D{getControllingPlayer == thisPlayer?}
    D -- Yes, owner matches --> F
    D -- No, wrong owner --> G[Enter DEBUG_CRASH block]
    G --> H{getControllingPlayer returns null?}
    H -- Not null --> I[Log error message and break]
    H -- Is null --> J[NULL DEREFERENCE CRASH]
    F --> K[groupDoSpecialPower / variant]

Loading

_{Last reviewed commit: 45ef03d}

[greptile-apps]

_{1 file reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

[Skyaero42]

Does this resolve an actual bug/crash or is it solving a potential issue in the future?

[xezon]

Very important fix.

Can you do more audits over other messages for follow ups?

For example GameMessage::MSG_ENTER has no player check but GameMessage::MSG_EXIT does.

[stephanmeesters]

Can you do more audits over other messages for follow ups?

Yes I should have some follow up PR's ready soon

[xezon]

One more thought on this: This would cause mismatch if someone cheated, for example with Control Hack. Are we ok with this?

It probably needs to go behind RETAIL_COMPATIBLE_CRC for the upmost correctness

[stephanmeesters]

IMO mismatch is desirable here, and would be nice if the first release can have all these cases solid so it's a more "trustworthy" client (although, it doesn't actually communicate around that it did a good thing..)

[xezon]

If we decide to go that route, then at least put a commented // #if RETAIL_COMPATIBLE_CRC and explain why we accept this mismatch.

[Caball009]

IMO mismatch is desirable here,

I agree. It's also consistent with all the other places that already check for the controlling player to prevent blatant abuse.

My only mild concern is that we may run into replays that mismatch because of this, and it's going to take time and effort to find out why they started to mismatch.

[xezon]

My only mild concern is that we may run into replays that mismatch because of this, and it's going to take time and effort to find out why they started to mismatch.

This is also my concern. If it was crashing, then its fine to omit RETAIL_COMPATIBLE_CRC, but if the game kept on working, then cheated behavior may be more acceptable than mismatch. Not sure. Mismatch is game ending. Cheat is not.