fix: NetworkBehaviour and NetworkVariableDelta length safety checks (#3405)

<!-- Replace this block with what this PR does and why. Describe what
you'd like reviewers to know, how you applied the engineering
principles, and any interesting tradeoffs made. Delete bullet points
below that don't apply, and update the changelog section as appropriate.
-->

<!-- Add short version of the JIRA ticket to the PR title (e.g. "feat:
new shiny feature [MTT-123]") -->
[MTTB-1080](https://jira.unity3d.com/browse/MTTB-1080)

fixes: #3335 

## Changelog

- Fixed: NetworkBehaviour length safety checks use `int` instead of
`ushort`
- Fixed: NetworkVariable `EnsureNetworkVariableLengthSafety` checks use
`int` instead of `ushort`
- Fixed: NetworkVariableDeltas did not work with
`DistributedAuthorityMode` and `EnsureNetworkVariableLengthSafety`

## Testing and Documentation

- Includes unit tests.
- No documentation changes or additions were necessary.

<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

## Backport

<!-- If this is a backport:
 - Add the following to the PR title: "\[Backport\] ..." .
 - Link to the original PR.
If this needs a backport - state this here
If a backport is not needed please provide the reason why.
If the "Backports" section is not present it will lead to a CI test
failure.
-->

Backported in #3415

---------

Co-authored-by: Noel Stephens <[email protected]>

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,6 +14,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issues with the `NetworkBehaviour` and `NetworkVariable` length safety checks. (#3405)
 - Fixed memory leaks when domain reload is disabled. (#3427)
 - Fixed an exception being thrown when unregistering a custom message handler from within the registered callback. (#3417)
 

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();
             }