Skip to content

Commit 9687c8a

Browse files
EmandMmichalChrobot
EmandM
and
michalChrobot
authored
fix: [Backport] NetworkBehaviour and NetworkVariable length safety checks (#3415)
<!-- 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) 1.x for #3335 ## Changelog - Fixed: NetworkBehaviour length safety checks use `int` instead of `ushort` - Fixed: NetworkVariable `EnsureNetworkVariableLengthSafety` checks use `int` instead of `ushort` ## Testing and Documentation - No tests have been added. <!-- 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. --> This is a backport of #3405 --------- Co-authored-by: Michał Chrobot <[email protected]>
1 parent f167039 commit 9687c8a

File tree

8 files changed

+111
-148
lines changed

8 files changed

+111
-148
lines changed

‎com.unity.netcode.gameobjects/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
1616

1717
### Fixed
1818

19+
- Fixed issues with the `NetworkBehaviour` and `NetworkVariable` length safety checks. (#3415)
1920
- 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)
2021
- 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)
2122
- 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)

‎com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs‎

Lines changed: 21 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,49 +1112,33 @@ internal void MarkOwnerReadVariablesDirty()
11121112
/// </remarks>
11131113
internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
11141114
{
1115-
if (NetworkVariableFields.Count == 0)
1115+
foreach (var field in NetworkVariableFields)
11161116
{
1117-
return;
1118-
}
1119-
1120-
for (int j = 0; j < NetworkVariableFields.Count; j++)
1121-
{
1122-
1123-
if (NetworkVariableFields[j].CanClientRead(targetClientId))
1117+
if (field.CanClientRead(targetClientId))
11241118
{
11251119
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
11261120
{
11271121
var writePos = writer.Position;
11281122
// Note: This value can't be packed because we don't know how large it will be in advance
1129-
// we reserve space for it, then write the data, then come back and fill in the space
1130-
// to pack here, we'd have to write data to a temporary buffer and copy it in - which
1131-
// isn't worth possibly saving one byte if and only if the data is less than 63 bytes long...
1132-
// The way we do packing, any value > 63 in a ushort will use the full 2 bytes to represent.
1133-
writer.WriteValueSafe((ushort)0);
1123+
writer.WriteValueSafe(0);
11341124
var startPos = writer.Position;
11351125
// Write the NetworkVariable field value
1136-
// WriteFieldSynchronization will write the current value only if there are no pending changes.
1137-
// Otherwise, it will write the previous value if there are pending changes since the pending
1138-
// changes will be sent shortly after the client's synchronization.
1139-
NetworkVariableFields[j].WriteFieldSynchronization(writer);
1126+
field.WriteFieldSynchronization(writer);
11401127
var size = writer.Position - startPos;
11411128
writer.Seek(writePos);
1142-
writer.WriteValueSafe((ushort)size);
1129+
writer.WriteValueSafe(size);
11431130
writer.Seek(startPos + size);
11441131
}
11451132
else
11461133
{
11471134
// Write the NetworkVariable field value
1148-
// WriteFieldSynchronization will write the current value only if there are no pending changes.
1149-
// Otherwise, it will write the previous value if there are pending changes since the pending
1150-
// changes will be sent shortly after the client's synchronization.
1151-
NetworkVariableFields[j].WriteFieldSynchronization(writer);
1135+
field.WriteFieldSynchronization(writer);
11521136
}
11531137
}
11541138
else // Only if EnsureNetworkVariableLengthSafety, otherwise just skip
11551139
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
11561140
{
1157-
writer.WriteValueSafe((ushort)0);
1141+
writer.WriteValueSafe(0);
11581142
}
11591143
}
11601144
}
@@ -1169,51 +1153,37 @@ internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClie
11691153
/// </remarks>
11701154
internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
11711155
{
1172-
if (NetworkVariableFields.Count == 0)
1156+
foreach (var field in NetworkVariableFields)
11731157
{
1174-
return;
1175-
}
1176-
1177-
for (int j = 0; j < NetworkVariableFields.Count; j++)
1178-
{
1179-
var varSize = (ushort)0;
1158+
int expectedBytesToRead = 0;
11801159
var readStartPos = 0;
11811160
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
11821161
{
1183-
reader.ReadValueSafe(out varSize);
1184-
if (varSize == 0)
1162+
reader.ReadValueSafe(out expectedBytesToRead);
1163+
if (expectedBytesToRead == 0)
11851164
{
11861165
continue;
11871166
}
11881167
readStartPos = reader.Position;
11891168
}
11901169
else // If the client cannot read this field, then skip it
1191-
if (!NetworkVariableFields[j].CanClientRead(clientId))
1170+
if (!field.CanClientRead(clientId))
11921171
{
11931172
continue;
11941173
}
11951174

1196-
NetworkVariableFields[j].ReadField(reader);
1175+
field.ReadField(reader);
11971176

11981177
if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
11991178
{
1200-
if (reader.Position > (readStartPos + varSize))
1201-
{
1202-
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
1203-
{
1204-
NetworkLog.LogWarning($"Var data read too far. {reader.Position - (readStartPos + varSize)} bytes.");
1205-
}
1206-
1207-
reader.Seek(readStartPos + varSize);
1208-
}
1209-
else if (reader.Position < (readStartPos + varSize))
1179+
var totalBytesRead = reader.Position - readStartPos;
1180+
if (totalBytesRead != expectedBytesToRead)
12101181
{
1211-
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
1182+
if (NetworkManager.LogLevel <= LogLevel.Normal)
12121183
{
1213-
NetworkLog.LogWarning($"Var data read too little. {(readStartPos + varSize) - reader.Position} bytes.");
1184+
NetworkLog.LogWarning($"[{name}][NetworkObjectId: {NetworkObjectId}][NetworkBehaviourId: {NetworkBehaviourId}][{field.Name}] NetworkVariable read {totalBytesRead} bytes but was expected to read {expectedBytesToRead} bytes during synchronization deserialization!");
12141185
}
1215-
1216-
reader.Seek(readStartPos + varSize);
1186+
reader.Seek(readStartPos + expectedBytesToRead);
12171187
}
12181188
}
12191189
}
@@ -1295,7 +1265,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
12951265
// Save our position where we will write the final size being written so we can skip over it in the
12961266
// event an exception occurs when deserializing.
12971267
var sizePosition = writer.Position;
1298-
writer.WriteValueSafe((ushort)0);
1268+
writer.WriteValueSafe(0);
12991269

13001270
// Save our position before synchronizing to determine how much was written
13011271
var positionBeforeSynchronize = writer.Position;
@@ -1333,7 +1303,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
13331303
// Write the number of bytes serialized to handle exceptions on the deserialization side
13341304
var bytesWritten = finalPosition - positionBeforeSynchronize;
13351305
writer.Seek(sizePosition);
1336-
writer.WriteValueSafe((ushort)bytesWritten);
1306+
writer.WriteValueSafe(bytesWritten);
13371307
writer.Seek(finalPosition);
13381308
}
13391309
return true;
@@ -1342,7 +1312,7 @@ internal bool Synchronize<T>(ref BufferSerializer<T> serializer, ulong targetCli
13421312
{
13431313
var reader = serializer.GetFastBufferReader();
13441314
// We will always read the expected byte count
1345-
reader.ReadValueSafe(out ushort expectedBytesToRead);
1315+
reader.ReadValueSafe(out int expectedBytesToRead);
13461316

13471317
// Save our position before we begin synchronization deserialization
13481318
var positionBeforeSynchronize = reader.Position;

‎com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,15 @@ internal void NetworkBehaviourUpdate(bool forceSend = false)
108108
}
109109

110110
// Now, reset all the no-longer-dirty variables
111-
foreach (var dirtyobj in m_DirtyNetworkObjects)
111+
foreach (var dirtyObj in m_DirtyNetworkObjects)
112112
{
113-
dirtyobj.PostNetworkVariableWrite(forceSend);
113+
foreach (var behaviour in dirtyObj.ChildNetworkBehaviours)
114+
{
115+
behaviour.PostNetworkVariableWrite(forceSend);
116+
}
117+
114118
// Once done processing, we set the previous owner id to the current owner id
115-
dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
119+
dirtyObj.PreviousOwnerId = dirtyObj.OwnerClientId;
116120
}
117121
m_DirtyNetworkObjects.Clear();
118122
}

