## What `Dictionary<K,V>.Remove` and `HashSet<T>.Remove` do not bump the collection's version, so removing an entry during a `foreach` does not invalidate the enumerator. A number of loops were still paying for a `PooledRefQueue`/`PooledRefList` to collect keys and drain them in a second pass. This drops those guards. ## Why it's safe Verified against .NET 10.0.10 rather than taken on trust, since the documented guarantee covers only `Dictionary<TKey,TValue>.Remove` while several of these call sites are `HashSet<T>` or enumerate `.Keys`/`.Values`: | Case | Result | |---|---| | `Dictionary` foreach + `Remove` | safe, all entries visited | | `Dictionary.Keys` / `.Values` foreach + `Remove` | safe, all entries visited | | `HashSet` foreach + `Remove` | safe, all entries visited | | `Dictionary` foreach + `Remove` **then `Add`** | throws `InvalidOperationException` | Reflection on `_version` confirms the mechanism: neither `Dictionary.Remove` nor `HashSet.Remove` touches it. Because `Remove` never bumps the version, the `Keys` and `Values` enumerators are just as safe as the dictionary's own, even though only `Dictionary.Remove` documents the behaviour. No entries were skipped in any case. The `HashSet` half is confirmed by [stephentoub on dotnet/dotnet-api-docs#8177](https://github.com/dotnet/dotnet-api-docs/issues/8177#issuecomment-1167251052): *"Both HashSet and Dictionary have been improved to support removal during enumeration. The docs may just benefit from updating."* The gap is in the documentation, not the runtime. `Remove` followed by `Add` in the same enumeration still throws. That is the line this PR does not cross. ## Guards removed `VisibilityList`, `ChampionTitleSystem`, `Channel`, `BombingRun`, `Ruleset`, `PuzzleChest`, `RaceChangeGump`, `StepCache`, `PlayerMurderSystem`, `VirtueSystem`, `ProjectedItem`, `StaminaSystem`, `AIGroupMovement`, `PromotedGuard`, `AutoDenylist`, `LoginAllowlist`, `AntiMacroSystem`, `DetectHidden`. Both collection kinds are covered: `Dictionary` (including loops over `.Keys` and `.Values`) and `HashSet` (`ProjectedItem._active`, `PlayerMurderSystem._contextTerms`, `StaminaSystem._resetHash`). In `StaminaSystem.ResetTimer` the `Count == queue.Count → Clear()` branch goes away with the queue — it only existed to avoid paying for N individual removes. Where the collection supports it, `Contains` + `Remove` and `TryGetValue` + `Remove` also collapse into a single lookup (`if (list.Remove(x))`, `if (m_Pending.Remove(ns, out var state))`). `Utility.Tidy<K,V>` keeps its two branches: when `K` is serializable the value is not inspected, otherwise the value is. Only the serializable side may be cast, so `Dictionary<Mobile, int>` and `Dictionary<Mobile, string>` stay valid. ## Deliberately unchanged **`BaseCreature.LoyaltyTimer.OnTick`** keeps its deferred-delete queue. Removing from `World.Mobiles` while enumerating it is safe, but `Mobile.Delete()` is not a `Remove` — it runs `OnDelete`/`OnAfterDelete`, the `OnParentDeleted` cascade over the creature's pack, `DropHolding()`, and region and guild callbacks. Anything in that surface that constructs a `Mobile` is an `Add` into the dictionary being enumerated, which does invalidate it. `BaseHire.PayTimer.OnTick` has the same shape and is likewise untouched. **Spatial-query buffers** — `GuardedRegion.CallGuards`, `Thunderstorm`, `Exorcism`, `LeverPuzzleController`, `BaseCreature.TeleportPets` — are a different hazard. They buffer the result of a range query because the drain moves or harms mobiles, which mutates sectors mid-enumeration. **Re-entrant drains.** The `_users` sets in `Firebomb` and the explosion, conflagration and confusion-blast potions look like this pattern but are not: the loop collects, `Clear()`s, and only then runs `Target.Cancel` on each, which can re-enter. `AnimalTrainer` enumerates `pm.Stabled` and drains through `RemoveStabled`, which nulls the `Stabled` field once it empties — safe for an in-flight enumerator, which holds the set reference rather than the field, but subtle enough not to be worth inlining on a cold path. ## Verification `dotnet build` clean with 0 warnings; 810 Server and 684 UOContent tests pass.
214 lines
7 KiB
C#
214 lines
7 KiB
C#
/*************************************************************************
|
|
* ModernUO *
|
|
* Copyright 2019-2026 - ModernUO Development Team *
|
|
* Email: hi@modernuo.com *
|
|
* File: AutoDenylist.cs *
|
|
* *
|
|
* This program is free software: you can redistribute it and/or modify *
|
|
* it under the terms of the GNU General Public License as published by *
|
|
* the Free Software Foundation, either version 3 of the License, or *
|
|
* (at your option) any later version. *
|
|
* *
|
|
* You should have received a copy of the GNU General Public License *
|
|
* along with this program. If not, see <http://www.gnu.org/licenses/>. *
|
|
*************************************************************************/
|
|
|
|
using System;
|
|
using System.Collections.Generic;
|
|
using System.Net;
|
|
using System.Threading;
|
|
using Server.Logging;
|
|
using Server.Network.Bans;
|
|
|
|
namespace Server.Network;
|
|
|
|
/// <summary>
|
|
/// A short-lived, in-memory denylist of addresses the shard itself just caught misbehaving.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// The local half of promotion. Contributing to CrowdSec only helps once an OS bouncer reacts; until then
|
|
/// every reconnect costs a socket, a buffer and a <c>NetState</c> slot — and the verdicts that matter most
|
|
/// are reachable only after reading bytes, like a zero seed. It is also the whole defence on a shard running
|
|
/// no bouncer, which is the default. Not persisted, by design: a holding pen that survives restarts is a ban
|
|
/// without a ban's review. Only <see cref="BanReasons.IsBehavioral"/> verdicts are held.
|
|
/// </remarks>
|
|
public static class AutoDenylist
|
|
{
|
|
private static readonly ILogger logger = LogFactory.GetLogger(typeof(AutoDenylist));
|
|
|
|
// Address (normalized v6 bits) -> Core.TickCount at which the hold lapses. Loop-only.
|
|
private static readonly Dictionary<UInt128, long> _held = [];
|
|
|
|
private static bool _enabled;
|
|
private static long _durationMs;
|
|
private static int _maxEntries;
|
|
private static bool _warnedFull;
|
|
|
|
public static int Count => _held.Count;
|
|
|
|
public static void Configure()
|
|
{
|
|
AutoDenylistConfiguration.Load();
|
|
var s = AutoDenylistConfiguration.Settings;
|
|
|
|
_enabled = s.Enabled && s.Duration > TimeSpan.Zero && s.MaxEntries > 0;
|
|
if (!_enabled)
|
|
{
|
|
return;
|
|
}
|
|
|
|
_durationMs = (long)s.Duration.TotalMilliseconds;
|
|
_maxEntries = s.MaxEntries;
|
|
|
|
ConnectionFilters.Register(new AutoDenylistFilter());
|
|
BanChannel.Register(new AutoDenylistReporter());
|
|
}
|
|
|
|
/// <summary>
|
|
/// Holds an address for the configured duration. Ignores non-behavioural verdicts and refuses to grow
|
|
/// past the cap: the flood this exists for must not become a memory leak.
|
|
/// </summary>
|
|
public static void Hold(IPAddress address, string reason) => Hold(address, reason, Core.TickCount);
|
|
|
|
internal static bool Hold(IPAddress address, string reason, long nowTicks)
|
|
{
|
|
if (!_enabled || address == null || !BanReasons.IsBehavioral(reason))
|
|
{
|
|
return false;
|
|
}
|
|
|
|
var key = address.ToUInt128();
|
|
|
|
// An address already held is just extended, so no cap check is needed.
|
|
if (!_held.ContainsKey(key) && _held.Count >= _maxEntries)
|
|
{
|
|
Sweep(nowTicks);
|
|
|
|
if (_held.Count >= _maxEntries)
|
|
{
|
|
if (!_warnedFull)
|
|
{
|
|
_warnedFull = true;
|
|
logger.Warning(
|
|
"Auto-denylist is full at {Max} addresses; further detections are disconnected but not held",
|
|
_maxEntries
|
|
);
|
|
}
|
|
|
|
return false;
|
|
}
|
|
}
|
|
|
|
_held[key] = nowTicks + _durationMs;
|
|
return true;
|
|
}
|
|
|
|
public static bool IsDenied(IPAddress address) => IsDenied(address, Core.TickCount);
|
|
|
|
/// <summary>The pure decision, split out so the accept-path policy can be tested without a clock.</summary>
|
|
internal static bool IsDenied(IPAddress address, long nowTicks)
|
|
{
|
|
if (!_enabled || address == null)
|
|
{
|
|
return false;
|
|
}
|
|
|
|
// Decided on read, so a lapsed hold cannot deny even before the sweep. Subtraction: TickCount wraps.
|
|
return _held.TryGetValue(address.ToUInt128(), out var expires) && expires - nowTicks > 0;
|
|
}
|
|
|
|
/// <summary>Releases an address early, e.g. when an operator retracts a ban.</summary>
|
|
public static void Release(IPAddress address)
|
|
{
|
|
if (_enabled && address != null)
|
|
{
|
|
_held.Remove(address.ToUInt128());
|
|
}
|
|
}
|
|
|
|
internal static void Sweep(long nowTicks)
|
|
{
|
|
if (_held.Count == 0)
|
|
{
|
|
return;
|
|
}
|
|
|
|
var lapsed = 0;
|
|
|
|
foreach (var (address, expires) in _held)
|
|
{
|
|
if (expires - nowTicks <= 0)
|
|
{
|
|
_held.Remove(address);
|
|
lapsed++;
|
|
}
|
|
}
|
|
|
|
if (lapsed > 0)
|
|
{
|
|
_warnedFull = false;
|
|
}
|
|
}
|
|
|
|
internal static void LoadForTesting(bool enabled, long durationMs, int maxEntries)
|
|
{
|
|
_held.Clear();
|
|
_enabled = enabled;
|
|
_durationMs = durationMs;
|
|
_maxEntries = maxEntries;
|
|
_warnedFull = false;
|
|
}
|
|
}
|
|
|
|
/// <summary>Accept-path gate for <see cref="AutoDenylist"/>.</summary>
|
|
public sealed class AutoDenylistFilter : IConnectionFilter
|
|
{
|
|
public string Name => "auto-denylist";
|
|
|
|
public void Register()
|
|
{
|
|
}
|
|
|
|
public void Start(CancellationToken token)
|
|
{
|
|
// Only an optimisation: IsDenied expires on read.
|
|
Timer.DelayCall(TimeSpan.FromMinutes(1), TimeSpan.FromMinutes(1), () => AutoDenylist.Sweep(Core.TickCount));
|
|
}
|
|
|
|
public void Stop()
|
|
{
|
|
}
|
|
|
|
public bool ShouldDeny(IPAddress address) => AutoDenylist.IsDenied(address);
|
|
}
|
|
|
|
/// <summary>
|
|
/// Feeds <see cref="AutoDenylist"/> from the ban channel. A reporter rather than a direct call, because the
|
|
/// detection sites live in the engine and must not reach into content.
|
|
/// </summary>
|
|
public sealed class AutoDenylistReporter : IBanReporter
|
|
{
|
|
public string Name => "auto-denylist";
|
|
|
|
public bool CanRetract => true;
|
|
|
|
public void Register()
|
|
{
|
|
}
|
|
|
|
public void Start(CancellationToken token)
|
|
{
|
|
}
|
|
|
|
public void Stop()
|
|
{
|
|
}
|
|
|
|
/// <summary>
|
|
/// The contributed <paramref name="ttl"/> is ignored: how long a bouncer should ban an address is a
|
|
/// different question from how long this shard holds it at accept.
|
|
/// </summary>
|
|
public void Report(IPAddress address, TimeSpan ttl, string reason) => AutoDenylist.Hold(address, reason);
|
|
|
|
public void Retract(IPAddress address) => AutoDenylist.Release(address);
|
|
}
|