Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Externals/FreeSurround: Fix pointer created through new[] being freed via delete #11573

Merged
merged 1 commit into from Feb 16, 2023

Conversation

Pokechu22
Copy link
Contributor

Doing so is not allowed (presumably because compilers are allowed to use a different algorithm for allocating between the two/store extra data such as the length of the array before the pointer).

This bug existed in the original implementation at https://web.archive.org/web/20140708092159/http://www.hydrogenaud.io/forums/index.php?showtopic=52235 and causes Valgrind to emit a warning. Note that this ended up happening even if DSPLLE and the DPL decoder are not enabled in Dolphin.

Here's the warning in question:
==115== Thread 12 Emuthread - Sta:
==115== Mismatched free() / delete / delete []
==115==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==115==    by 0xBBA07B: DPL2FSDecoder::~DPL2FSDecoder() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAFAB5: AudioCommon::SurroundDecoder::~SurroundDecoder() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAF9D7: Mixer::~Mixer() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB1370: AlsaSound::~AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB13BC: AlsaSound::~AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBABE7C: AudioCommon::InitSoundStream(Core::System&) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x560699: Core::EmuThread(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x56203A: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo), std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo> > >::_M_run() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xA574DE3: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==115==    by 0xA483608: start_thread (pthread_create.c:477)
==115==    by 0xA8EE132: clone (clone.S:95)
==115==  Address 0x1d863110 is 0 bytes inside a block of size 41,248 alloc'd
==115==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==115==    by 0xBC0DB4: kiss_fftr_alloc (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBBABF9: DPL2FSDecoder::Init(channel_setup, unsigned int, unsigned int) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAF736: Mixer::Mixer(unsigned int) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB1620: AlsaSound::AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAAE65: AudioCommon::CreateSoundStreamForBackend(std::basic_string_view<char, std::char_traits<char> >) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBABB60: AudioCommon::InitSoundStream(Core::System&) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x560699: Core::EmuThread(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x56203A: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo), std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo> > >::_M_run() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xA574DE3: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==115==    by 0xA483608: start_thread (pthread_create.c:477)
==115==    by 0xA8EE132: clone (clone.S:95)
==115==
==115== Mismatched free() / delete / delete []
==115==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==115==    by 0xBBA08C: DPL2FSDecoder::~DPL2FSDecoder() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAFAB5: AudioCommon::SurroundDecoder::~SurroundDecoder() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAF9D7: Mixer::~Mixer() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB1370: AlsaSound::~AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB13BC: AlsaSound::~AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBABE7C: AudioCommon::InitSoundStream(Core::System&) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x560699: Core::EmuThread(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x56203A: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo), std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo> > >::_M_run() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xA574DE3: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==115==    by 0xA483608: start_thread (pthread_create.c:477)
==115==    by 0xA8EE132: clone (clone.S:95)
==115==  Address 0xdd08330 is 0 bytes inside a block of size 41,248 alloc'd
==115==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==115==    by 0xBC0DB4: kiss_fftr_alloc (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBBAC11: DPL2FSDecoder::Init(channel_setup, unsigned int, unsigned int) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAF736: Mixer::Mixer(unsigned int) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBB1620: AlsaSound::AlsaSound() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBAAE65: AudioCommon::CreateSoundStreamForBackend(std::basic_string_view<char, std::char_traits<char> >) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xBABB60: AudioCommon::InitSoundStream(Core::System&) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x560699: Core::EmuThread(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo) (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0x56203A: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo), std::unique_ptr<BootParameters, std::default_delete<BootParameters> >, WindowSystemInfo> > >::_M_run() (in /mnt/c/users/pokechu22/source/repos/dolphin/WSL/Binaries/dolphin-emu)
==115==    by 0xA574DE3: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==115==    by 0xA483608: start_thread (pthread_create.c:477)
==115==    by 0xA8EE132: clone (clone.S:95)

As far as I can tell, there isn't really an upstream for FreeSurround anymore, and we've already modified it a fair bit (#5235 modified it). So perhaps we should move it out of externals and into the main codebase?

… via delete

Doing so is not allowed (presumably because compilers are allowed to use a different algorithm for allocating between the two/store extra data such as the length of the array before the pointer).

This bug existed in the original implementation at https://web.archive.org/web/20140708092159/http://www.hydrogenaud.io/forums/index.php?showtopic=52235 and causes Valgrind to emit a warning. Note that this ended up happening even if DSPLLE and the DPL decoder are not enabled in Dolphin.
@lioncash lioncash merged commit f004080 into dolphin-emu:master Feb 16, 2023
@Zopolis4
Copy link
Contributor

Alternativley, would it make sense to host this as a dolphin-emu organization library? If people other than us could possibly find value in it, it makes a lot more sense to host it as a library rather than integrated into our codebase. Espescially if it was originally distributed as a library.

@Pokechu22
Copy link
Contributor Author

FreeSurround is (as far as I can tell) originally distributed as a plugin for foobar2000, an audio player program. FreeSurround also uses the kissfft library which does exist elsewhere, but the version in FreeSurround seems to have been modified to use C++ (I'm not sure what exactly has been changed, if anything). I don't think dolphin currently has another FFT library, so it might make sense to move FreeSurround into dolphin code and add a more recent version of kissfft to externals, but I'm not sure of that.

@shuffle2
Copy link
Contributor

Can we just use https://learn.microsoft.com/en-us/windows/win32/medfound/dolby-audio-decoder on windows at least?

@Pokechu22
Copy link
Contributor Author

That says it only supports "Dolby Digital, also called Dolby AC-3" and "Dolby Digital Plus, also called Enhanced AC-3 (E-AC-3)"; to my understanding Dolby Digital is different from and incompatible with Dolby Pro Logic II. Dolby Pro Logic is, to my understanding, based on embedding surround data into a 2-channel stereo audio signal (while that signal also works as a regular stereo signal), while the page you linked says it only works with "raw Dolby streams" (whatever that means).

I don't have a surround setup personally so I don't really have a good way of checking if things work, unfortunately.

@shuffle2
Copy link
Contributor

shuffle2 commented Feb 16, 2023

I wouldn't be surprised if DPL2 is supported somehow by mucking around in windows audio decoder stuff. For example there is KSNODETYPE_PROLOGIC_DECODER and some mentions of ProLogic in headers. I also wouldn't be surprised if it's quite unclear how to get it to work unless you're familiar with windows audio system, tho.

ffmpeg (which we already also depend on) also seems to have some mentions of dpl2, although i'm not sure if it fits our needs.

I don't have a surround setup personally so I don't really have a good way of checking if things work, unfortunately.

seems we're using freesurround to decode the byte buffer (to another buffer)? So could just compare output bytes?

anyway, just throwing some ideas out to replace the cruft.

FreeSurround appears to be almost single-file so 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants