Skip to content

fix: [Backport] NetworkBehaviour and NetworkVariable length safety checks #3415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

fix: [Backport] NetworkBehaviour and NetworkVariable length safety checks #3415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
458fc99
fix: NetworkBehaviour and NetworkVariable length safety checks
EmandM Apr 18, 2025
ac521a8
Update CHANGELOG.md
EmandM Apr 18, 2025
7d024af
use int for NetworkBehaviour size
EmandM Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Fixed

- Fixed issues with the `NetworkBehaviour` and `NetworkVariable` length safety checks. (#3415)
- Fixed issue where during a `NetworkObject`'s spawn if you instantiated, spawned, and parented another network prefab under the currently spawning `NetworkObject` the parenting message would not properly defer until the parent `NetworkObject` was spawned. (#3403)
- Fixed issue where in-scene placed `NetworkObjects` could fail to synchronize its transform properly (especially without a `NetworkTransform`) if their parenting changes from the default when the scene is loaded and if the same scene remains loaded between network sessions while the parenting is completely different from the original hierarchy. (#3388)
- Fixed an issue in `UnityTransport` where the transport would accept sends on invalid connections, leading to a useless memory allocation and confusing error message. (#3383)
Expand Down
72 changes: 21 additions & 51 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,49 +1112,33 @@ internal void MarkOwnerReadVariablesDirty()
/// </remarks>
internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
{
if (NetworkVariableFields.Count == 0)
foreach (var field in NetworkVariableFields)
{
return;
}

for (int j = 0; j < NetworkVariableFields.Count; j++)
{

if (NetworkVariableFields[j].CanClientRead(targetClientId))
if (field.CanClientRead(targetClientId))
{
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
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);
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);
var size = writer.Position - startPos;
writer.Seek(writePos);
writer.WriteValueSafe((ushort)size);
writer.WriteValueSafe(size);
writer.Seek(startPos + size);
}
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 // Only if EnsureNetworkVariableLengthSafety, otherwise just skip
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
writer.WriteValueSafe((ushort)0);
writer.WriteValueSafe(0);
}
}
}
Expand All @@ -1169,51 +1153,37 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
/// </remarks>
internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
{
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;
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
reader.ReadValueSafe(out varSize);
if (varSize == 0)
reader.ReadValueSafe(out expectedBytesToRead);
if (expectedBytesToRead == 0)
{
continue;
}
readStartPos = reader.Position;
}
else // If the client cannot read this field, then skip it
if (!NetworkVariableFields[j].CanClientRead(clientId))
if (!field.CanClientRead(clientId))
{
continue;
}

NetworkVariableFields[j].ReadField(reader);
field.ReadField(reader);

if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
if (reader.Position > (readStartPos + varSize))
{
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
{
NetworkLog.LogWarning($"Var data read too far. {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($"Var data read too little. {(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);
}
}
}
Expand Down Expand Up @@ -1295,7 +1265,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;
Expand Down Expand Up @@ -1333,7 +1303,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;
Expand All @@ -1342,7 +1312,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;
Expand Down
10 changes: 7 additions & 3 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,15 @@ 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);
}

// Once done processing, we set the previous owner id to the current owner id
dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
dirtyObj.PreviousOwnerId = dirtyObj.OwnerClientId;
}
m_DirtyNetworkObjects.Clear();
}
Expand Down
82 changes: 34 additions & 48 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,16 +1476,6 @@ internal List<NetworkBehaviour> ChildNetworkBehaviours
}
}

internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
{
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++)
Expand Down Expand Up @@ -1525,18 +1515,6 @@ internal static void VerifyParentingStatus()
}
}

/// <summary>
/// Only invoked during first synchronization of a NetworkObject (late join or newly spawned)
/// </summary>
internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
{
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
{
var behaviour = ChildNetworkBehaviours[i];
behaviour.InitializeVariables();
behaviour.SetNetworkVariableData(reader, clientId);
}
}

internal ushort GetNetworkBehaviourOrderIndex(NetworkBehaviour instance)
{
Expand Down Expand Up @@ -1744,14 +1722,6 @@ public void Deserialize(FastBufferReader reader)
}
}

internal void PostNetworkVariableWrite(bool forceSend)
{
for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
{
ChildNetworkBehaviours[k].PostNetworkVariableWrite(forceSend);
}
}

/// <summary>
/// Handles synchronizing NetworkVariables and custom synchronization data for NetworkBehaviours.
/// </summary>
Expand All @@ -1765,11 +1735,16 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
{
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);
Expand All @@ -1791,7 +1766,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);
Expand All @@ -1803,23 +1778,34 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
else
{
var reader = serializer.GetFastBufferReader();

reader.ReadValueSafe(out ushort sizeOfSynchronizationData);
reader.ReadValueSafe(out int sizeOfSynchronizationData);
var seekToEndOfSynchData = reader.Position + sizeOfSynchronizationData;
// Apply the network variable synchronization data
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++)
try
{
// Apply the network variable synchronization data
foreach (var behaviour in ChildNetworkBehaviours)
{
behaviour.InitializeVariables();
behaviour.SetNetworkVariableData(reader, targetClientId);
}

// Read the number of NetworkBehaviours to synchronize
reader.ReadValueSafe(out byte numberSynchronized);

// 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 ushort networkBehaviourId);
var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
networkBehaviour.Synchronize(ref serializer, targetClientId);
}
}
catch
{
serializer.SerializeValue(ref networkBehaviourId);
var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
networkBehaviour.Synchronize(ref serializer, targetClientId);
reader.Seek(seekToEndOfSynchData);
}
}
}
Expand Down Expand Up @@ -1916,7 +1902,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)
Expand Down
Loading