diff --git a/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListHashTests.cs b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListHashTests.cs new file mode 100644 index 000000000..19a8704b7 --- /dev/null +++ b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListHashTests.cs @@ -0,0 +1,147 @@ +using System; +using System.Collections.Generic; +using Server; +using Xunit; + +namespace Server.Tests; + +public class ObjectPropertyListHashTests +{ + private static int BuildHash(params (int cliloc, string arg)[] properties) + { + var opl = new ObjectPropertyList(null); + + foreach (var (cliloc, arg) in properties) + { + if (arg == null) + { + opl.Add(cliloc); + } + else + { + opl.Add(cliloc, arg.AsSpan()); + } + } + + opl.Terminate(); + return opl.Hash; + } + + // Two properties sharing an argument: the XOR fold mixed it in twice and cancelled it, so + // "10%" and "5%" produced the same revision and the client kept the stale tooltip. + [Fact] + public void RepeatedArgument_DoesNotCancelOut() + { + var ten = BuildHash( + (1063752, "10"), + (1063737, "10"), + (1063740, null) + ); + + var five = BuildHash( + (1063752, "5"), + (1063737, "5"), + (1063740, null) + ); + + Assert.NotEqual(ten, five); + } + + // XOR is self-inverse: any value mixed in an even number of times vanished. + [Fact] + public void DuplicateProperty_ChangesHash() + { + var once = BuildHash((1060658, null)); + var twice = BuildHash((1060658, null), (1060658, null)); + + Assert.NotEqual(once, twice); + } + + // XOR is commutative, so emission order was invisible to the hash but visible in the tooltip. + [Fact] + public void PropertyOrder_ChangesHash() + { + var forward = BuildHash((1060658, "Alpha"), (1060659, "Beta")); + var reversed = BuildHash((1060659, "Beta"), (1060658, "Alpha")); + + Assert.NotEqual(forward, reversed); + } + + [Fact] + public void SwappedArguments_ChangeHash() + { + var forward = BuildHash((1063752, "10"), (1063737, "5")); + var swapped = BuildHash((1063752, "5"), (1063737, "10")); + + Assert.NotEqual(forward, swapped); + } + + [Fact] + public void IdenticalContent_ProducesIdenticalHash() + { + var first = BuildHash((1063752, "10"), (1063737, "5"), (1063740, null)); + var second = BuildHash((1063752, "10"), (1063737, "5"), (1063740, null)); + + Assert.Equal(first, second); + } + + // The client masks 0x40000000 off the 0xDC revision to match the 0xD6 hash, so the hash has + // to stay below that bit. + [Fact] + public void Hash_StaysWithinTheRevisionMask() + { + var opl = new ObjectPropertyList(null); + opl.Add(1063752, "A rather long argument that pushes the buffer past its initial size".AsSpan()); + opl.Add(1063737, "12345"); + opl.Add(1063740); + opl.Terminate(); + + Assert.Equal(0x40000000, opl.Hash & ~0x3FFFFFF); + } + + // 6- and 8-byte blocks take xxHash3's short-input paths. They still avalanche across all 26 + // kept bits, so a counter ticking down never repeats the revision it just had. + [Fact] + public void ShortNumericArguments_ConsecutiveValuesDiffer() + { + var previous = BuildHash((1060584, "0")); + + for (var charges = 1; charges < 20000; charges++) + { + var current = BuildHash((1060584, charges.ToString())); + Assert.NotEqual(previous, current); + previous = current; + } + } + + [Fact] + public void SmallPropertyBlocks_StayWellDistributed() + { + const int count = 20000; + + var withArgument = new HashSet(); + var withoutArgument = new HashSet(); + + for (var i = 0; i < count; i++) + { + withArgument.Add(BuildHash((1060584, i.ToString()))); + withoutArgument.Add(BuildHash((1060000 + i, null))); + } + + // Birthday expects ~3 collisions over a 26-bit space; allow an order of magnitude so the + // bound holds for any seed. A hash that stopped mixing collapses far past it. + Assert.True(withArgument.Count >= count - 30, $"8-byte blocks: {withArgument.Count}/{count}"); + Assert.True(withoutArgument.Count >= count - 30, $"6-byte blocks: {withoutArgument.Count}/{count}"); + } + + [Fact] + public void EmptyList_IsNonZeroAndDistinctFromPopulated() + { + var empty = new ObjectPropertyList(null); + empty.Terminate(); + + Assert.NotEqual(0, empty.Hash); + Assert.Equal(0x40000000, empty.Hash & ~0x3FFFFFF); + Assert.NotEqual(empty.Hash, BuildHash((1060658, null))); + } +} diff --git a/Projects/Server/PropertyList/ObjectPropertyList.cs b/Projects/Server/PropertyList/ObjectPropertyList.cs index 58b247990..d1955ad2d 100644 --- a/Projects/Server/PropertyList/ObjectPropertyList.cs +++ b/Projects/Server/PropertyList/ObjectPropertyList.cs @@ -46,6 +46,13 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable // under the empirically confirmed ~510-char ceiling. For multi-line content use AddChunked(). public const int MaxArgumentLength = 504; + // 0xD6 header: packet id, length, unknown, serial, unknown, hash. Properties start after it. + private const int HeaderLength = 15; + + // Terminate writes the bare hash into 0xD6, SendOPLInfo writes Hash with bit 30 set, and the + // client recovers one from the other by masking off 0x40000000. The hash must stay below it. + private const int HashMask = 0x3FFFFFF; + private int _hash; private int _stringNumbersIndex; private byte[] _buffer; @@ -89,7 +96,7 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void Reset() { - _bufferPos = 15; + _bufferPos = HeaderLength; _hash = 0; _stringNumbersIndex = 0; Header = 0; @@ -120,6 +127,11 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable Resize(length); } + // xxHash3 over the finished property block. Order and repetition sensitive, unlike the + // XOR fold it replaces, which collided whenever properties were reordered or shared an + // argument. + _hash = (int)(HashUtility.ComputeHash64(_buffer.AsSpan(HeaderLength, _bufferPos - HeaderLength)) & HashMask); + var writer = new SpanWriter(_buffer); writer.Seek(_bufferPos, SeekOrigin.Begin); writer.Write(0); @@ -129,12 +141,6 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable writer.WritePacketLength(); } - private void AddHash(int val) - { - _hash ^= val & 0x3FFFFFF; - _hash ^= (val >> 26) & 0x3F; - } - public void Add(int number) { if (number == 0) @@ -148,8 +154,6 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable HeaderArgs = ""; } - AddHash(number); - var length = _bufferPos + 6; while (length > _buffer.Length) { @@ -245,9 +249,6 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable HeaderArgs = chars.ToString(); } - AddHash(number); - AddHash(string.GetHashCode(chars, StringComparison.Ordinal)); - var strLength = chars.Length * 2; var length = _bufferPos + 6 + strLength; while (length > _buffer.Length) @@ -295,9 +296,6 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable HeaderArgs = chars.ToString(); } - AddHash(number); - AddHash(string.GetHashCode(chars, StringComparison.Ordinal)); - var strLength = chars.Length * 2; var length = _bufferPos + 6 + strLength; while (length > _buffer.Length) diff --git a/Projects/UOContent.Tests/Tests/Engines/Plants/PlantItemPropertyListTests.cs b/Projects/UOContent.Tests/Tests/Engines/Plants/PlantItemPropertyListTests.cs new file mode 100644 index 000000000..b8a4f9d4b --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/Plants/PlantItemPropertyListTests.cs @@ -0,0 +1,36 @@ +using Server.Engines.Plants; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class PlantItemPropertyListTests +{ + // The getter used to re-initialize without a Reset, appending another copy of every property + // per read. SendOPLPacketTo and SendPropertiesTo both go through it, so a pre-7.0.12 client + // looking at a plant grew the buffer without bound. + [Fact] + public void OldClientPropertyList_BuildsOnceAndIsStableAcrossReads() + { + var plant = new PlantItem(); + + try + { + var first = plant.OldClientPropertyList; + var length = first.Buffer.Length; + var hash = first.Hash; + + var second = plant.OldClientPropertyList; + var third = plant.OldClientPropertyList; + + Assert.Same(first, second); + Assert.Same(first, third); + Assert.Equal(length, third.Buffer.Length); + Assert.Equal(hash, third.Hash); + } + finally + { + plant.Delete(); + } + } +} diff --git a/Projects/UOContent/Engines/Plants/PlantItem.cs b/Projects/UOContent/Engines/Plants/PlantItem.cs index f829b0e20..5633db48c 100644 --- a/Projects/UOContent/Engines/Plants/PlantItem.cs +++ b/Projects/UOContent/Engines/Plants/PlantItem.cs @@ -73,7 +73,15 @@ public partial class PlantItem : Item, ISecurable { get { - InitializePropertyList(_oldClientPropertyList ??= new ObjectPropertyList(this)); + // Build once, like Item.PropertyList. Initializing on every read appended another copy + // of every property to the same list. InvalidateProperties rebuilds it, Reset first. + if (_oldClientPropertyList == null) + { + var list = new ObjectPropertyList(this); + _oldClientPropertyList = list; + InitializePropertyList(list); + } + return _oldClientPropertyList; } }