From 273f5e1f26750ff8a2471fb828997648d71a4cb9 Mon Sep 17 00:00:00 2001
From: Ryan Houdek <Sonicadvance1@gmail.com>
Date: Sun, 19 Feb 2023 16:12:06 -0800
Subject: [PATCH] Arm64: Fixes a race condition on syscall spilling SRA

When executing a non-inlined syscall, we spill all static registers.
We weren't storing in to the thread context that we have done this.
If a signal occured between FEX returning from the syscall (after the
blr) and before the `FillStaticRegs` then the signal handler would get
the incorrect register state.

This typically manifested as Steam getting a SIGCHLD, trying to recover
the guest stack pointer, and it that pointer would be zero or some other
corrupt value. Thus crashing inside of the signal handler.

Surprising that we hadn't hit this way more before this point, must have
needed hardware that tickled the race condition *just* right.
---
 .../Interface/Core/Dispatcher/Dispatcher.cpp  |  9 +----
 .../Interface/Core/JIT/Arm64/BranchOps.cpp    | 35 +++++++++++--------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/External/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp b/External/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp
index bcb4563aa..b02f3bff0 100644
--- a/External/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp
+++ b/External/FEXCore/Source/Interface/Core/Dispatcher/Dispatcher.cpp
@@ -1009,14 +1009,7 @@ bool Dispatcher::HandleGuestSignal(FEXCore::Core::InternalThreadState *Thread, i
         // We need to spill SRA but only some of it, since some values have already been spilled
         // Lower 16 bits tells us which registers are already spilled to the context
         // So we ignore spilling those ones
-        uint16_t NumRegisters = std::popcount(Frame->InSyscallInfo & 0xFFFF);
-        if (NumRegisters >= 4) {
-          // Unhandled case
-          IgnoreMask = 0;
-        }
-        else {
-          IgnoreMask = Frame->InSyscallInfo & 0xFFFF;
-        }
+        IgnoreMask = Frame->InSyscallInfo & 0xFFFF;
       }
       else {
         // We must spill everything
diff --git a/External/FEXCore/Source/Interface/Core/JIT/Arm64/BranchOps.cpp b/External/FEXCore/Source/Interface/Core/JIT/Arm64/BranchOps.cpp
index b71160a79..1703a6b97 100644
--- a/External/FEXCore/Source/Interface/Core/JIT/Arm64/BranchOps.cpp
+++ b/External/FEXCore/Source/Interface/Core/JIT/Arm64/BranchOps.cpp
@@ -167,14 +167,23 @@ DEF_OP(Syscall) {
   FEXCore::IR::SyscallFlags Flags = Op->Flags;
   PushDynamicRegsAndLR(TMP1);
 
-  if ((Flags & FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY) != FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY) {
-    SpillStaticRegs();
-  }
-  else {
+  uint32_t GPRSpillMask = ~0U;
+  uint32_t FPRSpillMask = ~0U;
+  if ((Flags & FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY) == FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY) {
     // Need to spill all caller saved registers still
-    SpillStaticRegs(true, CALLER_GPR_MASK, CALLER_FPR_MASK);
+    GPRSpillMask = CALLER_GPR_MASK;
+    FPRSpillMask = CALLER_FPR_MASK;
   }
 
+  SpillStaticRegs(true, GPRSpillMask, FPRSpillMask);
+
+  // Now that we are spilled, store in the state that we are in a syscall
+  // Still without overwriting registers that matter
+  // 16bit LoadConstant to be a single instruction
+  // This gives the signal handler a value to check to see if we are in a syscall at all
+  LoadConstant(ARMEmitter::Size::i64Bit, ARMEmitter::Reg::r0, GPRSpillMask & 0xFFFF);
+  str(ARMEmitter::XReg::x0, STATE, offsetof(FEXCore::Core::CpuStateFrame, InSyscallInfo));
+
   uint64_t SPOffset = AlignUp(FEXCore::HLE::SyscallArguments::MAX_ARGS * 8, 16);
   sub(ARMEmitter::Size::i64Bit, ARMEmitter::Reg::rsp, ARMEmitter::Reg::rsp, SPOffset);
   for (uint32_t i = 0; i < FEXCore::HLE::SyscallArguments::MAX_ARGS; ++i) {
@@ -196,19 +205,17 @@ DEF_OP(Syscall) {
 
   add(ARMEmitter::Size::i64Bit, ARMEmitter::Reg::rsp, ARMEmitter::Reg::rsp, SPOffset);
 
-  if ((Flags & FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY) != FEXCore::IR::SyscallFlags::NOSYNCSTATEONENTRY &&
-      (Flags & FEXCore::IR::SyscallFlags::NORETURN) != FEXCore::IR::SyscallFlags::NORETURN) {
-    FillStaticRegs();
-  }
-  else {
+  if ((Flags & FEXCore::IR::SyscallFlags::NORETURN) != FEXCore::IR::SyscallFlags::NORETURN) {
     // Result is now in x0
     // Fix the stack and any values that were stepped on
-    FillStaticRegs(true, CALLER_GPR_MASK, CALLER_FPR_MASK);
-  }
+    FillStaticRegs(true, GPRSpillMask, FPRSpillMask);
 
-  PopDynamicRegsAndLR();
+    // Now the registers we've spilled are back in their original host registers
+    // We can safely claim we are no longer in a syscall
+    str(ARMEmitter::XReg::zr, STATE, offsetof(FEXCore::Core::CpuStateFrame, InSyscallInfo));
+
+    PopDynamicRegsAndLR();
 
-  if ((Flags & FEXCore::IR::SyscallFlags::NORETURN) != FEXCore::IR::SyscallFlags::NORETURN) {
     // Move result to its destination register
     mov(ARMEmitter::Size::i64Bit, GetReg(Node), ARMEmitter::Reg::r0);
   }