NetKDRequestDevice: Fix sporadic crashes during emulation shutdown #12463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Dentomologist
commented
Dec 25, 2023
Comment on lines
+194
to
+195
| m_scheduler_work_queue.Shutdown(); | ||
| m_work_queue.Shutdown(); |
There was a problem hiding this comment.
Calling Shutdown(true) would cancel the remaining items and be a bit faster, but I'm not familiar enough with NetKD to know if that would cause any problems.
|
Makes sense to me, though I suspect it's a code smell that our IOS Device lifetimes aren't well-defined during destruction. I don't think it should be necessary for individual IOS Devices to hold shared_ptrs to eachother, the main Kernel should manage the lifetimes... You don't need to fix this here but yeah. |
sepalani
reviewed
Dec 25, 2023
Comment on lines
19
to
+21
| #include "Core/IOS/Network/KD/NWC24Config.h" | ||
| #include "Core/IOS/Network/KD/NWC24DL.h" | ||
| #include "Core/IOS/Network/KD/NetKDTime.h" |
There was a problem hiding this comment.
It seems clang-format doesn't sort it alphabetically correctly (or rather its sorting is not case insensitive, probably ASCII based since lowercase is after uppercase). Feel free to ignore this comment since there is not much you can do as it isn't related to this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the last few months I've had a handful of random crashes when shutting down emulation which happened in NetKDRequestDevice::KDDownload().
The first was the result of using-after-freeing of some of NetKDRequestDevices's members by its WorkQueueThreads, which this PR resolves by calling the latter's Shutdown function in NetKDRequestDevice's destructor so the threads are finished before destroying any members.
The other crash was caused by one of two possible nullptr dereferences. The first is when calling GetIOS() in KDDownload(), which could return nullptr if called after IOS::HLE::Shutdown had reset s_ios. The other happened with the following sequence:
This crash is resolved by storing a shared_ptr to NetKDTimeDevice inside NetKDRequestDevice, which skips the lookup via GetIOS and ensures the NetKDTimeDevice will exist through NetKDRequestDevice's lifespan.