‎com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs‎

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1476,16 +1476,6 @@ internal List<NetworkBehaviour> ChildNetworkBehaviours
14761476
}
14771477
}
14781478

1479-
internal void WriteNetworkVariableData(FastBufferWriter writer, ulong targetClientId)
1480-
{
1481-
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
1482-
{
1483-
var behavior = ChildNetworkBehaviours[i];
1484-
behavior.InitializeVariables();
1485-
behavior.WriteNetworkVariableData(writer, targetClientId);
1486-
}
1487-
}
1488-
14891479
internal void MarkVariablesDirty(bool dirty)
14901480
{
14911481
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
@@ -1525,18 +1515,6 @@ internal static void VerifyParentingStatus()
15251515
}
15261516
}
15271517

1528-
/// <summary>
1529-
/// Only invoked during first synchronization of a NetworkObject (late join or newly spawned)
1530-
/// </summary>
1531-
internal void SetNetworkVariableData(FastBufferReader reader, ulong clientId)
1532-
{
1533-
for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
1534-
{
1535-
var behaviour = ChildNetworkBehaviours[i];
1536-
behaviour.InitializeVariables();
1537-
behaviour.SetNetworkVariableData(reader, clientId);
1538-
}
1539-
}
15401518

15411519
internal ushort GetNetworkBehaviourOrderIndex(NetworkBehaviour instance)
15421520
{
@@ -1744,14 +1722,6 @@ public void Deserialize(FastBufferReader reader)
17441722
}
17451723
}
17461724

