From 54b4efff6b14a2dc0be6d6a284be8b27750ceecb Mon Sep 17 00:00:00 2001 From: mathieui Date: Sat, 30 Apr 2016 13:29:11 +0200 Subject: [PATCH 1/2] GCAdapter: improve thread safety MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit make sure Reset() can’t be run concurrently with AddGCAdapter() or ResetRumble() (which is called on other threads) which can cause crashes (issue #9462) --- Source/Core/InputCommon/GCAdapter.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/Source/Core/InputCommon/GCAdapter.cpp b/Source/Core/InputCommon/GCAdapter.cpp index eceed1ed0b..4728a9f49f 100644 --- a/Source/Core/InputCommon/GCAdapter.cpp +++ b/Source/Core/InputCommon/GCAdapter.cpp @@ -22,6 +22,7 @@ namespace GCAdapter { static bool CheckDeviceAccess(libusb_device* device); static void AddGCAdapter(libusb_device* device); +static void ResetRumbleLockNeeded(); static bool s_detected = false; static libusb_device_handle* s_handle = nullptr; @@ -37,6 +38,7 @@ static std::atomic s_controller_payload_size = {0}; static std::thread s_adapter_thread; static Common::Flag s_adapter_thread_running; +static std::mutex s_init_mutex; static std::thread s_adapter_detect_thread; static Common::Flag s_adapter_detect_thread_running; @@ -77,7 +79,10 @@ static int HotplugCallback(libusb_context* ctx, libusb_device* dev, libusb_hotpl if (event == LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED) { if (s_handle == nullptr && CheckDeviceAccess(dev)) + { + std::lock_guard lk(s_init_mutex); AddGCAdapter(dev); + } } else if (event == LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT) { @@ -115,6 +120,7 @@ static void ScanThreadFunc() { if (s_handle == nullptr) { + std::lock_guard lk(s_init_mutex); Setup(); if (s_detected && s_detect_callback != nullptr) s_detect_callback(); @@ -306,7 +312,7 @@ static void AddGCAdapter(libusb_device* device) s_detected = true; if (s_detect_callback != nullptr) s_detect_callback(); - ResetRumble(); + ResetRumbleLockNeeded(); } void Shutdown() @@ -329,6 +335,9 @@ void Shutdown() void Reset() { + std::unique_lock lock(s_init_mutex, std::defer_lock); + if (!lock.try_lock()) + return; if (!s_detected) return; @@ -439,10 +448,20 @@ bool UseAdapter() void ResetRumble() { - if (!UseAdapter()) + std::unique_lock lock(s_init_mutex, std::defer_lock); + if (!lock.try_lock()) return; - if (s_handle == nullptr || !s_detected) + ResetRumbleLockNeeded(); +} + +// Needs to be called when s_init_mutex is locked in order to avoid +// being called while the libusb state is being reset +static void ResetRumbleLockNeeded() +{ + if (!UseAdapter() || (s_handle == nullptr || !s_detected)) + { return; + } std::fill(std::begin(s_controller_rumble), std::end(s_controller_rumble), 0); From 1bbbe92cd203d39f0aa0da80d5bd41ae641c3e5c Mon Sep 17 00:00:00 2001 From: mathieui Date: Thu, 19 May 2016 21:52:44 +0200 Subject: [PATCH 2/2] GCAdapter: protect some more functions Reset() and Setup() are not used outside of this namespace --- Source/Core/InputCommon/GCAdapter.cpp | 6 ++++-- Source/Core/InputCommon/GCAdapter.h | 2 -- Source/Core/InputCommon/GCAdapter_Android.cpp | 7 +++++-- Source/Core/InputCommon/GCAdapter_Null.cpp | 2 -- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Source/Core/InputCommon/GCAdapter.cpp b/Source/Core/InputCommon/GCAdapter.cpp index 4728a9f49f..adc0445697 100644 --- a/Source/Core/InputCommon/GCAdapter.cpp +++ b/Source/Core/InputCommon/GCAdapter.cpp @@ -23,6 +23,8 @@ namespace GCAdapter static bool CheckDeviceAccess(libusb_device* device); static void AddGCAdapter(libusb_device* device); static void ResetRumbleLockNeeded(); +static void Reset(); +static void Setup(); static bool s_detected = false; static libusb_device_handle* s_handle = nullptr; @@ -183,7 +185,7 @@ void StopScanThread() } } -void Setup() +static void Setup() { libusb_device** list; ssize_t cnt = libusb_get_device_list(s_libusb_context, &list); @@ -333,7 +335,7 @@ void Shutdown() s_libusb_driver_not_supported = false; } -void Reset() +static void Reset() { std::unique_lock lock(s_init_mutex, std::defer_lock); if (!lock.try_lock()) diff --git a/Source/Core/InputCommon/GCAdapter.h b/Source/Core/InputCommon/GCAdapter.h index a1780cdf1e..5568b0ae9d 100644 --- a/Source/Core/InputCommon/GCAdapter.h +++ b/Source/Core/InputCommon/GCAdapter.h @@ -19,9 +19,7 @@ enum ControllerTypes CONTROLLER_WIRELESS = 2 }; void Init(); -void Reset(); void ResetRumble(); -void Setup(); void Shutdown(); void SetAdapterCallback(std::function func); void StartScanThread(); diff --git a/Source/Core/InputCommon/GCAdapter_Android.cpp b/Source/Core/InputCommon/GCAdapter_Android.cpp index 46ec417f2b..b05a066478 100644 --- a/Source/Core/InputCommon/GCAdapter_Android.cpp +++ b/Source/Core/InputCommon/GCAdapter_Android.cpp @@ -24,6 +24,9 @@ extern JavaVM* g_java_vm; namespace GCAdapter { +static void Setup(); +static void Reset(); + // Java classes static jclass s_adapter_class; @@ -207,7 +210,7 @@ void Init() StartScanThread(); } -void Setup() +static void Setup() { s_fd = 0; s_detected = true; @@ -220,7 +223,7 @@ void Setup() s_read_adapter_thread = std::thread(Read); } -void Reset() +static void Reset() { if (!s_detected) return; diff --git a/Source/Core/InputCommon/GCAdapter_Null.cpp b/Source/Core/InputCommon/GCAdapter_Null.cpp index b7bc81dfa7..d7f13c2917 100644 --- a/Source/Core/InputCommon/GCAdapter_Null.cpp +++ b/Source/Core/InputCommon/GCAdapter_Null.cpp @@ -9,9 +9,7 @@ namespace GCAdapter { void Init() {} -void Reset() {} void ResetRumble() {} -void Setup() {} void Shutdown() {} void SetAdapterCallback(std::function func) {} void StartScanThread() {}