redis: raise the RESP line-length cap from 512KB to 512MB - #34312
Conversation
read_until_crlf capped its CRLF scan at MAX_LINE_LEN = 512KB, so any simple string (+), error (-), integer (:), double (,), boolean (#), null (_) or big-number (() reply whose payload exceeded 512KB was rejected with LineTooLong -> ERR_REDIS_INVALID_RESPONSE, and the scan window was additionally off by one so the break point was exactly 512*1024+1 bytes. The cap has no equivalent in the 1.3.14 Zig parser (readUntilCRLF scanned the whole buffer) and the RESP spec places no length limit on line-terminated types; a real Redis server emits arbitrary-length + / - lines for Lua redis.status_reply() / redis.error_reply(). Remove the cap and the LineTooLong variant. The incremental ReplyScanner's crlf_skip bookkeeping already ensures each buffered byte is examined once across socket reads, so the scan stays linear. The zombie-client half of the report (fail() latches without closing the socket) is #33479.
WalkthroughChangesRESP line reply handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:12 AM PT - Jul 16th, 2026
❌ @robobun, your commit 0ea8158 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34312That installs a local version of the PR into your bun-34312 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ound Keep the LineTooLong guard so a server that streams an unterminated simple-string line cannot grow read_buffer without bound, matching the existing 512MB cap on length-prefixed blobs. The regression (600KB Lua status_reply rejected) is fixed either way since 600KB << 512MB.
robobun
changed the title
There was a problem hiding this comment.
LGTM — 0f80981 addresses the earlier concern by capping line-terminated replies at MAX_BULK_LEN instead of removing the bound, so +/- lines now have the same 512MB buffer-growth ceiling as $ blobs. The inline nit about the two test comments still describing the intermediate no-cap state is comment-only and doesn't affect the assertions.
Extended reasoning...
Overview
The final diff is effectively a one-line constant change in src/valkey/valkey_protocol.rs (MAX_LINE_LEN: 512 KiB → MAX_BULK_LEN as usize = 512 MiB) plus a doc comment, three new mock-server tests in test/js/valkey/reliability/resp-nesting-depth.test.ts covering the 600 KB + reply, the 524289-byte boundary, and a 600 KB - reply, and an updated assertion in test/js/valkey/valkey-gc.test.ts (a 600 KB unterminated line now surfaces as ERR_REDIS_CONNECTION_CLOSED rather than ERR_REDIS_INVALID_RESPONSE because 600 KB is far below the new cap).
Follow-up on prior review
My previous comment flagged that removing MAX_LINE_LEN entirely left line-terminated replies with no buffer-growth bound. The author pushed 0f80981 which reintroduces the constant at MAX_BULK_LEN — exactly the suggested fix — so line-terminated types now have parity with length-prefixed blobs. I re-checked the boundary semantics of the unchanged read_until_crlf loop with the new constant: a line of exactly 512 MiB is accepted and 512 MiB + 1 returns LineTooLong, which matches the > MAX_BULK_LEN check on bulk strings. The updated valkey-gc test's 600 000-byte unterminated payload correctly falls through to InvalidResponse → NeedMoreData → connection-closed on socket end.
Security risks
The only security-adjacent concern was the unbounded-buffer-growth path, now closed at 512 MiB (parity with what a hostile server can already force via a $536870912\r\n bulk string). The threat model still requires the user to have pointed the client at a hostile server. No new parsing of untrusted lengths, no allocation-size arithmetic, no FFI surface change.
Level of scrutiny
Low-to-medium: a constant bump in a network protocol parser with existing test coverage, restoring 1.3.14 behavior for a filed regression. The read_until_crlf scan logic itself is untouched — only the ceiling moved.
Other factors
The one bug-hunter finding this run is a documentation nit: the valkey-gc.test.ts block comment (and a sibling in valkey-incremental-scan.test.ts) still describe the intermediate commit's no-cap / 512 KiB state rather than the final 512 MiB cap. Both tests' assertions remain valid (600 KB and 410 KB are well under 512 MiB); only the prose is stale. Not a merge blocker.
There was a problem hiding this comment.
LGTM — both earlier review rounds were addressed (cap raised to MAX_BULK_LEN rather than removed in 0f80981; stale test comments synced in af1fbb4), and the net Rust change is now a single constant bump.
What was reviewed:
read_until_crlfscan-window /LineTooLongguard retained, so line-terminated replies are still bounded (now at 512MB, matching$/=/!).- New tests cover the 600KB
+reply, the 524289-byte boundary, and a 600KB-error reply with follow-up command; the updatedvalkey-gc.test.tsassertion (ERR_REDIS_CONNECTION_CLOSED) is correct for a 600KB unterminated line under the new cap. - Grepped
test/js/valkey/for remaining "512 KiB" /MAX_LINE_LENreferences — none left besides the intentional boundary test.
Extended reasoning...
Overview
The functional change is a one-line constant edit in src/valkey/valkey_protocol.rs: MAX_LINE_LEN goes from 512 * 1024 to Self::MAX_BULK_LEN as usize (512MB), with a doc comment explaining the parity. The read_until_crlf scan window, the .take(limit) bound, and the LineTooLong error variant are all retained unchanged. Three new mock-server tests are added in resp-nesting-depth.test.ts; valkey-gc.test.ts updates one assertion and its comments to reflect that 600KB is now under the cap; valkey-incremental-scan.test.ts drops a stale comment clause only.
Security risks
The change touches a DoS-adjacent guard (buffer-growth bound on untrusted server bytes). An earlier revision of this PR removed the bound entirely; that was flagged in my first review pass and the author reintroduced it at MAX_BULK_LEN in 0f80981, exactly as suggested. Line-terminated replies now have the same 512MB ceiling as length-prefixed blobs — a hostile server can force no more buffering via + than it already could via $. The threat model (user-chosen Redis URL) is unchanged. No new attack surface.
Level of scrutiny
Low-to-moderate. The runtime edit is a constant value with clear justification (RESP spec places no limit on +/- lines; real Redis emits >512KB lines via Lua redis.status_reply/redis.error_reply; 1.3.14 accepted them). The mechanism (read_until_crlf, ReplyScanner.crlf_skip) is untouched, so no new edge cases in the parser itself. Test changes are additive except for the valkey-gc.test.ts assertion swap, which I traced and found correct: 600 000 < 536 870 912, so the reader returns "need more data" until the mock server closes, and the pending command is rejected with ERR_REDIS_CONNECTION_CLOSED.
Other factors
This is my third pass on the PR. Both prior inline comments (unbounded buffer growth; stale test comments) are marked resolved and the fixes match the suggestions verbatim. No CODEOWNERS entry for src/valkey/. The bug-hunting system found nothing on this revision. I grepped test/js/valkey/ for lingering "512 KiB"/MAX_LINE_LEN references and found none beyond the deliberate 524289-byte boundary test. The one thing lost is direct test coverage of the LineTooLong branch itself (exercising it would need >512MB of mock traffic), but that branch's code is byte-for-byte unchanged from before this PR.
There was a problem hiding this comment.
LGTM — both earlier review notes are addressed; the net production change is now just the one-constant bump plus a doc comment.
What was reviewed:
read_until_crlfscan-window /LineTooLongguard retained; onlyMAX_LINE_LENvalue changed toMAX_BULK_LEN as usize— no overflow on supported (64-bit) targets,crlf_skipstill keeps the scan linear.LineTooLongvariant still mapped inprotocol_jsc.rs; greppedtest/js/valkeyandsrc/for stale 512 KiB /MAX_LINE_LENreferences — none remain.valkey-gc.test.tsexpected-error change (ERR_REDIS_INVALID_RESPONSE→ERR_REDIS_CONNECTION_CLOSED) follows mechanically: 600 KB < 512 MB so the reader waits, then rejects on socket close.- New mock-server tests reuse the file's existing
createMockRedisServerhelper and cover the 600 KB+, 524289-byte boundary, and 600 KB-cases with a follow-up PING each.
Extended reasoning...
Overview
The final net diff is a single constant change in src/valkey/valkey_protocol.rs: MAX_LINE_LEN goes from 512 * 1024 to Self::MAX_BULK_LEN as usize (512 MB), with a 3-line doc comment explaining it mirrors the bulk-string bound. The read_until_crlf body, the .min(MAX_LINE_LEN + 1) scan window, and the LineTooLong early-return are all unchanged. Test changes: three new regression cases in resp-nesting-depth.test.ts reusing the existing in-process mock server, one expected-error-code update in valkey-gc.test.ts, and comment-only updates in valkey-gc.test.ts and valkey-incremental-scan.test.ts.
Security risks
None new. The earlier revision removed the line-length bound entirely, which I flagged as a hardening regression (unbounded read_buffer growth from a hostile server). The author responded by keeping the bound and raising it to match MAX_BULK_LEN, so line-terminated replies now have exactly the same 512 MB buffer-growth ceiling that length-prefixed blobs already had — parity, not a new exposure. The threat model (user-chosen Redis URL) is unchanged.
Level of scrutiny
Low-to-moderate. The production change is a one-line constant adjustment in a protocol parser with no new control flow, no allocation changes, and no cross-platform surface. The as usize cast of a 512 M i64 is safe on all Bun targets (all 64-bit). The incremental ReplyScanner::crlf_skip bookkeeping that keeps the CRLF scan linear across socket reads is pre-existing and untouched, so raising the cap doesn't reintroduce quadratic rescans.
Other factors
Both prior review rounds were addressed exactly as suggested and the threads are resolved. The valkey-gc.test.ts update is a mechanical consequence of the constant change (600 KB is now under the cap, so an unterminated 600 KB line is treated as partial and rejects with ERR_REDIS_CONNECTION_CLOSED when the mock server closes). The LineTooLong branch itself is no longer exercised by a test — doing so would need a >512 MB payload, which is impractical for CI — but the branch is a two-line early return that's byte-identical to before. Grepped for remaining stale 512 KiB / MAX_LINE_LEN references under test/js/valkey/ and src/; none remain (the 512KB mentions in the new test names/comments correctly describe the old regressed behavior being guarded against). LineTooLong is still wired to REDIS_INVALID_RESPONSE in protocol_jsc.rs.
|
Diff is ready. All three valkey test files touched here pass locally on both debug+ASAN and release at 0ea8158. CI build 73807 is red only on tests this change does not touch (all tagged pre-existing or flaky by the scraper; none are
The only production change is the one-constant bump in |
2 participants
What
A RESP simple string (
+) or error (-) reply longer than 512KB is rejected withERR_REDIS_INVALID_RESPONSE "Failed to read data (buffer path)", after which the client rejects every subsequent command withERR_REDIS_CONNECTION_CLOSEDwhile still reportingconnected: true. 1.3.14 resolved the same reply fine.Repro
Cause
read_until_crlfinsrc/valkey/valkey_protocol.rswas capped atMAX_LINE_LEN = 512 * 1024by a later hardening pass; the 1.3.14 Zig parser (readUntilCRLF) scanned the whole buffer. The cap backs every line-terminated RESP type (+ - : _ , # (). The RESP spec places no length limit on these, and a real Redis server emits arbitrary-length+/-lines for Luaredis.status_reply()/redis.error_reply(). Bulk strings ($) carry a length prefix and were never affected.Fix
Raise
MAX_LINE_LENto matchMAX_BULK_LEN(512MB, the server-sideproto-max-bulk-lendefault), so line-terminated replies get the same buffer-growth bound as length-prefixed blobs. The scan window andLineTooLongcheck are kept so a server that streams an unterminated line still cannot grow the read buffer without bound. The incrementalReplyScanner'scrlf_skipbookkeeping keeps the scan linear across socket reads.The second half of the report (a protocol error latches
flags.failedwithout closing the socket, leaving a zombie client) is #33479; this PR only removes the trigger that made valid replies hit that path.Verification
test/js/valkey/reliability/resp-nesting-depth.test.tsgains three cases driven by an in-process mock server (no Docker): a 600KB+reply resolves and the client serves a follow-upPING; the 524289-byte boundary resolves; a 600KB-reply rejects with the server's original error text and the client survives. All three fail on the released bun withERR_REDIS_INVALID_RESPONSE "Failed to read data (buffer path)"and pass with the fix.test/js/valkey/valkey-gc.test.ts's "line terminator never arrives" case is updated: 600KB unterminated followed by socket close is now under the cap, so the pending command is rejected withERR_REDIS_CONNECTION_CLOSEDrather thanERR_REDIS_INVALID_RESPONSE.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file