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

WiimoteReal: Fix Dolphin shutdown crash #9948

Merged

Conversation

Dentomologist
Copy link
Contributor

Fix a race condition in the WiimoteScanner thread which can cause a crash on Dolphin shutdown. This crash occurs when the following happens:

  • Scanner: runs its main loop while m_scan_thread_running is set
  • Main thread: calls WiimoteScanner::StopThread() which calls m_scan_thread_running.TestAndClear()
  • Main thread: starts iterating over m_backends, using backend as a reference to each element
  • Scan thread: finishes its current iteration of the loop. m_scan_thread_running is no longer set, so it breaks out of the loop.
  • Scan thread: creates a lock_guard on m_backends_mutex then calls m_backends.clear(), calling the destructors of the underlying unique_ptrs
  • Main thread: tries to dereference backend and crashes

This fix simply defers clearing the m_scan_thread_running flag until after the main thread has finished iterating over the backends.

I considered locking the m_backends_mutex from the main thread instead, but this would create a new problem. The reason the main thread iterates over the backends is to call RequestStopSearching(), which does nothing on most platforms but sets m_stop_scanning on macOS. This variable is used as a flag to break out of the scan loop in WiimoteScannerDarwin::FindWiimotes, and if it's not set I believe the function never returns (unless it finds a new device). If the scan thread locked the mutex first it would clear m_backends and the main thread wouldn't have any backends to call RequestStopSearching on.

The only other reason we would need to lock the mutex in the main thread would be if the call to RequestStopSearching had a race condition, but it doesn't. Once m_stop_scanning is set to true it stays set, so even if the loop goes into another iteration FindWiimotes will stop looping immediately, allowing the scan thread to exit its loop shortly after the main loop clears m_scan_thread_running.

Clear m_scan_thread_running later to avoid accessing m_backends after it
has been cleared.
@Tilka Tilka merged commit 4b022a4 into dolphin-emu:master Aug 2, 2021
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants