|
| 1 | +# Bounded Request Processing Design |
| 2 | + |
| 3 | +## Goal |
| 4 | + |
| 5 | +Prevent unauthenticated or slow clients from causing unbounded memory growth in |
| 6 | +the active RESP parsing path and the optional command pipeline, while preserving |
| 7 | +Redis-compatible request limits and existing command behavior. |
| 8 | + |
| 9 | +## Scope |
| 10 | + |
| 11 | +The change covers three proven ownership problems: |
| 12 | + |
| 13 | +1. RESP length headers can trigger oversized aggregate reservations before |
| 14 | + authentication, bulk frames can grow the parser buffer without a declared |
| 15 | + length check, and recursive aggregates have no nesting limit. |
| 16 | +2. The active network path returns parsed `RespData` but leaves a second |
| 17 | + `RespCommand` copy in `RespParse`, retaining command history for the lifetime |
| 18 | + of a connection. |
| 19 | +3. `PipelineConfig::command_queue_size` is reported but ignored because the |
| 20 | + optional pipeline uses an unbounded channel. |
| 21 | + |
| 22 | +The storage findings from issue #395 are not implementation scope. Normal |
| 23 | +client `DEL` and `MSETNX` calls already hold the executor's |
| 24 | +`STORAGE_EXCLUSIVE` gate, Raft apply is single-writer, and expiration compaction |
| 25 | +is currently a no-op. Those facts invalidate their current P0 data-loss |
| 26 | +descriptions; adding local locks would not repair the claimed Raft interleaving. |
| 27 | + |
| 28 | +## RESP Limits |
| 29 | + |
| 30 | +`RespParse` owns immutable limits. Production constructors use: |
| 31 | + |
| 32 | +- 64 KiB maximum first-line length for inline commands and every RESP type or |
| 33 | + aggregate header. |
| 34 | +- 512 MiB maximum bulk, bulk-error, and verbatim-string payload length. |
| 35 | +- 1 GiB maximum buffered frame length for authenticated/general parsing. |
| 36 | +- `i32::MAX` maximum aggregate item or pair count, matching Redis' multibulk |
| 37 | + header boundary. |
| 38 | +- 128 maximum aggregate nesting depth. |
| 39 | +- At most 1024 elements of initial aggregate reservation, followed by fallible |
| 40 | + incremental reservation. |
| 41 | +- At most 65,536 decoded `RespData` nodes in one frame. The `i32::MAX` wire |
| 42 | + count remains accepted as a protocol boundary, but it cannot force that many |
| 43 | + objects to be materialized. |
| 44 | +- At most 1,000,000 cumulative node visits while an incomplete frame is |
| 45 | + reparsed. This bounds repeated aggregate-prefix work without changing the |
| 46 | + public parser API. |
| 47 | + |
| 48 | +Tests use a private constructor with small limits, so boundaries are exercised |
| 49 | +without allocating large buffers. Limits are checked before extending the |
| 50 | +buffer or reserving aggregate memory. Limit violations are terminal parser |
| 51 | +errors rather than incomplete frames. |
| 52 | + |
| 53 | +The active network path also enforces a 1 MiB buffer limit before authentication. |
| 54 | +It checks the existing incomplete-frame size plus the next read before passing |
| 55 | +bytes to the parser. A violation closes only that connection through the |
| 56 | +existing protocol-error path. |
| 57 | + |
| 58 | +## Parser Ownership |
| 59 | + |
| 60 | +The public parser API remains compatible in this PR. All network-side parser |
| 61 | +consumers route through one helper that drains the corresponding legacy |
| 62 | +`next_command()` entry after each complete frame. This keeps the internal queue |
| 63 | +bounded without combining an API removal with the security fix. Removing the |
| 64 | +duplicate command representation is a follow-up API cleanup. |
| 65 | + |
| 66 | +## Pipeline Backpressure |
| 67 | + |
| 68 | +The optional pipeline uses `tokio::sync::mpsc::channel` with a minimum capacity |
| 69 | +of one. `submit_command` awaits bounded-channel capacity and applies the existing |
| 70 | +30-second timeout to queue admission as well as response delivery. Channel |
| 71 | +closure remains a distinct error. Statistics report the normalized, real queue |
| 72 | +capacity. |
| 73 | + |
| 74 | +## Error Handling |
| 75 | + |
| 76 | +- Malformed or over-limit RESP frames become `RespParseResult::Error`. |
| 77 | +- An over-limit unauthenticated connection is closed without affecting other |
| 78 | + clients or the server process. |
| 79 | +- Queue admission timeout returns `PipelineError::Timeout`. |
| 80 | +- No production path uses `unwrap`, `expect`, or infallible allocation for |
| 81 | + attacker-controlled aggregate capacity. |
| 82 | + |
| 83 | +## Verification |
| 84 | + |
| 85 | +The implementation must prove red-green behavior for oversized first lines and |
| 86 | +length headers, nesting, decoded-node amplification, cumulative incomplete |
| 87 | +aggregate work, chunked buffer growth, parser reset, legacy queue draining, and |
| 88 | +bounded pipeline capacity. Final gates are targeted tests, workspace tests, |
| 89 | +strict Clippy, formatting, `git diff --check`, Linux/WSL verification, and |
| 90 | +current PR Head/check reconciliation. |
0 commit comments