1403185 - Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | `anonymous namespace''::WindowsGamepadService::HandleRawInput

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P3)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-5ab2194b-133c-4b8d-aa74-c65a60170924.
=============================================================

Ting-Yu noticed this crash while triaging Nightly crashes. They are all from a single installation (though I also see a similar crash on 56), but they are all an array bounds access crash so maybe this can be fixed by code inspection.

This is the crash reason for all of these:
  ElementAt(aIndex = 117, aLength = 67)

FWIW, the URLs for these crashes are all NSFW VR sites.

Comment 1

[Andrew McCreight [:mccr8]]

Kyle, just a heads up here in case you want to take a look at these crashes.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, I don't think this has to do with the visited site specifically, as it's probably just using the gamepad API as yet another fingerprinting technique. These sites don't use a WebVR based player yet, though I'm not sure if they're using WebVR detection or not, and we don't have enough info in the crash stack to tell.

The only way we can get this kind of overflow on the button array is if the system is presenting us with an HID device that reports in a way we don't expect, and it doesn't look like we cap values there.

The gamepad API is a good candidate for fuzzing at some point, but for now we should just be able to cap gamepad resource array size as the gamepad info is being built, and report OOB values as invalid or something.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1403185 - Fix button value index lookup oob for Windows Gamepads;

We can get button indexes in HID usage reports that do not actually
correspond to a button we store, meaning we can overstep bounds of the
button array. Check validity before accessing array.

Review commit: https://reviewboard.mozilla.org/r/228598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228598/

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8959749 [details]
Bug 1403185 - Fix button value index lookup oob for Windows Gamepads;

https://reviewboard.mozilla.org/r/228598/#review235034

::: dom/gamepad/windows/WindowsGamepad.cpp:866
(Diff revision 1)
>  
>    for (unsigned i = 0; i < usageLength; i++) {
> +    // The button index in usages may be larger than what we detected when
> +    // enumerating gamepads. If so, warn and continue.
> +    if ((usages[i] - 1) >= buttons.Length()) {
> +      NS_WARNING("Trying to set gamepad button with out of bounds index!");

I doubt this warning is going to be useful to anyone, but you're the one maintaining this code now. :)

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8959749 [details]
Bug 1403185 - Fix button value index lookup oob for Windows Gamepads;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228598/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51394984ff2b
Fix button value index lookup oob for Windows Gamepads; r=ted

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2fa1060ea47
Backed out changeset 51394984ff2b for build bustages on a CLOSED TREE

Comment 9

[Arthur Iakab [arthur_iakab]]

Backed out changeset 51394984ff2b (bug 1403185) for build bustages on a CLOSED TREE

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170366482&repo=autoland

Backout revision https://hg.mozilla.org/integration/autoland/rev/a2fa1060ea47b8d138d283d86580e4d2819cc014

Failed push:https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=80a1cfc4f0698c4703c58df7aa868fea523a3604&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/356346485da8
Fix button value index lookup oob for Windows Gamepads; r=ted

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Fixed. Forgot to keep length check unsigned.

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/356346485da8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Not sure this is worth backporting given the frequency, but feel free to nominate if you feel strongly otherwise.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm pretty sure this has always happened on literally one poor soul's machine with some really odd HID thing hooked up, so not worth uplift unless frequency increases.