refactor: Ensure lengths are always written as uints (#2946)

* refactor: Ensure lengths are always written as uints

* Update CHANGELOG.md

---------

Co-authored-by: Noel Stephens <noel.stephens@unity3d.com>

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -8,10 +8,17 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ## [Unreleased]
 
+### Added
+
+
 ### Fixed
 
 - Fixed issue where SessionOwner message was being treated as a new entry for the new message indexing when it should have been ordinally sorted with the legacy message indices. (#2942)
 
+### Changed
+- Changed `FastBufferReader` and `FastBufferWriter` so that they always ensure the length of items serialized is always serialized as an `uint` and added a check before casting for safe reading and writing.(#2946)
+
+
 ## [2.0.0-exp.4] - 2024-05-31
 
 ### Added

com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs

@@ -523,8 +523,8 @@ public void ReadNetworkSerializableInPlace<T>(ref T value) where T : INetworkSer
         /// <param name="oneByteChars">Whether or not to use one byte per character. This will only allow ASCII</param>
         public unsafe void ReadValue(out string s, bool oneByteChars = false)
         {
-            ReadValue(out uint length);
-            s = "".PadRight((int)length);
+            ReadLength(out int length);
+            s = "".PadRight(length);
             int target = s.Length;
             fixed (char* native = s)
             {
@@ -562,18 +562,18 @@ public unsafe void ReadValueSafe(out string s, bool oneByteChars = false)
             }
 #endif
 
-            if (!TryBeginReadInternal(sizeof(uint)))
+            if (!TryBeginReadInternal(SizeOfLengthField()))
             {
                 throw new OverflowException("Reading past the end of the buffer");
             }
 
-            ReadValue(out uint length);
+            ReadLength(out int length);
 
-            if (!TryBeginReadInternal((int)length * (oneByteChars ? 1 : sizeof(char))))
+            if (!TryBeginReadInternal(length * (oneByteChars ? 1 : sizeof(char))))
             {
                 throw new OverflowException("Reading past the end of the buffer");
             }
-            s = "".PadRight((int)length);
+            s = "".PadRight(length);
             int target = s.Length;
             fixed (char* native = s)
             {
@@ -592,6 +592,33 @@ public unsafe void ReadValueSafe(out string s, bool oneByteChars = false)
             }
         }
 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static int SizeOfLengthField() => sizeof(uint);
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void ReadLengthSafe(out uint length) => ReadUnmanagedSafe(out length);
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void ReadLength(out uint length) => ReadUnmanaged(out length);
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void ReadLengthSafe(out int length)
+        {
+            ReadLengthSafe(out uint temp);
+            if (temp > int.MaxValue)
+            {
+                throw new InvalidCastException("length value outside of int32 range");
+            }
+            length = (int)temp;
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void ReadLength(out int length)
+        {
+            ReadLength(out uint temp);
+            length = (int)temp;
+        }
+
         /// <summary>
         /// Read a partial value. The value is zero-initialized and then the specified number of bytes is read into it.
         /// </summary>
@@ -777,7 +804,7 @@ internal unsafe void ReadUnmanagedSafe<T>(out T value) where T : unmanaged
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanaged<T>(out T[] value) where T : unmanaged
         {
-            ReadUnmanaged(out int sizeInTs);
+            ReadLength(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value = new T[sizeInTs];
             fixed (T* ptr = value)
@@ -789,7 +816,7 @@ internal unsafe void ReadUnmanaged<T>(out T[] value) where T : unmanaged
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanagedSafe<T>(out T[] value) where T : unmanaged
         {
-            ReadUnmanagedSafe(out int sizeInTs);
+            ReadLengthSafe(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value = new T[sizeInTs];
             fixed (T* ptr = value)
@@ -801,7 +828,7 @@ internal unsafe void ReadUnmanagedSafe<T>(out T[] value) where T : unmanaged
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanaged<T>(out NativeArray<T> value, Allocator allocator) where T : unmanaged
         {
-            ReadUnmanaged(out int sizeInTs);
+            ReadLength(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value = new NativeArray<T>(sizeInTs, allocator);
             byte* bytes = (byte*)value.GetUnsafePtr();
@@ -810,7 +837,7 @@ internal unsafe void ReadUnmanaged<T>(out NativeArray<T> value, Allocator alloca
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanagedSafe<T>(out NativeArray<T> value, Allocator allocator) where T : unmanaged
         {
-            ReadUnmanagedSafe(out int sizeInTs);
+            ReadLengthSafe(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value = new NativeArray<T>(sizeInTs, allocator);
             byte* bytes = (byte*)value.GetUnsafePtr();
@@ -820,7 +847,7 @@ internal unsafe void ReadUnmanagedSafe<T>(out NativeArray<T> value, Allocator al
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanagedInPlace<T>(ref NativeList<T> value) where T : unmanaged
         {
-            ReadUnmanaged(out int sizeInTs);
+            ReadLength(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value.Resize(sizeInTs, NativeArrayOptions.UninitializedMemory);
             byte* bytes = (byte*)value.GetUnsafePtr();
@@ -829,7 +856,7 @@ internal unsafe void ReadUnmanagedInPlace<T>(ref NativeList<T> value) where T :
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal unsafe void ReadUnmanagedSafeInPlace<T>(ref NativeList<T> value) where T : unmanaged
         {
-            ReadUnmanagedSafe(out int sizeInTs);
+            ReadLengthSafe(out int sizeInTs);
             int sizeInBytes = sizeInTs * sizeof(T);
             value.Resize(sizeInTs, NativeArrayOptions.UninitializedMemory);
             byte* bytes = (byte*)value.GetUnsafePtr();
@@ -1078,7 +1105,7 @@ public void ReadValueSafeInPlace<T>(ref NativeList<T> value, FastBufferWriter.Fo
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal void ReadValueSafeInPlace<T>(ref NativeHashSet<T> value) where T : unmanaged, IEquatable<T>
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value.Clear();
             for (var i = 0; i < length; ++i)
             {
@@ -1093,7 +1120,7 @@ internal void ReadValueSafeInPlace<TKey, TVal>(ref NativeHashMap<TKey, TVal> val
             where TKey : unmanaged, IEquatable<TKey>
             where TVal : unmanaged
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value.Clear();
             for (var i = 0; i < length; ++i)
             {
@@ -1553,7 +1580,7 @@ internal void ReadValueSafeInPlace<TKey, TVal>(ref NativeHashMap<TKey, TVal> val
         /// This method is a little difficult to use, since you have to know the size of the string before
         /// reading it, but is useful when the string is a known, fixed size. Note that the size of the
         /// string is also encoded, so the size to call TryBeginRead on is actually the fixed size (in bytes)
-        /// plus sizeof(int)
+        /// plus sizeof(uint)
         /// </summary>
         /// <param name="value">the value to read</param>
         /// <param name="unused">An unused parameter used for enabling overload resolution based on generic constraints</param>
@@ -1562,7 +1589,7 @@ internal void ReadValueSafeInPlace<TKey, TVal>(ref NativeHashMap<TKey, TVal> val
         public unsafe void ReadValue<T>(out T value, FastBufferWriter.ForFixedStrings unused = default)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanaged(out int length);
+            ReadLength(out int length);
             value = new T
             {
                 Length = length
@@ -1584,7 +1611,7 @@ public unsafe void ReadValue<T>(out T value, FastBufferWriter.ForFixedStrings un
         public unsafe void ReadValueSafe<T>(out T value, FastBufferWriter.ForFixedStrings unused = default)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value = new T
             {
                 Length = length
@@ -1606,7 +1633,7 @@ public unsafe void ReadValueSafe<T>(out T value, FastBufferWriter.ForFixedString
         public unsafe void ReadValueSafeInPlace<T>(ref T value, FastBufferWriter.ForFixedStrings unused = default)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value.Length = length;
             ReadBytesSafe(value.GetUnsafePtr(), length);
         }
@@ -1625,7 +1652,7 @@ public unsafe void ReadValueSafeInPlace<T>(ref T value, FastBufferWriter.ForFixe
         public unsafe void ReadValueSafe<T>(out NativeArray<T> value, Allocator allocator)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value = new NativeArray<T>(length, allocator);
             var ptr = (T*)value.GetUnsafePtr();
             for (var i = 0; i < length; ++i)
@@ -1647,7 +1674,7 @@ public unsafe void ReadValueSafe<T>(out NativeArray<T> value, Allocator allocato
         public unsafe void ReadValueSafeTemp<T>(out NativeArray<T> value)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value = new NativeArray<T>(length, Allocator.Temp);
             var ptr = (T*)value.GetUnsafePtr();
             for (var i = 0; i < length; ++i)
@@ -1669,7 +1696,7 @@ public unsafe void ReadValueSafeTemp<T>(out NativeArray<T> value)
         public void ReadValueSafe<T>(out T[] value, FastBufferWriter.ForFixedStrings unused = default)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value = new T[length];
             for (var i = 0; i < length; ++i)
             {
@@ -1691,7 +1718,7 @@ public void ReadValueSafe<T>(out T[] value, FastBufferWriter.ForFixedStrings unu
         public void ReadValueSafeInPlace<T>(ref NativeList<T> value)
             where T : unmanaged, INativeList<byte>, IUTF8Bytes
         {
-            ReadUnmanagedSafe(out int length);
+            ReadLengthSafe(out int length);
             value.Resize(length, NativeArrayOptions.UninitializedMemory);
             for (var i = 0; i < length; ++i)
             {