From c0954035da25d040ff32b24eefb46fa3e4fed512 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Fri, 10 Mar 2023 13:12:54 +0900 Subject: [PATCH 1/4] Fail fast on trying to dispose import hook that has been overwritten by something else --- .../Internal/FunctionPointerVariableHook.cs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index 3272f50b3..80bd65cf6 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -13,10 +13,12 @@ namespace Dalamud.Hooking.Internal; /// Delegate type to represents a function prototype. This must be the same prototype as original function do. internal class FunctionPointerVariableHook : Hook where T : Delegate { - private readonly IntPtr pfnOriginal; - private readonly T originalDelegate; + private readonly nint pfnDetour; private readonly T detourDelegate; + private nint pfnOriginal; + private T? originalDelegate; + private bool enabled = false; /// @@ -40,9 +42,8 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate if (!HookManager.MultiHookTracker.TryGetValue(this.Address, out var indexList)) indexList = HookManager.MultiHookTracker[this.Address] = new(); - this.pfnOriginal = Marshal.ReadIntPtr(this.Address); - this.originalDelegate = Marshal.GetDelegateForFunctionPointer(this.pfnOriginal); this.detourDelegate = detour; + this.pfnDetour = Marshal.GetFunctionPointerForDelegate(detour); // Add afterwards, so the hookIdent starts at 0. indexList.Add(this); @@ -100,7 +101,10 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate if (!NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), MemoryProtection.ExecuteReadWrite, out var oldProtect)) throw new Win32Exception(Marshal.GetLastWin32Error()); - Marshal.WriteIntPtr(this.Address, Marshal.GetFunctionPointerForDelegate(this.detourDelegate)); + this.pfnOriginal = Marshal.ReadIntPtr(this.Address); + this.originalDelegate = Marshal.GetDelegateForFunctionPointer(this.pfnOriginal); + Marshal.WriteIntPtr(this.Address, this.pfnDetour); + NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); } } @@ -118,6 +122,9 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate if (!NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), MemoryProtection.ExecuteReadWrite, out var oldProtect)) throw new Win32Exception(Marshal.GetLastWin32Error()); + if (Marshal.ReadIntPtr(this.Address) != this.pfnOriginal) + Environment.FailFast("Cannot disable this hook in a sane manner."); + Marshal.WriteIntPtr(this.Address, this.pfnOriginal); NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); } From 9ee8ad67b4bc18e0bde39b87ffd6e788fab78c26 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Fri, 10 Mar 2023 22:28:08 +0900 Subject: [PATCH 2/4] Fix import hook --- .../Internal/FunctionPointerVariableHook.cs | 88 ++++++++++----- Dalamud/Hooking/Internal/HookManager.cs | 7 ++ Dalamud/NativeFunctions.cs | 105 ++++++++++++++++++ 3 files changed, 174 insertions(+), 26 deletions(-) diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index 80bd65cf6..c80033509 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.ComponentModel; using System.Reflection; using System.Runtime.InteropServices; @@ -11,13 +12,20 @@ namespace Dalamud.Hooking.Internal; /// Manages a hook with MinHook. /// /// Delegate type to represents a function prototype. This must be the same prototype as original function do. -internal class FunctionPointerVariableHook : Hook where T : Delegate +internal unsafe class FunctionPointerVariableHook : Hook + where T : Delegate { private readonly nint pfnDetour; + + // Keep it referenced so that pfnDetour doesn't become invalidated. + // ReSharper disable once NotAccessedField.Local private readonly T detourDelegate; - private nint pfnOriginal; - private T? originalDelegate; + private readonly byte* pfnThunk; + private readonly nint* ppfnThunkJumpTarget; + + private readonly nint pfnOriginal; + private readonly T originalDelegate; private bool enabled = false; @@ -40,11 +48,45 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate } if (!HookManager.MultiHookTracker.TryGetValue(this.Address, out var indexList)) - indexList = HookManager.MultiHookTracker[this.Address] = new(); + { + indexList = HookManager.MultiHookTracker[this.Address] = new List(); + } this.detourDelegate = detour; this.pfnDetour = Marshal.GetFunctionPointerForDelegate(detour); + this.pfnThunk = (byte*)NativeFunctions.HeapAlloc(HookManager.NoFreeExecutableHeap, 0, 12); + if (this.pfnThunk == null) + { + throw new OutOfMemoryException("Failed to allocate memory for import hooks."); + } + + // movabs rax, imm + this.pfnThunk[0] = 0x48; + this.pfnThunk[1] = 0xB8; + this.ppfnThunkJumpTarget = (nint*)&this.pfnThunk[2]; + + // jmp rax + this.pfnThunk[10] = 0xFF; + this.pfnThunk[11] = 0xE0; + + if (!NativeFunctions.VirtualProtect( + this.Address, + (UIntPtr)Marshal.SizeOf(), + MemoryProtection.ExecuteReadWrite, + out var oldProtect)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + this.pfnOriginal = Marshal.ReadIntPtr(this.Address); + this.originalDelegate = Marshal.GetDelegateForFunctionPointer(this.pfnOriginal); + *this.ppfnThunkJumpTarget = this.pfnOriginal; + Marshal.WriteIntPtr(this.Address, this.pfnDetour); + + // This really should not fail, but then even if it does, whatever. + NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); + // Add afterwards, so the hookIdent starts at 0. indexList.Add(this); @@ -79,7 +121,9 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate public override void Dispose() { if (this.IsDisposed) + { return; + } this.Disable(); @@ -94,19 +138,15 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate { this.CheckDisposed(); - if (!this.enabled) + if (this.enabled) { - lock (HookManager.HookEnableSyncRoot) - { - if (!NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), MemoryProtection.ExecuteReadWrite, out var oldProtect)) - throw new Win32Exception(Marshal.GetLastWin32Error()); + return; + } - this.pfnOriginal = Marshal.ReadIntPtr(this.Address); - this.originalDelegate = Marshal.GetDelegateForFunctionPointer(this.pfnOriginal); - Marshal.WriteIntPtr(this.Address, this.pfnDetour); - - NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); - } + lock (HookManager.HookEnableSyncRoot) + { + *this.ppfnThunkJumpTarget = this.pfnDetour; + this.enabled = true; } } @@ -115,19 +155,15 @@ internal class FunctionPointerVariableHook : Hook where T : Delegate { this.CheckDisposed(); - if (this.enabled) + if (!this.enabled) { - lock (HookManager.HookEnableSyncRoot) - { - if (!NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), MemoryProtection.ExecuteReadWrite, out var oldProtect)) - throw new Win32Exception(Marshal.GetLastWin32Error()); + return; + } - if (Marshal.ReadIntPtr(this.Address) != this.pfnOriginal) - Environment.FailFast("Cannot disable this hook in a sane manner."); - - Marshal.WriteIntPtr(this.Address, this.pfnOriginal); - NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); - } + lock (HookManager.HookEnableSyncRoot) + { + *this.ppfnThunkJumpTarget = this.pfnOriginal; + this.enabled = false; } } } diff --git a/Dalamud/Hooking/Internal/HookManager.cs b/Dalamud/Hooking/Internal/HookManager.cs index 303802ff6..d26bcefa0 100644 --- a/Dalamud/Hooking/Internal/HookManager.cs +++ b/Dalamud/Hooking/Internal/HookManager.cs @@ -16,6 +16,13 @@ namespace Dalamud.Hooking.Internal; [ServiceManager.EarlyLoadedService] internal class HookManager : IDisposable, IServiceType { + /// + /// Handle to an executable heap that we shall never free anything unless we can be absolutely sure that nothing is + /// referencing to it anymore. + /// + internal static readonly nint NoFreeExecutableHeap = + NativeFunctions.HeapCreate(NativeFunctions.HeapOptions.CreateEnableExecute, 0, 0); + private static readonly ModuleLog Log = new("HM"); [ServiceManager.ServiceConstructor] diff --git a/Dalamud/NativeFunctions.cs b/Dalamud/NativeFunctions.cs index 1ed05b8ba..50a58bfe7 100644 --- a/Dalamud/NativeFunctions.cs +++ b/Dalamud/NativeFunctions.cs @@ -1393,6 +1393,40 @@ internal static partial class NativeFunctions WriteCombine = 0x400, } + /// + /// HEAP_* from heapapi + /// + [Flags] + public enum HeapOptions + { + /// + /// Serialized access is not used when the heap functions access this heap. This option applies to all + /// subsequent heap function calls. Alternatively, you can specify this option on individual heap function + /// calls. The low-fragmentation heap (LFH) cannot be enabled for a heap created with this option. A heap + /// created with this option cannot be locked. + /// + NoSerialize = 0x00000001, + + /// + /// The system raises an exception to indicate failure (for example, an out-of-memory condition) for calls to + /// HeapAlloc and HeapReAlloc instead of returning NULL. + /// + GenerateExceptions = 0x00000004, + + /// + /// The allocated memory will be initialized to zero. Otherwise, the memory is not initialized to zero. + /// + ZeroMemory = 0x00000008, + + /// + /// All memory blocks that are allocated from this heap allow code execution, if the hardware enforces data + /// execution prevention. Use this flag heap in applications that run code from the heap. If + /// HEAP_CREATE_ENABLE_EXECUTE is not specified and an application attempts to run code from a protected page, + /// the application receives an exception with the status code STATUS_ACCESS_VIOLATION. + /// + CreateEnableExecute = 0x00040000, + } + /// /// See https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-setevent /// Sets the specified event object to the signaled state. @@ -1630,6 +1664,77 @@ internal static partial class NativeFunctions [DllImport("kernel32.dll")] public static extern IntPtr SetUnhandledExceptionFilter(IntPtr lpTopLevelExceptionFilter); + /// + /// HeapCreate - Creates a private heap object that can be used by the calling process. + /// The function reserves space in the virtual address space of the process and allocates + /// physical storage for a specified initial portion of this block. + /// Ref: https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapcreate + /// + /// + /// The heap allocation options. + /// These options affect subsequent access to the new heap through calls to the heap functions. + /// + /// + /// The initial size of the heap, in bytes. + /// + /// This value determines the initial amount of memory that is committed for the heap. + /// The value is rounded up to a multiple of the system page size. The value must be smaller than dwMaximumSize. + /// + /// If this parameter is 0, the function commits one page. To determine the size of a page on the host computer, + /// use the GetSystemInfo function. + /// + /// + /// The maximum size of the heap, in bytes. The HeapCreate function rounds dwMaximumSize up to a multiple of the + /// system page size and then reserves a block of that size in the process's virtual address space for the heap. + /// If allocation requests made by the HeapAlloc or HeapReAlloc functions exceed the size specified by + /// dwInitialSize, the system commits additional pages of memory for the heap, up to the heap's maximum size. + /// + /// If dwMaximumSize is not zero, the heap size is fixed and cannot grow beyond the maximum size. Also, the largest + /// memory block that can be allocated from the heap is slightly less than 512 KB for a 32-bit process and slightly + /// less than 1,024 KB for a 64-bit process. Requests to allocate larger blocks fail, even if the maximum size of + /// the heap is large enough to contain the block. + /// + /// If dwMaximumSize is 0, the heap can grow in size. The heap's size is limited only by the available memory. + /// Requests to allocate memory blocks larger than the limit for a fixed-size heap do not automatically fail; + /// instead, the system calls the VirtualAlloc function to obtain the memory that is needed for large blocks. + /// Applications that need to allocate large memory blocks should set dwMaximumSize to 0. + /// + /// + /// If the function succeeds, the return value is a handle to the newly created heap. + /// + /// If the function fails, the return value is NULL. To get extended error information, call GetLastError. + /// + [DllImport("kernel32.dll", SetLastError = true)] + public static extern nint HeapCreate(HeapOptions flOptions, nuint dwInitialSize, nuint dwMaximumSize); + + /// + /// Allocates a block of memory from a heap. The allocated memory is not movable. + /// + /// + /// A handle to the heap from which the memory will be allocated. This handle is returned by the HeapCreate or + /// GetProcessHeap function. + /// + /// + /// The heap allocation options. Specifying any of these values will override the corresponding value specified when + /// the heap was created with HeapCreate. + /// + /// The number of bytes to be allocated. + /// + /// If the heap specified by the hHeap parameter is a "non-growable" heap, dwBytes must be less than 0x7FFF8. + /// You create a non-growable heap by calling the HeapCreate function with a nonzero value. + /// + /// + /// If the function succeeds, the return value is a pointer to the allocated memory block. + /// + /// If the function fails and you have not specified HEAP_GENERATE_EXCEPTIONS, the return value is NULL. + /// + /// If the function fails and you have specified HEAP_GENERATE_EXCEPTIONS, the function may generate either of the + /// exceptions listed in the following table. The particular exception depends upon the nature of the heap + /// corruption. For more information, see GetExceptionCode. + /// + [DllImport("kernel32.dll", SetLastError=false)] + public static extern nint HeapAlloc(nint hHeap, HeapOptions dwFlags, nuint dwBytes); + /// /// See https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc. /// Reserves, commits, or changes the state of a region of pages in the virtual address space of the calling process. From f16628beb0f48d7681b42aafd0794fd160d60cbb Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Fri, 10 Mar 2023 22:53:48 +0900 Subject: [PATCH 3/4] another fix --- .../Internal/FunctionPointerVariableHook.cs | 44 +++++++++++-------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index c80033509..970c55fee 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -12,7 +12,7 @@ namespace Dalamud.Hooking.Internal; /// Manages a hook with MinHook. /// /// Delegate type to represents a function prototype. This must be the same prototype as original function do. -internal unsafe class FunctionPointerVariableHook : Hook +internal class FunctionPointerVariableHook : Hook where T : Delegate { private readonly nint pfnDetour; @@ -21,13 +21,13 @@ internal unsafe class FunctionPointerVariableHook : Hook // ReSharper disable once NotAccessedField.Local private readonly T detourDelegate; - private readonly byte* pfnThunk; - private readonly nint* ppfnThunkJumpTarget; + private readonly nint pfnThunk; + private readonly nint ppfnThunkJumpTarget; private readonly nint pfnOriginal; private readonly T originalDelegate; - private bool enabled = false; + private bool enabled; /// /// Initializes a new instance of the class. @@ -55,20 +55,26 @@ internal unsafe class FunctionPointerVariableHook : Hook this.detourDelegate = detour; this.pfnDetour = Marshal.GetFunctionPointerForDelegate(detour); - this.pfnThunk = (byte*)NativeFunctions.HeapAlloc(HookManager.NoFreeExecutableHeap, 0, 12); - if (this.pfnThunk == null) + unsafe { - throw new OutOfMemoryException("Failed to allocate memory for import hooks."); + var pfnThunkBytes = (byte*)NativeFunctions.HeapAlloc(HookManager.NoFreeExecutableHeap, 0, 12); + if (pfnThunkBytes == null) + { + throw new OutOfMemoryException("Failed to allocate memory for import hooks."); + } + + // movabs rax, imm + pfnThunkBytes[0] = 0x48; + pfnThunkBytes[1] = 0xB8; + + // jmp rax + pfnThunkBytes[10] = 0xFF; + pfnThunkBytes[11] = 0xE0; + + this.pfnThunk = (nint)pfnThunkBytes; } - // movabs rax, imm - this.pfnThunk[0] = 0x48; - this.pfnThunk[1] = 0xB8; - this.ppfnThunkJumpTarget = (nint*)&this.pfnThunk[2]; - - // jmp rax - this.pfnThunk[10] = 0xFF; - this.pfnThunk[11] = 0xE0; + this.ppfnThunkJumpTarget = this.pfnThunk + 2; if (!NativeFunctions.VirtualProtect( this.Address, @@ -81,8 +87,8 @@ internal unsafe class FunctionPointerVariableHook : Hook this.pfnOriginal = Marshal.ReadIntPtr(this.Address); this.originalDelegate = Marshal.GetDelegateForFunctionPointer(this.pfnOriginal); - *this.ppfnThunkJumpTarget = this.pfnOriginal; - Marshal.WriteIntPtr(this.Address, this.pfnDetour); + Marshal.WriteIntPtr(this.ppfnThunkJumpTarget, this.pfnOriginal); + Marshal.WriteIntPtr(this.Address, this.pfnThunk); // This really should not fail, but then even if it does, whatever. NativeFunctions.VirtualProtect(this.Address, (UIntPtr)Marshal.SizeOf(), oldProtect, out _); @@ -145,7 +151,7 @@ internal unsafe class FunctionPointerVariableHook : Hook lock (HookManager.HookEnableSyncRoot) { - *this.ppfnThunkJumpTarget = this.pfnDetour; + Marshal.WriteIntPtr(this.ppfnThunkJumpTarget, this.pfnDetour); this.enabled = true; } } @@ -162,7 +168,7 @@ internal unsafe class FunctionPointerVariableHook : Hook lock (HookManager.HookEnableSyncRoot) { - *this.ppfnThunkJumpTarget = this.pfnOriginal; + Marshal.WriteIntPtr(this.ppfnThunkJumpTarget, this.pfnOriginal); this.enabled = false; } } From 8aea17047926f9c2a4bef4e696f1040fa82f0ef7 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Fri, 10 Mar 2023 22:54:57 +0900 Subject: [PATCH 4/4] Use UsedImplicitly instead of resharper comments --- Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index 970c55fee..3e88780eb 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -5,6 +5,7 @@ using System.Reflection; using System.Runtime.InteropServices; using Dalamud.Memory; +using JetBrains.Annotations; namespace Dalamud.Hooking.Internal; @@ -18,7 +19,7 @@ internal class FunctionPointerVariableHook : Hook private readonly nint pfnDetour; // Keep it referenced so that pfnDetour doesn't become invalidated. - // ReSharper disable once NotAccessedField.Local + [UsedImplicitly] private readonly T detourDelegate; private readonly nint pfnThunk;