FIX: Reenabling the VirtualMouseInput component may lead to NullReferenceException

[K-Tone]

Description

Fixed an exception that happens when you toggle enable/disable a few times on the VirtualMouseInput component.

This exception is occasionally raised from within InputActionState.ApplyProcessors as it tries to read from the processors array that is null in this case. By the normal flow of things, we shouldn't have tried to index into a null array since just a few lines above there is a check that ensures there are more than zero processors before doing this. However, when no bindings are present , processorCount will actually be a random value. Let me explain how.

        internal TValue ApplyProcessors<TValue>(int bindingIndex, TValue value, InputControl<TValue> controlOfType = null)
            where TValue : struct
        {
            var processorCount = bindingStates[bindingIndex].processorCount;
            if (processorCount > 0)
            {
                var processorStartIndex = bindingStates[bindingIndex].processorStartIndex;
                for (var i = 0; i < processorCount; ++i)
                {
                    if (processors[processorStartIndex + i] is InputProcessor<TValue> processor)
                        value = processor.Process(value, controlOfType);
                }
            }

            return value;
        }

So in InputActionState.ApplyProcessors, we read processorCount from the bindingStates array using bindingIndex. However, when there are no bindings present, bindingIndex has a chance to be a fairly large random value if you disable and enable the VirtualMouseInput component. Note that bindingStates is an unsafe array so nothing prevents from reading out of bounds. How can thisbindingIndex go wrong?

Moving a bit up the call stack, in InputAction.ReadValue we see that bindingIndex is taken from InputActionState.TriggerState's bindingIndex field.

        public unsafe TValue ReadValue<TValue>()
            where TValue : struct
        {
            var state = GetOrCreateActionMap().m_State;
            if (state == null)
                return default(TValue);

            var actionStatePtr = &state.actionStates[m_ActionIndexInState];
            return actionStatePtr->phase.IsInProgress()
                ? state.ReadValue<TValue>(actionStatePtr->bindingIndex, actionStatePtr->controlIndex)
                : state.ApplyProcessors(actionStatePtr->bindingIndex, default(TValue));
        }

This field is normally correct, however when we disable the VirtualMouseInput component there is some interesting logic that tries to restore the binding index to what is stored in the corresponding arrays. It is part of InputActionState.ResetActionState method, and the problem here arises from memory.actionBindingIndices being an invalid pointer when no bindings are present. This is an unsafe pointer, and it is not null in case the array contains no elements by design of the UnmanagedMemory class. Reading from this pointer will get random data that is used after you Enable the VirtualMouseInput component.

            actionState->phase = toPhase;
             actionState->controlIndex = kInvalidIndex;
             actionState->bindingIndex = memory.actionBindingIndices[memory.actionBindingIndicesAndCounts[actionIndex]];
             actionState->interactionIndex = kInvalidIndex;
             actionState->startTime = 0;
             actionState->time = 0;

Let me pull up some code from UnmanagedMemory that I find a bit problematic at the moment:

            public int sizeInBytes =>
                mapCount * sizeof(ActionMapIndices) + // mapIndices
                actionCount * sizeof(TriggerState) + // actionStates
                bindingCount * sizeof(BindingState) + // bindingStates
                interactionCount * sizeof(InteractionState) + // interactionStates
                controlCount * sizeof(float) + // controlMagnitudes
                compositeCount * sizeof(float) + // compositeMagnitudes
                controlCount * sizeof(int) + // controlIndexToBindingIndex
                controlCount * sizeof(ushort) * 2 + // controlGrouping
                actionCount * sizeof(ushort) * 2 + // actionBindingIndicesAndCounts
                bindingCount * sizeof(ushort) + // actionBindingIndices
                (controlCount + 31) / 32 * sizeof(int); // enabledControlsArray

               public void Allocate(int mapCount, int actionCount, int bindingCount, int controlCount, int interactionCount, int compositeCount)
             {
                 Debug.Assert(basePtr == null, "Memory already allocated! Free first!");
                 // NOTE: This depends on the individual structs being sufficiently aligned in order to not
                 //       cause any misalignment here. TriggerState, InteractionState, and BindingState all
                 //       contain doubles so put them first in memory to make sure they get proper alignment.
                 actionStates = (TriggerState*)ptr; ptr += actionCount * sizeof(TriggerState);
                 interactionStates = (InteractionState*)ptr; ptr += interactionCount * sizeof(InteractionState);
                 bindingStates = (BindingState*)ptr; ptr += bindingCount * sizeof(BindingState);
                 mapIndices = (ActionMapIndices*)ptr; ptr += mapCount * sizeof(ActionMapIndices);
                 controlMagnitudes = (float*)ptr; ptr += controlCount * sizeof(float);
                 compositeMagnitudes = (float*)ptr; ptr += compositeCount * sizeof(float);
                 controlIndexToBindingIndex = (int*)ptr; ptr += controlCount * sizeof(int);
                 controlGroupingAndComplexity = (ushort*)ptr; ptr += controlCount * sizeof(ushort) * 2;
                 actionBindingIndicesAndCounts = (ushort*)ptr; ptr += actionCount * sizeof(ushort) * 2;
                 actionBindingIndices = (ushort*)ptr; ptr += bindingCount * sizeof(ushort);
                 enabledControls = (int*)ptr; ptr += (controlCount + 31) / 32 * sizeof(int);
             }

