From 341d3b66a870751415f5648834809be57c3e769a Mon Sep 17 00:00:00 2001 From: "Herman S." <429230+has207@users.noreply.github.com> Date: Tue, 24 Feb 2026 16:28:39 +0900 Subject: [PATCH] [Kernel] Thread safety fixes for xfile, xmutant, and xtimer objects --- src/xenia/kernel/xfile.cc | 30 +++++++++++++++++++++++------- src/xenia/kernel/xfile.h | 6 ++++++ src/xenia/kernel/xmutant.cc | 6 ++++-- src/xenia/kernel/xmutant.h | 4 +++- src/xenia/kernel/xtimer.cc | 14 ++++++++++---- src/xenia/kernel/xtimer.h | 3 +++ 6 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/xenia/kernel/xfile.cc b/src/xenia/kernel/xfile.cc index e5a4ff24b..1c722b874 100644 --- a/src/xenia/kernel/xfile.cc +++ b/src/xenia/kernel/xfile.cc @@ -38,6 +38,7 @@ XFile::~XFile() { X_STATUS XFile::QueryDirectory(X_FILE_DIRECTORY_INFORMATION* out_info, size_t length, const std::string_view file_name, bool restart) { + std::lock_guard lock(file_lock_); assert_not_null(out_info); vfs::Entry* entry = nullptr; @@ -91,6 +92,15 @@ X_STATUS XFile::QueryDirectory(X_FILE_DIRECTORY_INFORMATION* out_info, X_STATUS XFile::Read(uint32_t buffer_guest_address, uint32_t buffer_length, uint64_t byte_offset, uint32_t* out_bytes_read, uint32_t apc_context, bool notify_completion) { + std::lock_guard lock(file_lock_); + return ReadInternal(buffer_guest_address, buffer_length, byte_offset, + out_bytes_read, apc_context, notify_completion); +} + +X_STATUS XFile::ReadInternal(uint32_t buffer_guest_address, + uint32_t buffer_length, uint64_t byte_offset, + uint32_t* out_bytes_read, uint32_t apc_context, + bool notify_completion) { if (byte_offset == uint64_t(-1)) { // Read from current position. byte_offset = position_; @@ -184,6 +194,7 @@ X_STATUS XFile::Read(uint32_t buffer_guest_address, uint32_t buffer_length, X_STATUS XFile::ReadScatter(uint32_t segments_guest_address, uint32_t length, uint64_t byte_offset, uint32_t* out_bytes_read, uint32_t apc_context) { + std::lock_guard lock(file_lock_); X_STATUS result = X_STATUS_SUCCESS; // segments points to an array of buffer pointers of type @@ -206,12 +217,13 @@ X_STATUS XFile::ReadScatter(uint32_t segments_guest_address, uint32_t length, } uint32_t bytes_read = 0; - result = Read(read_buffer, read_length, - byte_offset ? ((byte_offset != -1 && byte_offset != -2) - ? byte_offset + read_total - : byte_offset) - : -1, - &bytes_read, apc_context, false); + result = + ReadInternal(read_buffer, read_length, + byte_offset ? ((byte_offset != -1 && byte_offset != -2) + ? byte_offset + read_total + : byte_offset) + : -1, + &bytes_read, apc_context, false); if (result != X_STATUS_SUCCESS) { break; @@ -240,6 +252,7 @@ X_STATUS XFile::ReadScatter(uint32_t segments_guest_address, uint32_t length, X_STATUS XFile::Write(uint32_t buffer_guest_address, uint32_t buffer_length, uint64_t byte_offset, uint32_t* out_bytes_written, uint32_t apc_context) { + std::lock_guard lock(file_lock_); if (byte_offset == uint64_t(-1)) { // Write from current position. byte_offset = position_; @@ -269,7 +282,10 @@ X_STATUS XFile::Write(uint32_t buffer_guest_address, uint32_t buffer_length, return result; } -X_STATUS XFile::SetLength(size_t length) { return file_->SetLength(length); } +X_STATUS XFile::SetLength(size_t length) { + std::lock_guard lock(file_lock_); + return file_->SetLength(length); +} X_STATUS XFile::Rename(const std::filesystem::path file_path) { entry()->Rename(file_path); return X_STATUS_SUCCESS; diff --git a/src/xenia/kernel/xfile.h b/src/xenia/kernel/xfile.h index b0f201de0..1414b9c6b 100644 --- a/src/xenia/kernel/xfile.h +++ b/src/xenia/kernel/xfile.h @@ -10,6 +10,7 @@ #ifndef XENIA_KERNEL_XFILE_H_ #define XENIA_KERNEL_XFILE_H_ +#include #include #include "xenia/kernel/xiocompletion.h" @@ -185,9 +186,14 @@ class XFile : public XObject { private: XFile(); + X_STATUS ReadInternal(uint32_t buffer_guest_address, uint32_t buffer_length, + uint64_t byte_offset, uint32_t* out_bytes_read, + uint32_t apc_context, bool notify_completion); + vfs::File* file_ = nullptr; std::unique_ptr async_event_ = nullptr; + std::mutex file_lock_; std::mutex completion_port_lock_; std::vector>> completion_ports_; diff --git a/src/xenia/kernel/xmutant.cc b/src/xenia/kernel/xmutant.cc index 0a67f2183..58e379730 100644 --- a/src/xenia/kernel/xmutant.cc +++ b/src/xenia/kernel/xmutant.cc @@ -59,7 +59,8 @@ bool XMutant::Save(ByteStream* stream) { return false; } - uint32_t owning_thread_handle = owning_thread_ ? owning_thread_->handle() : 0; + XThread* owner = owning_thread_.load(); + uint32_t owning_thread_handle = owner ? owner->handle() : 0; stream->Write(owning_thread_handle); XELOGD("XMutant {:08X} (owner: {:08X})", handle(), owning_thread_handle); @@ -83,7 +84,8 @@ object_ref XMutant::Restore(KernelState* kernel_state, mutant->owning_thread_ = kernel_state->object_table() ->LookupObject(owning_thread_handle) .get(); - mutant->owning_thread_->AcquireMutantOnStartup(retain_object(mutant)); + mutant->owning_thread_.load()->AcquireMutantOnStartup( + retain_object(mutant)); } return object_ref(mutant); diff --git a/src/xenia/kernel/xmutant.h b/src/xenia/kernel/xmutant.h index 5dddf905b..93d557f4c 100644 --- a/src/xenia/kernel/xmutant.h +++ b/src/xenia/kernel/xmutant.h @@ -10,6 +10,8 @@ #ifndef XENIA_KERNEL_XMUTANT_H_ #define XENIA_KERNEL_XMUTANT_H_ +#include + #include "xenia/base/threading.h" #include "xenia/kernel/xobject.h" #include "xenia/xbox.h" @@ -42,7 +44,7 @@ class XMutant : public XObject { XMutant(); std::unique_ptr mutant_; - XThread* owning_thread_ = nullptr; + std::atomic owning_thread_{nullptr}; }; } // namespace kernel diff --git a/src/xenia/kernel/xtimer.cc b/src/xenia/kernel/xtimer.cc index 6d9adcc6b..c1b806b1e 100644 --- a/src/xenia/kernel/xtimer.cc +++ b/src/xenia/kernel/xtimer.cc @@ -45,6 +45,8 @@ X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, return X_STATUS_TIMER_RESUME_IGNORED; } + std::lock_guard lock(timer_lock_); + period_ms = Clock::ScaleGuestDurationMillis(period_ms); WinSystemClock::time_point due_tp; if (due_time < 0) { @@ -63,9 +65,13 @@ X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, callback_routine_arg_ = routine_arg; // This callback will only be issued when the timer is fired. + // Capture values by value to avoid racing with a future SetTimer() call. std::function callback = nullptr; if (callback_routine_) { - callback = [this]() { + auto cb_thread = callback_thread_; + auto cb_routine = callback_routine_; + auto cb_routine_arg = callback_routine_arg_; + callback = [cb_thread, cb_routine, cb_routine_arg]() { // Queue APC to call back routine with (arg, low, high). // It'll be executed on the thread that requested the timer. uint64_t time = xe::Clock::QueryGuestSystemTime(); @@ -73,9 +79,8 @@ X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, uint32_t time_high = static_cast(time >> 32); XELOGI( "XTimer enqueuing timer callback to {:08X}({:08X}, {:08X}, {:08X})", - callback_routine_, callback_routine_arg_, time_low, time_high); - callback_thread_->EnqueueApc(callback_routine_, callback_routine_arg_, - time_low, time_high); + cb_routine, cb_routine_arg, time_low, time_high); + cb_thread->EnqueueApc(cb_routine, cb_routine_arg, time_low, time_high); }; } @@ -91,6 +96,7 @@ X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, } X_STATUS XTimer::Cancel() { + std::lock_guard lock(timer_lock_); return timer_->Cancel() ? X_STATUS_SUCCESS : X_STATUS_UNSUCCESSFUL; } diff --git a/src/xenia/kernel/xtimer.h b/src/xenia/kernel/xtimer.h index 0a3dee618..bbdfb391a 100644 --- a/src/xenia/kernel/xtimer.h +++ b/src/xenia/kernel/xtimer.h @@ -10,6 +10,8 @@ #ifndef XENIA_KERNEL_XTIMER_H_ #define XENIA_KERNEL_XTIMER_H_ +#include + #include "xenia/base/threading.h" #include "xenia/kernel/xobject.h" #include "xenia/xbox.h" @@ -37,6 +39,7 @@ class XTimer : public XObject { private: std::unique_ptr timer_; + std::mutex timer_lock_; XThread* callback_thread_ = nullptr; uint32_t callback_routine_ = 0;