fix: NetworkBehaviour and NetworkVariable length safety checks

Author: EmandM

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -1112,49 +1112,33 @@ internal void MarkOwnerReadVariablesDirty()
         /// </remarks>
         internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
         {
-            if (NetworkVariableFields.Count == 0)
+            foreach (var field in NetworkVariableFields)
             {
-                return;
-            }
-
-            for (int j = 0; j < NetworkVariableFields.Count; j++)
-            {
-
-                if (NetworkVariableFields[j].CanClientRead(targetClientId))
+                if (field.CanClientRead(targetClientId))
                 {
                     if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                     {
                         var writePos = writer.Position;
                         // Note: This value can't be packed because we don't know how large it will be in advance
-                        // we reserve space for it, then write the data, then come back and fill in the space
-                        // to pack here, we'd have to write data to a temporary buffer and copy it in - which
-                        // isn't worth possibly saving one byte if and only if the data is less than 63 bytes long...
-                        // The way we do packing, any value > 63 in a ushort will use the full 2 bytes to represent.
-                        writer.WriteValueSafe((ushort)0);
+                        writer.WriteValueSafe(0);
                         var startPos = writer.Position;
                         // Write the NetworkVariable field value
-                        // WriteFieldSynchronization will write the current value only if there are no pending changes.
-                        // Otherwise, it will write the previous value if there are pending changes since the pending
-                        // changes will be sent shortly after the client's synchronization.
-                        NetworkVariableFields[j].WriteFieldSynchronization(writer);
+                        field.WriteFieldSynchronization(writer);
                         var size = writer.Position - startPos;
                         writer.Seek(writePos);
-                        writer.WriteValueSafe((ushort)size);
+                        writer.WriteValueSafe(size);
                         writer.Seek(startPos + size);
                     }
                     else
                     {
                         // Write the NetworkVariable field value
-                        // WriteFieldSynchronization will write the current value only if there are no pending changes.
-                        // Otherwise, it will write the previous value if there are pending changes since the pending
-                        // changes will be sent shortly after the client's synchronization.
-                        NetworkVariableFields[j].WriteFieldSynchronization(writer);
+                        field.WriteFieldSynchronization(writer);
                     }
                 }
                 else // Only if EnsureNetworkVariableLengthSafety, otherwise just skip
                 if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                 {
-                    writer.WriteValueSafe((ushort)0);
+                    writer.WriteValueSafe(0);
                 }
             }
         }
@@ -1169,51 +1153,37 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
         /// </remarks>
         internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
         {
-            if (NetworkVariableFields.Count == 0)
+            foreach (var field in NetworkVariableFields)
             {
-                return;
-            }
-
-            for (int j = 0; j < NetworkVariableFields.Count; j++)
-            {
-                var varSize = (ushort)0;
+                int expectedBytesToRead = 0;
                 var readStartPos = 0;
                 if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                 {
-                    reader.ReadValueSafe(out varSize);
-                    if (varSize == 0)
+                    reader.ReadValueSafe(out expectedBytesToRead);
+                    if (expectedBytesToRead == 0)
                     {
                         continue;
                     }
                     readStartPos = reader.Position;
                 }
                 else // If the client cannot read this field, then skip it
-                if (!NetworkVariableFields[j].CanClientRead(clientId))
+                if (!field.CanClientRead(clientId))
                 {
                     continue;
                 }
 
-                NetworkVariableFields[j].ReadField(reader);
+                field.ReadField(reader);
 
                 if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                 {
-                    if (reader.Position > (readStartPos + varSize))
-                    {
-                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                        {
-                            NetworkLog.LogWarning($"Var data read too far. {reader.Position - (readStartPos + varSize)} bytes.");
-                        }
-
-                        reader.Seek(readStartPos + varSize);
-                    }
-                    else if (reader.Position < (readStartPos + varSize))
+                    var totalBytesRead = reader.Position - readStartPos;
+                    if (totalBytesRead != expectedBytesToRead)
                     {
-                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        if (NetworkManager.LogLevel <= LogLevel.Normal)
                         {
-                            NetworkLog.LogWarning($"Var data read too little. {(readStartPos + varSize) - reader.Position} bytes.");
+                            NetworkLog.LogWarning($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{field.Name}] NetworkVariable read {totalBytesRead} bytes but was expected to read {expectedBytesToRead} bytes during synchronization deserialization!");
                         }
-
-                        reader.Seek(readStartPos + varSize);
+                        reader.Seek(readStartPos + expectedBytesToRead);
                     }
                 }
             }
@@ -1295,7 +1265,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
                 // Save our position where we will write the final size being written so we can skip over it in the
                 // event an exception occurs when deserializing.
                 var sizePosition = writer.Position;
-                writer.WriteValueSafe((ushort)0);
+                writer.WriteValueSafe(0);
 
                 // Save our position before synchronizing to determine how much was written
                 var positionBeforeSynchronize = writer.Position;
@@ -1333,7 +1303,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
                     // Write the number of bytes serialized to handle exceptions on the deserialization side
                     var bytesWritten = finalPosition - positionBeforeSynchronize;
                     writer.Seek(sizePosition);
-                    writer.WriteValueSafe((ushort)bytesWritten);
+                    writer.WriteValueSafe(bytesWritten);
                     writer.Seek(finalPosition);
                 }
                 return true;
