fix: OPL revision hash collided on reordered and repeated properties (#2615)
## 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`
This commit is contained in:
Kamron Batman
GitHub
parent
ab738d90af
commit
93e46a88b2
4 changed files with 205 additions and 16 deletions
|
|
@ -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<int>();
|
||||
var withoutArgument = new HashSet<int>();
|
||||
|
||||
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)));
|
||||
}
|
||||
}
|
||||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue