Race condition in ComposableAttribute: NullReferenceException when a magic effect expires while an attribute is recalculated
Summary
ComposableAttribute stores its elements in a plain, unsynchronized List<IElement> and enumerates it live while recomputing a value. Magic-effect expiration mutates that same list (RemoveElement) from a System.Threading.Timer callback on a thread-pool thread, without any lock shared with the reader. When an effect expires at the exact moment an attribute in the same dependency chain is being recalculated, the concurrent List mutation transiently exposes a null slot to the enumeration, throwing a NullReferenceException.
It is a long-standing latent thread-safety gap (the unsynchronized element list dates back years), but it was effectively never hit until server-side offline bots started reading attributes in a tight loop and buffing themselves. The offline MU Helper catches the exception, so bots keep running, but the server log is flooded with errors. Online players are theoretically exposed to the same race (far less frequently), and for them the exception is not caught by the helper's safety net.
Environment
- Branch:
master
- Affected files:
src/AttributeSystem/ComposableAttribute.cs, src/AttributeSystem/AttributeSystem.cs, src/GameLogic/MagicEffectsList.cs, src/GameLogic/MagicEffect.cs
Observed error
Logged repeatedly (bursts of several bots at once), swallowed by OfflinePlayerMuHelper.SafeTickAsync:
[Error] [MUnique.OpenMU.GameLogic.Bots.BotPlayer] Error in offline player helper tick for "bot0019".
System.NullReferenceException: Object reference not set to an instance of an object.
at MUnique.OpenMU.AttributeSystem.ComposableAttribute.<>c.<GetAndCacheValue>b__10_0(IElement e) in ComposableAttribute.cs:line 65
at MUnique.OpenMU.AttributeSystem.ComposableAttribute.GetAndCacheValue() in ComposableAttribute.cs:line 59
at MUnique.OpenMU.AttributeSystem.AttributeRelationshipElement.CalculateValue() in AttributeRelationshipElement.cs:line 70
at MUnique.OpenMU.AttributeSystem.AttributeRelationshipElement.get_Value() in AttributeRelationshipElement.cs:line 54
... (nested AttributeRelationshipElement / ComposableAttribute frames) ...
at MUnique.OpenMU.GameLogic.Offline.HealingHandler.PerformSelfHealingAsync()
at MUnique.OpenMU.GameLogic.Offline.HealingHandler.PerformHealthRecoveryAsync() in HealingHandler.cs:line 73
at MUnique.OpenMU.GameLogic.Offline.OfflinePlayerMuHelper.TickAsync(...) in OfflinePlayerMuHelper.cs:line 207
The leaf frame is the Where predicate in GetAndCacheValue (e => e.AggregateType == ...) dereferencing a null element e.
Root cause
-
Unsynchronized shared list, live enumeration (reader).
ComposableAttribute keeps private readonly IList<IElement> _elementList = new List<IElement>(); and GetAndCacheValue() enumerates it directly:
var rawValues = this.Elements.Where(e => e.AggregateType == AggregateType.AddRaw).Sum(e => e.Value);
this.Elements returns the live _elementList; there is no snapshot and no lock.
-
Off-thread mutation (writer).
A MagicEffect schedules expiry with System.Threading.Timer (MagicEffect.cs). On timeout, MagicEffectsList.OnEffectTimeOutAsync runs on a thread-pool thread and calls this._owner.Attributes.RemoveElement(powerUp.Element, powerUp.Target) — after releasing _addLock (MagicEffectsList.cs). AttributeSystem.RemoveElement forwards to ComposableAttribute.RemoveElement → _elementList.Remove(element). Nothing serializes this against readers.
-
The race.
List<T>.Remove shifts the backing array and clears the last slot. A reader enumerating with the pre-shift _size can observe a null in that slot, so e.AggregateType throws NullReferenceException. Because this happens deep in a chain of AttributeRelationshipElements (e.g. Maximum Health / Current Health derivations), any expiring buff that contributes to the recalculated attribute can trigger it.
Ruled out (not a data problem): a buff never contributes a null element — MagicEffectPowerUpExtensions throws when Boost is missing and CreateElement never returns null; the leaf element types (SimpleElement, ConstantElement) have trivial Value getters that cannot throw. The null only exists transiently during concurrent mutation.
How to reproduce / why it surfaced now
Any holder that (a) reads an attribute frequently and (b) has magic effects expiring concurrently will eventually hit it. Server-side offline bots make this reliable:
- The offline MU Helper ticks every ~500 ms and reads
MaximumHealth / CurrentHealth (health-threshold checks in HealingHandler).
- Bots buff themselves, so they hold magic effects that expire on timers.
- Buffs applied around the same time expire around the same time → several timers fire in one window → multiple bot ticks catch the torn read together (hence the bursts).
Before offline bots, offline players had no self-buffs and no tight read loop, so the pre-existing race was essentially never exercised.
Impact
- Bots: non-fatal.
OfflinePlayerMuHelper.SafeTickAsync catches the exception, logs it, and the bot continues; the affected 500 ms tick simply skips healing. Main harm is log spam (can fill disk on busy servers).
- Online players: the same race is possible (e.g. damage calculation reading defense/max HP as a buff expires). It is much rarer, but there is no MU-Helper safety net around a player's attribute reads, so the exception can bubble up into the action/packet handler.
Suggested fix
Make ComposableAttribute thread-safe, since it is the shared point where both the mutation and the enumeration meet:
- Guard
AddElement / RemoveElement and the enumeration inside GetAndCacheValue with a private lock (lock / System.Threading.Lock), or
- At minimum, snapshot the list before enumerating in
GetAndCacheValue (_elementList.ToArray()), combined with serialized mutations so two concurrent writers cannot corrupt the list.
The lock overhead is negligible: it is only taken on element add/remove and on an actual recompute (cache miss); reads that hit _cachedValue don't touch the list. Fixing it in ComposableAttribute protects every attribute holder (players included), not just the offline path.
A complementary hardening would be to perform the Attributes.RemoveElement loop in MagicEffectsList.OnEffectTimeOutAsync under the same lock used for the effect bookkeeping, so effect expiry never mutates attribute state lock-free.
Race condition in
ComposableAttribute:NullReferenceExceptionwhen a magic effect expires while an attribute is recalculatedSummary
ComposableAttributestores its elements in a plain, unsynchronizedList<IElement>and enumerates it live while recomputing a value. Magic-effect expiration mutates that same list (RemoveElement) from aSystem.Threading.Timercallback on a thread-pool thread, without any lock shared with the reader. When an effect expires at the exact moment an attribute in the same dependency chain is being recalculated, the concurrentListmutation transiently exposes anullslot to the enumeration, throwing aNullReferenceException.It is a long-standing latent thread-safety gap (the unsynchronized element list dates back years), but it was effectively never hit until server-side offline bots started reading attributes in a tight loop and buffing themselves. The offline MU Helper catches the exception, so bots keep running, but the server log is flooded with errors. Online players are theoretically exposed to the same race (far less frequently), and for them the exception is not caught by the helper's safety net.
Environment
mastersrc/AttributeSystem/ComposableAttribute.cs,src/AttributeSystem/AttributeSystem.cs,src/GameLogic/MagicEffectsList.cs,src/GameLogic/MagicEffect.csObserved error
Logged repeatedly (bursts of several bots at once), swallowed by
OfflinePlayerMuHelper.SafeTickAsync:The leaf frame is the
Wherepredicate inGetAndCacheValue(e => e.AggregateType == ...) dereferencing anullelemente.Root cause
Unsynchronized shared list, live enumeration (reader).
ComposableAttributekeepsprivate readonly IList<IElement> _elementList = new List<IElement>();andGetAndCacheValue()enumerates it directly:this.Elementsreturns the live_elementList; there is no snapshot and no lock.Off-thread mutation (writer).
A
MagicEffectschedules expiry withSystem.Threading.Timer(MagicEffect.cs). On timeout,MagicEffectsList.OnEffectTimeOutAsyncruns on a thread-pool thread and callsthis._owner.Attributes.RemoveElement(powerUp.Element, powerUp.Target)— after releasing_addLock(MagicEffectsList.cs).AttributeSystem.RemoveElementforwards toComposableAttribute.RemoveElement→_elementList.Remove(element). Nothing serializes this against readers.The race.
List<T>.Removeshifts the backing array and clears the last slot. A reader enumerating with the pre-shift_sizecan observe anullin that slot, soe.AggregateTypethrowsNullReferenceException. Because this happens deep in a chain ofAttributeRelationshipElements (e.g. Maximum Health / Current Health derivations), any expiring buff that contributes to the recalculated attribute can trigger it.Ruled out (not a data problem): a buff never contributes a
nullelement —MagicEffectPowerUpExtensionsthrows whenBoostis missing andCreateElementnever returnsnull; the leaf element types (SimpleElement,ConstantElement) have trivialValuegetters that cannot throw. Thenullonly exists transiently during concurrent mutation.How to reproduce / why it surfaced now
Any holder that (a) reads an attribute frequently and (b) has magic effects expiring concurrently will eventually hit it. Server-side offline bots make this reliable:
MaximumHealth/CurrentHealth(health-threshold checks inHealingHandler).Before offline bots, offline players had no self-buffs and no tight read loop, so the pre-existing race was essentially never exercised.
Impact
OfflinePlayerMuHelper.SafeTickAsynccatches the exception, logs it, and the bot continues; the affected 500 ms tick simply skips healing. Main harm is log spam (can fill disk on busy servers).Suggested fix
Make
ComposableAttributethread-safe, since it is the shared point where both the mutation and the enumeration meet:AddElement/RemoveElementand the enumeration insideGetAndCacheValuewith a private lock (lock/System.Threading.Lock), orGetAndCacheValue(_elementList.ToArray()), combined with serialized mutations so two concurrent writers cannot corrupt the list.The lock overhead is negligible: it is only taken on element add/remove and on an actual recompute (cache miss); reads that hit
_cachedValuedon't touch the list. Fixing it inComposableAttributeprotects every attribute holder (players included), not just the offline path.A complementary hardening would be to perform the
Attributes.RemoveElementloop inMagicEffectsList.OnEffectTimeOutAsyncunder the same lock used for the effect bookkeeping, so effect expiry never mutates attribute state lock-free.