From ccefa8344ba0d4cc9dcaed800187229e5312dcc3 Mon Sep 17 00:00:00 2001 From: Diogo Martins Date: Wed, 7 Oct 2026 21:52:34 +0100 Subject: [PATCH] redis: a reply nested more than 512 arrays deep fails the command instead of overflowing the stack TryParseAt recursed once per array level with no bound, on the reactor thread's stack. A reply of one-element arrays nested 100,000 deep, 400 KB on the wire and well inside the 64 MB receive ceiling, overflowed the stack at about 11,400 levels and killed the process: a stack overflow cannot be caught. An array nested more than 512 deep now throws a RedisException, which breaks that connection and fails its commands like any malformed reply. Real replies nest a few levels. No new state; the depth is a parameter of the recursion. New tests in RedisReplyTests PING a FakeServer (a new harness helper for scripted loopback peers) that answers with :1 inside nested one-element arrays: 100,000 levels must fail with the nesting error, and a control with 8 levels must parse to depth 8. Confirmed both ways: on f298af0 the test process aborts with "Stack overflow" (TryParseAt repeated 11,389 times), and with the fix it gets the RedisException. The control passes on both. --- src/clients/ioxide.redis/RespProtocol.cs | 14 ++- tests/Ioxide.Tests.Harness/FakeServer.cs | 112 ++++++++++++++++++++ tests/Ioxide.Tests.Redis/Program.cs | 1 + tests/Ioxide.Tests.Redis/RedisReplyTests.cs | 82 ++++++++++++++ 4 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 tests/Ioxide.Tests.Harness/FakeServer.cs create mode 100644 tests/Ioxide.Tests.Redis/RedisReplyTests.cs diff --git a/src/clients/ioxide.redis/RespProtocol.cs b/src/clients/ioxide.redis/RespProtocol.cs index 8b79544e..d6bb76ee 100644 --- a/src/clients/ioxide.redis/RespProtocol.cs +++ b/src/clients/ioxide.redis/RespProtocol.cs @@ -5,6 +5,9 @@ namespace ioxide.redis; /// RESP2 wire format: write a command as an array of bulk strings; parse one reply. internal static class RespProtocol { + // Real replies nest a few arrays deep; the parser recurses per level, on the reactor's stack. + private const int MaxDepth = 512; + /// /// Bytes needed to encode a command as a RESP array of bulk strings, given a pre-framed name /// token ($len\r\nNAME\r\n, see ) plus the argument list. @@ -95,7 +98,7 @@ public static bool TryParse(ReadOnlySpan buffer, out RespValue value, out { int pos = 0; needed = 0; - if (TryParseAt(buffer, ref pos, out value, ref needed)) + if (TryParseAt(buffer, ref pos, out value, ref needed, depth: 0)) { consumed = pos; return true; @@ -104,7 +107,7 @@ public static bool TryParse(ReadOnlySpan buffer, out RespValue value, out return false; } - private static bool TryParseAt(ReadOnlySpan buffer, ref int pos, out RespValue value, ref int needed) + private static bool TryParseAt(ReadOnlySpan buffer, ref int pos, out RespValue value, ref int needed, int depth) { value = default; if (pos >= buffer.Length) @@ -180,11 +183,16 @@ private static bool TryParseAt(ReadOnlySpan buffer, ref int pos, out RespV throw new RedisException($"invalid array length {count}"); } + if (depth >= MaxDepth) + { + throw new RedisException($"RESP reply nested more than {MaxDepth} arrays deep"); + } + var items = new RespValue[count]; int p = afterLine; for (int i = 0; i < count; i++) { - if (!TryParseAt(buffer, ref p, out RespValue item, ref needed)) + if (!TryParseAt(buffer, ref p, out RespValue item, ref needed, depth + 1)) { return false; } diff --git a/tests/Ioxide.Tests.Harness/FakeServer.cs b/tests/Ioxide.Tests.Harness/FakeServer.cs new file mode 100644 index 00000000..d33fef1b --- /dev/null +++ b/tests/Ioxide.Tests.Harness/FakeServer.cs @@ -0,0 +1,112 @@ +using System.Net; +using System.Net.Sockets; + +namespace Ioxide.Tests; + +/// +/// A loopback server that misbehaves on purpose, for the far end of a protocol client: each +/// connection it accepts runs a script on a thread of its own, which speaks just enough of the +/// protocol and then sends the offending bytes or goes silent. Disposing it closes the listener +/// and every connection it accepted. +/// +public sealed class FakeServer : IDisposable +{ + private readonly Socket _listener; + private readonly Action _script; + private readonly List _accepted = []; + private readonly Lock _lock = new(); + private bool _disposed; + + /// The port it listens on, on 127.0.0.1. + public int Port { get; } + + /// + /// Run once per accepted connection, with its index (0 for the first) and its socket. A script + /// that returns leaves the connection open until the server is disposed. + /// + public FakeServer(Action script) + { + _script = script; + _listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + _listener.Bind(new IPEndPoint(IPAddress.Loopback, 0)); + _listener.Listen(16); + Port = ((IPEndPoint)_listener.LocalEndPoint!).Port; + + new Thread(AcceptLoop) { IsBackground = true, Name = $"fake-server-{Port}" }.Start(); + } + + /// Reads exactly bytes, or throws if the peer closes first. + public static byte[] Receive(Socket socket, int count) + { + var buffer = new byte[count]; + for (int filled = 0; filled < count;) + { + int n = socket.Receive(buffer, filled, count - filled, SocketFlags.None); + if (n == 0) + { + throw new IOException($"the peer closed after {filled} of {count} bytes"); + } + + filled += n; + } + + return buffer; + } + + private void AcceptLoop() + { + for (int index = 0; ; index++) + { + Socket accepted; + try + { + accepted = _listener.Accept(); + } + catch (Exception) + { + return; // disposed + } + + lock (_lock) + { + if (_disposed) + { + accepted.Dispose(); + return; + } + + _accepted.Add(accepted); + } + + int which = index; + new Thread(() => + { + try + { + _script(which, accepted); + } + catch (Exception) + { + // The client hung up mid-script, or Dispose closed the socket under it. + } + }) + { IsBackground = true, Name = $"fake-server-{Port}-{which}" }.Start(); + } + } + + public void Dispose() + { + lock (_lock) + { + _disposed = true; + foreach (Socket socket in _accepted) + { + socket.Dispose(); + } + + _accepted.Clear(); + } + + _listener.Dispose(); + } +} diff --git a/tests/Ioxide.Tests.Redis/Program.cs b/tests/Ioxide.Tests.Redis/Program.cs index 727bcdcb..ed108e23 100644 --- a/tests/Ioxide.Tests.Redis/Program.cs +++ b/tests/Ioxide.Tests.Redis/Program.cs @@ -18,6 +18,7 @@ private static int Main() Console.WriteLine($"redis {(up ? "up" : "down")} ({redis.Host}:{redis.Port})\n"); RedisTests.Register(runner, redis, up); + RedisReplyTests.Register(runner); return runner.Summary(); } } diff --git a/tests/Ioxide.Tests.Redis/RedisReplyTests.cs b/tests/Ioxide.Tests.Redis/RedisReplyTests.cs new file mode 100644 index 00000000..7f311e79 --- /dev/null +++ b/tests/Ioxide.Tests.Redis/RedisReplyTests.cs @@ -0,0 +1,82 @@ +using System.Net.Sockets; +using System.Text; +using ioxide; +using ioxide.redis; + +namespace Ioxide.Tests; + +/// Replies from a fake server that nests arrays without end. Needs no redis. +internal static class RedisReplyTests +{ + public static void Register(Runner runner) + { + // The parser recursed once per level, so a deep enough reply overflowed the reactor's stack and killed the process. + runner.Test("redis: a reply nested 100,000 arrays deep fails the command instead of overflowing the stack", () => + { + string outcome = Ping(100_000); + Assert.True(outcome.StartsWith("RedisException") && outcome.Contains("deep"), + $"expected a RedisException about the nesting, got [{outcome}]"); + }); + + runner.Test("control: the same fake server's reply nested 8 arrays deep is parsed", () => + { + Assert.Equal("depth 8", Ping(8)); + }); + } + + // PINGs a fake server that answers with :1 inside depth one-element arrays, and reports the depth it parsed. + private static string Ping(int depth) + { + byte[] reply = Encoding.ASCII.GetBytes(string.Concat(Enumerable.Repeat("*1\r\n", depth)) + ":1\r\n"); + using var fake = new FakeServer((_, socket) => Answer(socket, reply)); + var options = new RedisOptions { Port = (ushort)fake.Port }; + + int port = TestServer.Start((r, conn) => Handle(r, conn, options)); + (int status, string body) = Client.Get(port, "/ping"); + Assert.Equal(200, status); + return body; + } + + private static void Answer(Socket socket, byte[] reply) + { + FakeServer.Receive(socket, "*1\r\n$4\r\nPING\r\n".Length); + socket.Send(reply); + } + + private static async Task Handle(Reactor r, TcpConnection conn, RedisOptions options) + { + try + { + RecvSnapshot snapshot = await conn.ReadAsync(); + if (Wire.ReadPath(conn, snapshot) != "/ping") + { + return; // the harness's listen probe + } + + string outcome; + try + { + using RedisConnection redis = await RedisConnection.ConnectAsync(r, options); + RespValue reply = await redis.ExecuteAsync("PING"); + int depth = 0; + for (; reply.Kind == RespKind.Array; depth++) + { + reply = reply.Items[0]; + } + + outcome = $"depth {depth}"; + } + catch (Exception e) + { + outcome = $"{e.GetType().Name}: {e.Message}"; + } + + Wire.Write(conn, 200, outcome); + await conn.FlushAsync(); + } + finally + { + conn.DecRef(); + } + } +}