diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index ff9f37f6a8..045b685fc8 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Issue where `NetworkAnimator` did no bounds check on the parameter index read prior to obtaining a pointer to the location within the array. (#4090) - Issue where scene migration in a distributed authority session would throw an exception on clients migrating their owned `NetworkObject` instances to a different scene.(#4086) - Issue where migrating a dynamically spawned or in-scene placed `NetworkObject` into a different scene during awake could result in that spawned instance to not be synchronized. (#4086) - Issue where a NullReferenceException was thrown when a non-authority failed to spawn a NetworkObject. (#4067) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs b/com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs index cdb25aa598..c999c26e9f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs @@ -1377,6 +1377,13 @@ private unsafe void ReadParameters(FastBufferReader reader) while (totalParametersRead < totalParametersToRead) { ByteUnpacker.ReadValuePacked(reader, out uint parameterIndex); + + // Do bounds check prior to getting the element as a reference at that index. + if (parameterIndex >= m_CachedAnimatorParameters.Length) + { + NetworkManager.Log.ErrorServer(new Logging.Context(LogLevel.Error, $"[{nameof(NetworkAnimator)}][{name}] Invalid index of {parameterIndex} was received when there are only {m_CachedAnimatorParameters.Length} parameters. Ignoring the remainger of this {nameof(ParametersUpdateMessage)}!")); + return; + } ref var cacheValue = ref UnsafeUtility.ArrayElementAsRef(m_CachedAnimatorParameters.GetUnsafePtr(), (int)parameterIndex); var hash = cacheValue.Hash; if (cacheValue.Type == AnimationParamEnumWrapper.AnimatorControllerParameterInt) diff --git a/testproject/Assets/Tests/Runtime/Animation/NetworkAnimatorTests.cs b/testproject/Assets/Tests/Runtime/Animation/NetworkAnimatorTests.cs index 715e7f6156..d685fb68ca 100644 --- a/testproject/Assets/Tests/Runtime/Animation/NetworkAnimatorTests.cs +++ b/testproject/Assets/Tests/Runtime/Animation/NetworkAnimatorTests.cs @@ -3,9 +3,11 @@ using System.Linq; using NUnit.Framework; using Unity.Netcode; +using Unity.Netcode.Components; using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; using UnityEngine.TestTools; +using static Unity.Netcode.Components.NetworkAnimator; namespace TestProject.RuntimeTests @@ -336,7 +338,40 @@ public void ParameterExcludedTests() VerboseDebug($" ------------------ Parameter Test [{m_OwnerShipMode}] Stopping ------------------ "); } + private unsafe void MockWritingParameters(ref FastBufferWriter writer) + { + writer.Seek(0); + writer.Truncate(); + + // Write out how many parameter entries to read + BytePacker.WriteValuePacked(writer, (uint)1); + // Write an invalid index level (anything would be invalid with no parameters) + BytePacker.WriteValuePacked(writer, (uint)1000); + // Write some value for the invalid parameter + BytePacker.WriteValuePacked(writer, (uint)10); + } + + [Test] + public void ParameterBoundsCheck() + { + var gameObject = new GameObject(); + gameObject.AddComponent(); + var networkAnimator = gameObject.AddComponent(); + var writer = new FastBufferWriter(40, Unity.Collections.Allocator.TempJob); + + // Mock a parameter update with an invalid index + MockWritingParameters(ref writer); + + var invalidParameters = new ParametersUpdateMessage() + { + Parameters = writer.ToArray() + }; + // Expect an error message + LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex($"parameters. Ignoring the remainger of this {nameof(ParametersUpdateMessage)}!")); + // Pass in the invalid ParametersUpdateMessage + networkAnimator.UpdateParameters(ref invalidParameters); + } private bool AllTriggersDetected(OwnerShipMode ownerShipMode) {