dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

AudioCommon: Fix scratch buffer overflow

Open stblr opened this issue 2 years ago • 8 comments

This would overwrite the std::FILE pointer in IOFile in WaveFileWriter and cause a segfault in the IOFile destructor when stopping a game.

I'm not familiar with audio code so maybe there is a better way to do it, but at least this fixes the crash.

stblr avatar May 15 '22 11:05 stblr

Wouldn't it be simpler to limit available_samples so it can't be bigger than MAX_SAMPLES?

JosJuice avatar May 15 '22 11:05 JosJuice

I don't think this would work in the case of MixSurround.

stblr avatar May 15 '22 11:05 stblr

Only fixed conflicts, no code changes.

stblr avatar Aug 03 '22 21:08 stblr

I fixed the same issue in #10741 (446e4fbbb0156383ffffadaadf5300d54378c777), though I specifically fixed the broken bounds check in WaveFile so that no audio would be recorded in those broken cases (and separately changed the audio code so that the amount of samples written at any time is much smaller).

Out of curiosity, what games did you run into this issue in? The only ones I've seen it happen in are unlicensed titles by Datel.

Pokechu22 avatar Aug 03 '22 21:08 Pokechu22

On my Mario Kart Wii mod which makes extensive use of the /dev/dolphin speed limit ioctl to speed up loading times (which made the bug so prevalent I think). But I haven't experienced the issue in a long time (something certainly changed either in Dolphin or in my own code but I have no idea what).

stblr avatar Aug 03 '22 23:08 stblr

I guess more importantly, did you have audio dumping (Movie → Dump Audio) enabled when the crashes occurred? (Are there a bunch of files in Dolphin Emulator/Dump/Audio?) If you didn't have it enabled, then that would have to be a separate overflow from the one I experienced and have a fix for (perhaps Mixer::m_scratch_buffer overflowing instead of WaveFileWriter::conv_buffer overflowing).

Pokechu22 avatar Aug 03 '22 23:08 Pokechu22

Audio dumping was not enabled.

stblr avatar Aug 19 '22 14:08 stblr

I've created #11011 to add some assertions to hopefully make it more obvious when this overflow occurs. I don't think a resizable buffer is the right choice, but I'm also not sure when the overflow occurs in practice.

Pokechu22 avatar Aug 25 '22 00:08 Pokechu22

I trigger one of the asserts from #11011 attempting to load MKW-SP on Dolphin 5.0-17995 on Windows 10. image image

riidefi avatar Dec 25 '22 17:12 riidefi