fix(netpacket): Fix undefined behavior with NetPacket buffer writes

[xezon]

• Follow up for refactor(netpacket): Simplify initialization of net packet field types #2288

This change fixes the undefined behavior with NetPacket buffer writes in NetPacket::FillBuffer functions.

TODO

• Test in Network match

[greptile-apps]

Greptile Overview

Greptile Summary

This PR fixes undefined behavior in all NetPacket::FillBufferWith* serialization functions by replacing placement-new construction directly into raw UnsignedByte* buffers with stack-local construction followed by memcpy.

• Root cause: The NetPacket* structs (defined in NetPacketStructs.h) have non-trivial constructors that initialize field type marker bytes. The target buffers are raw UnsignedByte arrays (NEW UnsignedByte[bufferSize] or m_packet[MAX_PACKET_SIZE]) with no alignment guarantees. Using placement-new on potentially misaligned buffers and then accessing through both raw byte and struct pointers violates strict aliasing rules — all of which constitute undefined behavior.
• Fix: Each of the 20 FillBufferWith* functions now constructs the packet struct as a local stack variable (guaranteeing proper alignment and correct constructor execution), then copies the initialized struct into the buffer using memcpy — the canonical safe serialization pattern in C++.
• Variable data portions (game messages, chat text, file data) that follow the fixed-size struct headers were already using memcpy and remain unchanged, with correct offset calculations referencing sizeof(PacketType).
• The change is purely mechanical and consistent across all affected functions, with no behavioral differences in the serialized output.

Confidence Score: 5/5

• This PR is safe to merge — it replaces undefined behavior with well-defined semantics while producing identical serialized output.
• The change is mechanical and consistent across all 20 FillBufferWith* functions. Each follows the same pattern: construct on stack, populate fields, memcpy to buffer. The struct definitions are pragma-packed to 1-byte alignment ensuring sizeof matches the wire format. Variable data portions already used memcpy and are unchanged. No new logic, no behavioral changes, no edge cases introduced.
• No files require special attention.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	Replaces placement-new into raw byte buffers with stack construction + memcpy across all 20 FillBufferWith* functions, eliminating alignment and strict aliasing undefined behavior. All changes are mechanical and consistent.

Flowchart

flowchart TD
    A["FillBufferWithCommand(buffer, ref)"] --> B{Switch on\nNetCommandType}
    B --> C["FillBufferWithGameCommand"]
    B --> D["FillBufferWithAckCommand"]
    B --> E["FillBufferWithFrameCommand"]
    B --> F["...18 other FillBufferWith* functions"]

    subgraph old["Old Pattern (Undefined Behavior)"]
        G["placement-new: NetPacketXxx* p = new (buffer) NetPacketXxx"] --> H["Write fields via p->field = value"]
    end

    subgraph new["New Pattern (Well-Defined)"]
        I["Stack construct: NetPacketXxx packet"] --> J["Write fields via packet.field = value"]
        J --> K["memcpy(buffer, &packet, sizeof(packet))"]
    end

    C --> new
    D --> new
    E --> new
    F --> new

    K --> L["Append variable data via memcpy at offset sizeof(NetPacketXxx)"]

Loading

_{Last reviewed commit: 55e69ea}

[Mauller]

Considering how frequently these are used, maybe we make these static so we aren't always initialising the structs?
The variable parts are always updated with each use so it shouldn't be an issue.

[xezon]

Unclear to me if that would give anything for performance. Making it static means it goes into the DATA segment. So CPU would need to write and read from DATA segment to heap buffer, which of course is fast, but maybe write and read from code segment to stack to heap buffer is just fine? Code instructions and Stack are hot. DATA segment maybe not as hot?

[Mauller]

It's fair, there's probably nothing to it in the grand scheme of things since we don't need to persist any values between calls.

[xezon]

We can revisit this when it indeed shows up in profiler. I doubt it will.

[Mauller]

Since this is a local variable, this offset adjustment seems redundant here.
I think fill buffer with chat command does the same thing.

seems like this was a copypasta in this situation from the other function

[Mauller]

seems like the file transfer command does the same thing

[xezon]

Yes. I already have a follow up refactor for afterwards that cleans this up. Outside the scope of this change.