PatchEngine/GeckoCode: Heuristic stack checks

Try to make sure the stack is sane before calling into the
codehandler. This is intended to reduce the possibility of random
memory corruption.

Source/Core/Core/Boot/Boot.cpp

@@ -483,9 +483,8 @@ bool CBoot::BootUp()
   // Not part of the binary itself, but either we or Gecko OS might insert
   // this, and it doesn't clear the icache properly.
   HLE::Patch(Gecko::ENTRY_POINT, "GeckoCodehandler");
-  if (SConfig::GetInstance().bEnableCheats)
-  {
+  // This has to always be installed even if cheats are not enabled because of the possiblity of
+  // loading a savestate where PC is inside the code handler while cheats are disabled.
   HLE::Patch(Gecko::HLE_TRAMPOLINE_ADDRESS, "GeckoHandlerReturnTrampoline");
-  }
   return true;
 }

Source/Core/Core/Boot/Boot_BS2Emu.cpp

@@ -178,10 +178,6 @@ bool CBoot::EmulatedBS2_GC(bool skipAppLoader)
   // Load patches
   PatchEngine::LoadPatches();
 
-  // If we have any patches that need to be applied very early, here's a good place
-  bool patched = PatchEngine::ApplyFramePatches();
-  _assert_(patched);
-
   return true;
 }
 

Source/Core/Core/GeckoCode.cpp

@@ -145,6 +145,7 @@ static Installation InstallCodeHandlerLocked()
   // Stop code. Tells the handler that this is the end of the list.
   PowerPC::HostWrite_U32(0xF0000000, next_address);
   PowerPC::HostWrite_U32(0x00000000, next_address + 4);
+  PowerPC::HostWrite_U32(0, HLE_TRAMPOLINE_ADDRESS);
 
   // Turn on codes
   PowerPC::HostWrite_U8(1, INSTALLER_BASE_ADDRESS + 7);
@@ -157,11 +158,18 @@ static Installation InstallCodeHandlerLocked()
   return Installation::Installed;
 }
 
+// FIXME: Gecko needs to participate in the savestate system (remember installation state).
+//   Current bug: Loading a savestate causes the handler to be replaced, if the PC is inside it
+//    and the on disk codehandler.bin is different then the PC will effectively be pointing to
+//    a random instruction different from the one when the state was created and break or crash.
+//    [Also, self-modifying handler will break it since the modifications will be reset]
 void RunCodeHandler()
 {
   if (!SConfig::GetInstance().bEnableCheats)
     return;
 
+  // NOTE: Need to release the lock because of GUI deadlocks with PanicAlert in HostWrite_*
+  {
     std::lock_guard<std::mutex> codes_lock(s_active_codes_lock);
     // Don't spam retry if the install failed. The corrupt / missing disk file is not likely to be
     // fixed within 1 frame of the last error.
@@ -176,18 +184,16 @@ void RunCodeHandler()
       if (s_code_handler_installed != Installation::Installed)
         return;
     }
+  }
 
