Repository navigation
redis: a reply nested more than 512 arrays deep fails the command instead of overflowing the stack - #317
Open
MDA2AV wants to merge 1 commit into
Open
redis: a reply nested more than 512 arrays deep fails the command instead of overflowing the stack#317MDA2AV wants to merge 1 commit into
MDA2AV wants to merge 1 commit into
Conversation
…tead 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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A redis server could kill the whole process with one deeply nested reply: the parser's recursion overflowed the reactor thread's stack.
RespProtocol.TryParseAtrecursed once per array level, with no bound, on the reactor thread. A reply of one-element arrays nested 100,000 deep is 400 KB on the wire, well inside the 64 MB receive ceiling. It overflowed the stack at about 11,400 levels. A stack overflow cannot be caught in .NET, so the process aborted, taking every reactor with it.An array nested more than 512 deep now throws a
RedisException. That breaks the connection and fails its commands, like any other malformed reply. Real replies nest a few levels, and 512 levels use under 5% of the depth that overflowed. No new state: the depth is a parameter of the recursion.Tests
New, in
RedisReplyTests(no redis needed):redis: a reply nested 100,000 arrays deep fails the command instead of overflowing the stackcontrol: the same fake server's reply nested 8 arrays deep is parsedBoth PING a
FakeServer, a new harness helper that runs a script per accepted loopback connection. Here the script answers with:1inside nested one-element arrays. The control walks the parsed reply and must report depth 8, so the refusal is about depth and not the fake server.Stack overflow.(TryParseAtrepeated 11,389 times)RedisException: RESP reply nested more than 512 arrays deepSuites: Unit 63 (1 pending, as on main), Pg 6, Redis 5, with both sidecars running (nothing skipped).
Hot path, barely: one integer compare per array in a reply, and one extra argument.
Seen and left alone, since neither is about depth:
*<count>allocatesnew RespValue[count]before any element has arrived, so a 12-byte*100000000\r\nasks for about 3 GB.FakeServer.csis byte-identical to the one in the message-length, SCRAM and open-deadline branches.