From 3b97a7bded04912e97bd510209a52d859e08d006 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Thu, 6 Nov 2025 11:38:29 -0800 Subject: [PATCH] CubebStream: Use WorkQueueThread::PushBlocking instead of sync_event Push and wait on WorkQueueThread items using PushBlocking. Previously we created a Common::Event sync_event on the caller's stack, called Wait on it, then had the WorkQueueThread call Set on the sync_event once the thread was done. In addition to being simpler the new way avoids a use-after-free that could happen in convoluted and unlikely yet possible thread scheduling sequences. One such case can be triggered as follows: * Set your audio backend to Cubeb * In CubebStream::SetVolume set a breakpoint at the call to Wait and at the call to cubeb_stream_set_volume. * Start a game. * Continue until the Cubeb Worker thread hits the cubeb_stream_set_volume breakpoint and Emuthread hits the Wait breakpoint, freezing each thread when it hits its breakpoint. * Unfreeze Cubeb Worker. * In Event::Set set a breakpoint at the end of the scope containing the lock_guard such that the guard has been constructed but not destructed when the breakpoint is hit. * Continue until that breakpoint is hit by Cubeb Worker. If other threads hit it first keep going. * Freeze Cubeb Worker. * For convenience remove the breakpoint in Event::Set so other threads don't trigger it. * In CubebStream::SetRunning set a breakpoint at the call to Wait. * Unfreeze Emuthread and continue until the breakpoint is hit. * In Cubeb Worker go to Event::Set and examine the values of m_mutex's member variables. In Visual Studio Debug these are locking_thread_id == 0xcccccc01 and ownership_levels == 0xcccccccc. This is the result of Visual Studio overwriting the memory used on the stack by sync_event in CubebStream::SetVolume with cc bytes to represent uninitialized memory on the stack (since that function already returned), and then allocating enough memory on the stack when calling AudioCommon::SetSoundStreamRunning and then CubebStream::SetRunning that it overwrote one byte of the memory formerly occupied by locking_thread_id. * If you unfreeze Cubeb Worker at this point it will trigger the lock guard's destructor which will then try to unlock m_mutex. Since m_mutex is no longer in scope this is a use-after-free, and in VS debug triggers a debug assert due to locking_thread_id not matching the current thread id. --- Source/Core/AudioCommon/CubebStream.cpp | 26 ++++---------------- Source/Core/Core/HW/EXI/EXI_DeviceMic.cpp | 30 +++++------------------ 2 files changed, 11 insertions(+), 45 deletions(-) diff --git a/Source/Core/AudioCommon/CubebStream.cpp b/Source/Core/AudioCommon/CubebStream.cpp index e602691a389..1f7fb5cf450 100644 --- a/Source/Core/AudioCommon/CubebStream.cpp +++ b/Source/Core/AudioCommon/CubebStream.cpp @@ -40,14 +40,11 @@ CubebStream::CubebStream() #ifdef _WIN32 : m_work_queue("Cubeb Worker") { - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { auto const result = CoInitializeEx(nullptr, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); m_coinit_success = result == S_OK; m_should_couninit = result == S_OK || result == S_FALSE; }); - sync_event.Wait(); } #else = default; @@ -60,11 +57,8 @@ bool CubebStream::Init() #ifdef _WIN32 if (!m_coinit_success) return false; - Common::Event sync_event; - m_work_queue.Push([this, &return_value, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this, &return_value] { #endif - m_ctx = CubebUtils::GetContext(); if (m_ctx) { @@ -98,7 +92,6 @@ bool CubebStream::Init() #ifdef _WIN32 }); - sync_event.Wait(); #endif return return_value; @@ -111,9 +104,7 @@ bool CubebStream::SetRunning(bool running) #ifdef _WIN32 if (!m_coinit_success) return false; - Common::Event sync_event; - m_work_queue.Push([this, running, &return_value, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this, running, &return_value] { #endif if (running) return_value = cubeb_stream_start(m_stream) == CUBEB_OK; @@ -121,7 +112,6 @@ bool CubebStream::SetRunning(bool running) return_value = cubeb_stream_stop(m_stream) == CUBEB_OK; #ifdef _WIN32 }); - sync_event.Wait(); #endif return return_value; @@ -130,9 +120,7 @@ bool CubebStream::SetRunning(bool running) CubebStream::~CubebStream() { #ifdef _WIN32 - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { #endif cubeb_stream_stop(m_stream); cubeb_stream_destroy(m_stream); @@ -144,7 +132,6 @@ CubebStream::~CubebStream() } m_coinit_success = false; }); - sync_event.Wait(); #endif m_ctx.reset(); } @@ -154,13 +141,10 @@ void CubebStream::SetVolume(int volume) #ifdef _WIN32 if (!m_coinit_success) return; - Common::Event sync_event; - m_work_queue.Push([this, volume, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this, volume] { #endif cubeb_stream_set_volume(m_stream, volume / 100.0f); #ifdef _WIN32 }); - sync_event.Wait(); #endif } diff --git a/Source/Core/Core/HW/EXI/EXI_DeviceMic.cpp b/Source/Core/Core/HW/EXI/EXI_DeviceMic.cpp index 995376486e0..0c336073002 100644 --- a/Source/Core/Core/HW/EXI/EXI_DeviceMic.cpp +++ b/Source/Core/Core/HW/EXI/EXI_DeviceMic.cpp @@ -36,14 +36,11 @@ void CEXIMic::StreamInit() #ifdef _WIN32 if (!m_coinit_success) return; - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { #endif m_cubeb_ctx = CubebUtils::GetContext(); #ifdef _WIN32 }); - sync_event.Wait(); #endif } @@ -56,14 +53,11 @@ void CEXIMic::StreamTerminate() #ifdef _WIN32 if (!m_coinit_success) return; - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { #endif m_cubeb_ctx.reset(); #ifdef _WIN32 }); - sync_event.Wait(); #endif } } @@ -104,9 +98,7 @@ void CEXIMic::StreamStart() #ifdef _WIN32 if (!m_coinit_success) return; - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { #endif // Open stream with current parameters stream_size = buff_size_samples * 500; @@ -142,7 +134,6 @@ void CEXIMic::StreamStart() INFO_LOG_FMT(EXPANSIONINTERFACE, "started cubeb stream"); #ifdef _WIN32 }); - sync_event.Wait(); #endif } @@ -151,9 +142,7 @@ void CEXIMic::StreamStop() if (m_cubeb_stream) { #ifdef _WIN32 - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { #endif if (cubeb_stream_stop(m_cubeb_stream) != CUBEB_OK) ERROR_LOG_FMT(EXPANSIONINTERFACE, "Error stopping cubeb stream"); @@ -161,7 +150,6 @@ void CEXIMic::StreamStop() m_cubeb_stream = nullptr; #ifdef _WIN32 }); - sync_event.Wait(); #endif } @@ -217,14 +205,11 @@ CEXIMic::CEXIMic(Core::System& system, int index) next_int_ticks = 0; #ifdef _WIN32 - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { auto result = ::CoInitializeEx(nullptr, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); m_coinit_success = result == S_OK; m_should_couninit = result == S_OK || result == S_FALSE; }); - sync_event.Wait(); #endif StreamInit(); @@ -237,13 +222,10 @@ CEXIMic::~CEXIMic() #ifdef _WIN32 if (m_should_couninit) { - Common::Event sync_event; - m_work_queue.Push([this, &sync_event] { - Common::ScopeGuard sync_event_guard([&sync_event] { sync_event.Set(); }); + m_work_queue.PushBlocking([this] { m_should_couninit = false; CoUninitialize(); }); - sync_event.Wait(); } m_coinit_success = false; #endif