From dd1f5f9726c2610870b6481122d445e564ebe6e8 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Thu, 21 Dec 2023 15:24:31 -0800 Subject: [PATCH 1/2] NetKDRequestDevice: Fix use-after-free crash Explicitly shut down work queues in NetKDRequestDevice's destructor to prevent their threads from accessing members after they've been freed. This crash would occur sporadically if NetKDRequestDevice's periodic download or mail checks happened to overlap with emulation shutdown in the wrong way. An example sequence of events that could cause the crash: * m_scheduler_timer_thread queues a periodic Download event in m_scheduler_work_queue, then waits for m_shutdown_event. * A request to stop emulation results in s_ios being reset by the CPU thread. This triggers NetKDRequestDevice's destructor which sets m_shutdown_event and joins m_scheduler_timer_thread. * m_scheduler_timer_thread wakes from m_shutdown_event and returns from its thread function, ending the thread. * The CPU thread resumes execution at the end of NetKDRequestDevice's destructor and begins destroying NetKDRequestDevice's members in reverse declaration order. * m_http is declared after m_scheduler_work_queue and is therefore destroyed earlier. * m_scheduler_work_queue's destructor calls its Shutdown function, which by default finishes the work items in the queue. * The queued Download event calls KDDownload which calls m_http.Get() which calls Fetch() which passes garbage data from the freed m_curl into curl_easy_setopt(). * Curl promptly crashes. Shutting down the work queues manually in the destructor prevents the above because m_http and the other members don't get freed until after the queue threads finish. --- Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp b/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp index 3d4dc35613..c2420a9dd1 100644 --- a/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp +++ b/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp @@ -190,6 +190,8 @@ NetKDRequestDevice::~NetKDRequestDevice() } m_scheduler_timer_thread.join(); + m_scheduler_work_queue.Shutdown(); + m_work_queue.Shutdown(); } void NetKDRequestDevice::Update() From ecf4f1b1f9309151ce82c2016181aa10d1a7f2de Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 24 Dec 2023 14:55:11 -0800 Subject: [PATCH 2/2] NetKDRequestDevice: Fix nullptr dereference crash Keep a shared_ptr to NetKDTimeDevice inside NetKDRequestDevice. This allows the KDDownload task to finish its work without potentially trying to dereference nullptr, which can potentially come from either GetIOS() or GetDeviceByName() if EmulationKernel's destructor has started running. --- Source/Core/Core/IOS/IOS.cpp | 7 +++++-- .../Core/Core/IOS/Network/KD/NetKDRequest.cpp | 17 ++++++----------- Source/Core/Core/IOS/Network/KD/NetKDRequest.h | 6 +++++- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/Source/Core/Core/IOS/IOS.cpp b/Source/Core/Core/IOS/IOS.cpp index 9e73644a22..35c06626a3 100644 --- a/Source/Core/Core/IOS/IOS.cpp +++ b/Source/Core/Core/IOS/IOS.cpp @@ -577,8 +577,11 @@ void EmulationKernel::AddStaticDevices() } if (HasFeature(features, Feature::KD)) { - AddDevice(std::make_unique(*this, "/dev/net/kd/request")); - AddDevice(std::make_unique(*this, "/dev/net/kd/time")); + constexpr auto time_device_name = "/dev/net/kd/time"; + AddDevice(std::make_unique(*this, time_device_name)); + const auto time_device = + std::static_pointer_cast(GetDeviceByName(time_device_name)); + AddDevice(std::make_unique(*this, "/dev/net/kd/request", time_device)); } if (HasFeature(features, Feature::NCD)) { diff --git a/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp b/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp index c2420a9dd1..56bda89f65 100644 --- a/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp +++ b/Source/Core/Core/IOS/Network/KD/NetKDRequest.cpp @@ -153,9 +153,10 @@ s32 NWC24MakeUserID(u64* nwc24_id, u32 hollywood_id, u16 id_ctr, HardwareModel h } } // Anonymous namespace -NetKDRequestDevice::NetKDRequestDevice(EmulationKernel& ios, const std::string& device_name) +NetKDRequestDevice::NetKDRequestDevice(EmulationKernel& ios, const std::string& device_name, + const std::shared_ptr& time_device) : EmulationDevice(ios, device_name), m_config{ios.GetFS()}, m_dl_list{ios.GetFS()}, - m_send_list{ios.GetFS()}, m_friend_list{ios.GetFS()} + m_send_list{ios.GetFS()}, m_friend_list{ios.GetFS()}, m_time_device{time_device} { // Enable all NWC24 permissions m_scheduler_buffer[1] = Common::swap32(-1); @@ -443,9 +444,7 @@ NWC24::ErrorCode NetKDRequestDevice::DetermineDownloadTask(u16* entry_index, // As the scheduler does not tell us which entry to download, we must determine that. // A correct entry is one that hasn't been downloaded the longest compared to other entries. // We first need current UTC. - const auto time_device = - std::static_pointer_cast(GetIOS()->GetDeviceByName("/dev/net/kd/time")); - const u64 current_utc = time_device->GetAdjustedUTC(); + const u64 current_utc = m_time_device->GetAdjustedUTC(); u64 lowest_timestamp = std::numeric_limits::max(); for (u16 i = 0; i < static_cast(NWC24::NWC24Dl::MAX_ENTRIES); i++) @@ -495,9 +494,7 @@ NWC24::ErrorCode NetKDRequestDevice::DetermineSubtask(u16 entry_index, if (m_dl_list.IsSubtaskDownloadDisabled(entry_index)) return NWC24::WC24_ERR_DISABLED; - const auto time_device = - std::static_pointer_cast(GetIOS()->GetDeviceByName("/dev/net/kd/time")); - const u64 current_utc = time_device->GetAdjustedUTC(); + const u64 current_utc = m_time_device->GetAdjustedUTC(); for (u8 i = 0; i < 32; i++) { if (!m_dl_list.IsValidSubtask(entry_index, i)) @@ -647,9 +644,7 @@ NWC24::ErrorCode NetKDRequestDevice::KDDownload(const u16 entry_index, { bool success = false; Common::ScopeGuard state_guard([&] { - const auto time_device = - std::static_pointer_cast(GetIOS()->GetDeviceByName("/dev/net/kd/time")); - const u64 current_utc = time_device->GetAdjustedUTC(); + const u64 current_utc = m_time_device->GetAdjustedUTC(); if (success) { // Set the next download time to the dl_margin diff --git a/Source/Core/Core/IOS/Network/KD/NetKDRequest.h b/Source/Core/Core/IOS/Network/KD/NetKDRequest.h index 0fcd43f52d..848454fa71 100644 --- a/Source/Core/Core/IOS/Network/KD/NetKDRequest.h +++ b/Source/Core/Core/IOS/Network/KD/NetKDRequest.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include @@ -17,6 +18,7 @@ #include "Core/IOS/Network/KD/Mail/WC24Send.h" #include "Core/IOS/Network/KD/NWC24Config.h" #include "Core/IOS/Network/KD/NWC24DL.h" +#include "Core/IOS/Network/KD/NetKDTime.h" namespace IOS::HLE { @@ -26,7 +28,8 @@ namespace IOS::HLE class NetKDRequestDevice : public EmulationDevice { public: - NetKDRequestDevice(EmulationKernel& ios, const std::string& device_name); + NetKDRequestDevice(EmulationKernel& ios, const std::string& device_name, + const std::shared_ptr& time_device); IPCReply HandleNWC24DownloadNowEx(const IOCtlRequest& request); NWC24::ErrorCode KDDownload(const u16 entry_index, const std::optional subtask_id); IPCReply HandleNWC24CheckMailNow(const IOCtlRequest& request); @@ -114,6 +117,7 @@ private: std::queue m_async_replies; u32 m_error_count = 0; std::array m_scheduler_buffer{}; + std::shared_ptr m_time_device; // TODO: Maybe move away from Common::HttpRequest? Common::HttpRequest m_http{std::chrono::minutes{1}}; u32 m_download_span = 2;