1747-
internal void PostNetworkVariableWrite(bool forceSend)
1748-
{
1749-
for (int k = 0; k < ChildNetworkBehaviours.Count; k++)
1750-
{
1751-
ChildNetworkBehaviours[k].PostNetworkVariableWrite(forceSend);
1752-
}
1753-
}
1754-
17551725
/// <summary>
17561726
/// Handles synchronizing NetworkVariables and custom synchronization data for NetworkBehaviours.
17571727
/// </summary>
@@ -1765,11 +1735,16 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
17651735
{
17661736
var writer = serializer.GetFastBufferWriter();
17671737
var positionBeforeSynchronizing = writer.Position;
1768-
writer.WriteValueSafe((ushort)0);
1738+
writer.WriteValueSafe(0);
17691739
var sizeToSkipCalculationPosition = writer.Position;
17701740

17711741
// Synchronize NetworkVariables
1772-
WriteNetworkVariableData(writer, targetClientId);
1742+
foreach (var behavior in ChildNetworkBehaviours)
1743+
{
1744+
behavior.InitializeVariables();
1745+
behavior.WriteNetworkVariableData(writer, targetClientId);
1746+
}
1747+
17731748
// Reserve the NetworkBehaviour synchronization count position
17741749
var networkBehaviourCountPosition = writer.Position;
17751750
writer.WriteValueSafe((byte)0);
@@ -1791,7 +1766,7 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
17911766
// synchronization.
17921767
writer.Seek(positionBeforeSynchronizing);
17931768
// We want the size of everything after our size to skip calculation position
1794-
var size = (ushort)(currentPosition - sizeToSkipCalculationPosition);
1769+
var size = currentPosition - sizeToSkipCalculationPosition;
17951770
writer.WriteValueSafe(size);
17961771
// Write the number of NetworkBehaviours synchronized
17971772
writer.Seek(networkBehaviourCountPosition);
@@ -1803,23 +1778,34 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
18031778
else
18041779
{
18051780
var reader = serializer.GetFastBufferReader();
1806-
1807-
reader.ReadValueSafe(out ushort sizeOfSynchronizationData);
1781+
reader.ReadValueSafe(out int sizeOfSynchronizationData);
18081782
var seekToEndOfSynchData = reader.Position + sizeOfSynchronizationData;
1809-
// Apply the network variable synchronization data
1810-
SetNetworkVariableData(reader, targetClientId);
1811-
// Read the number of NetworkBehaviours to synchronize
1812-
reader.ReadValueSafe(out byte numberSynchronized);
1813-
var networkBehaviourId = (ushort)0;
18141783

1815-
// If a NetworkBehaviour writes synchronization data, it will first
1816-
// write its NetworkBehaviourId so when deserializing the client-side
1817-
// can find the right NetworkBehaviour to deserialize the synchronization data.
1818-
for (int i = 0; i < numberSynchronized; i++)
1784+
try
1785+
{
1786+
// Apply the network variable synchronization data
1787+
foreach (var behaviour in ChildNetworkBehaviours)
1788+
{
1789+
behaviour.InitializeVariables();
1790+
behaviour.SetNetworkVariableData(reader, targetClientId);
1791+
}
1792+
1793+
// Read the number of NetworkBehaviours to synchronize
1794+
reader.ReadValueSafe(out byte numberSynchronized);
1795+
1796+
// If a NetworkBehaviour writes synchronization data, it will first
1797+
// write its NetworkBehaviourId so when deserializing the client-side
1798+
// can find the right NetworkBehaviour to deserialize the synchronization data.
1799+
for (int i = 0; i < numberSynchronized; i++)
1800+
{
1801+
reader.ReadValueSafe(out ushort networkBehaviourId);
1802+
var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
1803+
networkBehaviour.Synchronize(ref serializer, targetClientId);
1804+
}
1805+
}
1806+
catch
18191807
{
1820-
serializer.SerializeValue(ref networkBehaviourId);
1821-
var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
1822-
networkBehaviour.Synchronize(ref serializer, targetClientId);
1808+
reader.Seek(seekToEndOfSynchData);
18231809
}
18241810
}
18251811
}
@@ -1916,7 +1902,7 @@ internal static NetworkObject AddSceneObject(in SceneObject sceneObject, FastBuf
19161902
try
19171903
{
19181904
// If we failed to load this NetworkObject, then skip past the Network Variable and (if any) synchronization data
1919-
reader.ReadValueSafe(out ushort networkBehaviourSynchronizationDataLength);
1905+
reader.ReadValueSafe(out int networkBehaviourSynchronizationDataLength);
19201906
reader.Seek(reader.Position + networkBehaviourSynchronizationDataLength);
19211907
}
19221908
catch (Exception ex)

0 commit comments

Comments
 (0)