fix: NetworkBehaviour and NetworkVariableDelta length safety checks

Author: EmandM

com.unity.netcode.gameobjects/Runtime/Configuration/SessionConfig.cs

@@ -9,9 +9,10 @@ internal class SessionConfig
         public const uint BypassFeatureCompatible = 1;
         public const uint ServerDistributionCompatible = 2;
         public const uint SessionStateToken = 3;
+        public const uint NetworkBehaviourSerializationSafety = 4;
 
         // The most current session version (!!!!set this when you increment!!!!!)
-        public static uint PackageSessionVersion => SessionStateToken;
+        public static uint PackageSessionVersion => NetworkBehaviourSerializationSafety;
 
         internal uint SessionVersion;
 

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

@@ -1232,55 +1232,41 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
             var networkManager = NetworkManager;
             var ensureLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety;
 
-
-            // Exit early if there are no NetworkVariables
-            if (NetworkVariableFields.Count == 0)
-            {
-                return;
-            }
-
-            for (int j = 0; j < NetworkVariableFields.Count; j++)
+            foreach (var field in NetworkVariableFields)
             {
                 // Client-Server: Try to write values only for clients that have read permissions.
                 // Distributed Authority: All clients have read permissions, always try to write the value.
-                if (NetworkVariableFields[j].CanClientRead(targetClientId))
+                if (field.CanClientRead(targetClientId))
                 {
-                    // Write additional NetworkVariable information when length safety is enabled or when in distributed authority mode
+                    // Write additional NetworkVariable information when length safety is enabled
                     if (ensureLengthSafety)
                     {
                         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);
+                        // we reserve space for it, then write the data, then come back and write the final size value
+                        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);
+
+                        // Write the NetworkVariable field value size
                         var size = writer.Position - startPos;
                         writer.Seek(writePos);
-                        // Write the NetworkVariable field value size
-                        writer.WriteValueSafe((ushort)size);
+                        writer.WriteValueSafe(size);
                         writer.Seek(startPos + size);
                     }
-                    else // Client-Server Only: Should only ever be invoked when using a client-server NetworkTopology
+                    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 if (ensureLengthSafety)
                 {
                     // Client-Server Only: If the client cannot read this field, then skip it but write a 0 for this NetworkVariable's position
                     {
-                        writer.WriteValueSafe((ushort)0);
+                        writer.WriteValueSafe(0);
                     }
                 }
             }
@@ -1300,26 +1286,20 @@ internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
             var networkManager = NetworkManager;
             var ensureLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety;
 
-            // Exit early if nothing else to read
-            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;
                 // Client-Server: Clients that only have read permissions will try to read the value
                 // Distributed Authority: All clients have read permissions, always try to read the value
-                if (NetworkVariableFields[j].CanClientRead(clientId))
+                if (field.CanClientRead(clientId))
                 {
                     if (ensureLengthSafety)
                     {
-                        reader.ReadValueSafe(out varSize);
-                        if (varSize == 0)
+                        reader.ReadValueSafe(out expectedBytesToRead);
+                        if (expectedBytesToRead == 0)
                         {
-                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] Expected non-zero size readable NetworkVariable! (Skipping)");
+                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{field.Name}] Expected non-zero size readable NetworkVariable! (Skipping)");
                             continue;
                         }
                         readStartPos = reader.Position;
@@ -1330,38 +1310,29 @@ internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
                     // If skipping and length safety, then fill in a 0 size for this one spot
                     if (ensureLengthSafety)
                     {
-                        reader.ReadValueSafe(out ushort size);
-                        if (size != 0)
+                        reader.ReadValueSafe(out expectedBytesToRead);
+                        if (expectedBytesToRead != 0)
                         {
-                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] Expected zero size for non-readable NetworkVariable when EnsureNetworkVariableLengthSafety is enabled! (Skipping)");
+                            Debug.LogError($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{field.Name}] Expected zero size for non-readable NetworkVariable when EnsureNetworkVariableLengthSafety is enabled! (Skipping)");
                         }
                     }
                     continue;
                 }
 
                 // Read the NetworkVariable value
-                NetworkVariableFields[j].ReadField(reader);
+                field.ReadField(reader);
 
                 // When EnsureNetworkVariableLengthSafety always do a bounds check
                 if (ensureLengthSafety)
                 {
-                    if (reader.Position > (readStartPos + varSize))
-                    {
-                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                        {
-                            NetworkLog.LogWarning($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] NetworkVariable data read too big. {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($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{NetworkVariableFields[j].Name}] NetworkVariable data read too small. {(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);
                     }
                 }
             }
@@ -1443,7 +1414,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;
@@ -1481,7 +1452,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;
@@ -1490,7 +1461,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

@@ -104,10 +104,14 @@ 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);
+                    }
                 }
                 m_DirtyNetworkObjects.Clear();
             }

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

