Skip to content

Commit

Permalink
fix: NetworkIdentity component bitmask shifting overflows (#3941)
Browse files Browse the repository at this point in the history 
* Failing test for netid bitmask shifting

* Fix netid bitmask shifting typing

Otherwise it uses ints and overflows when shifting more than 32
  • Loading branch information
@imerr
imerr authored Nov 9, 2024
1 parent 499e4da commit 1187a59
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 13 deletions.
6 changes: 3 additions & 3 deletions Assets/Mirror/Core/NetworkIdentity.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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)
{
Expand All @@ -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;
}

Expand Down
51 changes: 51 additions & 0 deletions Assets/Mirror/Tests/Common/MirrorTest.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<NetworkIdentity> preAwake)
{
go = new GameObject();
identity = go.AddComponent<NetworkIdentity>();
preAwake(identity);
// Awake is only called in play mode.
// call manually for initialization.
identity.Awake();
// track
instantiated.Add(go);
}

// create GameObject + NetworkIdentity + NetworkBehaviour<T>
// add to tracker list if needed (useful for cleanups afterwards)
protected void CreateNetworked<T>(out GameObject go, out NetworkIdentity identity, out T component)
Expand Down Expand Up @@ -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<NetworkIdentity> serverPreAwake,
out GameObject clientGO, out NetworkIdentity clientIdentity, Action<NetworkIdentity> 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<T>(out GameObject go, out NetworkIdentity identity, out T component, NetworkConnectionToClient ownerConnection = null)
Expand Down
138 changes: 128 additions & 10 deletions Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// OnDe/SerializeSafely tests.
using System.Collections.Generic;
using System.Text.RegularExpressions;
using Mirror.Tests.EditorBehaviours.NetworkIdentities;
using NUnit.Framework;
Expand Down Expand Up @@ -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
Expand All @@ -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<SerializeTest1NetworkBehaviour> serverNBs = new List<SerializeTest1NetworkBehaviour>();
List<SerializeTest1NetworkBehaviour> clientNBs = new List<SerializeTest1NetworkBehaviour>();
// 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<SerializeTest1NetworkBehaviour>();
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<SerializeTest1NetworkBehaviour>();
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]
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 1187a59

Please sign in to comment.