From dbce5f9ab50332773c2e4282da278d65fd4190bd Mon Sep 17 00:00:00 2001 From: Diogo Martins Date: Thu, 5 Nov 2026 22:28:02 +0000 Subject: [PATCH] nghttp2: what a request's header blocks decode to is bounded by a new MaxHeaderListSize, so one 16 KB frame can no longer make 62 MB nghttp2 bounds the blocks it reads, not what they decode to, and one byte of HPACK can stand for a whole table entry. Past the limit a request is marked as an oversized body is, so nothing more is copied and its stream is reset with ENHANCE_YOUR_CALM once ih2_read unwinds. Fixes #508. --- .../Connection/Nghttp2Connection.Callbacks.cs | 21 +- .../Connection/Nghttp2Connection.cs | 6 +- .../Connection/Nghttp2Options.cs | 13 +- tests/Ioxide.Tests.Unit/Http2TrailerTests.cs | 2 +- .../Nghttp2HeaderListTests.cs | 195 ++++++++++++++++++ tests/Ioxide.Tests.Unit/Program.cs | 1 + 6 files changed, 229 insertions(+), 9 deletions(-) create mode 100644 tests/Ioxide.Tests.Unit/Nghttp2HeaderListTests.cs 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);