refactor(netpacket): Simplify NetPacket functions for packet buffer reads

[xezon]

• Follow up for refactor(netpacket): Simplify NetPacket functions for packet buffer writes and size tests #2329

This change simplifies all NetPacket functions for packet buffer reads.

TODO

• Basic Network testing
• Test file transfer
• Test Network matches and communications

[greptile-apps]

Greptile Summary

This PR refactors all NetPacket read functions into a unified readMessage virtual-dispatch pattern backed by NetPacketBuf and NetCommandDataChunk. Previously-flagged issues from the prior review thread have been addressed: the getBufferForRead(0) assertion is guarded in readStringWithoutNull, readStringWithNull now advances by the full on-wire string length (not the truncated copy length), the break-instead-of-continue desync risk in the packet parser is fixed, argument evaluation order in sendFile is corrected, and the previously uninitialised m_relay member is now zeroed in the NetCommandRef constructor.

Confidence Score: 5/5

Safe to merge; all previously-flagged P0/P1 blockers are resolved and remaining feedback is non-blocking.

All prior review concerns are addressed. The one new P2 finding (uncapped network-controlled allocation in wrapper/file readMessage) requires a malformed or malicious packet to trigger and is consistent with this codebase's existing trust model for LAN peers. No P0 or P1 issues remain.

Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp — wrapper and file readMessage allocation sizing

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetPacketStructs.h	New NetPacketBuf and network:: read/write helpers; readStringWithoutNull has zero-length guard; readStringWithNull advances by full on-wire string length via strnlen(src.size())
Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp	Implements SmallNetPacketCommandBase field-dispatch loop and all per-command readMessage bodies; network-controlled dataLength is allocated without bounding to remaining buffer in wrapper/file commands
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	Adds NetCommandDataChunk RAII wrapper with release() ownership transfer; replaces raw-pointer setData/setFileData overloads; adds readMessageData virtual dispatch
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	ConstructNetCommandMsgFromRawData rewritten using NetPacketBuf/readMessage dispatch; getCommandList parsing loop correctly continues past unknown field types
Core/GameEngine/Source/GameNetwork/NetCommandRef.cpp	Constructor null-checks m_msg before attach(); initialises previously-uninitialised m_relay to 0
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	sendFile correctly separates size()/readEntireAndClose() calls; wraps buffer in NetCommandDataChunk for RAII lifetime management
Core/GameEngine/Source/GameNetwork/NetCommandList.cpp	Adds addMessage(NetCommandRef*&) overload that takes ownership of an existing ref and nulls it on deduplication
Core/GameEngine/Include/GameNetwork/NetCommandList.h	Declares new addMessage(NetCommandRef*&) overload
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp	setData/setFileData now accept NetCommandDataChunk& and use release() for correct ownership transfer

Sequence Diagram

sequenceDiagram
    participant Receiver
    participant NetPacket
    participant SmallNetPacketCommandBase
    participant NetCommandMsg
    participant CommandData

    Receiver->>NetPacket: getCommandList()
    NetPacket->>SmallNetPacketCommandBase: readMessage(ref, base, buf)
    SmallNetPacketCommandBase->>SmallNetPacketCommandBase: dispatch field-type bytes (T/R/F/P/C/D)
    SmallNetPacketCommandBase->>NetCommandMsg: constructNetCommandMsg(base)
    NetCommandMsg-->>SmallNetPacketCommandBase: msg*
    SmallNetPacketCommandBase-->>NetPacket: ref (populated), bytes consumed
    NetPacket->>CommandData: ref->getCommand()->readMessageData(*ref, buf.offset(size))
    CommandData->>CommandData: network::readObject / readString / readBytes
    CommandData-->>NetPacket: bytes consumed
    NetPacket->>NetCommandList: addMessage(ref)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetPacketStructs.cpp
Line: 1046-1079

Comment:
**Uncapped network-controlled allocation in `readMessage`**

`NetCommandDataChunk dataChunk(data.dataLength)` allocates `data.dataLength` bytes whose value comes directly from the packet, without clamping to the remaining buffer size. `readBytes` will only copy `min(data.dataLength, buf.remaining())` bytes, but the full allocation still happens. A malformed packet with `dataLength = UINT_MAX` would attempt to allocate ~4 GiB and crash since this codebase does not use exception handling. The same pattern appears in `NetPacketFileCommandData::readMessage`.

Consider bounding the allocation to the available buffer:
```cpp
const UnsignedInt safeLength = std::min<UnsignedInt>(data.dataLength, static_cast<UnsignedInt>(buf.offset(size).size()));
NetCommandDataChunk dataChunk(safeLength);
```

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

_{Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.}

_{Reviews (6): Last reviewed commit: "Continue on unrecognized packet instead ..." | Re-trigger Greptile}

[xezon]

@greptileai You write:

Unhandled argument data type silently desynchronises remaining arguments

This is as per the original implementation and outside the scope of this change.

[greptile-apps]

Good point! If it's existing behavior, then it's fine to leave it as-is in this refactoring. Keeping the scope focused on simplification makes sense.

[Mauller]

Looks good overall, but we want to do some thorough testing of all network messages

[Mauller]

@greptileai You write:

Unhandled argument data type silently desynchronises remaining arguments

This is as per the original implementation and outside the scope of this change.

Since we have the "RETAIL_COMPATIBLE_NETWORKING" conditional now, a followup could be done to implement corrected behaviour.

[greptile-apps]

Good point - you're right to keep this refactoring focused on simplification. Adding the corrected behavior under RETAIL_COMPATIBLE_NETWORKING in a followup makes sense since it would be a behavioral change rather than just a structural improvement.

_{Tip: You can give me more context with .greptile/rules so I get this right next time.}

[xezon]

Yes this needs testing. I think no one is on it right now.

[Mauller]

Yes this needs testing. I think no one is on it right now.

I will see about getting some people together, could you rebase this up to current main?

[xezon]

I will see about getting some people together, could you rebase this up to current main?

Rebased.