search

ColinPitrat / caprice32

An emulator of the Amstrad CPC 8bit home computer range.
GNU General Public License v2.0
148 stars 32 forks source link

UAF fix proposal. Not always reproducible but with two different apps #195

Closed devnexen closed 3 years ago

devnexen commented 3 years ago

at shutdown.

ColinPitrat commented 3 years ago

Do you have more details where the UAF happens?

This patch is unlikely to fix any issue because pbSndBuffer is copied in CPC.snd_bufferptr and used from there. Even if pbSndBuffer is used directly, setting it to nullptr will transform the UAF in a segfault.

As for audio_spec, it's not used anywhere outside of audio_init. I don't think we need to keep it so it should be moved there.

ColinPitrat commented 3 years ago

Note: I removed audio_spec as it is unused, and replaced the desired & obtained pointers by local heap-allocated variables: https://github.com/ColinPitrat/caprice32/commit/668d9bc1a610a889c9222ac46a870d01359c0c0d

ColinPitrat commented 3 years ago

OK, the UAF can happen because audio_update is passed as a callback to SDL_OpenAudioDevice. It seems that with the replacement of SDL_OpenAudio by SDL_OpenAudioDevice, I should also have replaced SDL_CloseAudio by SDL_CloseAudioDevice.

From https://wiki.libsdl.org/SDL_CloseAudio:

This function is equivalent to calling SDL_CloseAudioDevice(1); and is only useful if you used the legacy SDL_OpenAudio() function.

The deviceId returned by SDL_OpenAudioDevice is usually not 1 so this has to be changed.

devnexen commented 3 years ago

unfortunately it s kind of random, I tried to reproduce but it took me by surprise in two very different moments and apps, maybe due to underlying race conditions with global vars ? Would it be possible in the mid/long term using more smart pointers rather than C's allocation ?

devnexen commented 3 years ago

OK, the UAF can happen because audio_update is passed as a callback to SDL_OpenAudioDevice. It seems that with the replacement of SDL_OpenAudio by SDL_OpenAudioDevice, I should also have replaced SDL_CloseAudio by SDL_CloseAudioDevice.

From https://wiki.libsdl.org/SDL_CloseAudio:

This function is equivalent to calling SDL_CloseAudioDevice(1); and is only useful if you used the legacy SDL_OpenAudio() function.

The deviceId returned by SDL_OpenAudioDevice is usually not 1 so this has to be changed.

ah right ok

ColinPitrat commented 3 years ago

This should fix it: https://github.com/ColinPitrat/caprice32/commit/13f0a2a6350dd4a0f6053b67a067f378f241fc51

ColinPitrat commented 3 years ago

Thanks for finding this issue. Please report if you run in any other problem!

ColinPitrat commented 3 years ago

Would it be possible in the mid/long term using more smart pointers rather than C's allocation ?

Modernizing the code is definitely a good thing. It's being done bit by bit and is not always trivial, but any help is welcome :-) An easy pick is to replace malloc/free by new/delete for example, or even better objects on the stack. Removing global variables is also part of the effort but tends to be slightly harder. Adding unit tests in the process is also nice.

devnexen commented 3 years ago

This should fix it: 13f0a2a

Not entirely but it s mostly build breakage :)