So essentially this thing makes one allocation to host all the smaller bits of data including the aggregated arrays. However, the problem is that if some array has zero length, we don't set its pointer to null. Instead, we just shift the current pointer in the blob by how much memory we needed for the previous array. Because of that, some array pointers can be equal (imagine a few empty arrays going in a sequence). Also, especially problematic are the last pointers in the blob -- because if they are zero sized, then they are set to invalid pointer, pointing to the first byte right after the blob. That's precisely the reason why we had random bindingIndex - note how actionBindingindices will be set to point to the first byte after the blob in case bindingCount is zero.

I believe there are a few ways we could attack this problem.

One, that would aim for minimal footprint is to change just one line in InputActionState.ApplyProcessors to also include a check for an empty processors array, like this:

if (processorCount > 0 && processors != null)

However, in my mind that is slightly unfortunate because it leaves the core issue of having bad bindingIndices around, and it also still leaves us with invalid pointers being around. It's only until next time someone doesn't check for an empty array in the right way. When debugging, it's somewhat uneasy to understand the pointers that look somewhat reasonable are actually pointers to unallocated space.

Because of that, I thought to propose a change that assigns null to any pointers in the UnmanagedMemory class that reflect zero-sized arrays. That way, if someone tries to access them, we get an instant exception that is easy to diagnose. Please advise what you think.

Testing status & QA

Local testing

Overall Product Risks

• Complexity: Medium
• Halo Effect: Medium

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

• Changelog entry added.
  • Explains the change in Changed, Fixed, Added sections.
  • For API change contains an example snippet and/or migration example.
  • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
  • Jira port for the next release set as "Resolved".
• Tests added/changed, if applicable.
  • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
  • Performance tests.
  • Integration tests.
• Docs for new/changed API's.
  • Xmldoc cross references are set correctly.
  • Added explanation how the API works.
  • Usage code examples added.
  • The manual is updated, if needed.

During merge:

• Commit message for squash-merge is prefixed with one of the list:
  • NEW: ___.
  • FIX: ___.
  • DOCS: ___.
  • CHANGE: ___.
  • RELEASE: 1.1.0-preview.3.

After merge:

• Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

[ekcoh]

Would it even be smart to set bindingIndex to -1 to indicate it's not a valid index?
It would allow doing (actionState->bindingIndex >= 0) as an indirect null-check, but if other indices are zero it makes sense to keep it at zero for consistency

[K-Tone]

This is a very good question to be honest. I'm surprised it's not been set to -1 before. I should see if that's safe to change in order not to break some stuff somewhere, this code is kinda interconnected quite a bit.

[ekcoh]

Awesome, it was just a thought and not something I consider a must-have if it adds complexity

[K-Tone]

Ok we have some failing tests apparently - Actions_CanAddCompositeBindingsToActions_AfterActionHasAlreadyResolvedControls and Actions_WhenShortcutsEnabled_CanConsumeInput, maybe a few others as well. Investigating.

[ekcoh]

Looks like you will consider the index (not set value), but changes LGTM to me already now so approving.

[ekcoh]

Just saw the failing tests so updating this to "Request Changes" since clearly more work is needed on figuring out why