@@ -1342,7 +1312,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
             {
                 var reader = serializer.GetFastBufferReader();
                 // We will always read the expected byte count
-                reader.ReadValueSafe(out ushort expectedBytesToRead);
+                reader.ReadValueSafe(out int expectedBytesToRead);
 
                 // Save our position before we begin synchronization deserialization
                 var positionBeforeSynchronize = reader.Position;

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs

@@ -108,11 +108,15 @@ internal void NetworkBehaviourUpdate(bool forceSend = false)
                 }
 
                 // Now, reset all the no-longer-dirty variables
-                foreach (var dirtyobj in m_DirtyNetworkObjects)
+                foreach (var dirtyObj in m_DirtyNetworkObjects)
                 {
-                    dirtyobj.PostNetworkVariableWrite(forceSend);
+                    foreach (var behaviour in dirtyObj.ChildNetworkBehaviours)
+                    {
+                        behaviour.PostNetworkVariableWrite(forceSend);
+                    }
+
                     // Once done processing, we set the previous owner id to the current owner id
-                    dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
+                    dirtyObj.PreviousOwnerId = dirtyObj.OwnerClientId;
                 }
                 m_DirtyNetworkObjects.Clear();
             }

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -1476,16 +1476,6 @@ internal List<NetworkBehaviour> ChildNetworkBehaviours
             }
         }
 
-        internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
-        {
-            for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
-            {
-                var behavior = ChildNetworkBehaviours[i];
-                behavior.InitializeVariables();
-                behavior.WriteNetworkVariableData(writer, targetClientId);
-            }
-        }
-
         internal void MarkVariablesDirty(bool dirty)
         {
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
@@ -1525,18 +1515,6 @@ internal static void VerifyParentingStatus()
             }
         }
 
-        /// <summary>
-        /// Only invoked during first synchronization of a NetworkObject (late join or newly spawned)
-        /// </summary>
-        internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
-        {
-            for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
-            {
-                var behaviour = ChildNetworkBehaviours[i];
-                behaviour.InitializeVariables();
-                behaviour.SetNetworkVariableData(reader, clientId);
-            }
-        }
 
         internal ushort GetNetworkBehaviourOrderIndex(NetworkBehaviour instance)
         {
@@ -1744,14 +1722,6 @@ public void Deserialize(FastBufferReader reader)
             }
         }
 
-        internal void PostNetworkVariableWrite(bool forceSend)
-        {
-            for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
-            {
-                ChildNetworkBehaviours[k].PostNetworkVariableWrite(forceSend);
-            }
-        }
-
         /// <summary>
         /// Handles synchronizing NetworkVariables and custom synchronization data for NetworkBehaviours.
         /// </summary>
@@ -1769,7 +1739,12 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
                 var sizeToSkipCalculationPosition = writer.Position;
 
                 // Synchronize NetworkVariables
-                WriteNetworkVariableData(writer, targetClientId);
+                foreach (var behavior in ChildNetworkBehaviours)
+                {
+                    behavior.InitializeVariables();
+                    behavior.WriteNetworkVariableData(writer, targetClientId);
+                }
+
                 // Reserve the NetworkBehaviour synchronization count position
                 var networkBehaviourCountPosition = writer.Position;
                 writer.WriteValueSafe((byte)0);
@@ -1803,23 +1778,35 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
             else
             {
                 var reader = serializer.GetFastBufferReader();
-
                 reader.ReadValueSafe(out ushort sizeOfSynchronizationData);
                 var seekToEndOfSynchData = reader.Position + sizeOfSynchronizationData;
-                // Apply the network variable synchronization data
-                SetNetworkVariableData(reader, targetClientId);
-                // Read the number of NetworkBehaviours to synchronize
-                reader.ReadValueSafe(out byte numberSynchronized);
-                var networkBehaviourId = (ushort)0;
 
-                // If a NetworkBehaviour writes synchronization data, it will first
-                // write its NetworkBehaviourId so when deserializing the client-side
-                // can find the right NetworkBehaviour to deserialize the synchronization data.
-                for (int i = 0; i < numberSynchronized; i++)
+                try
+                {
+                    // Apply the network variable synchronization data
+                    foreach (var behaviour in ChildNetworkBehaviours)
+                    {
+                        behaviour.InitializeVariables();
+                        behaviour.SetNetworkVariableData(reader, targetClientId);
+                    }
+
+                    // Read the number of NetworkBehaviours to synchronize
+                    reader.ReadValueSafe(out byte numberSynchronized);
+                    var networkBehaviourId = (ushort)0;
+
+                    // If a NetworkBehaviour writes synchronization data, it will first
+                    // write its NetworkBehaviourId so when deserializing the client-side
+                    // can find the right NetworkBehaviour to deserialize the synchronization data.
+                    for (int i = 0; i < numberSynchronized; i++)
+                    {
+                        serializer.SerializeValue(ref networkBehaviourId);
+                        var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
+                        networkBehaviour.Synchronize(ref serializer, targetClientId);
+                    }
+                }
+                catch
                 {
-                    serializer.SerializeValue(ref networkBehaviourId);
-                    var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
-                    networkBehaviour.Synchronize(ref serializer, targetClientId);
+                    reader.Seek(seekToEndOfSynchData);
                 }
             }
         }
@@ -1916,7 +1903,7 @@ internal static NetworkObject AddSceneObject(in SceneObject sceneObject, FastBuf
                 try
                 {
                     // If we failed to load this NetworkObject, then skip past the Network Variable and (if any) synchronization data
-                    reader.ReadValueSafe(out ushort networkBehaviourSynchronizationDataLength);
+                    reader.ReadValueSafe(out int networkBehaviourSynchronizationDataLength);
                     reader.Seek(reader.Position + networkBehaviourSynchronizationDataLength);
                 }
                 catch (Exception ex)