FIX: memory alignment issue when deserializing InputEventTraces (ISXB-317) (#1596)

* FIX: prevent infinite loop when deserializing InputEventTrace

* review changes. replace alignment value with enum constant

* review changes: add more robust test and fix for incorrect event size

Author: jamesmcgill

Assets/Tests/InputSystem/CoreTests_Events.cs

@@ -1414,6 +1414,94 @@ public void Events_WhenTraceIsFull_WillStartOverwritingOldEvents()
         }
     }
 
+    [Test]
+    [Category("Events")]
+    public void Events_CanDeserializeInputEventTraceFromMemory()
+    {
+        var dataStream = new MemoryStream();
+        long sourceTraceEventSize = 0;
+        long sourceEventCount = 0;
+
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+
+        // Generate a trace
+        using (var trace = new InputEventTrace())
+        {
+            trace.Enable();
+
+            // Enough events that the aggregate of any padding introduced due to alignment would
+            // start to skew data in last event
+            Press(gamepad.buttonEast);
+            Set(gamepad.leftStick, new Vector2(0.123f, 0.234f));
+            Press(gamepad.buttonSouth);
+            Press(gamepad.buttonWest);
+            Press(gamepad.buttonNorth);
+
+            Release(gamepad.buttonEast);
+            Release(gamepad.buttonSouth);
+            Release(gamepad.buttonWest);
+            Release(gamepad.buttonNorth);
+
+            Press(gamepad.buttonEast);
+            Press(gamepad.buttonSouth);
+            Press(gamepad.buttonWest);
+            Press(gamepad.buttonNorth);
+            Set(gamepad.rightStick, new Vector2(-0.123f, -0.234f));
+
+            InputSystem.Update();
+
+            Assert.That(trace.eventCount, Is.EqualTo(14));
+            sourceEventCount = trace.eventCount;
+            sourceTraceEventSize = trace.totalEventSizeInBytes;
+
+            // Serialize trace to memory
+            trace.WriteTo(dataStream);
+            trace.Disable();
+            dataStream.Flush();
+        }
+
+        // Deserialize trace from memory into a fresh new buffer
+        using (var trace = new InputEventTrace())
+        {
+            dataStream.Position = 0;
+            trace.ReadFrom(dataStream);
+
+            // Check that we have read the correct number of bytes
+            Assert.That(trace.allocatedSizeInBytes, Is.EqualTo(sourceTraceEventSize));
+            Assert.That(trace.totalEventSizeInBytes, Is.EqualTo(sourceTraceEventSize));
+
+            // This line should not cause an infinite loop
+            trace.Replay().PlayAllEvents();
+
+            // Collect content and event count manually using enumeration
+            var retrievedEvents = new List<InputEventPtr>();
+            int eventCount = 0;
+            long aggregateEventSize = 0;
+            var current = default(InputEventPtr);
+            while (trace.GetNextEvent(ref current))
+            {
+                eventCount++;
+                aggregateEventSize += current.sizeInBytes;
+                retrievedEvents.Add(current);
+            }
+
+            // Check sum of enumerated values and header values match
+            Assert.That(trace.eventCount, Is.EqualTo(sourceEventCount));
+            Assert.That(eventCount, Is.EqualTo(trace.eventCount));
+            Assert.That(trace.totalEventSizeInBytes, Is.GreaterThanOrEqualTo(aggregateEventSize));
+
+            // Check event data is correct
+            Assert.That(retrievedEvents[0].deviceId, Is.EqualTo(gamepad.deviceId));
+            Assert.That(gamepad.buttonEast.ReadUnprocessedValueFromEvent(retrievedEvents[0]), Is.EqualTo(1));
+            Assert.That(gamepad.buttonSouth.ReadUnprocessedValueFromEvent(retrievedEvents[0]), Is.EqualTo(0));
+
+            // Especially the last one
+            var lastIdx = eventCount - 1;
+            Assert.That(retrievedEvents[lastIdx].deviceId, Is.EqualTo(gamepad.deviceId));
+            Assert.That(gamepad.rightStick.ReadUnprocessedValueFromEvent(retrievedEvents[lastIdx]), Is.EqualTo(new Vector2(-0.123f, -0.234f)));
+        }
+    }
+
     [Test]
     [Category("Events")]
     public void Events_CanClearEventTrace()

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -14,6 +14,7 @@ however, it has to be formatted properly to pass verification tests.
 - Fixed `ArgumentNullException` when opening the Prefab Overrides window and selecting a component with an `InputAction`.
 - Fixed `{fileID: 0}` getting appended to `ProjectSettings.asset` file when building a project ([case ISXB-296](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-296)).
 - Fixed `Type of instance in array does not match expected type` assertion when using PlayerInput in combination with Control Schemes and Interactions ([case ISXB-282](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-282)).
