diff --git a/Cyaim.WebSocketServer/Cyaim.WebSocketServer/Infrastructure/Handlers/MvcHandler/MvcChannelHandler.cs b/Cyaim.WebSocketServer/Cyaim.WebSocketServer/Infrastructure/Handlers/MvcHandler/MvcChannelHandler.cs index 2fe2c41..17d1da0 100644 --- a/Cyaim.WebSocketServer/Cyaim.WebSocketServer/Infrastructure/Handlers/MvcHandler/MvcChannelHandler.cs +++ b/Cyaim.WebSocketServer/Cyaim.WebSocketServer/Infrastructure/Handlers/MvcHandler/MvcChannelHandler.cs @@ -99,11 +99,55 @@ public MvcChannelHandler(int receiveBufferSize = 4 * 1024, int sendBufferSize = /// - /// Associated with the connection, limit the total number of forwarding requests being processed by the connection. - /// WebSocketRouteOption.MaxParallelForwardLimit + /// Process-wide, despite the name — kept only so existing code compiles. /// + /// + /// + /// This field said "associated with the connection" and was not: AddMvcChannel builds + /// one handler per channel (new MvcChannelHandler(...).ConnectionEntry) and hands the + /// same delegate to every connection, so every connection on the channel shared this one + /// semaphore. Setting MaxConnectionParallelForwardLimit to a small number in the belief + /// that it was per-connection therefore serialised the whole process — the opposite of what the + /// name promised, and worse the more connections the server carried. + /// + /// + /// The per-connection gate is now a local in MvcForward. Nothing reads this field any + /// more; it stays for one release so a downstream that assigns it still compiles. + /// + /// + /// 名字说「与连接关联」,而它不是:AddMvcChannel 每个通道只建**一个** handler, + /// 把同一个委托交给每一条连接,于是整条通道上所有连接共用这一个信号量。 + /// 有人以为它是每连接的、把上限设成一个小数字,实际效果是把整个进程串行化—— + /// 与名字承诺的正好相反,而且服务器扛的连接越多越糟。 + /// 真正的每连接闸门现在是 MvcForward 里的局部变量。这个字段已无人读取, + /// 保留一个版本,只为让下游赋值它的代码还能编译。 + /// + /// + [Obsolete("This was process-wide, not per-connection. The gate is now a per-connection local; this field is no longer read.")] public SemaphoreSlim ParallelForwardLimitSlim = null; + /// + /// In-flight requests allowed per connection when MaxConnectionParallelForwardLimit is unset. + /// + /// + /// + /// Unset used to mean "no gate at all", and the gate that existed released at dispatch + /// rather than at completion, so neither setting it nor leaving it bounded the number of + /// requests actually in flight. A client that pipelines without waiting for responses therefore + /// held one DI scope, one controller instance and one parsed body per request, with nothing + /// counting them: in-flight ≈ attacker bandwidth × backend latency, and it is a positive + /// feedback loop, because the memory and thread-pool pressure slow the backend further. + /// + /// + /// 16 is far above what a real client pipelines and far below what an attacker needs. The + /// aggregate is bounded too: this cap times MaxConnectionLimit. + /// 不设它曾经等于「完全没有闸门」,而存在的那个闸门是在**派发**处释放而不是**完成**处, + /// 所以设不设都没有约束住真正在途的请求数。16 远高于真实客户端的流水线深度, + /// 远低于攻击者需要的量;总量也有界:这个上限乘以 MaxConnectionLimit。 + /// + /// + internal const int DefaultConnectionInflightLimit = 16; + /// /// After processing a message, the per-connection receive stream keeps at most this capacity; /// larger spikes are released so a one-off big multi-frame message doesn't retain peak memory @@ -236,12 +280,10 @@ public async Task ConnectionEntry(HttpContext context, ILogger { - ((ILogger)state).LogInformation(t.Exception, I18nText.ConnectionEntry_DisconnectedInternalExceptions); - }, logger, TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously); + (ILogger log, SemaphoreSlim inflight, SemaphoreSlim endpoint) s = + ((ILogger, SemaphoreSlim, SemaphoreSlim))state; + + if (t.IsFaulted) + { + s.log.LogInformation(t.Exception, I18nText.ConnectionEntry_DisconnectedInternalExceptions); + } + + s.inflight.Release(); + s.endpoint?.Release(); + // + // The ILogger cast is not cosmetic. `logger` is ILogger, + // so `(logger, ...)` boxes a ValueTuple, ...>, + // and unboxing a value tuple demands the *exact* type — the cast inside the + // continuation would throw InvalidCastException, which a continuation swallows. + // The permits would then never come back and the connection would wedge at the + // cap: a security fix that silently turns into a deadlock. Caught by + // InflightRequestLimitTests.Finished_requests_give_their_permit_back. + // 这个 ILogger 转型不是修饰。logger 的静态类型是 ILogger, + // 于是 (logger, ...) 装箱的是 ValueTuple, ...>, + // 而值元组拆箱要求类型**完全一致**——续体里那次强转会抛 InvalidCastException, + // 而续体会把它吞掉。票于是一张都还不回来,连接卡死在上限上: + // 一个悄悄变成死锁的安全修复。由 Finished_requests_give_their_permit_back 抓到。 + }, ((ILogger)logger, connectionInflight, endPointSlim), TaskContinuationOptions.ExecuteSynchronously); } CONTINUE_RECEIVE:; @@ -897,12 +1021,9 @@ await bandwidthLimitManager.WaitForBandwidthAsync( reservedReceiveBytes = 0; } - // 释放信号量 - if (ParallelForwardLimitSlim != null) - { - ParallelForwardLimitSlim.Release(); - } - if (endPointSlim != null) + // 释放信号量——只有在闸门还没交给任务时才归本轮所有。 + // Release the gates only while this iteration still owns them. + if (!gatesOwnedByTask && endPointSlim != null) { endPointSlim.Release(); } @@ -1666,8 +1787,19 @@ private async Task MvcChannel_OnDisconnected(HttpContext context, WebSocketClose } } + // Still cleaned up, still wrong, and both on purpose. This handler is shared by every + // connection on the channel, so disposing here on *one* connection's teardown pulled the + // semaphore out from under all the others — which is the same confusion that made the + // field process-wide in the first place. Nothing reads it now, so the only thing this + // disposes is a value a downstream assigned; it goes away with the field next release. + // 仍然清理、仍然是错的,两者都是刻意的:handler 由整条通道共用, + // 在**一条**连接断开时 Dispose 它,等于把信号量从其余所有连接脚下抽走—— + // 正是同一个混淆当初把这个字段做成了进程级。现在没人读它, + // 这里 Dispose 掉的只可能是下游赋进来的值;它会随字段在下个版本一起消失。 +#pragma warning disable CS0618 // deliberately touching the obsolete field, to clean up what a caller may have assigned ParallelForwardLimitSlim?.Dispose(); ParallelForwardLimitSlim = null; +#pragma warning restore CS0618 } } diff --git a/Cyaim.WebSocketServer/Tests/Cyaim.WebSocketServer.Tests/InflightRequestLimitTests.cs b/Cyaim.WebSocketServer/Tests/Cyaim.WebSocketServer.Tests/InflightRequestLimitTests.cs new file mode 100644 index 0000000..a5bf35e --- /dev/null +++ b/Cyaim.WebSocketServer/Tests/Cyaim.WebSocketServer.Tests/InflightRequestLimitTests.cs @@ -0,0 +1,425 @@ +using System.Net.WebSockets; +using System.Text; +using System.Text.Json; +using Cyaim.WebSocketServer.Infrastructure.Configures; +using Cyaim.WebSocketServer.Infrastructure.Handlers.MvcHandler; +using Cyaim.WebSocketServer.Middlewares; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +namespace Cyaim.WebSocketServer.Tests +{ + /// + /// Per-host probe: counts how many calls are inside the endpoint, and parks them until released. + /// + /// + /// Injected rather than static, and that is not tidiness. With static counters the previous test's + /// connection kept incrementing them after the next test had reset them — the disposal of one host + /// releases its parked requests, and those completions land during the following test. It showed up + /// as 16 pipelined requests being counted as 20, which reads like the cap failing when it is the + /// harness leaking. + /// 用注入而不是静态,不是整洁强迫症:静态计数会让上一条测试的连接在下一条 Reset 之后继续加数—— + /// 销毁一个 host 会释放它停住的请求,那些完成落在下一条测试期间。 + /// 现象是 16 条流水线被数成 20,读起来像上限失效,其实是脚手架在漏。 + /// + public sealed class InflightProbe + { + private int _current; + private int _peak; + private int _entered; + private readonly TaskCompletionSource _release = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public int Peak => Volatile.Read(ref _peak); + + public int Entered => Volatile.Read(ref _entered); + + public int Current => Volatile.Read(ref _current); + + public void ReleaseAll() => _release.TrySetResult(); + + public async Task EnterAsync() + { + int now = Interlocked.Increment(ref _current); + Interlocked.Increment(ref _entered); + + // Peak is a max, not a sample: the assertion is about the highest concurrency the gate ever + // allowed, and sampling would miss the moment it was exceeded. + // peak 取的是最大值而不是采样:断言问的是闸门**曾经**放进去多少,采样会错过越界的那一刻。 + int seen; + while (now > (seen = Volatile.Read(ref _peak))) + { + Interlocked.CompareExchange(ref _peak, now, seen); + } + + await _release.Task.ConfigureAwait(false); + Interlocked.Decrement(ref _current); + } + } + + /// The endpoint the tests pipeline into. Target resolves as "probe.park". + public class ProbeController + { + private readonly InflightProbe _probe; + + public ProbeController(InflightProbe probe) => _probe = probe; + + public async Task Park() + { + await _probe.EnterAsync().ConfigureAwait(false); + return "parked"; + } + } + + /// + /// That a client cannot pipeline an unbounded number of requests into flight. + /// + /// + /// + /// The gate used to be released in the receive loop's finally — that is, the moment the + /// request was dispatched, not when it finished. A client that pipelines without + /// waiting for responses therefore had no bound at all: each in-flight request holds a DI scope, + /// a controller instance and a parsed body, and nothing counted them. In-flight ≈ attacker + /// bandwidth × backend latency, and it feeds back on itself, because the pressure slows the + /// backend and slower backend means more in flight. + /// 闸门此前在接收循环的 finally 里释放——也就是请求被**派发**的那一刻,而不是它**完成**的那一刻。 + /// 于是不等响应就流水线发送的客户端完全不受约束:每条在途都持有一个 DI Scope、 + /// 一个控制器实例和一份解析好的请求体,而没有任何东西在数它们。 + /// 在途数 ≈ 攻击者带宽 × 后端时延,而且是正反馈:压力让后端更慢,后端更慢就有更多在途。 + /// + /// + /// The gate was also a field on the handler, and AddMvcChannel builds one handler per + /// channel — so it was shared by every connection despite its name. Both halves are asserted + /// here: the cap binds, and it binds per connection. + /// 那个闸门还是 handler 上的字段,而 AddMvcChannel 每通道只建一个 handler—— + /// 于是它名为「每连接」,实为所有连接共用。两半都在这里断言:上限生效,且是**每连接**生效。 + /// + /// + [Collection("StaticState")] + public class InflightRequestLimitTests : IDisposable + { + private static readonly TimeSpan TestTimeout = TimeSpan.FromSeconds(20); + + private readonly IServiceProvider _previousServices; + private readonly InflightProbe _probe = new(); + + public InflightRequestLimitTests() + { + _previousServices = WebSocketRouteOption.ApplicationServices; + MvcTestSupport.ResetCachedScopeFactory(); + } + + public void Dispose() + { + _probe.ReleaseAll(); + WebSocketRouteOption.ApplicationServices = _previousServices; + MvcTestSupport.ResetCachedScopeFactory(); + } + + private async Task StartHostAsync(uint? limit) + { + var option = new WebSocketRouteOption + { + WebSocketChannels = new Dictionary + { + ["/ws"] = new MvcChannelHandler().ConnectionEntry + }, + WatchAssemblyContext = MvcTestSupport.BuildContext(typeof(ProbeController)), + MaxConnectionParallelForwardLimit = limit, + }; + + var host = new HostBuilder() + .ConfigureWebHost(webHost => webHost + .UseTestServer() + .ConfigureServices(services => + { + services.AddSingleton(option); + services.AddSingleton(_probe); + }) + .Configure(app => + { + // TestServer leaves HttpContext.Connection.Id null and the handler requires one. + // TestServer 不填 Connection.Id,而 handler 需要它非空。 + app.Use(async (ctx, next) => + { + ctx.Connection.Id ??= Guid.NewGuid().ToString("N"); + await next(); + }); + app.UseWebSockets(); + app.UseWebSocketServer(); + })) + .Build(); + + await host.StartAsync(); + return host; + } + + private static async Task ConnectAsync(IHost host) + { + var server = host.GetTestServer(); + var client = server.CreateWebSocketClient(); + using var cts = new CancellationTokenSource(TestTimeout); + return await client.ConnectAsync(new Uri(server.BaseAddress, "/ws"), cts.Token); + } + + private static Task PipelineAsync(WebSocket socket, int count, CancellationToken ct) + { + return Task.Run(async () => + { + for (int i = 0; i < count; i++) + { + string frame = JsonSerializer.Serialize(new + { + Id = "p" + i, + // BuildContext keys endpoints as "{type name minus Controller}.{method}", lowercased. + // Getting this wrong does not fail loudly: the request resolves to nothing, zero + // handlers run, and an assertion like "peak <= limit" passes with peak = 0. + // 端点键是「类名去掉 Controller」+「.」+ 方法名,全小写。写错了不会响亮地失败: + // 请求解析不到任何东西、零个处理器执行,而「peak <= limit」这种断言在 peak = 0 时照样通过。 + Target = "probe.park", + Body = new { }, + }); + + await socket.SendAsync( + Encoding.UTF8.GetBytes(frame), WebSocketMessageType.Text, true, ct).ConfigureAwait(false); + } + }, ct); + } + + /// + /// Drains responses in the background, so the connection is not held up by an unread socket. + /// + /// + /// A test that never reads is testing backpressure, not the permit. With the gate released on + /// completion, and completion including "the response was sent", a client that does not read + /// stops being served at exactly the cap — which is the correct behaviour and was measured + /// here before this reader existed: 12 pipelined, 3 admitted, and nothing moved after the + /// endpoint was let go. That is the bound doing its job, not a leak, and it is why the permit + /// test needs a reader while the cap tests deliberately do not have one. + /// 从不读 socket 的测试测的是背压,不是票。闸门在**完成**时释放,而完成包含「响应已发出」, + /// 于是不读的客户端恰好卡在上限上——这是正确行为,而且在这个读取端存在之前就实测到了: + /// 流水线 12 条、放进 3 条,放开端点之后一动不动。那是界在起作用,不是漏票。 + /// 这也是为什么「还票」那条测试需要读取端,而「上限」那两条刻意不需要。 + /// + private static Task DrainAsync(WebSocket socket, CancellationToken ct) + { + return Task.Run(async () => + { + var buffer = new byte[8 * 1024]; + try + { + while (socket.State == WebSocketState.Open && !ct.IsCancellationRequested) + { + await socket.ReceiveAsync(buffer, ct).ConfigureAwait(false); + } + } + catch + { + // The socket closing under the reader is the normal end of this loop. + // 读取端在 socket 关闭时结束,是正常终止。 + } + }, ct); + } + + /// + /// Ends a test's connection so its leftovers cannot land on the next test's probe. + /// + /// + /// WebSocketRouteOption.ApplicationServices is static, so a receive loop that is still + /// draining pipelined messages after its own host is gone resolves controllers out of whichever + /// host is current — the next test's. It shows up as that test counting more requests + /// than it sent (16 pipelined, 20 counted), which reads exactly like the cap failing. Releasing + /// the parked calls and closing the socket is what stops one test from writing into another. + /// WebSocketRouteOption.ApplicationServices 是静态的:一条在自己 host 消失之后还在排空 + /// 流水线消息的接收循环,会从**当前**那个 host 里解析控制器——也就是下一条测试的。 + /// 现象是那条测试数到的请求比它发出去的还多(发 16、数到 20),读起来就像上限失效。 + /// 放开停住的调用、关掉 socket,才能让一条测试不写进另一条。 + /// + private async Task EndConnectionsAsync(params WebSocket[] sockets) + { + _probe.ReleaseAll(); + + foreach (var socket in sockets) + { + try + { + if (socket.State == WebSocketState.Open) + { + using var closeCts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + await socket.CloseAsync(WebSocketCloseStatus.NormalClosure, "done", closeCts.Token) + .ConfigureAwait(false); + } + } + catch + { + // A socket the server already aborted is closed enough for this purpose. + // 服务端已经 Abort 掉的 socket,对这里的目的而言已经足够「关上了」。 + } + } + + // Give the server's loops a moment to notice the close before the host goes away. + // 给服务端的循环一点时间察觉关闭,然后 host 才消失。 + await Task.Delay(200).ConfigureAwait(false); + } + + /// Waits for the probe to stop admitting new calls, or for the timeout. + private async Task SettleAsync() + { + int stable = 0, last = -1; + for (int i = 0; i < 200 && stable < 10; i++) + { + await Task.Delay(20).ConfigureAwait(false); + int now = _probe.Entered; + stable = now == last ? stable + 1 : 0; + last = now; + } + } + + [Fact] + public async Task A_client_cannot_pipeline_more_requests_into_flight_than_the_limit() + { + const int limit = 3; + const int pipelined = 24; + + using var host = await StartHostAsync(limit); + using var cts = new CancellationTokenSource(TestTimeout); + var socket = await ConnectAsync(host); + + await PipelineAsync(socket, pipelined, cts.Token); + await SettleAsync(); + + // Anti-vacuity first: "peak <= limit" is trivially true when nothing ran at all, which is + // exactly what a mistyped target produces. Assert the path was exercised before asserting + // anything about the bound. + // 先反真空:什么都没跑时「peak <= limit」恒真,而目标写错正好产出这个结果。 + // 先断言这条路径真的被走过,再去断言那条界。 + Assert.True( + _probe.Entered > 0, + "no request reached the endpoint at all — this test would pass for the wrong reason"); + + Assert.True( + _probe.Peak <= limit, + $"pipelined {pipelined} requests behind a limit of {limit}; {_probe.Peak} were in flight at once"); + + // And the gate is actually holding the rest back rather than the client being slow: + // without the cap all 24 would have entered. + // 而且确实是闸门在挡,不是客户端慢:不设界的话 24 条都会进来。 + Assert.True( + _probe.Entered <= limit, + $"{_probe.Entered} requests entered the endpoint; only {limit} should have"); + + await EndConnectionsAsync(socket); + } + + /// + /// The permits come back. This is the risk the fix introduces, and the reason it is asserted + /// separately: a gate that never releases turns a denial of service into a deadlock, which is + /// not an improvement. + /// 票要还得回来。这是这次修复引入的风险,所以单独断言: + /// 一个永不释放的闸门把拒绝服务变成死锁,那不叫改进。 + /// + [Fact] + public async Task Finished_requests_give_their_permit_back() + { + const int limit = 3; + const int pipelined = 12; + + using var host = await StartHostAsync(limit); + using var cts = new CancellationTokenSource(TestTimeout); + var socket = await ConnectAsync(host); + + _ = DrainAsync(socket, cts.Token); + + await PipelineAsync(socket, pipelined, cts.Token); + await SettleAsync(); + + int held = _probe.Entered; + Assert.True(held <= limit, $"{held} entered before release; the cap was {limit}"); + + _probe.ReleaseAll(); + + // Every queued request must now drain. If a permit leaked, this stalls at `held`. + // 排队的请求现在必须全部流干。漏了票的话,这里会停在 held 上不动。 + for (int i = 0; i < 400 && _probe.Entered < pipelined; i++) + { + await Task.Delay(25); + } + + Assert.Equal(pipelined, _probe.Entered); + + await EndConnectionsAsync(socket); + } + + /// + /// Leaving the limit unset must not mean "no limit". It used to. + /// 不设上限不能等于「没有上限」。它曾经就是。 + /// + [Fact] + public async Task An_unset_limit_still_bounds_in_flight_requests() + { + int pipelined = MvcChannelHandler.DefaultConnectionInflightLimit * 4; + + using var host = await StartHostAsync(null); + using var cts = new CancellationTokenSource(TestTimeout); + var socket = await ConnectAsync(host); + + await PipelineAsync(socket, pipelined, cts.Token); + await SettleAsync(); + + Assert.True( + _probe.Entered > 0, + "no request reached the endpoint at all — this test would pass for the wrong reason"); + + Assert.True( + _probe.Peak <= MvcChannelHandler.DefaultConnectionInflightLimit, + $"with no configured limit, {_probe.Peak} requests were in flight; " + + $"the default is {MvcChannelHandler.DefaultConnectionInflightLimit}"); + + await EndConnectionsAsync(socket); + } + + /// + /// The cap is per connection, not per process. + /// + /// + /// The old gate lived on the handler, and AddMvcChannel builds one handler per channel, + /// so a cap of N was N for the whole server. On a gateway carrying a million connections that + /// is not a smaller bound, it is a different failure: every connection queues behind every + /// other one. This asserts the bound scales with connections instead. + /// 旧闸门挂在 handler 上,而 AddMvcChannel 每通道只建一个 handler,于是上限 N 是**整台服务器** N。 + /// 在扛百万连接的网关上这不是「更严的界」,是另一种故障:每条连接都排在其余所有连接后面。 + /// 这条断言的是界随连接数放大。 + /// + [Fact] + public async Task The_limit_is_per_connection_not_per_process() + { + const int limit = 2; + + using var host = await StartHostAsync(limit); + using var cts = new CancellationTokenSource(TestTimeout); + + var a = await ConnectAsync(host); + var b = await ConnectAsync(host); + + await PipelineAsync(a, 8, cts.Token); + await PipelineAsync(b, 8, cts.Token); + await SettleAsync(); + + // Two connections, so two connections' worth of permits — a process-wide gate would have + // admitted `limit` in total no matter how many connections were asking. + // 两条连接就该有两条连接的票。进程级闸门无论多少连接在要,总共只放 limit 条。 + Assert.True( + _probe.Entered > limit, + $"two connections admitted only {_probe.Entered} requests, " + + $"which is what a single process-wide gate of {limit} would do"); + + Assert.True( + _probe.Entered <= limit * 2, + $"two connections admitted {_probe.Entered}; the per-connection cap of {limit} allows at most {limit * 2}"); + + await EndConnectionsAsync(a, b); + } + } +}