[Kernel] Thread safety fixes for xfile, xmutant, and xtimer objects

Author: has207

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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;

src/xenia/kernel/xfile.h

@@ -10,6 +10,7 @@
 #ifndef XENIA_KERNEL_XFILE_H_
 #define XENIA_KERNEL_XFILE_H_
 
+#include <mutex>
 #include <string>
 
 #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<threading::Event> async_event_ = nullptr;
 
+  std::mutex file_lock_;
   std::mutex completion_port_lock_;
   std::vector<std::pair<uint32_t, object_ref<XIOCompletion>>> completion_ports_;
 

src/xenia/kernel/xmutant.h

@@ -10,6 +10,8 @@
 #ifndef XENIA_KERNEL_XMUTANT_H_
 #define XENIA_KERNEL_XMUTANT_H_
 
+#include <atomic>
+
 #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<xe::threading::Mutant> mutant_;
-  XThread* owning_thread_ = nullptr;
+  std::atomic<XThread*> owning_thread_{nullptr};
 };
 
 }  // namespace kernel

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<std::mutex> lock(timer_lock_);
+
   period_ms = Clock::ScaleGuestDurationMillis(period_ms);
   WinSystemClock::time_point due_tp;
   if (due_time < 0) {
@@ -63,19 +65,22 @@ 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<void()> 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();
       uint32_t time_low = static_cast<uint32_t>(time);
       uint32_t time_high = static_cast<uint32_t>(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<std::mutex> lock(timer_lock_);
   return timer_->Cancel() ? X_STATUS_SUCCESS : X_STATUS_UNSUCCESSFUL;
 }
 

src/xenia/kernel/xtimer.h

@@ -10,6 +10,8 @@
 #ifndef XENIA_KERNEL_XTIMER_H_
 #define XENIA_KERNEL_XTIMER_H_
 
+#include <mutex>
+
 #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<xe::threading::Timer> timer_;
+  std::mutex timer_lock_;
 
   XThread* callback_thread_ = nullptr;
   uint32_t callback_routine_ = 0;