search

speps / XInputDotNet

C# wrapper around XInput, works with any Mono or .NET application (eg. Unity3D)
466 stars 96 forks source link

Fix struct size for XInputGetStateEx #30

Closed ColdPie1 closed 6 years ago

ColdPie1 commented 6 years ago

XInputGetStateEx is undocumented. It requires another four bytes of padding at the end of the XINPUT_STATE struct.

This can crash. See similar issues in the mumble-voip project:

https://github.com/mumble-voip/mumble/issues/2016 https://github.com/mumble-voip/mumble/issues/2018 https://github.com/mumble-voip/mumble/issues/2019

I'm not sure why this doesn't crash for your clients on Windows. I discovered this trying to get THOTH to work in Wine, where it crashed due to us writing to this missing field. Perhaps MS's .NET allocates memory differently from wine-mono, meaning the overrun doens't trigger a crash.

Anyway this PR should fix it going forward.

ColdPie1 commented 6 years ago

Upon further research, this may be unnecessary. The mumble-voip issues were caused by a calling convention disagreement; the extra bytes of padding may have been a red herring. The ultimate source of those padding bytes seems to be the SDL_xinput.h header file, but it's unclear where they got them from. The line's commit history is lost, at least to Git.

I wrote some Wine tests for xinput, and they imply that Windows has never written to this field. So that would explain why your users don't see a crash. You can likely close out this PR. Apologies for the noise.

speps commented 6 years ago

Thanks for the investigation anyway :)

CookiePLMonster commented 6 years ago

Just a FYI - nothing implies XInputGetStateEx ever needs any extra fields - even disassembling xinput DLLs doesn't seem to show anything - in fact, both GetState and GetStateEx eventually call the same function, then GetState masks out the Guide button! That's a pretty solid proof both functions need the same struct.