From 64fddf10bb77a844cbb3410f09ba6c479c69be50 Mon Sep 17 00:00:00 2001 From: nebel Date: Sat, 3 Jun 2023 22:44:23 +0900 Subject: [PATCH 1/3] Improve unhooking of hooked functions --- Dalamud/Hooking/AsmHook.cs | 16 +--- .../Internal/FunctionPointerVariableHook.cs | 9 +- Dalamud/Hooking/Internal/HookManager.cs | 48 +++++----- Dalamud/Hooking/Internal/MinHookHook.cs | 11 +-- Dalamud/Hooking/Internal/ReloadedHook.cs | 11 +-- Dalamud/Hooking/Internal/Unhooker.cs | 91 +++++++++++++++++++ 6 files changed, 127 insertions(+), 59 deletions(-) create mode 100644 Dalamud/Hooking/Internal/Unhooker.cs diff --git a/Dalamud/Hooking/AsmHook.cs b/Dalamud/Hooking/AsmHook.cs index 4c551db04..b05347a0a 100644 --- a/Dalamud/Hooking/AsmHook.cs +++ b/Dalamud/Hooking/AsmHook.cs @@ -35,12 +35,8 @@ public sealed class AsmHook : IDisposable, IDalamudHook { address = HookManager.FollowJmp(address); - var hasOtherHooks = HookManager.Originals.ContainsKey(address); - if (!hasOtherHooks) - { - MemoryHelper.ReadRaw(address, 0x32, out var original); - HookManager.Originals[address] = original; - } + // We cannot call TrimAfterHook here because the hook is activated by the caller. + HookManager.RegisterUnhooker(address); this.address = address; this.hookImpl = ReloadedHooks.Instance.CreateAsmHook(assembly, address.ToInt64(), (Reloaded.Hooks.Definitions.Enums.AsmHookBehaviour)asmHookBehaviour); @@ -65,12 +61,8 @@ public sealed class AsmHook : IDisposable, IDalamudHook { address = HookManager.FollowJmp(address); - var hasOtherHooks = HookManager.Originals.ContainsKey(address); - if (!hasOtherHooks) - { - MemoryHelper.ReadRaw(address, 0x32, out var original); - HookManager.Originals[address] = original; - } + // We cannot call TrimAfterHook here because the hook is activated by the caller. + HookManager.RegisterUnhooker(address); this.address = address; this.hookImpl = ReloadedHooks.Instance.CreateAsmHook(assembly, address.ToInt64(), (Reloaded.Hooks.Definitions.Enums.AsmHookBehaviour)asmHookBehaviour); diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index e75d9c180..1c0bcfa05 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -41,12 +41,7 @@ internal class FunctionPointerVariableHook : Hook { lock (HookManager.HookEnableSyncRoot) { - var hasOtherHooks = HookManager.Originals.ContainsKey(this.Address); - if (!hasOtherHooks) - { - MemoryHelper.ReadRaw(this.Address, 0x32, out var original); - HookManager.Originals[this.Address] = original; - } + var unhooker = HookManager.RegisterUnhooker(this.Address); if (!HookManager.MultiHookTracker.TryGetValue(this.Address, out var indexList)) { @@ -104,6 +99,8 @@ internal class FunctionPointerVariableHook : Hook // Add afterwards, so the hookIdent starts at 0. indexList.Add(this); + unhooker.TrimAfterHook(); + HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly)); } } diff --git a/Dalamud/Hooking/Internal/HookManager.cs b/Dalamud/Hooking/Internal/HookManager.cs index 303802ff6..11a7be337 100644 --- a/Dalamud/Hooking/Internal/HookManager.cs +++ b/Dalamud/Hooking/Internal/HookManager.cs @@ -16,7 +16,10 @@ namespace Dalamud.Hooking.Internal; [ServiceManager.EarlyLoadedService] internal class HookManager : IDisposable, IServiceType { - private static readonly ModuleLog Log = new("HM"); + /// + /// Logger shared with + /// + internal static readonly ModuleLog Log = new("HM"); [ServiceManager.ServiceConstructor] private HookManager() @@ -34,9 +37,21 @@ internal class HookManager : IDisposable, IServiceType internal static ConcurrentDictionary TrackedHooks { get; } = new(); /// - /// Gets a static dictionary of original code for a hooked address. + /// Gets a static dictionary of unhookers for a hooked address. /// - internal static ConcurrentDictionary Originals { get; } = new(); + internal static ConcurrentDictionary Unhookers { get; } = new(); + + /// + /// Creates a new Unhooker instance for the provided address if no such unhooker was already registered, or returns + /// an existing instance if the address registered previously. + /// + /// The address of the instruction. + /// A new Unhooker instance. + public static Unhooker RegisterUnhooker(IntPtr address) + { + Log.Verbose($"Registering hook at 0x{address.ToInt64():X}"); + return Unhookers.GetOrAdd(address, adr => new Unhooker(adr)); + } /// /// Gets a static dictionary of the number of hooks on a given address. @@ -48,7 +63,7 @@ internal class HookManager : IDisposable, IServiceType { RevertHooks(); TrackedHooks.Clear(); - Originals.Clear(); + Unhookers.Clear(); } /// @@ -60,7 +75,7 @@ internal class HookManager : IDisposable, IServiceType { while (true) { - var hasOtherHooks = HookManager.Originals.ContainsKey(address); + var hasOtherHooks = HookManager.Unhookers.ContainsKey(address); if (hasOtherHooks) { // This address has been hooked already. Do not follow a jmp into a trampoline of our own making. @@ -124,28 +139,11 @@ internal class HookManager : IDisposable, IServiceType return address; } - private static unsafe void RevertHooks() + private static void RevertHooks() { - foreach (var (address, originalBytes) in Originals) + foreach (var unhooker in Unhookers.Values) { - var i = 0; - var current = (byte*)address; - // Find how many bytes have been modified by comparing to the saved original - for (; i < originalBytes.Length; i++) - { - if (current[i] == originalBytes[i]) - break; - } - - var snippet = originalBytes[0..i]; - - if (i > 0) - { - Log.Verbose($"Reverting hook at 0x{address.ToInt64():X} ({snippet.Length} bytes)"); - MemoryHelper.ChangePermission(address, i, MemoryProtection.ExecuteReadWrite, out var oldPermissions); - MemoryHelper.WriteRaw(address, snippet); - MemoryHelper.ChangePermission(address, i, oldPermissions); - } + unhooker.Unhook(); } } } diff --git a/Dalamud/Hooking/Internal/MinHookHook.cs b/Dalamud/Hooking/Internal/MinHookHook.cs index 0da289371..89a7d9206 100644 --- a/Dalamud/Hooking/Internal/MinHookHook.cs +++ b/Dalamud/Hooking/Internal/MinHookHook.cs @@ -1,8 +1,6 @@ using System; using System.Reflection; -using Dalamud.Memory; - namespace Dalamud.Hooking.Internal; /// @@ -24,12 +22,7 @@ internal class MinHookHook : Hook where T : Delegate { lock (HookManager.HookEnableSyncRoot) { - var hasOtherHooks = HookManager.Originals.ContainsKey(this.Address); - if (!hasOtherHooks) - { - MemoryHelper.ReadRaw(this.Address, 0x32, out var original); - HookManager.Originals[this.Address] = original; - } + var unhooker = HookManager.RegisterUnhooker(this.Address); if (!HookManager.MultiHookTracker.TryGetValue(this.Address, out var indexList)) indexList = HookManager.MultiHookTracker[this.Address] = new(); @@ -41,6 +34,8 @@ internal class MinHookHook : Hook where T : Delegate // Add afterwards, so the hookIdent starts at 0. indexList.Add(this); + unhooker.TrimAfterHook(); + HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly)); } } diff --git a/Dalamud/Hooking/Internal/ReloadedHook.cs b/Dalamud/Hooking/Internal/ReloadedHook.cs index 77c0c9c19..172bd9671 100644 --- a/Dalamud/Hooking/Internal/ReloadedHook.cs +++ b/Dalamud/Hooking/Internal/ReloadedHook.cs @@ -1,7 +1,6 @@ using System; using System.Reflection; -using Dalamud.Memory; using Reloaded.Hooks; namespace Dalamud.Hooking.Internal; @@ -25,17 +24,13 @@ internal class ReloadedHook : Hook where T : Delegate { lock (HookManager.HookEnableSyncRoot) { - var hasOtherHooks = HookManager.Originals.ContainsKey(this.Address); - if (!hasOtherHooks) - { - MemoryHelper.ReadRaw(this.Address, 0x32, out var original); - HookManager.Originals[this.Address] = original; - } - + var unhooker = HookManager.RegisterUnhooker(address); this.hookImpl = ReloadedHooks.Instance.CreateHook(detour, address.ToInt64()); this.hookImpl.Activate(); this.hookImpl.Disable(); + unhooker.TrimAfterHook(); + HookManager.TrackedHooks.TryAdd(Guid.NewGuid(), new HookInfo(this, detour, callingAssembly)); } } diff --git a/Dalamud/Hooking/Internal/Unhooker.cs b/Dalamud/Hooking/Internal/Unhooker.cs new file mode 100644 index 000000000..e609dd85e --- /dev/null +++ b/Dalamud/Hooking/Internal/Unhooker.cs @@ -0,0 +1,91 @@ +using System; + +using Dalamud.Memory; + +namespace Dalamud.Hooking.Internal; + +/// +/// A class which stores a copy of the bytes at a location which will be hooked in the future, such that those bytes can +/// be restored later to "unhook" the function. +/// +public class Unhooker +{ + private readonly IntPtr address; + private byte[] originalBytes; + private bool trimmed; + + /// + /// Initializes a new instance of the class. Upon creation, the Unhooker stores a copy of + /// the bytes stored at the provided address, and can be used to restore these bytes when the hook should be + /// removed. As such this class should be instantiated before the function is actually hooked. + /// + /// The address which will be hooked. + public Unhooker(IntPtr address) + { + this.address = address; + MemoryHelper.ReadRaw(address, 0x32, out this.originalBytes); + } + + /// + /// When called after a hook is created, checks the pre-hook original bytes and post-hook modified bytes, trimming + /// the original bytes stored and removing unmodified bytes from the end of the byte sequence. Assuming no + /// concurrent actions modified the same address space, this should result in storing only the minimum bytes + /// required to unhook the function. + /// + public void TrimAfterHook() + { + if (this.trimmed) + { + return; + } + + this.originalBytes = this.originalBytes[..this.GetFullHookLength()]; + this.trimmed = true; + } + + /// + /// Attempts to unhook the function by replacing the hooked bytes with the original bytes. If + /// was called, the trimmed original bytes stored at that time will be used for + /// unhooking. Otherwise, a naive algorithm which only restores bytes until the first unchanged byte will be used in + /// order to avoid overwriting adjacent data. + /// + public void Unhook() + { + var len = this.trimmed ? this.originalBytes.Length : this.GetNaiveHookLength(); + if (len > 0) + { + HookManager.Log.Verbose($"Reverting hook at 0x{this.address.ToInt64():X} ({len} bytes, trimmed={this.trimmed})"); + MemoryHelper.ChangePermission(this.address, len, MemoryProtection.ExecuteReadWrite, out var oldPermissions); + MemoryHelper.WriteRaw(this.address, this.originalBytes[..len]); + MemoryHelper.ChangePermission(this.address, len, oldPermissions); + } + } + + private unsafe int GetNaiveHookLength() + { + var current = (byte*)this.address; + for (var i = 0; i < this.originalBytes.Length; i++) + { + if (current[i] == this.originalBytes[i]) + { + return i; + } + } + + return 0; + } + + private unsafe int GetFullHookLength() + { + var current = (byte*)this.address; + for (var i = this.originalBytes.Length - 1; i >= 0; i--) + { + if (current[i] != this.originalBytes[i]) + { + return i + 1; + } + } + + return 0; + } +} From 429316747de4dd1921cd0bb1cb81fff2e1cbd9dd Mon Sep 17 00:00:00 2001 From: nebel Date: Sun, 4 Jun 2023 01:48:18 +0900 Subject: [PATCH 2/3] Add minBytes and maxBytes to Unhooker --- .../Internal/FunctionPointerVariableHook.cs | 2 +- Dalamud/Hooking/Internal/HookManager.cs | 8 ++++--- Dalamud/Hooking/Internal/Unhooker.cs | 21 ++++++++++++++----- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs index 1c0bcfa05..e1900a903 100644 --- a/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs +++ b/Dalamud/Hooking/Internal/FunctionPointerVariableHook.cs @@ -41,7 +41,7 @@ internal class FunctionPointerVariableHook : Hook { lock (HookManager.HookEnableSyncRoot) { - var unhooker = HookManager.RegisterUnhooker(this.Address); + var unhooker = HookManager.RegisterUnhooker(this.Address, 8, 8); if (!HookManager.MultiHookTracker.TryGetValue(this.Address, out var indexList)) { diff --git a/Dalamud/Hooking/Internal/HookManager.cs b/Dalamud/Hooking/Internal/HookManager.cs index 11a7be337..43858d012 100644 --- a/Dalamud/Hooking/Internal/HookManager.cs +++ b/Dalamud/Hooking/Internal/HookManager.cs @@ -46,11 +46,13 @@ internal class HookManager : IDisposable, IServiceType /// an existing instance if the address registered previously. /// /// The address of the instruction. + /// The minimum amount of bytes to restore when unhooking. Defaults to 0. + /// The maximum amount of bytes to restore when unhooking. Defaults to 0x32. /// A new Unhooker instance. - public static Unhooker RegisterUnhooker(IntPtr address) + public static Unhooker RegisterUnhooker(IntPtr address, int minBytes = 0, int maxBytes = 0x32) { - Log.Verbose($"Registering hook at 0x{address.ToInt64():X}"); - return Unhookers.GetOrAdd(address, adr => new Unhooker(adr)); + Log.Verbose($"Registering hook at 0x{address.ToInt64():X} (minBytes=0x{minBytes:X}, maxBytes=0x{maxBytes:X})"); + return Unhookers.GetOrAdd(address, _ => new Unhooker(address, minBytes, maxBytes)); } /// diff --git a/Dalamud/Hooking/Internal/Unhooker.cs b/Dalamud/Hooking/Internal/Unhooker.cs index e609dd85e..09b071ee9 100644 --- a/Dalamud/Hooking/Internal/Unhooker.cs +++ b/Dalamud/Hooking/Internal/Unhooker.cs @@ -11,6 +11,7 @@ namespace Dalamud.Hooking.Internal; public class Unhooker { private readonly IntPtr address; + private readonly int minBytes; private byte[] originalBytes; private bool trimmed; @@ -20,10 +21,18 @@ public class Unhooker /// removed. As such this class should be instantiated before the function is actually hooked. /// /// The address which will be hooked. - public Unhooker(IntPtr address) + /// The minimum amount of bytes to restore when unhooking. + /// The maximum amount of bytes to restore when unhooking. + internal Unhooker(IntPtr address, int minBytes, int maxBytes) { + if (minBytes < 0 || minBytes > maxBytes) + { + throw new ArgumentException($"minBytes ({minBytes}) must be <= maxBytes ({maxBytes}) and nonnegative."); + } + this.address = address; - MemoryHelper.ReadRaw(address, 0x32, out this.originalBytes); + this.minBytes = minBytes; + MemoryHelper.ReadRaw(address, maxBytes, out this.originalBytes); } /// @@ -39,7 +48,9 @@ public class Unhooker return; } - this.originalBytes = this.originalBytes[..this.GetFullHookLength()]; + var len = int.Max(this.GetFullHookLength(), this.minBytes); + + this.originalBytes = this.originalBytes[..len]; this.trimmed = true; } @@ -51,7 +62,7 @@ public class Unhooker /// public void Unhook() { - var len = this.trimmed ? this.originalBytes.Length : this.GetNaiveHookLength(); + var len = this.trimmed ? this.originalBytes.Length : int.Max(this.GetNaiveHookLength(), this.minBytes); if (len > 0) { HookManager.Log.Verbose($"Reverting hook at 0x{this.address.ToInt64():X} ({len} bytes, trimmed={this.trimmed})"); @@ -82,7 +93,7 @@ public class Unhooker { if (current[i] != this.originalBytes[i]) { - return i + 1; + return int.Max(i + 1, this.minBytes); } } From 60a29e36c2a0d2f7a26195c96093501cb4ada110 Mon Sep 17 00:00:00 2001 From: nebel Date: Sun, 4 Jun 2023 02:49:59 +0900 Subject: [PATCH 3/3] Create RegisterUnhooker overload --- Dalamud/Hooking/Internal/HookManager.cs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/Dalamud/Hooking/Internal/HookManager.cs b/Dalamud/Hooking/Internal/HookManager.cs index 43858d012..f185450eb 100644 --- a/Dalamud/Hooking/Internal/HookManager.cs +++ b/Dalamud/Hooking/Internal/HookManager.cs @@ -43,13 +43,26 @@ internal class HookManager : IDisposable, IServiceType /// /// Creates a new Unhooker instance for the provided address if no such unhooker was already registered, or returns - /// an existing instance if the address registered previously. + /// an existing instance if the address was registered previously. By default, the unhooker will restore between 0 + /// and 0x32 bytes depending on the detected size of the hook. To specify the minimum and maximum bytes restored + /// manually, use . /// /// The address of the instruction. - /// The minimum amount of bytes to restore when unhooking. Defaults to 0. - /// The maximum amount of bytes to restore when unhooking. Defaults to 0x32. /// A new Unhooker instance. - public static Unhooker RegisterUnhooker(IntPtr address, int minBytes = 0, int maxBytes = 0x32) + public static Unhooker RegisterUnhooker(IntPtr address) + { + return RegisterUnhooker(address, 0, 0x32); + } + + /// + /// Creates a new Unhooker instance for the provided address if no such unhooker was already registered, or returns + /// an existing instance if the address was registered previously. + /// + /// The address of the instruction. + /// The minimum amount of bytes to restore when unhooking. + /// The maximum amount of bytes to restore when unhooking. + /// A new Unhooker instance. + public static Unhooker RegisterUnhooker(IntPtr address, int minBytes, int maxBytes) { Log.Verbose($"Registering hook at 0x{address.ToInt64():X} (minBytes=0x{minBytes:X}, maxBytes=0x{maxBytes:X})"); return Unhookers.GetOrAdd(address, _ => new Unhooker(address, minBytes, maxBytes));