+- Fixed Memory alignment issue with deserialized InputEventTraces that could cause infinite loops when playing back replays ([case ISXB-317](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-317)).
 - Fixed an InvalidOperationException when using Hold interaction, and by extension any interaction that changes to performed state after a timeout ([case ISXB-332](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-330)).
 - Fixed `Given object is neither an InputAction nor an InputActionMap` when using `InputActionTrace` on input action from an input action asset ([case ISXB-29](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-29)).
 

Packages/com.unity.inputsystem/InputSystem/Events/InputEventTrace.cs

@@ -342,7 +342,7 @@ public void ReadFrom(Stream stream)
                 }
                 else
                 {
-                    buffer = (byte*)UnsafeUtility.Malloc((long)totalEventSizeInBytes, 4, Allocator.Persistent);
+                    buffer = (byte*)UnsafeUtility.Malloc((long)totalEventSizeInBytes, InputEvent.kAlignment, Allocator.Persistent);
                     m_EventBufferSize = (long)totalEventSizeInBytes;
                 }
                 try
@@ -373,8 +373,8 @@ public void ReadFrom(Stream stream)
                         fixed(byte* tempBufferPtr = tempBuffer)
                         UnsafeUtility.MemCpy(tailPtr, tempBufferPtr, remainingSize);
 
-                        tailPtr += remainingSize;
-                        totalEventSize += eventSizeInBytes;
+                        tailPtr += remainingSize.AlignToMultipleOf(InputEvent.kAlignment);
+                        totalEventSize += eventSizeInBytes.AlignToMultipleOf(InputEvent.kAlignment);
 
                         if (tailPtr >= endPtr)
                             break;
@@ -501,7 +501,7 @@ public bool Resize(long newBufferSize, long newMaxBufferSize = -1)
                 newMaxBufferSize = newBufferSize;
 
             // Allocate.
-            var newEventBuffer = (byte*)UnsafeUtility.Malloc(newBufferSize, 4, Allocator.Persistent);
+            var newEventBuffer = (byte*)UnsafeUtility.Malloc(newBufferSize, InputEvent.kAlignment, Allocator.Persistent);
             if (newEventBuffer == default)
                 return false;
 
@@ -521,7 +521,7 @@ public bool Resize(long newBufferSize, long newMaxBufferSize = -1)
                     for (var i = 0; i < m_EventCount; ++i)
                     {
                         var eventSizeInBytes = fromPtr.sizeInBytes;
-                        var alignedEventSizeInBytes = eventSizeInBytes.AlignToMultipleOf(4);
+                        var alignedEventSizeInBytes = eventSizeInBytes.AlignToMultipleOf(InputEvent.kAlignment);
 
                         // We only start copying once we know that the remaining events we have fit in the new buffer.
                         // This way we get the newest events and not the oldest ones.
@@ -753,7 +753,7 @@ private byte* m_EventBufferTail
 
         private void Allocate()
         {
-            m_EventBuffer = (byte*)UnsafeUtility.Malloc(m_EventBufferSize, 4, Allocator.Persistent);
+            m_EventBuffer = (byte*)UnsafeUtility.Malloc(m_EventBufferSize, InputEvent.kAlignment, Allocator.Persistent);
         }
 
         private void Release()
@@ -805,7 +805,7 @@ private void OnInputEvent(InputEventPtr inputEvent, InputDevice device)
             if (m_EventBuffer == default)
                 return;
 
-            var bytesNeeded = inputEvent.sizeInBytes.AlignToMultipleOf(4);
+            var bytesNeeded = inputEvent.sizeInBytes.AlignToMultipleOf(InputEvent.kAlignment);
 
             // Make sure we can fit the event at all.
             if (bytesNeeded > m_MaxEventBufferSize)
@@ -830,7 +830,7 @@ private void OnInputEvent(InputEventPtr inputEvent, InputDevice device)
                 // If we haven't reached the max size yet, grow the buffer.
                 if (m_EventBufferSize < m_MaxEventBufferSize && !m_HasWrapped)
                 {
-                    var increment = Math.Max(m_GrowIncrementSize, bytesNeeded.AlignToMultipleOf(4));
+                    var increment = Math.Max(m_GrowIncrementSize, bytesNeeded.AlignToMultipleOf(InputEvent.kAlignment));
                     var newBufferSize = m_EventBufferSize + increment;
                     if (newBufferSize > m_MaxEventBufferSize)
                         newBufferSize = m_MaxEventBufferSize;