fix(logic): Improve handling of ENABLE_RETALIATION_MODE in GameLogicDispatch

[stephanmeesters]

Simplifies the code and prevents other players from setting each others retaliation mode. Tested with 1k replays and with own replay where I swap the retaliation mode on both sides couple times and test with some combat.

Generals (not Zero Hour) does not have a retaliation mode

Null check of thisPlayer will be handled by #2383

[greptile-apps]

Greptile Summary

This PR fixes a security issue where a player could craft a MSG_ENABLE_RETALIATION_MODE message to manipulate another player's retaliation state in non-RETAIL_COMPATIBLE_CRC builds, and simultaneously simplifies the message format by removing the now-redundant player-index argument behind the RETAIL_COMPATIBLE_CRC guard for backward compatibility.

• Player.cpp: Correctly gates appendIntegerArgument(getPlayerIndex()) behind #if RETAIL_COMPATIBLE_CRC so both sender and receiver use a consistent message format.
• GameLogicDispatch.cpp: The #else path cleanly uses thisPlayer (derived from msg->getPlayerIndex(), i.e. the actual sender) instead of a caller-supplied index, which correctly prevents cross-player manipulation in non-RETAIL_COMPATIBLE_CRC builds.
• RETAIL_COMPATIBLE_CRC caveat: In the legacy path a DEBUG_ASSERTCRASH is added to flag cross-player assignments during development, but in release builds this assertion is a no-op (((void)0)), so the underlying vulnerability is not blocked in RETAIL_COMPATIBLE_CRC release builds. The PR description should clarify this limitation.
• thisPlayer null safety: The new #else path calls thisPlayer->setLogicalRetaliationModeEnabled(...) without a null guard, consistent with the rest of the dispatcher function. The PR description acknowledges this is covered by PR fix(logic): Add nullptr check to thisPlayer in GameLogic::logicMessageDispatcher #2383.

Confidence Score: 3/5

• Safe to merge with awareness that the security fix only applies to non-RETAIL_COMPATIBLE_CRC builds; the legacy path retains the original vulnerability in release builds.
• The non-RETAIL_COMPATIBLE_CRC path is a clean, correct fix that achieves its stated goal. The RETAIL_COMPATIBLE_CRC path intentionally preserves old behaviour for CRC compatibility but the debug-only assertion provides no runtime protection in release builds, leaving the vulnerability open there. The null-pointer risk on thisPlayer is pre-existing and acknowledged as handled by a separate PR (fix(logic): Add nullptr check to thisPlayer in GameLogic::logicMessageDispatcher #2383).
• GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp — specifically the RETAIL_COMPATIBLE_CRC branch and its reliance on DEBUG_ASSERTCRASH for security.

Sequence Diagram

sequenceDiagram
    participant P as Player::update()
    participant MS as MessageStream
    participant D as GameLogicDispatch

    Note over P,D: Non-RETAIL_COMPATIBLE_CRC path (new, fixed)
    P->>MS: appendMessage(MSG_ENABLE_RETALIATION_MODE)
    P->>MS: appendBooleanArgument(enabled)
    MS->>D: logicMessageDispatcher(msg)
    D->>D: thisPlayer = getNthPlayer(msg->getPlayerIndex())
    D->>D: enableRetaliation = getArgument(0)->boolean
    D->>D: thisPlayer->setLogicalRetaliationModeEnabled(enableRetaliation)

    Note over P,D: RETAIL_COMPATIBLE_CRC path (old behaviour + debug assert)
    P->>MS: appendMessage(MSG_ENABLE_RETALIATION_MODE)
    P->>MS: appendIntegerArgument(playerIndex)
    P->>MS: appendBooleanArgument(enabled)
    MS->>D: logicMessageDispatcher(msg)
    D->>D: playerIndex = getArgument(0)->integer
    D->>D: enableRetaliation = getArgument(1)->boolean
    D->>D: player = getNthPlayer(playerIndex)
    D->>D: DEBUG_ASSERTCRASH(player == thisPlayer) [debug only]
    D->>D: player->setLogicalRetaliationModeEnabled(enableRetaliation)

Loading

_{Last reviewed commit: d4acfd6}

[Caball009]

I'm fine with the current code, but I do think either the comment could be more informative and / or there could be an assertion like DEBUG_ASSERTCRASH(ThePlayerList->getNthPlayer(msg->getArgument(0)->integer) == thisPlayer.

[stephanmeesters]

I'm fine with the current code, but I do think either the comment could be more informative and / or there could be an assertion like DEBUG_ASSERTCRASH(ThePlayerList->getNthPlayer(msg->getArgument(0)->integer) == thisPlayer.

Both this and #2380 are now under RETAIL_COMPATIBLE_CRC and debug logging was added.

[xezon]

I expect this will cause mismatch now.

Retaliation needs to be network synced otherwise the Unit behavior will mismatch between clients.

[stephanmeesters]

I did test it with local multiplayer multi-instance without issues.

The only effective change is to use the knowledge that a player can only change its own retaliation mode, so there is no apparent need to pass another player ID as message argument, as you can use the ID of the owner of the message (which is thisPlayer). This is also consistent with how it works in other message types

[xezon]

Ok I understand now. I was confused by thisPlayer. I suggest make a follow up and call it msgPlayer, because thisPlayer makes it sounds a bit like the local player.

[Caball009]

I can do that in #2383 if you like.

[xezon]

Looking good.