fix: Fixes ObjectPropertyList double return issue (#1969)

- Fixes double return issue with object property list that is causing corruption.
- Adds DEBUG_ARRAYPOOL define constant which will crash on double return or invalid return scenarios.

> [!IMPORTANT]
> **Developer Notes**
> STArrayPool rented arrays **MUST NOT** be returned **ONLY ONCE** otherwise there will be corruption from double-use.
> Use `DEBUG_ARRAYPOOL` to test potential broken STArrayPool use cases.

> [!NOTE]
> **Why can't I enable the debug all the time?**
> Other than the fact that it will crash due to bad code, the actual tracking system is highly detrimental/problematic for performance and memory consumption by creating objects that have a stack trace.

Projects/Server/Buffers/STArrayPool.cs

@@ -15,6 +15,16 @@ namespace Server.Buffers;
  */
 public class STArrayPool<T> : ArrayPool<T>
 {
+#if DEBUG_ARRAYPOOL
+    private class RentReturnStatus
+    {
+        public string StackTrace { get; set; }
+        public bool IsRented { get; set; }
+    }
+
+    private static readonly ConditionalWeakTable<T[], RentReturnStatus> _rentedArrays = new();
+#endif
+
     private const int StackArraySize = 32;
     private const int BucketCount = 27; // SelectBucketIndex(1024 * 1024 * 1024 + 1)
     private static readonly STArrayPool<T> _shared = new();
@@ -39,6 +49,12 @@ public class STArrayPool<T> : ArrayPool<T>
             if (buffer is not null)
             {
                 cachedBuckets[bucketIndex].Array = null;
+#if DEBUG_ARRAYPOOL
+                _rentedArrays.AddOrUpdate(
+                    buffer,
+                    new RentReturnStatus { IsRented = true }
+                );
+#endif
                 return buffer;
             }
         }
@@ -52,6 +68,12 @@ public class STArrayPool<T> : ArrayPool<T>
                 buffer = b.TryPop();
                 if (buffer is not null)
                 {
+#if DEBUG_ARRAYPOOL
+                    _rentedArrays.AddOrUpdate(
+                        buffer,
+                        new RentReturnStatus { IsRented = true }
+                    );
+#endif
                     return buffer;
                 }
             }
@@ -70,8 +92,16 @@ public class STArrayPool<T> : ArrayPool<T>
             throw new ArgumentOutOfRangeException(nameof(minimumLength));
         }
 
-        buffer = GC.AllocateUninitializedArray<T>(minimumLength);
-        return buffer;
+        var array = GC.AllocateUninitializedArray<T>(minimumLength);
+
+#if DEBUG_ARRAYPOOL
+        _rentedArrays.AddOrUpdate(
+            array,
+            new RentReturnStatus { IsRented = true, StackTrace = Environment.StackTrace }
+        );
+#endif
+
+        return array;
     }
 
     public override void Return(T[]? array, bool clearArray = false)
@@ -91,10 +121,26 @@ public class STArrayPool<T> : ArrayPool<T>
                 Array.Clear(array);
             }
 
+#if DEBUG_ARRAYPOOL
+            if (array.Length != GetMaxSizeForBucket(bucketIndex) || !_rentedArrays.TryGetValue(array, out var status))
+            {
+                throw new ArgumentException("Buffer is not from the pool", nameof(array));
+            }
+
+            if (!status!.IsRented)
+            {
+                throw new InvalidOperationException($"Array has already been returned.\nOriginal StackTrace:{status.StackTrace}\n");
+            }
+
+            // Mark it as returned
+            status.IsRented = false;
+            status.StackTrace = Environment.StackTrace;
+#else
             if (array.Length != GetMaxSizeForBucket(bucketIndex))
             {
                 throw new ArgumentException("Buffer is not from the pool", nameof(array));
             }
+#endif
 
             ref var bucketArray = ref cacheBuckets[bucketIndex];
             var prev = bucketArray.Array;

Projects/Server/PropertyList/ObjectPropertyList.cs

@@ -1,6 +1,6 @@
 /*************************************************************************
  * ModernUO                                                              *
- * Copyright 2019-2023 - ModernUO Development Team                       *
+ * Copyright 2019-2024 - ModernUO Development Team                       *
  * Email: hi@modernuo.com                                                *
  * File: ObjectPropertyList.cs                                           *
  *                                                                       *
@@ -79,8 +79,9 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
         _stringNumbersIndex = 0;
         Header = 0;
         HeaderArgs = null;
-        STArrayPool<char>.Shared.Return(_arrayToReturnToPool);
         _pos = 0;
+
+        Dispose();
     }
 
     private void Flush()
@@ -499,13 +500,19 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void Dispose()
     {
-        STArrayPool<char>.Shared.Return(_arrayToReturnToPool);
-        _arrayToReturnToPool = null;
+        if (_arrayToReturnToPool != null)
+        {
+            STArrayPool<char>.Shared.Return(_arrayToReturnToPool);
+            _arrayToReturnToPool = null;
+        }
     }
 
     ~ObjectPropertyList()
     {
-        STArrayPool<char>.Shared.Return(_arrayToReturnToPool);
-        _arrayToReturnToPool = null;
+        if (_arrayToReturnToPool != null)
+        {
+            STArrayPool<char>.Shared.Return(_arrayToReturnToPool);
+            _arrayToReturnToPool = null;
+        }
     }
 }

Projects/Server/Utilities/Utility.cs

@@ -1460,4 +1460,12 @@ public static partial class Utility
 
         return (r << 10) | (g << 5) | b;
     }
+
+    public static void AddOrUpdate<TKey, TValue>(this ConditionalWeakTable<TKey, TValue> table, TKey key, TValue value)
+        where TKey : class
+        where TValue : class
+    {
+        table.Remove(key);
+        table.Add(key, value);
+    }
 }