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 improves network game stability by adding player-ownership validation to four MSG_DO_SPECIAL_POWER message handlers in GameLogicDispatch. Before executing a special power for a specific source object, each handler now verifies that the source is controlled by the issuing player, breaking out of the dispatch with a DEBUG_CRASH diagnostic if the check fails. The pattern is consistent with existing ownership guards elsewhere in the same file (e.g. MSG_DO_EXIT, MSG_DO_UPGRADE, MSG_DO_SELL).

• Four identical validation blocks added for MSG_DO_SPECIAL_POWER, MSG_DO_SPECIAL_POWER_AT_LOCATION, MSG_DO_SPECIAL_POWER_AT_OBJECT, and MSG_DO_SPECIAL_POWER_OVERRIDE_DESTINATION.
• Each block correctly breaks out of the switch on failure, preventing the AI group command from being issued.
• The DEBUG_CRASH format string in every block eagerly dereferences source->getControllingPlayer() — if that returns nullptr (neutral/unowned object), the diagnostic itself will crash before printing in debug builds; a local null-check on the controller pointer would be safer.

Confidence Score: 4/5

• This PR is safe to merge; the validation logic is correct and consistent with existing patterns, with only a minor debug-only null-safety concern in the diagnostic message.
• The four validation blocks are structurally identical, follow established conventions in the file, and the break correctly exits the switch in both debug and release builds. The only concern is the eager evaluation of source->getControllingPlayer()->getPlayerDisplayName() inside DEBUG_CRASH, which could produce a secondary null dereference in debug builds if the source object has no owner. This does not affect release builds and the author reports successful testing on ~1k replays.
• No files require special attention beyond the minor debug-message null-safety note in GameLogicDispatch.cpp.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Adds four identical ownership-validation blocks for MSG_DO_SPECIAL_POWER and variants; logic is sound and consistent with existing patterns in the file, but the DEBUG_CRASH message arguments dereference the result of getControllingPlayer() without a null guard, risking a secondary crash in debug builds if the source object has no controlling player.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["logicMessageDispatcher(msg)"] --> B["thisPlayer = getNthPlayer(msg->getPlayerIndex())"]
    B --> C{MSG type?}
    C --> D["MSG_DO_SPECIAL_POWER\nMSG_DO_SPECIAL_POWER_AT_LOCATION\nMSG_DO_SPECIAL_POWER_AT_OBJECT\nMSG_DO_SPECIAL_POWER_OVERRIDE_DESTINATION"]
    D --> E["source = findObjectByID(sourceID)"]
    E --> F{source != nullptr?}
    F -- No --> G["Use currentlySelectedGroup"]
    F -- Yes --> H{"source->getControllingPlayer()\n== thisPlayer?"}
    H -- No --> I["DEBUG_CRASH diagnostic\n+ break (exit switch)"]
    H -- Yes --> J["Create AIGroup\nadd(source)\ngroupDoSpecialPower(...)"]
    J --> K["Destroy / removeAll AIGroup"]

Loading

_{Last reviewed commit: 1b37cba}

[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.