fix(logic): Improve validation of MSG_SET_RALLY_POINT in GameLogicDispatch

[stephanmeesters]

This fix prevents players from controlling rally points of buildings of another player in a multiplayer match.

Moved behind RETAIL_COMPATIBLE_CRC in case of any replays that have control hacks.

Todo

• Replicate in Generals

[greptile-apps]

Greptile Summary

This PR adds an ownership check to the MSG_SET_RALLY_POINT handler in GameLogicDispatch.cpp for both the base Generals and Zero Hour builds, preventing a multiplayer exploit where a player could set the rally point of a building they don't own. The check is intentionally guarded by #if !RETAIL_COMPATIBLE_CRC to preserve backward-compatible replay behaviour.

• The fix is correctly placed inside the existing if (obj) null-guard and follows the general structure of similar ownership checks elsewhere in the dispatcher (lines 1022, 1330, 1397, 1481).
• Bug: obj->getControllingPlayer() is called twice — once in the guard condition and again inside the DEBUG_CRASH format string. If getControllingPlayer() returns nullptr (e.g., a neutral / unowned building), the condition evaluates to true and the second dereference crashes in debug builds. This issue is present in both Generals/ and GeneralsMD/ copies of the file.
• Unlike every other ownership check in the same function, this check is wrapped in #if !RETAIL_COMPATIBLE_CRC, meaning the exploit remains exploitable in RETAIL_COMPATIBLE_CRC builds. This appears intentional for replay compatibility, but is worth noting as an inconsistency.

Confidence Score: 3/5

• Safe to merge for non-debug retail builds, but contains a null pointer dereference risk in debug builds that should be fixed before merging.
• The security intent is correct and consistent with existing patterns, but the DEBUG_CRASH message calls getControllingPlayer() a second time without a null guard — a neutral object would cause a crash in debug builds, which could affect developers during testing or when investigating replay hacks.
• Both Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp and GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp need the null-pointer fix in the DEBUG_CRASH block.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Adds player-ownership validation for MSG_SET_RALLY_POINT inside a #if !RETAIL_COMPATIBLE_CRC guard; DEBUG_CRASH message dereferences getControllingPlayer() a second time without null-checking it, risking a crash on neutral/unowned objects in debug builds.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Identical change to the Zero Hour counterpart; carries the same null-pointer-dereference risk in the DEBUG_CRASH format arguments when getControllingPlayer() returns nullptr.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MSG_SET_RALLY_POINT received] --> B[findObjectByID]
    B --> C{obj != nullptr?}
    C -- No --> Z[break / no-op]
    C -- Yes --> D{RETAIL_COMPATIBLE_CRC?}
    D -- Yes --> G[doSetRallyPoint - exploit still possible]
    D -- No --> E{obj->getControllingPlayer\n!= thisPlayer?}
    E -- No, owner matches --> F[doSetRallyPoint]
    E -- Yes, wrong owner --> H[DEBUG_CRASH + break]
    H --> I[⚠️ If getControllingPlayer returns nullptr\nnull deref crash in debug builds]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp
Line: 510-517

Comment:
**Null pointer dereference in DEBUG_CRASH message**

`obj->getControllingPlayer()` is called again inside the `DEBUG_CRASH` arguments (line 515) after having been used only for a pointer comparison on line 510. If `getControllingPlayer()` returns `nullptr` — for example on a neutral or unowned building — the condition `nullptr != thisPlayer` evaluates to `true`, the if-body is entered, and then `obj->getControllingPlayer()->getPlayerDisplayName().str()` dereferences a null pointer, causing a crash in debug builds.

The same pattern is consistently avoided by every other ownership check in this file (lines 1022, 1330, 1397, 1481), which all simply `break` without dereferencing the result of `getControllingPlayer()`.

Cache the result in a local variable before the check and guard the format string:

```cpp
Player *objectOwner = obj->getControllingPlayer();
if ( objectOwner != thisPlayer )
{
    DEBUG_CRASH( ("MSG_SET_RALLY_POINT: Player '%ls' attempted to set the rally point of object '%s' owned by player '%ls'.",
         thisPlayer->getPlayerDisplayName().str(),
         obj->getTemplate()->getName().str(),
         objectOwner ? objectOwner->getPlayerDisplayName().str() : L"<none>") );
    break;
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp
Line: 519-526

Comment:
**Null pointer dereference in DEBUG_CRASH message**

Same issue as in `Generals/` — `obj->getControllingPlayer()` is called inside the `DEBUG_CRASH` arguments after being used only for a pointer inequality check. If `getControllingPlayer()` returns `nullptr` (neutral / unowned object), the condition is `true` and the subsequent `->getPlayerDisplayName().str()` call dereferences a null pointer in debug builds.

Cache the result before the comparison and guard against null in the format string:

```cpp
Player *objectOwner = obj->getControllingPlayer();
if ( objectOwner != thisPlayer )
{
    DEBUG_CRASH( ("MSG_SET_RALLY_POINT: Player '%ls' attempted to set the rally point of object '%s' owned by player '%ls'.",
         thisPlayer->getPlayerDisplayName().str(),
         obj->getTemplate()->getName().str(),
         objectOwner ? objectOwner->getPlayerDisplayName().str() : L"<none>") );
    break;
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 775f022}

[xezon]

Looks good. Is this the only one that was left? Nothing else?

Needs to be replicated in Generals.

[stephanmeesters]

Looks good. Is this the only one that was left? Nothing else?

I think so but I'll do another pass soon. There was another one that lets you return to the main menu without stopping the game but that's not an exploit. There is also another way to force the DC bug but it's outside of GameLogicDispatch.

[Skyaero42]

Moved behind RETAIL_COMPATIBLE_CRC in case of any replays that have control hacks.

How likely is that, and shouldn't such a replay be disqualified anyways?

[xezon]

Is this ready for merge or do we have outstanding comments to resolve?