From 93e46a88b23c23cbac67f8a3ed3e8a1c951abb71 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:32:12 -0700 Subject: [PATCH] fix: OPL revision hash collided on reordered and repeated properties (#2615) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The bug `ObjectPropertyList.AddHash` folded each cliloc and a Marvin hash of each argument together with XOR, which is both commutative and self-inverse. Any value mixed in an even number of times cancelled outright, so two properties sharing one argument hashed identically no matter what that argument was: ``` _hash = 31659021 AddHash(1063752); AddHash(hash("10")) AddHash(1063737); AddHash(hash("10")) // the argument cancels here AddHash(1063740) // -> 32714560, for ANY argument ``` The 10% and 5% variants of the same item therefore produced the same revision. The client caches the tooltip by revision and only re-requests when it changes, so it kept rendering the stale percentage. Same root cause, three more shapes: | | old | new | |---|---|---| | Two properties sharing an argument | collides | distinct | | Properties reordered | collides | distinct | | Two properties trading arguments | collides | distinct | | Argument-less property added twice | cancels to 0 | distinct | ## The fix Hash the finished property block in `Terminate()` instead of accumulating per property. Those bytes — cliloc, length prefix and UTF-16 text, in emission order — are the exact content the client renders, so anything that changes the tooltip changes the hash. The length prefix also removes the concatenation ambiguity the old scheme had. `AddHash` and both `string.GetHashCode` calls are gone. ## Why 26 bits, and why not 64 The client never recomputes the hash — it stores what we send in 0xD6 and compares it for equality against the 0xDC revision (`ObjectPropertiesListManager.IsRevisionEquals`). So the algorithm is ours to choose, but the width is not: - Both packets carry a **4-byte** revision, so 64 bits is not available. A wider internal value would be worse than useless: the server would see a change and send a 0xDC whose truncated 32 bits are identical, and the client still wouldn't refresh. - `Terminate` writes the bare hash into 0xD6 while `SendOPLInfo` writes `Hash` with bit 30 set. The client recovers one from the other by masking off `0x40000000`, which only holds while the hash stays below that bit. Hence 26 bits, unchanged from before. - `Hash => 0x40000000 + _hash` keeps the revision non-zero; the client parks 0 as its "nothing cached" sentinel. ## Collision behaviour Enumerated real small-OPL spaces and counted 26-bit collisions against the birthday expectation for a uniform hash: | Scenario | block | n | collisions | expected | |---|---|---|---|---| | 1 cliloc, no arg (every cliloc 1.0M–3.2M) | 6 B | 2,200,000 | 35,591 | ~36,061 | | 1 cliloc + numeric arg 0–199,999 | 8–12 B | 200,000 | 300 | ~298 | | 2 clilocs, both with numeric args | 12–24 B | 202,500 | 288 | ~306 | | 1 cliloc + 1-char arg | 8 B | 4,000,000 | 116,291 | ~119,209 | Every case lands on the random-model line, and per-bit P(1) across all 26 bits is 0.4962–0.5024 — XXH3's short-input paths still avalanche fully, so a 6-byte block behaves like a uniform 26-bit draw. Masking the low 26 bits versus folding all 64 down was a wash (35,591 vs 35,558). The metric that matters is narrower, since the client compares a revision only against the previous revision **for the same serial**: 2^-26 = 1.5e-8 per genuine tooltip change. Consecutive-value transitions (charges 50 -> 49) collided 0 times in 200,000. Row 3 is the old scheme quantified: it collapsed 202,500 inputs into 100,954 distinct hashes, a 50% collision rate — systematic, not probabilistic. One honest regression: in row 1 the old scheme had zero collisions, because for a single argument-less cliloc under 2^26 the hash was the identity function. An item whose whole tooltip is one argument-less cliloc changing to a different one goes from never colliding to 1.5e-8. ## Performance Cheaper, not just correct — one xxHash3 pass replaces a Marvin hash per string property over those same bytes. A 12-property, 302-byte tooltip, steady state: ``` old (XOR + Marvin per property) 70.5 ns xxHash3 (streaming, HashUtility) 27.0 ns ``` `XxHash3.HashToUInt64` one-shot is a further ~7ns faster and produces byte-identical output, but taking it would mean editing `HashUtility.ComputeHash64`, whose values are baked into save files via `AssemblyHandler.GetTypeHash`. Not worth it. ## Second commit: plant old-client property list Found while tracing the `Hash` read sites. `PlantItem.OldClientPropertyList` called `InitializePropertyList` on every access instead of only when the list was null, and without a `Reset`. Every read appended another copy of every property to the same buffer. `SendOPLPacketTo` and `SendPropertiesTo` both go through the getter, so a pre-7.0.12 client looking at a plant grew the buffer without bound and moved the revision on reads alone. Now builds once, matching `Item.PropertyList`. `InvalidateProperties` already `Reset`s before rebuilding. ## Testing `Server.Tests` 869 passed, `UOContent.Tests` 779 passed. New regression tests, each confirmed failing against the old code first: - `RepeatedArgument_DoesNotCancelOut` — the reported case - `PropertyOrder_ChangesHash`, `SwappedArguments_ChangeHash`, `DuplicateProperty_ChangesHash` - `ShortNumericArguments_ConsecutiveValuesDiffer`, `SmallPropertyBlocks_StayWellDistributed` — short-input avalanche, bounded loosely enough to hold for any seed - `Hash_StaysWithinTheRevisionMask`, `EmptyList_IsNonZeroAndDistinctFromPopulated` — wire constraints - `PlantItemPropertyListTests.OldClientPropertyList_BuildsOnceAndIsStableAcrossReads` --- .../ObjectPropertyListHashTests.cs | 147 ++++++++++++++++++ .../Server/PropertyList/ObjectPropertyList.cs | 28 ++-- .../Plants/PlantItemPropertyListTests.cs | 36 +++++ .../UOContent/Engines/Plants/PlantItem.cs | 10 +- 4 files changed, 205 insertions(+), 16 deletions(-) create mode 100644 Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListHashTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Engines/Plants/PlantItemPropertyListTests.cs 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; } }