Skip to content

Commit

Permalink
Skin Fixer: Fix potential ref leak + add SRH
Browse files Browse the repository at this point in the history
`SafeResourceHandle` wraps a `ResourceHandle*` with auto `IncRef` / `DecRef`, to further help prevent leaks.
  • Loading branch information
Exter-N committed Aug 30, 2023
1 parent ec14efb commit f238049
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 14 deletions.
34 changes: 34 additions & 0 deletions Penumbra/Interop/SafeHandles/SafeResourceHandle.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System;
using System.Runtime.InteropServices;
using System.Threading;
using FFXIVClientStructs.FFXIV.Client.System.Resource.Handle;

namespace Penumbra.Interop.SafeHandles;

public unsafe class SafeResourceHandle : SafeHandle
{
public ResourceHandle* ResourceHandle => (ResourceHandle*)handle;

public override bool IsInvalid => handle == 0;

public SafeResourceHandle(ResourceHandle* handle, bool incRef, bool ownsHandle = true) : base(0, ownsHandle)
{
if (incRef && !ownsHandle)
throw new ArgumentException("Non-owning SafeResourceHandle with IncRef is unsupported");
if (incRef && handle != null)
handle->IncRef();
SetHandle((nint)handle);
}

public static SafeResourceHandle CreateInvalid()
=> new(null, incRef: false);

protected override bool ReleaseHandle()
{
var handle = Interlocked.Exchange(ref this.handle, 0);
if (handle != 0)
((ResourceHandle*)handle)->DecRef();

return true;
}
}
40 changes: 26 additions & 14 deletions Penumbra/Interop/Services/SkinFixer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Penumbra.GameData.Enums;
using Penumbra.Interop.PathResolving;
using Penumbra.Interop.ResourceLoading;
using Penumbra.Interop.SafeHandles;
using Penumbra.String.Classes;

namespace Penumbra.Interop.Services;
Expand Down Expand Up @@ -44,7 +45,7 @@ private struct OnRenderMaterialParams
private readonly ResourceLoader _resources;
private readonly CharacterUtility _utility;

private readonly ConcurrentDictionary<nint /* CharacterBase* */, nint /* ResourceHandle* */> _skinShpks = new();
private readonly ConcurrentDictionary<nint /* CharacterBase* */, SafeResourceHandle> _skinShpks = new();

private readonly object _lock = new();

Expand Down Expand Up @@ -89,8 +90,7 @@ protected virtual void Dispose(bool disposing)
_gameEvents.CharacterBaseCreated -= OnCharacterBaseCreated;
_gameEvents.CharacterBaseDestructor -= OnCharacterBaseDestructor;
foreach (var skinShpk in _skinShpks.Values)
if (skinShpk != nint.Zero)
((ResourceHandle*)skinShpk)->DecRef();
skinShpk.Dispose();
_skinShpks.Clear();
_moddedSkinShpkCount = 0;
}
Expand All @@ -105,29 +105,41 @@ private void OnCharacterBaseCreated(uint modelCharaId, nint customize, nint equi

Task.Run(delegate
{
nint skinShpk;
var skinShpk = SafeResourceHandle.CreateInvalid();
try
{
var data = _collectionResolver.IdentifyCollection((DrawObject*)drawObject, true);
skinShpk = data.Valid ? (nint)_resources.LoadResolvedResource(ResourceCategory.Shader, ResourceType.Shpk, SkinShpkPath.Path, data) : nint.Zero;
if (data.Valid)
{
var loadedShpk = _resources.LoadResolvedResource(ResourceCategory.Shader, ResourceType.Shpk, SkinShpkPath.Path, data);
skinShpk = new SafeResourceHandle((ResourceHandle*)loadedShpk, incRef: false);
}
}
catch (Exception e)
{
Penumbra.Log.Error($"Error while resolving skin.shpk for human {drawObject:X}: {e}");
skinShpk = nint.Zero;
}

if (skinShpk != nint.Zero && _skinShpks.TryAdd(drawObject, skinShpk) && skinShpk != _utility.DefaultSkinShpkResource)
Interlocked.Increment(ref _moddedSkinShpkCount);
if (!skinShpk.IsInvalid)
{
if (_skinShpks.TryAdd(drawObject, skinShpk))
{
if ((nint)skinShpk.ResourceHandle != _utility.DefaultSkinShpkResource)
Interlocked.Increment(ref _moddedSkinShpkCount);
}
else
skinShpk.Dispose();
}
});
}

private void OnCharacterBaseDestructor(nint characterBase)
{
if (_skinShpks.Remove(characterBase, out var skinShpk) && skinShpk != nint.Zero)
if (_skinShpks.Remove(characterBase, out var skinShpk))
{
((ResourceHandle*)skinShpk)->DecRef();
if (skinShpk != _utility.DefaultSkinShpkResource)
var handle = skinShpk.ResourceHandle;
skinShpk.Dispose();
if ((nint)handle != _utility.DefaultSkinShpkResource)
Interlocked.Decrement(ref _moddedSkinShpkCount);
}
}
Expand All @@ -136,12 +148,12 @@ private nint OnRenderHumanMaterial(nint human, OnRenderMaterialParams* param)
{
if (!_enabled || // Can be toggled on the debug tab.
_moddedSkinShpkCount == 0 || // If we don't have any on-screen instances of modded skin.shpk, we don't need the slow path at all.
!_skinShpks.TryGetValue(human, out var skinShpk) || skinShpk == nint.Zero)
!_skinShpks.TryGetValue(human, out var skinShpk))
return _onRenderMaterialHook!.Original(human, param);

var material = param->Model->Materials[param->MaterialIndex];
var shpkResource = ((Structs.MtrlResource*)material->MaterialResourceHandle)->ShpkResourceHandle;
if ((nint)shpkResource != skinShpk)
if ((nint)shpkResource != (nint)skinShpk.ResourceHandle)
return _onRenderMaterialHook!.Original(human, param);

Interlocked.Increment(ref _slowPathCallDelta);
Expand All @@ -154,7 +166,7 @@ private nint OnRenderHumanMaterial(nint human, OnRenderMaterialParams* param)
lock (_lock)
try
{
_utility.Address->SkinShpkResource = (Structs.ResourceHandle*)skinShpk;
_utility.Address->SkinShpkResource = (Structs.ResourceHandle*)skinShpk.ResourceHandle;
return _onRenderMaterialHook!.Original(human, param);
}
finally
Expand Down

0 comments on commit f238049

Please sign in to comment.