@@ -1961,7 +1961,11 @@ public void SpawnAsPlayerObject(ulong clientId, bool destroyWithScene = false)
         /// <param name="destroy">(true) the <see cref="GameObject"/> will be destroyed (false) the <see cref="GameObject"/> will persist after being despawned</param>
         public void Despawn(bool destroy = true)
         {
-            MarkVariablesDirty(false);
+            foreach (var behavior in ChildNetworkBehaviours)
+            {
+                behavior.MarkVariablesDirty(false);
+            }
+
             NetworkManager.SpawnManager.DespawnObject(this, destroy);
         }
 
@@ -2644,33 +2648,6 @@ internal List<NetworkBehaviour> ChildNetworkBehaviours
             }
         }
 
-        internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
-        {
-            if (NetworkManager.DistributedAuthorityMode)
-            {
-                writer.WriteValueSafe((ushort)ChildNetworkBehaviours.Count);
-                if (ChildNetworkBehaviours.Count == 0)
-                {
-                    return;
-                }
-            }
-            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++)
-            {
-                var behavior = ChildNetworkBehaviours[i];
-                behavior.MarkVariablesDirty(dirty);
-            }
-        }
-
         /// <summary>
         /// Used when changing ownership, this will mark any owner read permission base NetworkVariables as dirty
         /// and will check if any owner write permission NetworkVariables are dirty (primarily for collections) so
@@ -2728,31 +2705,6 @@ internal static void VerifyParentingStatus()
             }
         }
 
-        /// <summary>
-        /// Only invoked during first synchronization of a NetworkObject (late join or newly spawned)
-        /// </summary>
-        internal bool SetNetworkVariableData(FastBufferReader reader, ulong clientId)
-        {
-            if (NetworkManager.DistributedAuthorityMode)
-            {
-                var readerPosition = reader.Position;
-                reader.ReadValueSafe(out ushort behaviourCount);
-                if (behaviourCount != ChildNetworkBehaviours.Count)
-                {
-                    Debug.LogError($"[{name}] Network Behavior Count Mismatch! [In: {behaviourCount} vs Local: {ChildNetworkBehaviours.Count}][StartReaderPos: {readerPosition}] CurrentReaderPos: {reader.Position}]");
-                    return false;
-                }
-            }
-
-            for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
-            {
-                var behaviour = ChildNetworkBehaviours[i];
-                behaviour.InitializeVariables();
-                behaviour.SetNetworkVariableData(reader, clientId);
-            }
-            return true;
-        }
-
         /// <summary>
         /// Gets the order index of a NetworkBehaviour instance within the ChildNetworkBehaviours collection
         /// </summary>
@@ -3040,14 +2992,6 @@ public void Deserialize(FastBufferReader reader)
             }
         }
 
-        internal void PostNetworkVariableWrite(bool forced = false)
-        {
-            for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
-            {
-                ChildNetworkBehaviours[k].PostNetworkVariableWrite(forced);
-            }
-        }
-
         /// <summary>
         /// Handles synchronizing NetworkVariables and custom synchronization data for NetworkBehaviours.
         /// </summary>
@@ -3059,13 +3003,20 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
         {
             if (serializer.IsWriter)
             {
+                // write placeholder int.
+                // Can't be bitpacked because we don't know the value until we calculate it later
                 var writer = serializer.GetFastBufferWriter();
                 var positionBeforeSynchronizing = writer.Position;
-                writer.WriteValueSafe((ushort)0);
+                writer.WriteValueSafe(0);
                 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);
@@ -3087,7 +3038,7 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
                 // synchronization.
                 writer.Seek(positionBeforeSynchronizing);
                 // We want the size of everything after our size to skip calculation position
-                var size = (ushort)(currentPosition - sizeToSkipCalculationPosition);
+                var size = currentPosition - sizeToSkipCalculationPosition;
                 writer.WriteValueSafe(size);
                 // Write the number of NetworkBehaviours synchronized
                 writer.Seek(networkBehaviourCountPosition);
@@ -3102,26 +3053,25 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
                 var reader = serializer.GetFastBufferReader();
                 try
                 {
-                    reader.ReadValueSafe(out ushort sizeOfSynchronizationData);
+                    reader.ReadValueSafe(out int sizeOfSynchronizationData);
                     seekToEndOfSynchData = reader.Position + sizeOfSynchronizationData;
+
                     // Apply the network variable synchronization data
-                    if (!SetNetworkVariableData(reader, targetClientId))
+                    foreach (var behaviour in ChildNetworkBehaviours)
                     {
-                        reader.Seek(seekToEndOfSynchData);
-                        return;
+                        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++)
                     {
-                        reader.ReadValueSafe(out networkBehaviourId);
+                        reader.ReadValueSafe(out ushort networkBehaviourId);
                         var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
                         networkBehaviour.Synchronize(ref serializer, targetClientId);
                     }