NativeDetour for 32 bit Unity __thiscall functions doesn't work

[KingEnderBrine]

Description

Trying to make a NativeDetour for some Unity functions in a BepInEx patcher on Windows, it works for 64 bit Unity game, but not 32 bit.
The one that I currently have issues with is MonoManager::AwakeFromLoad(MonoManager *this, AwakeFromLoadMode param_1). Instead of getting a pointer in this and 3 in param_1 I get 3 in this and something in param_1 that changes between Unity versions (probably whatever is on the stack at the moment). So even just calling the original functions results in a crash.

In Ghydra I can see that the function is marked as __thiscall for 32 bit, but it's __cdecl for 64 bit, which is where the difference and the issue is.
From https://learn.microsoft.com/en-us/cpp/cpp/argument-passing-and-naming-conventions?view=msvc-170:

Keyword	Stack cleanup	Parameter passing
__cdecl	Caller	Pushes parameters on the stack, in reverse order (right to left)
__thiscall	Callee	Pushed on stack; this pointer stored in ECX

I'm not sure if that's mono being mono again or the way that MonoMod creates detour and trampoline.

Example

One of the test versions: Unity 2023.2.5f1 32bit
BepInEx 5.4.22

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Runtime.InteropServices;
using Mono.Cecil;
using MonoMod.RuntimeDetour;

internal static class FixPluginTypesSerializationPatcher
{
    public static IEnumerable<string> TargetDLLs { get; } = new string[0];

    //Changing CallingConvention actually does something but I'm not sure exactly what, because it still doesn't work correctly,
    //only thing I see is different values for arguments
    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
    private delegate void AwakeFromLoadDelegate(nint _monoManager, int awakeMode);

    private static AwakeFromLoadDelegate original;
    private static NativeDetour _detour;

    public static void Patch(AssemblyDefinition ass)
    {
    }

    public static void Initialize()
    {
        static bool IsUnityPlayer(ProcessModule p)
        {
            return p.ModuleName.ToLowerInvariant().Contains("unityplayer");
        }

        var proc = Process.GetCurrentProcess().Modules
            .Cast<ProcessModule>()
            .FirstOrDefault(IsUnityPlayer) ?? Process.GetCurrentProcess().MainModule;

        //In the actual code getting offset is more nuanced as it has to work for multiple Unity versions,
        //but for the example let's use this one for Unity 2023.2.5f1 32bit.
        //If you need an offset for any other version you want to test, let me know. 
        //Or you can checkout the whole repo https://github.com/xiaoxiao921/FixPluginTypesSerialization
        var from = (IntPtr)(proc.BaseAddress.ToInt64() + 0x4C6D70);

        var hookPtr = Marshal.GetFunctionPointerForDelegate(new AwakeFromLoadDelegate(OnAwakeFromLoad));
        _detour = new NativeDetour(from, hookPtr, new NativeDetourConfig { ManualApply = true });
        original = _detour.GenerateTrampoline<AwakeFromLoadDelegate>();
        _detour.Apply();
    }

    private static void OnAwakeFromLoad(nint _monoManager, int awakeMode)
    {
        //Calling original causes a crash on 32 bit, but not on 64 bit
        original(_monoManager, awakeMode);
    }
}

Let me know if you need more info.

[nike4613]

Given the API usage, this looks like legacy RuntimeDetour. Legacy NativeDetour is not really a "native method detour" solution, and GenerateTrampoline in particular is quite likely to not behave correctly for calling conventions which do not match the managed calling convention exactly (most of them, with thiscall being especially egregious.)

I'd suggest trying NativeHook in reorg (which may require pulling a nightly BepInEx build or substituting a custom HarmonyX build). It's still possible that it doesn't work very well (x86 calling conventions are hell), but it has a better chance.

[KingEnderBrine]

Yes, I tried with latest github release, which seems to be considered legacy, and BepInEx 5 lts version. I'm pretty sure there are methods without __thiscall that I can use (or at least I hope so) to still be compatible with old RuntimeDetour, but I will try using NativeHook just to see if it works and let you know how it will go, thanks.

[KingEnderBrine]

I've dirty patched BepInEx and Harmony to work with reorganize branch and with NativeHook it's a bit more progress, parameters _monoManager and awakeMode now have correct values in a hook, but calling orig still results in a crash. This is the updated code:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Runtime.InteropServices;
using Mono.Cecil;
using MonoMod.RuntimeDetour;

internal static class FixPluginTypesSerializationPatcher
{
    public static IEnumerable<string> TargetDLLs { get; } = new string[0];

    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
    private delegate void AwakeFromLoadDelegate(nint _monoManager, int awakeMode);

    private static NativeHook _hook;

    public static void Patch(AssemblyDefinition ass)
    {
    }

    public static void Initialize()
    {
        static bool IsUnityPlayer(ProcessModule p)
        {
            return p.ModuleName.ToLowerInvariant().Contains("unityplayer");
        }

        var proc = Process.GetCurrentProcess().Modules
            .Cast<ProcessModule>()
            .FirstOrDefault(IsUnityPlayer) ?? Process.GetCurrentProcess().MainModule;

        var from = (IntPtr)(proc.BaseAddress.ToInt64() + 0x4C6D70);

        _hook = new NativeHook(from, AwakeFromLoad);
    }

    private static void OnAwakeFromLoad(AwakeFromLoadDelegate orig, nint _monoManager, int awakeMode)
    {
        orig(_monoManager, awakeMode);
    }
}

I will try to investigate a bit more