fix(logic): Add nullptr check to thisPlayer in GameLogic::logicMessageDispatcher

[Caball009]

This PR changes the nullptr assertion on thisPlayer in GameLogic::logicMessageDispatcher to an early return. Although there are a couple of places in this function that check explicitly for it, a lot of code assumes that this pointer is not null, so it makes sense to check that up front.

TODO:

• Rename thisPlayer to msgPlayer.
• Replicate in Generals.
• Clean up commits.

[Mauller]

on 645 we have

case GameMessage::MSG_ENABLE_RETALIATION_MODE:
{
	//Logically turns on or off retaliation mode for a specified player.
	Int playerIndex = msg->getArgument( 0 )->integer;
	Bool enableRetaliation = msg->getArgument( 1 )->boolean;

	Player *player = ThePlayerList->getNthPlayer( playerIndex );
	if( player )
	{
		player->setLogicalRetaliationModeEnabled( enableRetaliation );
	}
	break;
}

I wonder if this needs clarifying that player here is a targetPlayer and not the same as thisPlayer

I am guessing that Int playerIndex = msg->getArgument( 0 )->integer; returns a different value compared to msg->getPlayerID

[stephanmeesters]

I wonder if this needs clarifying that player here is a targetPlayer and not the same as thisPlayer

I am guessing that Int playerIndex = msg->getArgument( 0 )->integer; returns a different value compared to msg->getPlayerID

I have a PR planned for this one which makes the change that player = thisPlayer, which seemed to pass 1k replays, but wanted to test that a bit more

[greptile-apps]

Greptile Summary

This PR hardens GameLogic::logicMessageDispatcher by replacing the debug-only DEBUG_ASSERTCRASH on thisPlayer (now msgPlayer) with a proper early return guarded by DEBUG_CRASH, ensuring release builds also bail safely on an invalid player index rather than continuing with a null pointer. The variable is also renamed from thisPlayer to msgPlayer for clarity, several now-redundant local null checks are removed (MSG_CREATE_SELECTED_GROUP, MSG_REMOVE_FROM_SELECTED_GROUP, and the science == SCIENCE_INVALID || thisPlayer == nullptr compound check in MSG_PURCHASE_SCIENCE), and one msg->getPlayerIndex() call in debug logging is aligned to use msgPlayer->getPlayerIndex() for consistency. The identical change is applied to both the vanilla Generals/ and the Zero Hour GeneralsMD/ trees.

Key changes:

• Null safety: msgPlayer == nullptr now causes an early return in all build configurations (previously only a debug crash, with execution continuing and a potential null-dereference in release).
• Rename: thisPlayer → msgPlayer applied at every usage site in both files.
• Dead code removal: Three locations that re-derived the player from msg->getPlayerIndex() and re-checked for null are deleted, as they are fully superseded by the new top-level guard.
• Consistency: msg->getPlayerIndex() in the debug log replaced with msgPlayer->getPlayerIndex(), matching the rest of the function.

Confidence Score: 5/5

• This PR is safe to merge — it is a well-scoped defensive fix with no logic changes beyond eliminating potential null-dereferences in release builds.
• All occurrences of thisPlayer are correctly renamed to msgPlayer in both trees. The early-return guard is semantically equivalent to the old debug-assert for debug builds and strictly safer in release builds. The removed redundant checks are all superseded by the new top-level guard. No behavioral changes to the actual game-logic switch cases, and both files are in sync.
• No files require special attention.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Replaces the debug-only assert on thisPlayer with a proper early return guard (with DEBUG_CRASH), renames thisPlayer → msgPlayer throughout, removes now-redundant local null checks in MSG_CREATE_SELECTED_GROUP / MSG_REMOVE_FROM_SELECTED_GROUP, and replaces one msg->getPlayerIndex() call with msgPlayer->getPlayerIndex() for consistency. All call-sites updated correctly.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Mirrors the Generals changes exactly: early-return null guard, thisPlayer → msgPlayer rename, removal of redundant local null checks, and msg->getPlayerIndex() → msgPlayer->getPlayerIndex() consistency fix. Zero Hour specific code (e.g. MSG_ENABLE_RETALIATION_MODE RETAIL_COMPATIBLE_CRC branch) also correctly updated.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[logicMessageDispatcher called] --> B{msg != nullptr?}
    B -- "No (debug assert)" --> Z[Crash / UB in release]
    B -- Yes --> C["msgPlayer = ThePlayerList->getNthPlayer(msg->getPlayerIndex())"]
    C --> D{msgPlayer == nullptr?}
    D -- Yes --> E[DEBUG_CRASH + return early]
    D -- No --> F[Build currentlySelectedGroup\nfrom msgPlayer's selection]
    F --> G[Process switch msgType]
    G --> H["MSG_NEW_GAME / MSG_CLEAR_GAME_DATA\n(game state - does not use msgPlayer)"]
    G --> I["Object-command cases\n(use msgPlayer for ownership checks)"]
    G --> J["MSG_CREATE_SELECTED_GROUP /\nMSG_REMOVE_FROM_SELECTED_GROUP\n(redundant null check removed)"]
    G --> K["MSG_PURCHASE_SCIENCE\n(redundant thisPlayer == nullptr check removed)"]
    G --> L["MSG_LOGIC_CRC\n(msgPlayer->getPlayerIndex() instead of msg->getPlayerIndex())"]

Loading

_{Last reviewed commit: "Added nullptr check ..."}

[xezon]

Nice cleanup. But there is CRC mismatch risk.

Also there are 2 more msg->getPlayerIndex() in commented and compiled out code.

[xezon]

Instead of doing thisPlayer->getPlayerIndex() you could also add a const int playerIndex = msg->getPlayerIndex() at the begin of the function and then use that.

[xezon]

Very hot. Needs replication in Generals.

[Caball009]

Renamed thisPlayer to msgPlayer, and replicated in Generals.

I don't understand what code the compiler errors are referring to, though. fixed, branch was lagging behind main branch.

@xezon The diff is not as clean as I'd like it to be. If you like I can redo the commits and split the changes into four commits:

1. ZH nullptr check,
2. Gen nullptr check,
3. ZH rename thisPlayer,
4. Gen rename thisPlayer.

[xezon]

The diff is not as clean as I'd like it to be. If you like I can redo the commits

Can do. I suggest make the renaming for both games first. Then the null test fixes for both games.

[Caball009]

Cleaned up the commits; no changes in the code. Ready to be merged.

[xezon]

Can you make it 2 commits that can be Merged with Rebase into main?

[Caball009]

Can you make it 2 commits that can be Merged with Rebase into main?

Just to be sure, you want me to combine commits 1 & 2 and 3 & 4?