-  // If the last block that just executed ended with a BLR instruction then we can intercept it and
-  // redirect control into the Gecko Code Handler. The Code Handler will automatically BLR back to
-  // the original return address (which will still be in the link register).
-  if (PC != LR)
-  {
-    // We're at a random address in the middle of something so we have to do this the hard way.
+  // We always do this to avoid problems with the stack since we're branching in random locations.
+  // Even with function call return hooks (PC == LR), hand coded assembler won't necessarily
+  // follow the ABI. [Volatile FPR, GPR, CR may not be volatile]
   // The codehandler will STMW all of the GPR registers, but we need to fix the Stack's Red
   // Zone, the LR, PC (return address) and the volatile floating point registers.
   // Build a function call stack frame.
   u32 SFP = GPR(1);                     // Stack Frame Pointer
-    GPR(1) -= 224;                        // Stack's Red Zone
+  GPR(1) -= 256;                        // Stack's Red Zone
   GPR(1) -= 16 + 2 * 14 * sizeof(u64);  // Our stack frame (HLE_Misc::GeckoReturnTrampoline)
   GPR(1) -= 8;                          // Fake stack frame for codehandler
   GPR(1) &= 0xFFFFFFF0;                 // Align stack to 16bytes
@@ -197,17 +203,17 @@ void RunCodeHandler()
   PowerPC::HostWrite_U32(SFP, SP + 8);  // Real stack frame
   PowerPC::HostWrite_U32(PC, SP + 12);
   PowerPC::HostWrite_U32(LR, SP + 16);
+  PowerPC::HostWrite_U32(PowerPC::CompactCR(), SP + 20);
   // Registers FPR0->13 are volatile
   for (int i = 0; i < 14; ++i)
   {
     PowerPC::HostWrite_U64(riPS0(i), SP + 24 + 2 * i * sizeof(u64));
     PowerPC::HostWrite_U64(riPS1(i), SP + 24 + (2 * i + 1) * sizeof(u64));
   }
-    LR = HLE_TRAMPOLINE_ADDRESS;
   DEBUG_LOG(ACTIONREPLAY, "GeckoCodes: Initiating phantom branch-and-link. "
                           "PC = 0x%08X, SP = 0x%08X, SFP = 0x%08X",
             PC, SP, SFP);
-  }
+  LR = HLE_TRAMPOLINE_ADDRESS;
   PC = NPC = ENTRY_POINT;
 }
 

Source/Core/Core/HLE/HLE_Misc.cpp

@@ -68,6 +68,7 @@ void GeckoReturnTrampoline()
   GPR(1) = PowerPC::HostRead_U32(SP + 8);
   NPC = PowerPC::HostRead_U32(SP + 12);
   LR = PowerPC::HostRead_U32(SP + 16);
+  PowerPC::ExpandCR(PowerPC::HostRead_U32(SP + 20));
   for (int i = 0; i < 14; ++i)
   {
     riPS0(i) = PowerPC::HostRead_U64(SP + 24 + 2 * i * sizeof(u64));

Source/Core/Core/PatchEngine.cpp

@@ -20,6 +20,7 @@
 #include <string>
 #include <vector>
 
+#include "Common/Assert.h"
 #include "Common/CommonPaths.h"
 #include "Common/FileUtil.h"
 #include "Common/IniFile.h"
@@ -203,6 +204,32 @@ static void ApplyPatches(const std::vector<Patch>& patches)
   }
 }
 
+// Requires MSR.DR, MSR.IR
+// There's no perfect way to do this, it's just a heuristic.
+// We require at least 2 stack frames, if the stack is shallower than that then it won't work.
+static bool IsStackSane()
+{
+  _dbg_assert_(ACTIONREPLAY, UReg_MSR(MSR).DR && UReg_MSR(MSR).IR);
+
+  // Check the stack pointer
+  u32 SP = GPR(1);
+  if (!PowerPC::HostIsRAMAddress(SP))
+    return false;
+
+  // Read the frame pointer from the stack (find 2nd frame from top), assert that it makes sense
+  u32 next_SP = PowerPC::HostRead_U32(SP);
+  if (next_SP <= SP || !PowerPC::HostIsRAMAddress(next_SP) ||
+      !PowerPC::HostIsRAMAddress(next_SP + 4))
+    return false;
+
+  // Check the link register makes sense (that it points to a valid IBAT address)
+  auto insn = PowerPC::TryReadInstruction(PowerPC::HostRead_U32(next_SP + 4));
+  if (!insn.valid || !insn.hex)
+    return false;
+
+  return true;
+}
+
 bool ApplyFramePatches()
 {
   // Because we're using the VI Interrupt to time this instead of patching the game with a
@@ -210,7 +237,7 @@ bool ApplyFramePatches()
   // We deal with this by returning false so that SystemTimers will reschedule us in a few cycles
   // where we can try again after the CPU hopefully returns back to the normal instruction flow.
   UReg_MSR msr = MSR;
-  if (!msr.DR || !msr.IR)
+  if (!msr.DR || !msr.IR || !IsStackSane())
   {
     INFO_LOG(
         ACTIONREPLAY,