From 0c554fe47dab3c3214f4aef9ee250d4f2420a878 Mon Sep 17 00:00:00 2001 From: MidoriKami <9083275+MidoriKami@users.noreply.github.com> Date: Sat, 14 Oct 2023 12:48:21 -0700 Subject: [PATCH] AddonLifecycle Extra Safety (#1483) * Disallow attempting to hook base AtkEventListener.ReceiveEvent * Fix incorrect address resolution * Add exception guards * Remove debug logging --- .../Game/Addon/Lifecycle/AddonLifecycle.cs | 87 ++++++++++++++++--- .../AddonLifecycleAddressResolver.cs | 7 ++ 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs b/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs index 07f12fe35..c42481d63 100644 --- a/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs +++ b/Dalamud/Game/Addon/Lifecycle/AddonLifecycle.cs @@ -26,6 +26,8 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType [ServiceManager.ServiceDependency] private readonly Framework framework = Service.Get(); + private readonly nint disallowedReceiveEventAddress; + private readonly AddonLifecycleAddressResolver address; private readonly CallHook onAddonSetupHook; private readonly CallHook onAddonSetup2Hook; @@ -47,6 +49,9 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType this.address = new AddonLifecycleAddressResolver(); this.address.Setup(sigScanner); + // We want value of the function pointer at vFunc[2] + this.disallowedReceiveEventAddress = ((nint*)this.address.AtkEventListener)![2]; + this.framework.Update += this.OnFrameworkUpdate; this.onAddonSetupHook = new CallHook(this.address.AddonSetup, this.OnAddonSetup); @@ -169,13 +174,18 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType try { // Hook the addon's ReceiveEvent function here, but only enable the hook if we have an active listener. + // Disallows hooking the core internal event handler. var addonName = MemoryHelper.ReadStringNullTerminated((nint)addon->Name); - var receiveEventHook = Hook.FromAddress((nint)addon->VTable->ReceiveEvent, this.OnReceiveEvent); - this.receiveEventHooks.TryAdd(addonName, receiveEventHook); - - if (this.eventListeners.Any(listener => (listener.EventType is AddonEvent.PostReceiveEvent or AddonEvent.PreReceiveEvent) && listener.AddonName == addonName)) + var receiveEventAddress = (nint)addon->VTable->ReceiveEvent; + if (receiveEventAddress != this.disallowedReceiveEventAddress) { - receiveEventHook.Enable(); + var receiveEventHook = Hook.FromAddress(receiveEventAddress, this.OnReceiveEvent); + this.receiveEventHooks.TryAdd(addonName, receiveEventHook); + + if (this.eventListeners.Any(listener => (listener.EventType is AddonEvent.PostReceiveEvent or AddonEvent.PreReceiveEvent) && listener.AddonName == addonName)) + { + receiveEventHook.Enable(); + } } } catch (Exception e) @@ -197,7 +207,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnAddonSetup pre-setup invoke."); } - addon->OnSetup(valueCount, values); + try + { + addon->OnSetup(valueCount, values); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonSetup. This may be a bug in the game or another plugin hooking this method."); + } try { @@ -240,7 +257,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnAddonFinalize pre-finalize invoke."); } - this.onAddonFinalizeHook.Original(unitManager, atkUnitBase); + try + { + this.onAddonFinalizeHook.Original(unitManager, atkUnitBase); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonFinalize. This may be a bug in the game or another plugin hooking this method."); + } } private void OnAddonDraw(AtkUnitBase* addon) @@ -254,7 +278,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnAddonDraw pre-draw invoke."); } - addon->Draw(); + try + { + addon->Draw(); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonDraw. This may be a bug in the game or another plugin hooking this method."); + } try { @@ -277,7 +308,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnAddonUpdate pre-update invoke."); } - addon->Update(delta); + try + { + addon->Update(delta); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonUpdate. This may be a bug in the game or another plugin hooking this method."); + } try { @@ -291,6 +329,8 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType private byte OnAddonRefresh(AtkUnitManager* atkUnitManager, AtkUnitBase* addon, uint valueCount, AtkValue* values) { + byte result = 0; + try { this.InvokeListeners(AddonEvent.PreRefresh, new AddonRefreshArgs @@ -305,7 +345,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnAddonRefresh pre-refresh invoke."); } - var result = this.onAddonRefreshHook.Original(atkUnitManager, addon, valueCount, values); + try + { + result = this.onAddonRefreshHook.Original(atkUnitManager, addon, valueCount, values); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonRefresh. This may be a bug in the game or another plugin hooking this method."); + } try { @@ -340,7 +387,14 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnRequestedUpdate pre-requestedUpdate invoke."); } - addon->OnUpdate(numberArrayData, stringArrayData); + try + { + addon->OnUpdate(numberArrayData, stringArrayData); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonRequestedUpdate. This may be a bug in the game or another plugin hooking this method."); + } try { @@ -375,8 +429,15 @@ internal unsafe class AddonLifecycle : IDisposable, IServiceType Log.Error(e, "Exception in OnReceiveEvent pre-receiveEvent invoke."); } - var addonName = MemoryHelper.ReadStringNullTerminated((nint)addon->Name); - this.receiveEventHooks[addonName].Original(addon, eventType, eventParam, atkEvent, data); + try + { + var addonName = MemoryHelper.ReadStringNullTerminated((nint)addon->Name); + this.receiveEventHooks[addonName].Original(addon, eventType, eventParam, atkEvent, data); + } + catch (Exception e) + { + Log.Error(e, "Caught exception when calling original AddonReceiveEvent. This may be a bug in the game or another plugin hooking this method."); + } try { diff --git a/Dalamud/Game/Addon/Lifecycle/AddonLifecycleAddressResolver.cs b/Dalamud/Game/Addon/Lifecycle/AddonLifecycleAddressResolver.cs index c308d1676..df25d0a46 100644 --- a/Dalamud/Game/Addon/Lifecycle/AddonLifecycleAddressResolver.cs +++ b/Dalamud/Game/Addon/Lifecycle/AddonLifecycleAddressResolver.cs @@ -43,6 +43,12 @@ internal class AddonLifecycleAddressResolver : BaseAddressResolver /// Gets the address of AtkUnitManager_vf10 which triggers addon onRefresh. /// public nint AddonOnRefresh { get; private set; } + + /// + /// Gets the address of AtkEventListener base vTable. + /// This is used to ensure that we do not hook ReceiveEvents that resolve back to the internal handler. + /// + public nint AtkEventListener { get; private set; } /// /// Scan for and setup any configured address pointers. @@ -57,5 +63,6 @@ internal class AddonLifecycleAddressResolver : BaseAddressResolver this.AddonUpdate = sig.ScanText("FF 90 ?? ?? ?? ?? 40 88 AF"); this.AddonOnRequestedUpdate = sig.ScanText("FF 90 98 01 00 00 48 8B 5C 24 30 48 83 C4 20"); this.AddonOnRefresh = sig.ScanText("48 89 5C 24 08 57 48 83 EC 20 41 8B F8 48 8B DA"); + this.AtkEventListener = sig.GetStaticAddressFromSig("4C 8D 3D ?? ?? ?? ?? 49 8D 8E"); } }