Reject SETRANGE offsets that overflow max record size#1947
Open
hexonal wants to merge 4 commits into
Open
Conversation
SETRANGE only validated offset < 0, so an offset large enough that offset + value length exceeds the 512MB max record size (e.g. SETRANGE key 600000000 x) reached the storage layer, which throws a raw TsavoriteException. That exception doesn't derive from GarnetException, so it skips the dedicated error-response handler in RespServerSession and falls into the generic catch-all, which logs and disposes the network sender without ever sending a RESP error - silently killing the client connection instead of returning a clean error. Reject the oversized offset up front in NetworkSetRange, following the same pattern already used by SETBIT/BITFIELD, which check their offsets against BitmapManager.MaxBitmapPayloadBytes before reaching storage.
Real Redis distinguishes "offset is out of range" (negative offset) from "string exceeds maximum allowed size (proto-max-bulk-len)" (offset+length beyond the max string size). Match that wording instead of reusing the generic offset-out-of-range message for both cases.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the SETRANGE RESP handler to prevent large-but-valid offsets from triggering an unhandled storage-layer exception (and dropping the client connection), instead returning a clean Redis-compatible ERR response.
Changes:
- Added a pre-dispatch size check in
NetworkSetRangeto rejectoffset + value.Lengthbeyond the max record size, returning a RESP error rather than allowing a Tsavorite exception to propagate. - Introduced a new
CmdStringserror constant matching Redis’s message for the max-size failure class. - Extended
SetRangeTestto validate the error reply and confirm the connection remains usable after the rejected command.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/standalone/Garnet.test/RespTests.cs | Adds a regression test ensuring oversized SETRANGE returns a server error (not a dropped connection) and the connection remains usable. |
| libs/server/Resp/CmdStrings.cs | Adds a dedicated RESP error string for “string exceeds maximum allowed size (proto-max-bulk-len)”. |
| libs/server/Resp/BasicCommands.cs | Adds early validation in NetworkSetRange to block oversized results before hitting the storage layer. |
vazois
self-requested a review
July 21, 2026 18:22
vazois
approved these changes
Jul 22, 2026
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.
Problem
SETRANGEwith a very large but syntactically valid offset (e.g.SETRANGE key 600000000 value) kills the client connection instead of returning a RESP error.NetworkSetRangeinlibs/server/Resp/BasicCommands.csonly rejects negative offsets before dispatching to the storage layer. Whenoffset + value.Lengthexceeds the maximum record size (BitmapManager.MaxBitmapPayloadBytes, 512 MiB — the same limit enforced forSETBIT/BITFIELD),RecordSizeInfo.CalculateSizesin the Tsavorite storage layer throws instead of returning a value the RESP layer can turn into a clean error. That exception propagates up as an unhandled server-side fault and the connection is dropped (RedisConnectionException: SocketClosed), rather than the client simply seeing anERRreply.Reproduce with
SETRANGEagainst a fresh key and a large offset — the connection is closed instead of getting an error response.Fix
Add a check in
NetworkSetRangethat rejectsoffset + value.Length > BitmapManager.MaxBitmapPayloadBytesbefore building the storage input, returning a clean RESP error instead of letting the storage layer throw. The error text matches real Redis's distinct message for this failure class ("ERR string exceeds maximum allowed size (proto-max-bulk-len)", added as a newCmdStringsconstant) — separate from the existing"ERR offset is out of range"used for negative offsets, matching Redis's ownt_string.cbehavior and the precedent already set in this codebase bySETBIT's dedicated out-of-range message.Testing
Garnet.server.csproj(net8.0 + net10.0 target frameworks) — clean, no warnings/errors.SetRangeTestintest/standalone/Garnet.test/RespTests.csagainstnet10.0— passes. (net8.0 runtime is not installed in this environment, so that TFM could not be executed directly; both TFMs share identical source, and the net8.0 build itself succeeded.)BasicCommands.csfix only (kept the test change) and reran the same test to confirm it reproduces the original failure:RedisConnectionException: SocketCloseddue to the connection being dropped. Restored the fix afterward.dotnet format Garnet.slnx --no-restore --verify-no-changeson the changed files — no formatting issues.Not covered by this PR (noted for reviewers): the added test exercises an offset far past the boundary and the new-key path only; it does not pin the exact 512 MiB boundary or an existing-key growth-past-limit case.