bugfix(network): Fix packet size setup mistakes

[bobtista]

Fixes latent bugs in network packet size calculations

Changes

1. Fix struct sizes (NetPacketStructs.h)

   • Add missing commandId field to NetPacketLoadCompleteMessage and NetPacketTimeOutGameStartMessage
   • Change averageFps from UnsignedByte to UnsignedShort in NetPacketRunAheadMetricsCommand

2. Fix isRoomFor* functions (NetPacket.cpp)

   • Add missing CommandId size calculation to isRoomForLoadCompleteMessage and isRoomForTimeOutGameStartMessage

3. Fix wrong function call (NetPacket.cpp)

   • addTimeOutGameStartMessage now calls isRoomForTimeOutGameStartMessage instead of isRoomForLoadCompleteMessage (These are accidentally/coincidentally identical, but it's now using the right one)

[xezon]

Can you describe the implications of this change? What does it fix in practice and can it be user facing at all?

[bobtista]

Can you describe the implications of this change? What does it fix in practice and can it be user facing at all?

There should be no practical change, and no user facing change. In theory, when a message is too large, the wrapper path uses sizeof(struct) which would cause problems when it's wrong.

• GetBufferSizeNeededForCommand -> getPackedByteCount() -> sizeof(NetPacketLoadCompleteMessage) = 7 bytes
• Allocate 7-byte buffer but then FillBufferWithLoadCompleteMessage writes 10 bytes
• Also bufferSize would be wrong, so the receiver would get incomplete data
  BUT
  LoadComplete/TimeOutGameStart are ~10 bytes, MAX_PACKET_SIZE is 512+, so addCommand() should always succeed and this path should never be hit.

That said, keeping the bug makes the serialization /deserialization code messier and hacky. We have to write the struct and then manually add the commandId, instead of just writing the struct. #1680 will be simpler after this fix

[xezon]

Are you very sure this causes no runtime issues?

UnsignedInt bufferSize = GetBufferSizeNeededForCommand(msg); // <---- expects good size
bigPacketData = NEW UnsignedByte[bufferSize]; // <---- allocates with size
FillBufferWithCommand(bigPacketData, ref); // <---- writes to sized buffer

Wouldn't the write overflow the buffer if it was too small?

[bobtista]

Yes exactly. The reason there's no runtime issue is that ConstructBigCommandPacketList is only called when packet->addCommand() returns false (message too large for single packet). In other words, I think this is a problematic bug that went unnoticed because the message sizes are just never too big. Do you see any downsides to fixing it?

[xezon]

Hmm ok maybe ConstructBigCommandPacketList is indeed seldom called. I would expect it to be called on file transfer, because file data certainly is bigger than single packet size.

I wonder how the isRoomFor* functions relate to this. Are these quasi duplicates of the getPackedByteCount functions?

Do you see any downsides to fixing it?

No. I am just trying to understand if this will fix anything user facing.

[bobtista]

Hmm ok maybe ConstructBigCommandPacketList is indeed seldom called. I would expect it to be called on file transfer, because file data certainly is bigger than single packet size.

I wonder how the isRoomFor* functions relate to this. Are these quasi duplicates of the getPackedByteCount functions?

Yes, file transfers use the big packet flow, but those sizes are correct - the bug was only in LoadComplete/TimeOutGameStart which never use the big packet path.

The isRoomFor* functions do similar work, but they handle the conditional inclusion of things like 'Did this change since the last message?' - while looking, I noticed they actually had the same bugs (missing CommandId counting), now fixed. Again, it never mattered for these messages because it's like 10 bytes being checked if it fits in 512 byte packets.

[xezon]

I have promoted this to bug fix because in theory this can corrupt memory, but is probably very unlikely (or even impossible) in practice. Basically a packet would have to be filled just enough for one of the missized packets to pass the incorrect size check, while their written contents would then exceed the packet buffer.

I hope we can consolidate these packet size code paths because this setup is just awful.