search

FNA-XNA / FAudio

FAudio - Accuracy-focused XAudio reimplementation for open platforms
https://fna-xna.github.io/
Other
546 stars 73 forks source link

Don't destroy voice when it is output to other voices #331

Closed gofman closed 7 months ago

gofman commented 7 months ago

Cosmoteer (Steam game id 799600) sometimes destroys a submix voice which is an output for another voice. That leads to use after free access in audio client thread performing mixing which leads to obscure random crashes.

MS docs [1] suggest that IXAudio2Voice::DestroyVoice fails in such a case ("If any other voice is currently sending audio to this voice, the method fails.").

I made a test on top of Wine xaudio2 tests which indirectly confirms that (attaching a patch). I don't immediately see a direct way to test if DestroyVoice actually failed or not as it doesn't return any status. However, I found that after successful voice destruction IXAudio2SubmixVoice_GetVoiceDetails() for that voice crashes on Windows and using this fact to probe for actual voice destruction (the first one in the test doesn't crash as, the second one guarded with if (0) crashes for me on Win10).

  1. https://learn.microsoft.com/en-us/windows/win32/api/xaudio2/nf-xaudio2-ixaudio2voice-destroyvoice

0001-xaudio2-tests-Test-destroying-a-voice-which-is-an-ou.txt

flibitijibibo commented 7 months ago

Actually it turns out the signature is correct...

https://learn.microsoft.com/en-us/windows/win32/api/xaudio2/nf-xaudio2-ixaudio2voice-destroyvoice

... but honestly I'd be up for exporting a new function since being able to see this failure seems important. DestroyVoiceSafeEXT maybe?

gofman commented 7 months ago

Yes, sure. I did it this way because technically it doesn't break the ABI, if old client uses void version and doesn't check value it is not broken with newer version returning the value. But it is looks cleaner to have a different function. I think both old and new one will check if the voice is in use and the difference will be only in the presence return value. Or do you want the old version work like before?

flibitijibibo commented 7 months ago

What we can probably do is have this in the header...

FAUDIOAPI void FAudioVoice_DestroyVoice(...);

FAUDIOAPI uint32_t FAudioVoice_DestroyVoiceSafeEXT(...);

... and then we'll rename the implementation we have now to DestroyVoiceSafeEXT, and DestroyVoice can call DestroyVoiceSafeEXT and discard the return value entirely. It's a bit roundabout, but we've done similar things with other extensions like voice filters and it's been easy enough to keep stable!