fix(connection): Fix unsafe Player ID usage in ConnectionManager, DisconnectManager

[xezon]

This change fixes unsafe Player ID usage in classes ConnectionManager, DisconnectManager.

This problem surfaced as an actual remote client crash event when an invalid Player ID was send in a packet to a remote client. It then tried to use that value as an index to arrays (in function ConnectionManager::sendRemoteCommand).

This change now guards all relevant Player ID usages with an appropriate MAX_SLOTS bound check.

Additionally, NUM_CONNECTIONS was removed because it is a duplicate of MAX_SLOTS. And all playerID >= 0 tests were removed because Player ID is an unsigned integer.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds runtime bounds checking (playerID >= MAX_SLOTS) before using Player IDs from network packets as array indices in ConnectionManager and DisconnectManager. This fixes a real crash caused by invalid Player IDs in network packets leading to out-of-bounds array accesses. The PR also removes the redundant NUM_CONNECTIONS enum (which was identical to MAX_SLOTS), removes always-true playerID >= 0 checks on unsigned types, and uses const and consistent UnsignedInt types for local player ID variables.

• Adds MAX_SLOTS bounds checks in 8+ functions that process network messages with player IDs (processNetCommand, processRunAheadMetrics, processChat, processDisconnectChat, processFileProgress, processFrameResendRequest, processDisconnectFrame, sendRemoteCommand)
• Removes duplicate NUM_CONNECTIONS enum from NetworkDefs.h and replaces all its usages with MAX_SLOTS
• isPlayerConnected only gets a debug assert, not a runtime guard — it could still be accessed out-of-bounds in release builds if called with an invalid ID

Confidence Score: 4/5

• This PR is a targeted safety improvement that correctly guards most network-facing player ID usages; safe to merge with one minor concern about isPlayerConnected.
• The changes consistently add bounds validation before using player IDs as array indices across ConnectionManager and DisconnectManager. The NUM_CONNECTIONS removal is verified correct (was equal to MAX_SLOTS). The only concern is that isPlayerConnected uses a debug-only assert rather than a runtime guard, though all callers from the changed code validate before calling it.
• Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp — the isPlayerConnected function at line 303 uses a debug-only assert without a runtime bounds check.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetworkDefs.h	Removes duplicate NUM_CONNECTIONS enum value (was equal to MAX_SLOTS). Correct and safe change.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Adds MAX_SLOTS bounds checks for player IDs from network packets, replaces NUM_CONNECTIONS with MAX_SLOTS, removes redundant >= 0 checks on unsigned types. One concern: isPlayerConnected has only a debug assert, not a runtime guard against out-of-bounds access.
Core/GameEngine/Source/GameNetwork/DisconnectManager.cpp	Adds MAX_SLOTS bounds check in processDisconnectFrame and removes redundant playerID < 0 check in processDisconnectScreenOff. Both changes are correct.

Flowchart

flowchart TD
    A[Network Packet Received] --> B[processNetCommand]
    B --> C{ACK Command?}
    C -->|Yes| D[processAck / processAckStage1 / processAckStage2]
    D --> D1{playerID < MAX_SLOTS?}
    D1 -->|Yes| D2[Process ACK on connection]
    D1 -->|No| D3[DEBUG_CRASH]
    C -->|No| E{playerID >= MAX_SLOTS?}
    E -->|Yes| F[Discard - Return TRUE]
    E -->|No| G{Connection exists?}
    G -->|No & not local| F
    G -->|Yes| H{Command Type}
    H -->|FRAMEINFO| I[processFrameInfo]
    H -->|RUNAHEADMETRICS| J[processRunAheadMetrics]
    H -->|CHAT| K[processChat]
    H -->|DISCONNECT*| L[DisconnectManager]
    H -->|FILEPROGRESS| M[processFileProgress]
    H -->|Other| N[sendRemoteCommand]
    I --> I1{playerID < MAX_SLOTS?}
    I1 -->|Yes| I2[Update frame data]
    I1 -->|No| I3[Skip]
    J --> J1{playerID < MAX_SLOTS?}
    J1 -->|Yes| J2[Update metrics]
    J1 -->|No| J3[Return]
    L --> L1[processDisconnectFrame]
    L1 --> L2{playerID >= MAX_SLOTS?}
    L2 -->|Yes| L3[Return]
    L2 -->|No| L4[Process disconnect]
    N --> N1{playerID < MAX_SLOTS?}
    N1 -->|Yes| N2[Use frameData manager]
    N1 -->|No| N3[Set frameDataMgr = nullptr]

Loading

_{Last reviewed commit: 2a615a8}

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp
isPlayerConnected() lacks bounds check before using playerID as array index in m_connections[playerID]

Bool ConnectionManager::isPlayerConnected( Int playerID )
{
	return ( playerID == m_localSlot || ((playerID >= 0 && playerID < MAX_SLOTS) && m_connections[playerID] && !m_connections[playerID]->isQuitting()) );
}

Prompt To Fix With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp
Line: 303:306

Comment:
`isPlayerConnected()` lacks bounds check before using `playerID` as array index in `m_connections[playerID]`

```suggestion
Bool ConnectionManager::isPlayerConnected( Int playerID )
{
	return ( playerID == m_localSlot || ((playerID >= 0 && playerID < MAX_SLOTS) && m_connections[playerID] && !m_connections[playerID]->isQuitting()) );
}
```

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

[xezon]

Does not look like ConnectionManager::isPlayerConnected needs a test, but I did add an assert to clarify the expectation.

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[Mauller]

Maybe MAX_SLOTS should be made unsigned and instances where it is used with an iterator should all be made unsigned int, i noticed there are a few places where a mix of int or unsigned int is used.

And in some instances MAX_SLOT is explicitly cast while in others it's not.
It would be better to clean this up with this change.

[xezon]

Follow up change