diff --git a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs index 3931eb3e4..0c7ff7537 100644 --- a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs @@ -20,8 +20,8 @@ using Xunit; namespace Server.Tests.Network.AutoDenylists; -// Static store, so every test resets it first. Addresses come from TEST-NET-2 (198.51.100.0/24). -// Sequential: the cap tests reach Sweep, which rents from STArrayPool, which is not thread-safe. +// Static store, so every test resets it first and none may run alongside another. +// Addresses come from TEST-NET-2 (198.51.100.0/24). [Collection("Sequential UOContent Tests")] public class AutoDenylistTests { @@ -104,6 +104,30 @@ public class AutoDenylistTests Assert.False(AutoDenylist.IsDenied(IPAddress.Parse("198.51.100.109"), Now)); } + // A sweep is O(maxEntries). Without the throttle a distinct-source flood re-scans the whole + // dictionary for every address it rejects, which is the case the cap exists to make cheap. + [Fact] + public void Cap_does_not_re_sweep_for_every_rejected_address() + { + Reset(maxEntries: 2); + + AutoDenylist.Hold(IPAddress.Parse("198.51.100.40"), BanReasons.InvalidSeed, Now); + AutoDenylist.Hold(IPAddress.Parse("198.51.100.41"), BanReasons.InvalidSeed, Now); + Assert.Equal(0, AutoDenylist.SweepCount); + + // At the cap with nothing lapsed: the first rejection sweeps, the rest in the window do not. + for (var i = 0; i < 5; i++) + { + Assert.False(AutoDenylist.Hold(IPAddress.Parse($"198.51.100.{50 + i}"), BanReasons.InvalidSeed, Now)); + } + + Assert.Equal(1, AutoDenylist.SweepCount); + + // Past the window it may try again, since entries can have lapsed by then. + Assert.False(AutoDenylist.Hold(IPAddress.Parse("198.51.100.60"), BanReasons.InvalidSeed, Now + 1000)); + Assert.Equal(2, AutoDenylist.SweepCount); + } + [Fact] public void Reaching_the_cap_reclaims_lapsed_entries_first() { diff --git a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs index b9b4dd7c1..4f468571e 100644 --- a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs +++ b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs @@ -39,13 +39,22 @@ public static class AutoDenylist // Address (normalized v6 bits) -> Core.TickCount at which the hold lapses. Loop-only. private static readonly Dictionary _held = []; + // Shortest gap between cap-triggered sweeps. A sweep is O(_maxEntries) and one that just ran cannot + // have freed more, so without this every rejected address re-scans the whole dictionary -- turning the + // bound into a cost multiplier under exactly the flood it exists to bound. + private const long SweepThrottleMs = 1000; + private static bool _enabled; private static long _durationMs; private static int _maxEntries; private static bool _warnedFull; + private static long _lastSweepAt; public static int Count => _held.Count; + // Test seam: lets the throttle be asserted rather than inferred. + internal static int SweepCount { get; private set; } + public static void Configure() { AutoDenylistConfiguration.Load(); @@ -60,6 +69,9 @@ public static class AutoDenylist _durationMs = (long)s.Duration.TotalMilliseconds; _maxEntries = s.MaxEntries; + // Seeded from a real tick: tick counts need not start near zero. See dev-docs/tick-counts.md. + _lastSweepAt = Core.TickCount; + ConnectionFilters.Register(new AutoDenylistFilter()); BanChannel.Register(new AutoDenylistReporter()); } @@ -82,7 +94,10 @@ public static class AutoDenylist // An address already held is just extended, so no cap check is needed. if (!_held.ContainsKey(key) && _held.Count >= _maxEntries) { - Sweep(nowTicks); + if (nowTicks - _lastSweepAt >= SweepThrottleMs) + { + Sweep(nowTicks); + } if (_held.Count >= _maxEntries) { @@ -128,6 +143,10 @@ public static class AutoDenylist internal static void Sweep(long nowTicks) { + // Stamped even when there is nothing to do, so the cap path throttles off the last attempt. + _lastSweepAt = nowTicks; + SweepCount++; + if (_held.Count == 0) { return; @@ -157,12 +176,16 @@ public static class AutoDenylist _durationMs = durationMs; _maxEntries = maxEntries; _warnedFull = false; + _lastSweepAt = 0; + SweepCount = 0; } } /// Accept-path gate for . public sealed class AutoDenylistFilter : IConnectionFilter { + private Timer _sweepTimer; + public string Name => "auto-denylist"; public void Register() @@ -171,12 +194,19 @@ public sealed class AutoDenylistFilter : IConnectionFilter public void Start(CancellationToken token) { - // Only an optimisation: IsDenied expires on read. - Timer.DelayCall(TimeSpan.FromMinutes(1), TimeSpan.FromMinutes(1), () => AutoDenylist.Sweep(Core.TickCount)); + // Only an optimization: IsDenied expires on read. + _sweepTimer = Timer.DelayCall( + TimeSpan.FromMinutes(1), + TimeSpan.FromMinutes(1), + () => AutoDenylist.Sweep(Core.TickCount) + ); } public void Stop() { + // Recurring, so an uncancelled sweep survives Stop and the next Start adds a second one. + _sweepTimer?.Stop(); + _sweepTimer = null; } public bool ShouldDeny(IPAddress address) => AutoDenylist.IsDenied(address); diff --git a/Projects/UOContent/Network/LoginAllowlist/LoginAllowlist.cs b/Projects/UOContent/Network/LoginAllowlist/LoginAllowlist.cs index 096a03a48..23075f7ff 100644 --- a/Projects/UOContent/Network/LoginAllowlist/LoginAllowlist.cs +++ b/Projects/UOContent/Network/LoginAllowlist/LoginAllowlist.cs @@ -19,6 +19,7 @@ using System.Globalization; using System.IO; using System.Net; using System.Text; +using System.Threading; using System.Threading.Tasks; using Server.Logging; using Server.Network.Bans; @@ -259,7 +260,7 @@ public static class LoginAllowlist /// private static void OnCrashed(ServerCrashedEventArgs e) { - if (System.Threading.Thread.CurrentThread == Core.Thread) + if (Thread.CurrentThread == Core.Thread) { OnShutdown(); }