diff --git a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.Callbacks.cs b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.Callbacks.cs index 9b6ff27b..566e35c6 100644 --- a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.Callbacks.cs +++ b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.Callbacks.cs @@ -36,6 +36,13 @@ private static unsafe void Fault(void* user, Exception e) } } + // Left pending, so nothing later in this read can start a fresh request on the stream; reset once ih2_read unwinds. + private void Overflow(PendingRequest pending) + { + pending.Overflowed = true; + _readyThisPass.Add(pending); + } + [UnmanagedCallersOnly] private static unsafe void CallbackBeginHeaders(void* user, int streamId) { @@ -65,8 +72,16 @@ private static unsafe void CallbackHeader(void* user, int streamId, byte* name, Nghttp2Connection connection = From(user); // A dispatched request's trailers are dropped: its arena backs memories a handler holds. - if (!connection._pending.TryGetValue(streamId, out PendingRequest? pending) || pending.BodyReader is not null) + if (!connection._pending.TryGetValue(streamId, out PendingRequest? pending) || pending.BodyReader is not null || pending.Overflowed) + { + return; + } + + // RFC 9113 6.5.2's measure. nghttp2 calls back once per decoded field, whatever a block arrived as. + pending.HeaderListSize += (long)nameLength + (long)valueLength + 32; + if (pending.HeaderListSize > connection._options.MaxHeaderListSize) { + connection.Overflow(pending); return; } @@ -139,9 +154,7 @@ private static unsafe void CallbackData(void* user, int streamId, byte* data, nu { if (pending.BodyLength + (long)dataLength > connection._options.MaxRequestBytes) { - // Left pending, so a trailer later in this read cannot start a fresh request on the stream. - pending.Overflowed = true; - connection._readyThisPass.Add(pending); + connection.Overflow(pending); return; } pending.AppendBody(new ReadOnlySpan(data, (int)dataLength)); diff --git a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.cs b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.cs index 86d3fa6e..4bbbae2e 100644 --- a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.cs +++ b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Connection.cs @@ -166,9 +166,12 @@ private sealed class PendingRequest : IDisposable public int StreamId; - // The body passed MaxRequestBytes: the stream is reset once ih2_read unwinds, never dispatched. + // The body passed MaxRequestBytes, or the headers MaxHeaderListSize: the stream is reset once ih2_read unwinds, never dispatched. public bool Overflowed; + // What this request's header blocks have decoded to so far. + public long HeaderListSize; + /// /// Set once a streamed body's request is dispatched at its headers, and kept after its handler /// returns: until the stream ends, its DATA and trailers still resolve here. @@ -265,6 +268,7 @@ public void Dispose() _arena = []; } _used = 0; + HeaderListSize = 0; } } } diff --git a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Options.cs b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Options.cs index ebe805fb..1ea44f0b 100644 --- a/src/protocols/ioxide.nghttp2/Connection/Nghttp2Options.cs +++ b/src/protocols/ioxide.nghttp2/Connection/Nghttp2Options.cs @@ -4,10 +4,17 @@ namespace ioxide.nghttp2; public sealed record Nghttp2Options { /// - /// Ceiling on one request's headers plus body. nghttp2 enforces its own frame and header-list - /// limits, but a body arrives as a stream of DATA frames with no length known up front, so - /// this is what bounds the arena a single stream can grow. It stops applying to the body when the + /// Ceiling on one request's body. A body arrives as a stream of DATA frames with no length known + /// up front, so this is what bounds the arena a single stream can grow. It stops applying when the /// handler takes a body reader - there is no arena for it to bound. /// public int MaxRequestBytes { get; init; } = 8 * 1024 * 1024; + + /// + /// Ceiling on what one request's header blocks decode to, measured as RFC 9113 6.5.2 does: each + /// field's name and value plus 32 bytes. nghttp2 bounds the blocks it reads, but one byte of HPACK can + /// stand for a whole table entry - the HPACK bomb. Past this the stream is reset with + /// ENHANCE_YOUR_CALM, never dispatched. + /// + public int MaxHeaderListSize { get; init; } = 64 * 1024; } diff --git a/tests/Ioxide.Tests.Unit/Http2TrailerTests.cs b/tests/Ioxide.Tests.Unit/Http2TrailerTests.cs index 1e1db4d5..ade94767 100644 --- a/tests/Ioxide.Tests.Unit/Http2TrailerTests.cs +++ b/tests/Ioxide.Tests.Unit/Http2TrailerTests.cs @@ -209,7 +209,7 @@ private static byte[] Request(string path) // A literal with a new name: without indexing, or with incremental indexing, which inserts it // into the dynamic table at index 62. - private static byte[] Field(string name, string value, bool index = false) + internal static byte[] Field(string name, string value, bool index = false) { var block = new List { (byte)(index ? 0x40 : 0x00) }; Literal(block, name); diff --git a/tests/Ioxide.Tests.Unit/Nghttp2HeaderListTests.cs b/tests/Ioxide.Tests.Unit/Nghttp2HeaderListTests.cs new file mode 100644 index 00000000..b900d948 --- /dev/null +++ b/tests/Ioxide.Tests.Unit/Nghttp2HeaderListTests.cs @@ -0,0 +1,195 @@ +using System.Buffers.Binary; +using System.Reflection; +using System.Text; +using ioxide.nghttp2; + +namespace Ioxide.Tests; + +/// +/// What a request's header blocks decode to is bounded, not only what they arrive as: once a large +/// field is in the dynamic table, each one-byte reference to it decodes to the whole field. +/// +internal static class Nghttp2HeaderListTests +{ + private const byte Headers = 0x1, RstStream = 0x3, GoAway = 0x7; + private const byte EndStream = 0x1, EndHeaders = 0x4; + + // :method GET, :scheme http, :path /, and an :authority, which nghttp2 requires. + private static readonly byte[] Get = [0x82, 0x86, 0x84, 0x01, 9, .. "a.example"u8.ToArray()]; + private static readonly byte[] Bomb = Enumerable.Repeat((byte)0xBE, 16_000).ToArray(); // index 62: the field below + private static readonly Nghttp2Response Ok = Nghttp2Response.Text("ok"); + + public static void Register(Runner runner) + { + // On main each of these streams holds 62 MB until the connection closes. + runner.Test("nghttp2 header list: a block that decodes past the limit resets its stream, keeps nothing and leaves HPACK in step", () => + { + using var client = new StrictClient(); + var connection = new Nghttp2Connection(client); + var served = new List(); + Task run = connection.RunAsync(request => + { + served.Add(Describe(request)); + return Ok; + }); + PutFieldInTable(client); + served.Clear(); + + long before = GC.GetTotalMemory(forceFullCollection: true); + long allocatedBefore = GC.GetAllocatedBytesForCurrentThread(); // parsed inline on this thread + var frames = new List(); + foreach (int id in new[] { 3, 5, 7 }) + { + client.Feed(Frame(Headers, EndHeaders, id, [.. Get, .. Bomb])); // no END_STREAM: it would stay until the close + frames.AddRange(client.Drain()); + } + long allocated = GC.GetAllocatedBytesForCurrentThread() - allocatedBefore; + long retained = GC.GetTotalMemory(forceFullCollection: true) - before; + + Assert.Equal("3 0xb, 5 0xb, 7 0xb", Resets(frames)); + Assert.Equal(0, frames.Count(f => f.Type == GoAway)); + Assert.True(retained < 2 * 1024 * 1024, $"3 open streams of 16 KB frames retained {retained / 1024} KiB"); + Assert.True(allocated < 4 * 1024 * 1024, $"decoding them allocated {allocated / 1024} KiB, though nothing past the limit is kept"); + Assert.Equal(0, served.Count); + + // Once a request, not once for each of its 16,000 fields: every entry is a native call when the read unwinds. + object ready = typeof(Nghttp2Connection).GetField("_readyThisPass", BindingFlags.NonPublic | BindingFlags.Instance)!.GetValue(connection)!; + int capacity = (int)ready.GetType().GetProperty("Capacity")!.GetValue(ready)!; + Assert.True(capacity < 64, $"the ready list grew to {capacity} entries for 3 requests"); + + client.Feed(Frame(Headers, EndHeaders | EndStream, 9, [.. Get, 0xBE])); // the same entry, so the table is in step + client.Drain(); + Assert.True(served.SequenceEqual(["9 x=4063"]), $"the next request was served as: {string.Join(", ", served)}"); + Assert.True(!connection.IsBroken, "one stream's headers failed the connection"); + client.Close(run); + connection.Dispose(); + }); + + // :method GET 42, :scheme http 43, :path / 38, :authority a.example 51, and x-a with 100 bytes 135: 309 by RFC 9113 6.5.2. + foreach ((int limit, bool fits) in new[] { (309, true), (308, false) }) + { + int max = limit; + bool expected = fits; + runner.Test($"nghttp2 header list: a request measuring 309 B against a limit of {max} is {(expected ? "served" : "reset")}", () => + { + using var client = new StrictClient(); + var connection = new Nghttp2Connection(client, new Nghttp2Options { MaxHeaderListSize = max }); + int served = 0; + Task run = connection.RunAsync(_ => + { + served++; + return Ok; + }); + client.SendRequests(); + client.Drain(); + + byte[] block = [.. Get, .. Http2TrailerTests.Field("x-a", new string('v', 100))]; + client.Feed(Frame(Headers, EndHeaders | EndStream, 1, block)); + List first = client.Drain(); + client.Feed(Frame(Headers, EndHeaders | EndStream, 3, block)); // a request's count is its own + List second = client.Drain(); + client.Close(run); + connection.Dispose(); + + Assert.Equal(expected ? 2 : 0, served); + Assert.Equal(expected ? "" : "1 0xb", Resets(first)); + Assert.Equal(expected ? "" : "3 0xb", Resets(second)); + }); + } + + runner.Test("nghttp2 header list: a handler taking a body reader never sees a request past the limit", () => + { + using var client = new StrictClient(); + var connection = new Nghttp2Connection(client, new Nghttp2Options { MaxHeaderListSize = 300 }); + int served = 0; + Task run = connection.RunAsync((request, _) => + { + served++; + return new ValueTask(Ok); + }); + client.SendRequests(); + client.Drain(); + + client.Feed(Frame(Headers, EndHeaders, 1, [.. Get, .. Http2TrailerTests.Field("x-a", new string('v', 100))])); + List frames = client.Drain(); + client.Close(run); + connection.Dispose(); + + Assert.Equal("1 0xb", Resets(frames)); + Assert.Equal(0, served); + }); + + runner.Test("nghttp2 header list: a streamed response's handler never sees a request past the limit", () => + { + using var client = new StrictClient(); + var connection = new Nghttp2Connection(client, new Nghttp2Options { MaxHeaderListSize = 300 }); + int served = 0; + Task run = connection.RunAsync((_, writer) => + { + served++; + return ValueTask.CompletedTask; + }); + client.SendRequests(); + client.Drain(); + + client.Feed(Frame(Headers, EndHeaders | EndStream, 1, [.. Get, .. Http2TrailerTests.Field("x-a", new string('v', 100))])); + List frames = client.Drain(); + client.Close(run); + connection.Dispose(); + + Assert.Equal("1 0xb", Resets(frames)); + Assert.Equal(0, served); + }); + + // Trailers land in the same request, so a block that fits does not get a fresh allowance for the one after it. + runner.Test("nghttp2 header list: trailers count with the block that opened the request", () => + { + using var client = new StrictClient(); + var connection = new Nghttp2Connection(client, new Nghttp2Options { MaxHeaderListSize = 500 }); + int served = 0; + Task run = connection.RunAsync(_ => + { + served++; + return Ok; + }); + client.SendRequests(); + client.Drain(); + + byte[] field = Http2TrailerTests.Field("x-a", new string('v', 100)); + client.Feed(Frame(Headers, EndHeaders, 1, [.. Get, .. field])); // 309 B + client.Feed(Frame(Headers, EndHeaders | EndStream, 1, [.. field, .. field])); // and 270 B more + List frames = client.Drain(); + client.Close(run); + connection.Dispose(); + + Assert.Equal("1 0xb", Resets(frames)); + Assert.Equal(0, served); + }); + } + + // Stream 1 inserts x with 4,063 bytes at dynamic index 62. + private static void PutFieldInTable(StrictClient client) + { + client.SendRequests(); // preface and SETTINGS + client.Drain(); + client.Feed(Frame(Headers, EndHeaders | EndStream, 1, [.. Get, .. Http2TrailerTests.Field("x", new string('v', 4063), index: true)])); + client.Drain(); + } + + private static byte[] Frame(byte type, int flags, int streamId, ReadOnlySpan payload) + => Http2FlowControlTests.FrameBytes(type, (byte)flags, streamId, payload); + + private static string Describe(Nghttp2Request request) + { + var fields = new List(); + foreach (KeyValuePair, ReadOnlyMemory> field in request.Headers.AsSpan()) + { + fields.Add($"{Encoding.ASCII.GetString(field.Key.Span)}={field.Value.Length}"); + } + return $"{request.StreamId} {string.Join(' ', fields)}"; + } + + private static string Resets(List frames) => string.Join(", ", frames + .Where(f => f.Type == RstStream) + .Select(f => $"{f.StreamId} 0x{BinaryPrimitives.ReadUInt32BigEndian(f.Payload):x}")); +} diff --git a/tests/Ioxide.Tests.Unit/Program.cs b/tests/Ioxide.Tests.Unit/Program.cs index fe2963c8..8c26a99c 100644 --- a/tests/Ioxide.Tests.Unit/Program.cs +++ b/tests/Ioxide.Tests.Unit/Program.cs @@ -28,6 +28,7 @@ private static int Main() Http2OutputQueueTests.Register(runner); Http2StreamedRequestTests.Register(runner); Http2TrailerTests.Register(runner); + Nghttp2HeaderListTests.Register(runner); Http2RequestHeaderTests.Register(runner); Http2StreamedBodyTests.Register(runner); Http2FlowControlTests.Register(runner);