Fix/cast messagesize as uint

[kaen]

Casts MessageSize as uint instead of ushort when writing the message header. See the associated issue (#1591) for how this causes failures. In short, if we send 0x10001 bytes of data here the message size gets clipped to ushort making it 0x0001. The full data is actually still written without issue. Just the size reported in the header is wrong.

On the receiving end, this value is used to allocate a buffer for reading the data. In the scenario above we get a one-byte buffer to hold our 64k of data. The actual read logic is scattered all over the place, in my case it was failing in NetworkVariable deserialize, but it will fail just about anywhere there's a buffer read. Those things don't actually check the message header's reported size, they just assume the data is there and attempt to read past the buffer.

Everywhere else in this file message size is semantically treated as int and cast to int. I don't think a negative message size has any sensible meaning and the cast on this line is currently unsigned, so I chose unsigned int.

In any case, it's not the outgoing MessagingSystem's job to silently clip the logical data size that gets written, especially when writing it out. It's the transport's job to complain if it can't serialize it, and it's the clients job to not read UINT_MAX bytes from a socket. The ByteWriter thing has its own check that the size value itself can be serialized (capped to 30 bits). And pretty much everything else down stream is responsible for what goes into the buffer and over the wire.

If MessagingSystem actually does care it needs to fail loudly and with an explicit check. Obviously, with the silent truncation this was sort of a debugging nightmare.

PR Checklist

• Have you added a backport label (if needed)? For example, the type:backport-release-* label. After you backport the PR, the label changes to stat:backported-release-*.
• Have you updated the changelog? Each package has a CHANGELOG.md file.
• Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR or a link to the documenation repo PR if this is a manual update.

Changelog

com.unity.netcode.gameobjects

• Fixed: Messages larger than 64k being written with incorrectly truncated message size in header

Testing and Documentation

• Includes unit tests.

[unity-cla-assistant]

All committers have signed the CLA.

[ShadauxCat]

Thank you for catching this. MessageSize used to be a ushort and I thought I'd caught all of these when I changed the size, but obviously I missed one.

[0xFA11]

awesome! nice contribution, thank you! :)
(we should mirror this branch internally to run our CI to merge into develop)

[ShadauxCat]

Those things don't actually check the message header's reported size, they just assume the data is there and attempt to read past the buffer.

Technically this isn't accurate. The MessageSize value is passed to FastBufferReader's constructor. Checking the size is done within FastBufferReader and it throws an exception if you try to read past that size. (This is what TryBeginRead() is doing.)

Everywhere else in this file message size is semantically treated as int and cast to int. I don't think a negative message size has any sensible meaning and the cast on this line is currently unsigned, so I chose unsigned int.

The reason int was chosen (edit: the reason it was chosen for the length value of FastBufferReader/FastBufferWriter) is that other size values in C# also use int (for example, Array.Length is an int). Using uint for some size values results in a lot of requirement to cast between int and uint so it's easier to use what's used in the standard for all of our length values. (edit: But we cast to unsigned for the message header because our integer packing functions are more efficient on unsigned values.)

Also - little known fact, in C and C++, signed integers often perform better than unsigned integers, because signed overflow is undefined behavior while unsigned is strictly defined to wrap around, so the compiler can do some optimizations on signed integers that it can't do on unsigned. (Conversely, however, division by a constant is usually faster for unsigned.) I'm not actually sure if any of that applies in C#, but it's interesting to know.

In any case, it's not the outgoing MessagingSystem's job to silently clip the logical data size that gets written, especially when writing it out.

Strictly speaking, the outgoing MessagingSystem doesn't clip it. It sends all the data and just reports an incorrect size in the header, which results in the incoming system discording most of the received data. Same result, but... ;) This was a bug, not an intended design. The MessageSize value is recorded in the header not to prevent overrunning the send buffer, but because one packet can contain multiple messages due to batching, and the incoming system needs to know where the boundaries between messages are before they're fully processed. This is important because it keeps the message handler code from over-reading and ending up reading part of another message, or under-reading and resulting in the next message starting to read its header data from the middle of the one before it - either of which could happen due to either a bug (but of course, hopefully not) or to malicious data in the received message.

[ShadauxCat]

Our CI doesn't run on PRs from forked repositories, so I've duplicated this PR in #1686, which we'll merge in once it's passed CI. Accordingly, I'm closing this PR.

[kaen]

awesome! Thanks for picking this up, I wasn't sure where to go with it next ♥️