From 1187a59b18c95ec20d35c38016557ff7f50a2ce5 Mon Sep 17 00:00:00 2001 From: Robin Rolf Date: Sat, 9 Nov 2024 16:45:35 +0000 Subject: [PATCH] fix: NetworkIdentity component bitmask shifting overflows (#3941) * Failing test for netid bitmask shifting * Fix netid bitmask shifting typing Otherwise it uses ints and overflows when shifting more than 32 --- Assets/Mirror/Core/NetworkIdentity.cs | 6 +- Assets/Mirror/Tests/Common/MirrorTest.cs | 51 +++++++ .../NetworkIdentitySerializationTests.cs | 138 ++++++++++++++++-- 3 files changed, 182 insertions(+), 13 deletions(-) diff --git a/Assets/Mirror/Core/NetworkIdentity.cs b/Assets/Mirror/Core/NetworkIdentity.cs index 17c55cde6..92fbae17a 100644 --- a/Assets/Mirror/Core/NetworkIdentity.cs +++ b/Assets/Mirror/Core/NetworkIdentity.cs @@ -863,7 +863,7 @@ internal void OnStopLocalPlayer() for (int i = 0; i < components.Length; ++i) { NetworkBehaviour component = components[i]; - ulong nthBit = (1u << i); + ulong nthBit = 1ul << i; bool dirty = component.IsDirty(); @@ -910,7 +910,7 @@ ulong ClientDirtyMask() // on client, only consider owned components with SyncDirection to server NetworkBehaviour component = components[i]; - ulong nthBit = (1u << i); + ulong nthBit = 1ul << i; if (isOwned && component.syncDirection == SyncDirection.ClientToServer) { @@ -928,7 +928,7 @@ ulong ClientDirtyMask() [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static bool IsDirty(ulong mask, int index) { - ulong nthBit = (ulong)(1 << index); + ulong nthBit = 1ul << index; return (mask & nthBit) != 0; } diff --git a/Assets/Mirror/Tests/Common/MirrorTest.cs b/Assets/Mirror/Tests/Common/MirrorTest.cs index 41f0ce58f..53fdf6e48 100644 --- a/Assets/Mirror/Tests/Common/MirrorTest.cs +++ b/Assets/Mirror/Tests/Common/MirrorTest.cs @@ -1,4 +1,5 @@ // base class for networking tests to make things easier. +using System; using System.Collections.Generic; using System.Linq; using NUnit.Framework; @@ -81,6 +82,18 @@ protected void CreateNetworked(out GameObject go, out NetworkIdentity identity) instantiated.Add(go); } + protected void CreateNetworked(out GameObject go, out NetworkIdentity identity, Action preAwake) + { + go = new GameObject(); + identity = go.AddComponent(); + preAwake(identity); + // Awake is only called in play mode. + // call manually for initialization. + identity.Awake(); + // track + instantiated.Add(go); + } + // create GameObject + NetworkIdentity + NetworkBehaviour // add to tracker list if needed (useful for cleanups afterwards) protected void CreateNetworked(out GameObject go, out NetworkIdentity identity, out T component) @@ -269,6 +282,44 @@ protected void CreateNetworkedAndSpawn( Assert.That(NetworkClient.spawned.ContainsKey(serverIdentity.netId)); } + // create GameObject + NetworkIdentity + NetworkBehaviour & SPAWN + // => preAwake callbacks can be used to add network behaviours to the NI + // => ownerConnection can be NetworkServer.localConnection if needed. + // => returns objects from client and from server. + // will be same in host mode. + protected void CreateNetworkedAndSpawn( + out GameObject serverGO, out NetworkIdentity serverIdentity, Action serverPreAwake, + out GameObject clientGO, out NetworkIdentity clientIdentity, Action clientPreAwake, + NetworkConnectionToClient ownerConnection = null) + { + // server & client need to be active before spawning + Debug.Assert(NetworkClient.active, "NetworkClient needs to be active before spawning."); + Debug.Assert(NetworkServer.active, "NetworkServer needs to be active before spawning."); + + // create one on server, one on client + // (spawning has to find it on client, it doesn't create it) + CreateNetworked(out serverGO, out serverIdentity, serverPreAwake); + CreateNetworked(out clientGO, out clientIdentity, clientPreAwake); + + // give both a scene id and register it on client for spawnables + clientIdentity.sceneId = serverIdentity.sceneId = (ulong)serverGO.GetHashCode(); + NetworkClient.spawnableObjects[clientIdentity.sceneId] = clientIdentity; + + // spawn + NetworkServer.Spawn(serverGO, ownerConnection); + ProcessMessages(); + + // double check isServer/isClient. avoids debugging headaches. + Assert.That(serverIdentity.isServer, Is.True); + Assert.That(clientIdentity.isClient, Is.True); + + // double check that we have authority if we passed an owner connection + if (ownerConnection != null) + Debug.Assert(clientIdentity.isOwned == true, $"Behaviour Had Wrong Authority when spawned, This means that the test is broken and will give the wrong results"); + + // make sure the client really spawned it. + Assert.That(NetworkClient.spawned.ContainsKey(serverIdentity.netId)); + } // create GameObject + NetworkIdentity + NetworkBehaviour & SPAWN // => ownerConnection can be NetworkServer.localConnection if needed. protected void CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity identity, out T component, NetworkConnectionToClient ownerConnection = null) diff --git a/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs b/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs index 9b8eab08e..f281ddf4c 100644 --- a/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs +++ b/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs @@ -1,4 +1,5 @@ // OnDe/SerializeSafely tests. +using System.Collections.Generic; using System.Text.RegularExpressions; using Mirror.Tests.EditorBehaviours.NetworkIdentities; using NUnit.Framework; @@ -42,7 +43,7 @@ public void SerializeAndDeserializeAll() ); // set sync modes - serverOwnerComp.syncMode = clientOwnerComp.syncMode = SyncMode.Owner; + serverOwnerComp.syncMode = clientOwnerComp.syncMode = SyncMode.Owner; serverObserversComp.syncMode = clientObserversComp.syncMode = SyncMode.Observers; // set unique values on server components @@ -65,10 +66,127 @@ public void SerializeAndDeserializeAll() // deserialize client object with OBSERVERS payload reader = new NetworkReader(observersWriter.ToArray()); clientIdentity.DeserializeClient(reader, true); - Assert.That(clientOwnerComp.value, Is.EqualTo(null)); // owner mode shouldn't be in data + Assert.That(clientOwnerComp.value, Is.EqualTo(null)); // owner mode shouldn't be in data Assert.That(clientObserversComp.value, Is.EqualTo(42)); // observers mode should be in data } + // test serialize -> deserialize of any supported number of components + [Test] + public void SerializeAndDeserializeN([NUnit.Framework.Range(1, 64)] int numberOfNBs) + { + List serverNBs = new List(); + List clientNBs = new List(); + // need two of both versions so we can serialize -> deserialize + CreateNetworkedAndSpawn( + out _, out NetworkIdentity serverIdentity, ni => + { + for (int i = 0; i < numberOfNBs; i++) + { + SerializeTest1NetworkBehaviour nb = ni.gameObject.AddComponent(); + nb.syncInterval = 0; + nb.syncMode = SyncMode.Observers; + serverNBs.Add(nb); + } + }, + out _, out NetworkIdentity clientIdentity, ni => + { + for (int i = 0; i < numberOfNBs; i++) + { + SerializeTest1NetworkBehaviour nb = ni.gameObject.AddComponent(); + nb.syncInterval = 0; + nb.syncMode = SyncMode.Observers; + clientNBs.Add(nb); + } + } + ); + + // INITIAL SYNC + // set unique values on server components + for (int i = 0; i < serverNBs.Count; i++) + { + serverNBs[i].value = (i + 1) * 3; + serverNBs[i].SetDirty(); + } + + // serialize server object + serverIdentity.SerializeServer(true, ownerWriter, observersWriter); + + // deserialize client object with OBSERVERS payload + NetworkReader reader = new NetworkReader(observersWriter.ToArray()); + clientIdentity.DeserializeClient(reader, true); + for (int i = 0; i < clientNBs.Count; i++) + { + int expected = (i + 1) * 3; + Assert.That(clientNBs[i].value, Is.EqualTo(expected), $"Expected the clientNBs[{i}] to have a value of {expected}"); + } + + // clear dirty bits for incremental sync + foreach (SerializeTest1NetworkBehaviour serverNB in serverNBs) + serverNB.ClearAllDirtyBits(); + + // INCREMENTAL SYNC ALL + // set unique values on server components + for (int i = 0; i < serverNBs.Count; i++) + { + serverNBs[i].value = (i + 1) * 11; + serverNBs[i].SetDirty(); + } + + ownerWriter.Reset(); + observersWriter.Reset(); + // serialize server object + serverIdentity.SerializeServer(false, ownerWriter, observersWriter); + + // deserialize client object with OBSERVERS payload + reader = new NetworkReader(observersWriter.ToArray()); + clientIdentity.DeserializeClient(reader, false); + for (int i = 0; i < clientNBs.Count; i++) + { + int expected = (i + 1) * 11; + Assert.That(clientNBs[i].value, Is.EqualTo(expected), $"Expected the clientNBs[{i}] to have a value of {expected}"); + } + + // clear dirty bits for incremental sync + foreach (SerializeTest1NetworkBehaviour serverNB in serverNBs) + serverNB.ClearAllDirtyBits(); + + // INCREMENTAL SYNC INDIVIDUAL + for (int i = 0; i < numberOfNBs; i++) + { + // reset all client nbs + foreach (SerializeTest1NetworkBehaviour clientNB in clientNBs) + clientNB.value = 0; + + int expected = (i + 1) * 7; + + // set unique value on server components + serverNBs[i].value = expected; + serverNBs[i].SetDirty(); + + ownerWriter.Reset(); + observersWriter.Reset(); + // serialize server object + serverIdentity.SerializeServer(false, ownerWriter, observersWriter); + + // deserialize client object with OBSERVERS payload + reader = new NetworkReader(observersWriter.ToArray()); + clientIdentity.DeserializeClient(reader, false); + for (int index = 0; index < clientNBs.Count; index++) + { + SerializeTest1NetworkBehaviour clientNB = clientNBs[index]; + if (index == i) + { + Assert.That(clientNB.value, Is.EqualTo(expected), $"Expected the clientNBs[{index}] to have a value of {expected}"); + } + else + { + Assert.That(clientNB.value, Is.EqualTo(0), $"Expected the clientNBs[{index}] to have a value of 0 since we're not syncing that index (on sync of #{i})"); + } + } + } + } + + // serialization should work even if a component throws an exception. // so if first component throws, second should still be serialized fine. [Test] @@ -150,20 +268,20 @@ public void TooManyComponents() public void ErrorCorrection() { int original = 0x12345678; - byte safety = 0x78; // last byte + byte safety = 0x78; // last byte // correct size shouldn't be corrected - Assert.That(NetworkBehaviour.ErrorCorrection(original + 0, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original + 0, safety), Is.EqualTo(original)); // read a little too much - Assert.That(NetworkBehaviour.ErrorCorrection(original + 1, safety), Is.EqualTo(original)); - Assert.That(NetworkBehaviour.ErrorCorrection(original + 2, safety), Is.EqualTo(original)); - Assert.That(NetworkBehaviour.ErrorCorrection(original + 42, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original + 1, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original + 2, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original + 42, safety), Is.EqualTo(original)); // read a little too less - Assert.That(NetworkBehaviour.ErrorCorrection(original - 1, safety), Is.EqualTo(original)); - Assert.That(NetworkBehaviour.ErrorCorrection(original - 2, safety), Is.EqualTo(original)); - Assert.That(NetworkBehaviour.ErrorCorrection(original - 42, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original - 1, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original - 2, safety), Is.EqualTo(original)); + Assert.That(NetworkBehaviour.ErrorCorrection(original - 42, safety), Is.EqualTo(original)); // reading way too much / less is expected to fail. // we can only correct the last byte, not more.