Skip to content
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.

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

FIX: Memory corruption in InputEventTrace (case 1262496). #1184

Merged
merged 3 commits into from Aug 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Expand Up @@ -1763,6 +1763,8 @@ public void Actions_CanRecordActions()

using (var trace = new InputActionTrace())
{
Assert.That(trace.buffer.capacityInBytes, Is.Zero);

action.performed += trace.RecordAction;

var state = new GamepadState {leftStick = new Vector2(0.123f, 0.234f)};
Expand All @@ -1772,6 +1774,7 @@ public void Actions_CanRecordActions()
InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.W), 0.0987);
InputSystem.Update();

Assert.That(trace.buffer.capacityInBytes, Is.EqualTo(2048)); // Default capacity increment.
Assert.That(trace.count, Is.EqualTo(3));

var events = trace.ToArray();
Expand Down
2 changes: 2 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Expand Up @@ -32,6 +32,8 @@ however, it has to be formatted properly to pass verification tests.
- Improved performance of `Touch.activeTouches` (most notably, a lot of time was spent in endlessly repetitive safety checks).
- Fixed `EnhancedTouch` APIs not indicating that they need to be enabled with `EnhancedTouchSupport.Enable()`.
- The APIs now throw `InvalidOperationException` when used without being enabled.
- Fixed memory corruption in `InputEventTrace.AllocateEvent` ([case 1262496](https://issuetracker.unity3d.com/issues/input-system-crash-with-various-stack-traces-when-using-inputactiontrace-dot-subscribetoall))
* Manifested itself, for example, as crashes when using `InputActionTrace.SubscribeToAll`.
- AxisControls and Vector2Controls' X and Y subcontrols on XR devices now have a minimum range of -1 and a maximum range of 1. This means they can now properly respond to modifiers and interactions in the binding system.

#### Actions
Expand Down
Expand Up @@ -151,25 +151,25 @@ public void AppendEvent(InputEvent* eventPtr, int capacityIncrementInBytes = 204
var alignedSizeInBytes = sizeInBytes.AlignToMultipleOf(InputEvent.kAlignment);

// See if we need to enlarge our buffer.
var necessaryCapacity = m_SizeInBytes + alignedSizeInBytes;
var currentCapacity = capacityInBytes;
if (currentCapacity < alignedSizeInBytes)
if (currentCapacity < necessaryCapacity)
{
// Yes, so reallocate.
var newCapacity = Math.Max(currentCapacity + capacityIncrementInBytes,
currentCapacity + alignedSizeInBytes);
var newSize = this.sizeInBytes + newCapacity;
if (newSize > int.MaxValue)

var newCapacity = necessaryCapacity.AlignToMultipleOf(capacityIncrementInBytes);
if (newCapacity > int.MaxValue)
throw new NotImplementedException("NativeArray long support");
var newBuffer =
new NativeArray<byte>((int)newSize, Allocator.Persistent, NativeArrayOptions.ClearMemory);
new NativeArray<byte>((int)newCapacity, Allocator.Persistent, NativeArrayOptions.ClearMemory);

if (m_Buffer.IsCreated)
{
UnsafeUtility.MemCpy(newBuffer.GetUnsafePtr(), m_Buffer.GetUnsafeReadOnlyPtr(), this.sizeInBytes);
else
m_SizeInBytes = 0;
if (m_WeOwnTheBuffer)
m_Buffer.Dispose();
}

if (m_WeOwnTheBuffer)
m_Buffer.Dispose();
m_Buffer = newBuffer;
m_WeOwnTheBuffer = true;
}
Expand Down
Expand Up @@ -13,6 +13,15 @@ public static int AlignToMultipleOf(this int number, int alignment)
return number + alignment - remainder;
}

public static long AlignToMultipleOf(this long number, long alignment)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test just for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go add something in the next PRs.

{
var remainder = number % alignment;
if (remainder == 0)
return number;

return number + alignment - remainder;
}

public static uint AlignToMultipleOf(this uint number, uint alignment)
{
var remainder = number % alignment;
Expand Down