From af995b8c038d9ce510c11de59763e8ef8991d5fd Mon Sep 17 00:00:00 2001 From: Emma Date: Mon, 14 Apr 2025 18:48:15 -0400 Subject: [PATCH 1/4] fix: NetworkBehaviour and NetworkVariableDelta length safety checks --- .../Runtime/Configuration/SessionConfig.cs | 3 +- .../Runtime/Core/NetworkBehaviour.cs | 91 ++++------- .../Runtime/Core/NetworkBehaviourUpdater.cs | 8 +- .../Runtime/Core/NetworkObject.cs | 92 +++-------- .../Messages/NetworkVariableDeltaMessage.cs | 152 ++++-------------- .../NetworkVariable/NetworkVariableBase.cs | 6 +- .../Runtime/NetcodeIntegrationTest.cs | 6 +- .../DistributedAuthorityCodecTests.cs | 16 ++ .../NetworkVariable/NetworkVariableTests.cs | 35 +++- .../NetworkVariableTestsHelperTypes.cs | 4 +- 10 files changed, 144 insertions(+), 269 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Configuration/SessionConfig.cs b/com.unity.netcode.gameobjects/Runtime/Configuration/SessionConfig.cs index 9a4ed0a931..aabc18dfc1 100644 --- a/com.unity.netcode.gameobjects/Runtime/Configuration/SessionConfig.cs +++ b/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; diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 7113917944..df1648dea4 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/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(ref BufferSerializer 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(ref BufferSerializer 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(ref BufferSerializer 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; diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs index b2623cb389..105f203c86 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs +++ b/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(); } diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index 502ffefcb6..b4c0428cdb 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1961,7 +1961,11 @@ public void SpawnAsPlayerObject(ulong clientId, bool destroyWithScene = false) /// (true) the will be destroyed (false) the will persist after being despawned 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 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); - } - } - /// /// 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() } } - /// - /// Only invoked during first synchronization of a NetworkObject (late join or newly spawned) - /// - 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; - } - /// /// Gets the order index of a NetworkBehaviour instance within the ChildNetworkBehaviours collection /// @@ -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); - } - } - /// /// Handles synchronizing NetworkVariables and custom synchronization data for NetworkBehaviours. /// @@ -3059,13 +3003,20 @@ internal void SynchronizeNetworkBehaviours(ref BufferSerializer 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(ref BufferSerializer 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(ref BufferSerializer 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); } diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs index 8db084cd7d..05ae29b32e 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs @@ -50,7 +50,7 @@ internal struct NetworkVariableDeltaMessage : INetworkMessage private List m_UpdatedNetworkVariables; [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void WriteNetworkVariable(ref FastBufferWriter writer, ref NetworkVariableBase networkVariable, bool distributedAuthorityMode, bool ensureNetworkVariableLengthSafety, int nonfragmentedSize, int fragmentedSize) + private void WriteNetworkVariable(ref FastBufferWriter writer, ref NetworkVariableBase networkVariable, bool ensureNetworkVariableLengthSafety, int nonfragmentedSize, int fragmentedSize) { if (ensureNetworkVariableLengthSafety) { @@ -67,27 +67,7 @@ private void WriteNetworkVariable(ref FastBufferWriter writer, ref NetworkVariab } else { - // TODO: Determine if we need to remove this with the 6.1 service updates - if (distributedAuthorityMode) - { - var size_marker = writer.Position; - writer.WriteValueSafe(0); - var start_marker = writer.Position; - networkVariable.WriteDelta(writer); - var end_marker = writer.Position; - writer.Seek(size_marker); - var size = end_marker - start_marker; - if (size == 0) - { - UnityEngine.Debug.LogError($"Invalid write size of zero!"); - } - writer.WriteValueSafe((ushort)size); - writer.Seek(end_marker); - } - else - { - networkVariable.WriteDelta(writer); - } + networkVariable.WriteDelta(writer); } } @@ -104,7 +84,6 @@ public void Serialize(FastBufferWriter writer, int targetVersion) var nonFragmentedMessageMaxSize = networkManager.MessageManager.NonFragmentedMessageMaxSize; var fragmentedMessageMaxSize = networkManager.MessageManager.FragmentedMessageMaxSize; var ensureNetworkVariableLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety; - var distributedAuthorityMode = networkManager.DistributedAuthorityMode; BytePacker.WriteValueBitPacked(writer, NetworkObjectId); BytePacker.WriteValueBitPacked(writer, NetworkBehaviourIndex); @@ -117,31 +96,17 @@ public void Serialize(FastBufferWriter writer, int targetVersion) // If we are forwarding the message, then proceed to forward state updates specific to the targeted client if (m_ForwardingMessage) { - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) - { - writer.WriteValueSafe((ushort)NetworkBehaviour.NetworkVariableFields.Count); - } - for (int i = 0; i < NetworkBehaviour.NetworkVariableFields.Count; i++) { var startingSize = writer.Length; var networkVariable = NetworkBehaviour.NetworkVariableFields[i]; var shouldWrite = m_ForwardUpdates[TargetClientId].Contains(i); - // This var does not belong to the currently iterating delivery group. - if (distributedAuthorityMode) - { - if (!shouldWrite) - { - writer.WriteValueSafe(0); - } - } - else if (ensureNetworkVariableLengthSafety) + if (ensureNetworkVariableLengthSafety) { if (!shouldWrite) { - BytePacker.WriteValueBitPacked(writer, (ushort)0); + BytePacker.WriteValueBitPacked(writer, 0); } } else @@ -151,7 +116,7 @@ public void Serialize(FastBufferWriter writer, int targetVersion) if (shouldWrite) { - WriteNetworkVariable(ref writer, ref networkVariable, distributedAuthorityMode, ensureNetworkVariableLengthSafety, nonFragmentedMessageMaxSize, fragmentedMessageMaxSize); + WriteNetworkVariable(ref writer, ref networkVariable, ensureNetworkVariableLengthSafety, nonFragmentedMessageMaxSize, fragmentedMessageMaxSize); networkManager.NetworkMetrics.TrackNetworkVariableDeltaSent(TargetClientId, obj, networkVariable.Name, typeName, writer.Length - startingSize); } } @@ -159,22 +124,12 @@ public void Serialize(FastBufferWriter writer, int targetVersion) } } - // DANGO TODO: Remove this when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) - { - writer.WriteValueSafe((ushort)NetworkBehaviour.NetworkVariableFields.Count); - } for (int i = 0; i < NetworkBehaviour.NetworkVariableFields.Count; i++) { if (!DeliveryMappedNetworkVariableIndex.Contains(i)) { - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) - { - writer.WriteValueSafe(0); - } - else if (ensureNetworkVariableLengthSafety) + if (ensureNetworkVariableLengthSafety) { BytePacker.WriteValueBitPacked(writer, (ushort)0); } @@ -211,19 +166,11 @@ public void Serialize(FastBufferWriter writer, int targetVersion) shouldWrite = false; } - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) + if (ensureNetworkVariableLengthSafety) { if (!shouldWrite) { - writer.WriteValueSafe(0); - } - } - else if (ensureNetworkVariableLengthSafety) - { - if (!shouldWrite) - { - BytePacker.WriteValueBitPacked(writer, (ushort)0); + BytePacker.WriteValueBitPacked(writer, 0); } } else @@ -233,7 +180,7 @@ public void Serialize(FastBufferWriter writer, int targetVersion) if (shouldWrite) { - WriteNetworkVariable(ref writer, ref networkVariable, distributedAuthorityMode, ensureNetworkVariableLengthSafety, nonFragmentedMessageMaxSize, fragmentedMessageMaxSize); + WriteNetworkVariable(ref writer, ref networkVariable, ensureNetworkVariableLengthSafety, nonFragmentedMessageMaxSize, fragmentedMessageMaxSize); networkManager.NetworkMetrics.TrackNetworkVariableDeltaSent(TargetClientId, obj, networkVariable.Name, typeName, writer.Length - startingSize); } } @@ -260,7 +207,6 @@ public void Handle(ref NetworkContext context) if (networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out NetworkObject networkObject)) { - var distributedAuthorityMode = networkManager.DistributedAuthorityMode; var ensureNetworkVariableLengthSafety = networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety; var networkBehaviour = networkObject.GetNetworkBehaviourAtOrderIndex(NetworkBehaviourIndex); var isServerAndDeltaForwarding = m_ReceivedMessageVersion >= k_ServerDeltaForwardingAndNetworkDelivery && networkManager.IsServer; @@ -276,16 +222,6 @@ public void Handle(ref NetworkContext context) } else { - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) - { - m_ReceivedNetworkVariableData.ReadValueSafe(out ushort variableCount); - if (variableCount != networkBehaviour.NetworkVariableFields.Count) - { - UnityEngine.Debug.LogError("Variable count mismatch"); - } - } - // (For client-server) As opposed to worrying about adding additional processing on the server to send NetworkVariable // updates at the end of the frame, we now track all NetworkVariable state updates, per client, that need to be forwarded // to the client. This creates a list of all remaining connected clients that could have updates applied. @@ -305,24 +241,13 @@ public void Handle(ref NetworkContext context) // Update NetworkVariable Fields for (int i = 0; i < networkBehaviour.NetworkVariableFields.Count; i++) { - int varSize = 0; + int expectedBytesToRead = 0; var networkVariable = networkBehaviour.NetworkVariableFields[i]; - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode) + if (ensureNetworkVariableLengthSafety) { - m_ReceivedNetworkVariableData.ReadValueSafe(out ushort variableSize); - varSize = variableSize; - - if (varSize == 0) - { - continue; - } - } - else if (ensureNetworkVariableLengthSafety) - { - ByteUnpacker.ReadValueBitPacked(m_ReceivedNetworkVariableData, out varSize); - if (varSize == 0) + ByteUnpacker.ReadValueBitPacked(m_ReceivedNetworkVariableData, out expectedBytesToRead); + if (expectedBytesToRead == 0) { continue; } @@ -339,7 +264,7 @@ public void Handle(ref NetworkContext context) if (networkManager.IsServer && !networkVariable.CanClientWrite(context.SenderId)) { // we are choosing not to fire an exception here, because otherwise a malicious client could use this to crash the server - if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety) + if (ensureNetworkVariableLengthSafety) { if (NetworkLog.CurrentLogLevel <= LogLevel.Developer) { @@ -347,7 +272,7 @@ public void Handle(ref NetworkContext context) NetworkLog.LogError($"[{networkVariable.GetType().Name}]"); } - m_ReceivedNetworkVariableData.Seek(m_ReceivedNetworkVariableData.Position + varSize); + m_ReceivedNetworkVariableData.Seek(m_ReceivedNetworkVariableData.Position + expectedBytesToRead); continue; } @@ -365,15 +290,14 @@ public void Handle(ref NetworkContext context) } return; } - int readStartPos = m_ReceivedNetworkVariableData.Position; + var readStartPos = m_ReceivedNetworkVariableData.Position; - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode || ensureNetworkVariableLengthSafety) + if (ensureNetworkVariableLengthSafety) { - var remainingBufferSize = m_ReceivedNetworkVariableData.Length - m_ReceivedNetworkVariableData.Position; - if (varSize > (remainingBufferSize)) + var remainingBufferSize = m_ReceivedNetworkVariableData.Length - readStartPos; + if (expectedBytesToRead > remainingBufferSize) { - UnityEngine.Debug.LogError($"[{networkBehaviour.name}][Delta State Read Error] Expecting to read {varSize} but only {remainingBufferSize} remains!"); + UnityEngine.Debug.LogError($"[{networkBehaviour.name}][Delta State Read Error] Expecting to read {expectedBytesToRead} but only {remainingBufferSize} remains!"); return; } } @@ -393,6 +317,19 @@ public void Handle(ref NetworkContext context) return; } + if (ensureNetworkVariableLengthSafety) + { + var totalBytesRead = m_ReceivedNetworkVariableData.Position - readStartPos; + if (totalBytesRead != expectedBytesToRead) + { + if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) + { + NetworkLog.LogWarning($"[{nameof(NetworkObjectId)}: {NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}][Delta State Read] NetworkVariable read {totalBytesRead} bytes but was expected to read {expectedBytesToRead} bytes!"); + } + m_ReceivedNetworkVariableData.Seek(readStartPos + expectedBytesToRead); + } + } + // (For client-server) As opposed to worrying about adding additional processing on the server to send NetworkVariable // updates at the end of the frame, we now track all NetworkVariable state updates, per client, that need to be forwarded // to the client. This happens once the server is finished processing all state updates for this message. @@ -422,29 +359,6 @@ public void Handle(ref NetworkContext context) networkVariable.Name, networkBehaviour.__getTypeName(), context.MessageSize); - - // DANGO TODO: Remove distributedAuthorityMode portion when we remove the service specific NetworkVariable stuff - if (distributedAuthorityMode || ensureNetworkVariableLengthSafety) - { - if (m_ReceivedNetworkVariableData.Position > (readStartPos + varSize)) - { - if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) - { - NetworkLog.LogWarning($"Var delta read too far. {m_ReceivedNetworkVariableData.Position - (readStartPos + varSize)} bytes. => {nameof(NetworkObjectId)}: {NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {networkObject.GetNetworkBehaviourOrderIndex(networkBehaviour)} - VariableIndex: {i}"); - } - - m_ReceivedNetworkVariableData.Seek(readStartPos + varSize); - } - else if (m_ReceivedNetworkVariableData.Position < (readStartPos + varSize)) - { - if (NetworkLog.CurrentLogLevel <= LogLevel.Normal) - { - NetworkLog.LogWarning($"Var delta read too little. {readStartPos + varSize - m_ReceivedNetworkVariableData.Position} bytes. => {nameof(NetworkObjectId)}: {NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {networkObject.GetNetworkBehaviourOrderIndex(networkBehaviour)} - VariableIndex: {i}"); - } - - m_ReceivedNetworkVariableData.Seek(readStartPos + varSize); - } - } } // If we are using the version of this message that includes network delivery, then diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs index 6b97ce90e3..94871ab329 100644 --- a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs +++ b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs @@ -434,12 +434,16 @@ internal virtual void PostDeltaRead() } /// + /// 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. + ///

/// There are scenarios, specifically with collections, where a client could be synchronizing and /// some NetworkVariables have pending updates. To avoid duplicating entries, this is invoked only /// when sending the full synchronization information. /// /// - /// Derrived classes should send the previous value for synchronization so when the updated value + /// Derived classes should send the previous value for synchronization so when the updated value /// is sent (after synchronizing the client) it will apply the updates. /// /// diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs index b518f26583..78beb129ef 100644 --- a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs @@ -1555,11 +1555,7 @@ private bool CheckClientsConnected(NetworkManager[] clientsToCheck) /// An array of clients to be checked protected bool WaitForClientsConnectedOrTimeOutWithTimeTravel(NetworkManager[] clientsToCheck) { - var remoteClientCount = clientsToCheck.Length; - var serverClientCount = m_ServerNetworkManager.IsHost ? remoteClientCount + 1 : remoteClientCount; - - return WaitForConditionOrTimeOutWithTimeTravel(() => clientsToCheck.Where((c) => c.IsConnectedClient).Count() == remoteClientCount && - m_ServerNetworkManager.ConnectedClients.Count == serverClientCount); + return WaitForConditionOrTimeOutWithTimeTravel(() => CheckClientsConnected(clientsToCheck)); } /// diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs index af69df9aa8..e16b344544 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs @@ -14,6 +14,12 @@ namespace Unity.Netcode.RuntimeTests { + internal enum NetworkConfigOptions + { + EnsureVariableLengthSafety, + Default, + } + /// /// This class tests the NGO message codec between the C# SDK and the Rust runtime. /// @@ -28,6 +34,8 @@ namespace Unity.Netcode.RuntimeTests /// The default behaviour when unity fails to connect to the echo-server is to ignore all tests in this class. /// This can be overridden by setting the environment variable "ENSURE_CODEC_TESTS" to any value - then the tests will fail. /// + [TestFixture(NetworkConfigOptions.EnsureVariableLengthSafety)] + [TestFixture(NetworkConfigOptions.Default)] internal class DistributedAuthorityCodecTests : NetcodeIntegrationTest { protected override int NumberOfClients => 1; @@ -45,6 +53,13 @@ internal class DistributedAuthorityCodecTests : NetcodeIntegrationTest private static readonly ushort k_TransportPort = GetPortToBind(); private const int k_ClientId = 0; + private bool m_EnsureVariableLengthSafety; + + public DistributedAuthorityCodecTests(NetworkConfigOptions configOptions) + { + m_EnsureVariableLengthSafety = configOptions == NetworkConfigOptions.EnsureVariableLengthSafety; + } + /// /// Configures the port to look for the rust echo-server. /// @@ -104,6 +119,7 @@ protected override void OnServerAndClientsCreated() Client.NetworkConfig.NetworkTransport = utpTransport; Client.NetworkConfig.EnableSceneManagement = false; Client.NetworkConfig.AutoSpawnPlayerPrefabClientSide = true; + Client.NetworkConfig.EnsureNetworkVariableLengthSafety = m_EnsureVariableLengthSafety; utpTransport.ConnectionData.Address = Dns.GetHostAddresses(m_TransportHost).First().ToString(); utpTransport.ConnectionData.Port = k_TransportPort; Client.LogLevel = LogLevel.Developer; diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs index 35718a474a..f2afc79730 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTests.cs @@ -396,10 +396,16 @@ public IEnumerator ModifyNetworkVariableOrListOnNetworkDespawn() } } + internal enum Serialization + { + EnsureLengthSafety, + Default, + } + #if !MULTIPLAYER_TOOLS - [TestFixture(true)] + [TestFixture(Serialization.EnsureLengthSafety)] #endif - [TestFixture(false)] + [TestFixture(Serialization.Default)] internal class NetworkVariableTests : NetcodeIntegrationTest { private const string k_StringTestValue = "abcdefghijklmnopqrstuvwxyz"; @@ -432,9 +438,9 @@ public static void ClientNetworkVariableTestSpawned(NetworkVariableTest networkV private readonly bool m_EnsureLengthSafety; - public NetworkVariableTests(bool ensureLengthSafety) + public NetworkVariableTests(Serialization serialization) { - m_EnsureLengthSafety = ensureLengthSafety; + m_EnsureLengthSafety = serialization == Serialization.EnsureLengthSafety; } protected override bool CanStartServerAndClients() @@ -467,15 +473,21 @@ private void InitializeServerAndClients(HostOrServer useHost) m_PlayerPrefab.AddComponent(); m_PlayerPrefab.AddComponent(); + m_DistributedAuthority = useHost == HostOrServer.DAHost; + m_NetworkTopologyType = m_DistributedAuthority ? NetworkTopologyTypes.DistributedAuthority : NetworkTopologyTypes.ClientServer; + m_ServerNetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety = m_EnsureLengthSafety; m_ServerNetworkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab; + SetDistributedAuthorityProperties(m_ServerNetworkManager); foreach (var client in m_ClientNetworkManagers) { client.NetworkConfig.EnsureNetworkVariableLengthSafety = m_EnsureLengthSafety; client.NetworkConfig.PlayerPrefab = m_PlayerPrefab; + SetDistributedAuthorityProperties(client); } - Assert.True(NetcodeIntegrationTestHelpers.Start(useHost == HostOrServer.Host, m_ServerNetworkManager, m_ClientNetworkManagers), "Failed to start server and client instances"); + var useServer = useHost == HostOrServer.Server; + Assert.True(NetcodeIntegrationTestHelpers.Start(!useServer, m_ServerNetworkManager, m_ClientNetworkManagers), "Failed to start server and client instances"); RegisterSceneManagerHandler(); @@ -491,7 +503,7 @@ private void InitializeServerAndClients(HostOrServer useHost) m_ServerNetworkManager, result); // Assign server-side client's player - m_Player1OnServer = result.Result.GetComponent(); + var serverVersionOfPlayer = result.Result.GetComponent(); // This is client1's view of itself NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentationWithTimeTravel( @@ -499,7 +511,14 @@ private void InitializeServerAndClients(HostOrServer useHost) m_ClientNetworkManagers[0], result); // Assign client-side local player - m_Player1OnClient1 = result.Result.GetComponent(); + var clientVersionOfPlayer = result.Result.GetComponent(); + + // In distributed authority mode, the client is the authority of itself, rather than the server. + var authority = m_DistributedAuthority ? clientVersionOfPlayer : serverVersionOfPlayer; + var nonAuthority = m_DistributedAuthority ? serverVersionOfPlayer : clientVersionOfPlayer; + + m_Player1OnServer = authority; + m_Player1OnClient1 = nonAuthority; m_Player1OnServer.TheList.Clear(); @@ -512,7 +531,7 @@ private void InitializeServerAndClients(HostOrServer useHost) throw new Exception("at least one client network container not empty at start"); } - var instanceCount = useHost == HostOrServer.Host ? NumberOfClients * 3 : NumberOfClients * 2; + var instanceCount = useHost == HostOrServer.Server ? NumberOfClients * 2 : NumberOfClients * 3; // Wait for the client-side to notify it is finished initializing and spawning. success = WaitForConditionOrTimeOutWithTimeTravel(() => s_ClientNetworkVariableTestInstances.Count == instanceCount); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTestsHelperTypes.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTestsHelperTypes.cs index f50811ba1f..698a47e49f 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTestsHelperTypes.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableTestsHelperTypes.cs @@ -889,12 +889,12 @@ internal class NetVarILPPClassForTests : NetworkBehaviour internal class TemplateNetworkBehaviourType : NetworkBehaviour { - public NetworkVariable TheVar; + public NetworkVariable TheVar = new NetworkVariable(); } internal class IntermediateNetworkBehavior : TemplateNetworkBehaviourType { - public NetworkVariable TheVar2; + public NetworkVariable TheVar2 = new NetworkVariable(); } #if !NGO_MINIMALPROJECT internal class ClassHavingNetworkBehaviour : IntermediateNetworkBehavior From 4734adee0080fa1080f5b3ca0acce3b99808c051 Mon Sep 17 00:00:00 2001 From: Emma Date: Mon, 14 Apr 2025 18:54:39 -0400 Subject: [PATCH 2/4] update CHANGELOG.md --- com.unity.netcode.gameobjects/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 93ed4d1732..ed4b767488 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -8,11 +8,12 @@ Additional documentation and release notes are available at [Multiplayer Documen ## [Unreleased] -### Added +### Added ### Fixed +- Fixed issues with the `NetworkBehaviour` and `NetworkVariable` length safety checks. (#3405) ### Changed From fc200af8f31784c4a5c3dd5fad817f24ecbd1412 Mon Sep 17 00:00:00 2001 From: Emma Date: Tue, 15 Apr 2025 09:53:48 -0400 Subject: [PATCH 3/4] Ensure scene object deserializaion also uses an int --- com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index b4c0428cdb..dad2add587 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -2909,6 +2909,7 @@ public void Serialize(FastBufferWriter writer) var writeSize = 0; writeSize += HasTransform ? FastBufferWriter.GetWriteSize() : 0; writeSize += FastBufferWriter.GetWriteSize(); + Debug.Log($"writeSize: {writeSize}"); if (!writer.TryBeginWrite(writeSize)) { @@ -2934,6 +2935,7 @@ public void Serialize(FastBufferWriter writer) // Synchronize NetworkVariables and NetworkBehaviours var bufferSerializer = new BufferSerializer(new BufferSerializerWriter(writer)); OwnerObject.SynchronizeNetworkBehaviours(ref bufferSerializer, TargetClientId); + Debug.Log("synchronized network behaviours"); } public void Deserialize(FastBufferReader reader) @@ -2974,6 +2976,7 @@ public void Deserialize(FastBufferReader reader) var readSize = 0; readSize += HasTransform ? FastBufferWriter.GetWriteSize() : 0; readSize += FastBufferWriter.GetWriteSize(); + Debug.Log($"readSize: {readSize}"); // Try to begin reading the remaining bytes if (!reader.TryBeginRead(readSize)) @@ -3186,7 +3189,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) From 016058d94ac88d61f3b9775ccd3f9ee9b6575d3b Mon Sep 17 00:00:00 2001 From: Emma Date: Tue, 15 Apr 2025 10:05:31 -0400 Subject: [PATCH 4/4] remove log lines --- com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index dad2add587..b684f5b743 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -2909,7 +2909,6 @@ public void Serialize(FastBufferWriter writer) var writeSize = 0; writeSize += HasTransform ? FastBufferWriter.GetWriteSize() : 0; writeSize += FastBufferWriter.GetWriteSize(); - Debug.Log($"writeSize: {writeSize}"); if (!writer.TryBeginWrite(writeSize)) { @@ -2935,7 +2934,6 @@ public void Serialize(FastBufferWriter writer) // Synchronize NetworkVariables and NetworkBehaviours var bufferSerializer = new BufferSerializer(new BufferSerializerWriter(writer)); OwnerObject.SynchronizeNetworkBehaviours(ref bufferSerializer, TargetClientId); - Debug.Log("synchronized network behaviours"); } public void Deserialize(FastBufferReader reader) @@ -2976,7 +2974,6 @@ public void Deserialize(FastBufferReader reader) var readSize = 0; readSize += HasTransform ? FastBufferWriter.GetWriteSize() : 0; readSize += FastBufferWriter.GetWriteSize(); - Debug.Log($"readSize: {readSize}"); // Try to begin reading the remaining bytes if (!reader.TryBeginRead(readSize))