Validate argument count in SET-family string commands

[hexonal (hexonal)]

Symptom

Several core string commands abort the whole session — and, in a Debug/CI build, terminate the server process — when a required argument is missing, instead of returning an error. The reply is empty and the connection is dropped, taking every pipelined command after it with it.

Debug build of GarnetServer at 4706f3f, raw RESP with a trailing PING so a dropped reply is visible:

C: SET k
C: PING
S: (connection closed, zero bytes)

server log:

Process terminated. Assertion Failed
  at Garnet.server.RespServerSession.NetworkSET[TGarnetApi](...) in libs/server/Resp/BasicCommands.cs:line 354

The same happens for, all unauthenticated and needing no prior state:

SET                 SET k               SET k v EX      (EX/PX with no value)
GETSET k            GETSET k v extra
SETEX k             SETEX k 10          PSETEX k 10
SETRANGE k          SETRANGE k 0
APPEND k
GETRANGE k          GETRANGE k 0        SUBSTR k 0

In a Release build the process does not crash — the assert is compiled out and the code instead reads the missing argument from a stale, in-allocation parse-state slot, so e.g. SETEX k 10 (no value) silently stores an empty value rather than erroring. Either way the command does not behave as it should.

The hash/list/set command handlers already validate their argument counts and return -ERR wrong number of arguments; these string handlers only Debug.Assert(parseState.Count == N) (or consume an option value without checking a token is present), relying on an argument-count guarantee the parser does not actually provide for these commands.

Root cause

libs/server/Resp/BasicCommands.cs:

• NetworkSET (SET), NetworkGETSET (GETSET), NetworkSETEX (SETEX/PSETEX), NetworkSetRange (SETRANGE), NetworkAppend (APPEND) read fixed argument positions after only a Debug.Assert on parseState.Count; NetworkGetRange (GETRANGE/SUBSTR) has no count check at all. A malformed arity reaches GetArgSliceByRef/TryGetInt at an index >= Count, which asserts (Debug) or reads out of bounds (Release).
• NetworkSETEXNX (the SET key value [EX seconds | PX milliseconds | NX | XX | GET | KEEPTTL] option parser) consumes the EX/PX value with parseState.TryGetInt(tokenIdx++, ...) without first checking tokenIdx < parseState.Count, so SET k v EX (option in final position) reads past the parse state.

Only bare SET k v reaches NetworkSET; every option-bearing form (SET k v EX 10, SET k v NX, SET k v GET, SET k v KEEPTTL, …) is parsed to a different RespCommand and a different handler, so requiring exactly two arguments here cannot reject a valid SET.

Fix

Validate the argument count at runtime, matching what the object-store handlers and the sibling NetworkSETWITHETAG already do:

• SET, GETSET, APPEND → exactly 2 args, else AbortWithWrongNumberOfArguments.
• SETEX/PSETEX, SETRANGE, GETRANGE/SUBSTR → exactly 3 args. (GETRANGE and SUBSTR share NetworkGetRange, so a malformed SUBSTR reports 'getrange' — a pre-existing shared-handler quirk, not introduced here.)
• NetworkSETEXNX → reject EX/PX as the final token (tokenIdx >= parseState.Count → syntax error) before consuming its value, mirroring the existing guard in NetworkSETWITHETAG. KEEPTTL (which takes no value) is unaffected, as the guard is only reached when the option is EX/PX.

Well-formed commands are unchanged; the error wire text matches Redis (wrong number of arguments for the arity cases, syntax error for a dangling EX/PX).

Tests

RespTests.StringCommandsWrongArityReturnErrorAndKeepSessionAlive (raw RESP via LightClientRequest, each malformed command followed by PING):

• Every malformed form above returns the expected error and then +PONG — proving the session survived. On unpatched main the process aborts / the reply is dropped, so the request times out and the test fails.
• Well-formed SET a 1, SETEX b 100 v, PSETEX c 5000 v, SETRANGE a 1 XY (:3), APPEND a Z (:4), GETSET a fresh ($4 1XYZ), GETRANGE a 0 2 ($3 fre) still behave exactly as before.

Full RespTests (352) stays green.

Scope

The same missing-validation pattern remains in two other string commands — INCRBYFLOAT (INCRBYFLOAT k aborts), and the ETag extension commands GETWITHETAG/GETIFNOTMATCH in BasicEtagCommands.cs. I left those out to keep this PR focused on the common read/write string commands, but I'm happy to follow up with them (or fold them in here) if you'd prefer to close the whole class at once.

[Copilot]

Pull request overview

Adds runtime arity validation for core string commands to prevent crashes and preserve sessions.

Changes:

• Validates fixed-arity string commands.
• Safely rejects dangling SET EX/PX options.
• Adds malformed-command and session-survival tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
libs/server/Resp/BasicCommands.cs	Adds argument validation and safe option parsing.
test/standalone/Garnet.test/RespTests.cs	Tests errors, connection survival, and valid commands.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kevin-montrose]

This is correct and needs to be addressed.

[kevin-montrose]

Copilot comment is correct, need to distinguish between two different commands using same impl.

[kevin-montrose]

This is correct and needs to be addressed.

[hexonal (hexonal)]

Addressed, and rebased onto current main.

NetworkGetRange now takes the dispatched RespCommand (following NetworkEXPIRE/SetIsMember) and reports it via AbortWithWrongNumberOfArguments(cmd.ToString()), so SUBSTR k now says 'SUBSTR' rather than 'GETRANGE'. The test that codified the wrong name is fixed, and I added coverage that SUBSTR a 0 2 still returns the same value as GETRANGE a 0 2.

Only the error name is command-dependent: StringInput still carries RespCommand.GETRANGE for both, since SUBSTR is a pure alias at the storage layer.

[hexonal (hexonal)]

Pushed a follow-up. Two things beyond the SUBSTR naming fix, both found by re-reviewing my own diff rather than by CI:

1. The arity check was wrong for SET itself. SET is declared with arity -3, but the fast parser only routes it to NetworkSETEXNX for array lengths 3..7 (RespCommand.cs, the >= ((3 << 4) | 3) and <= ((3 << 4) | 7) arm). Anything longer falls through to NetworkSET, where my parseState.Count != 2 turned it into a wrong-argument-count error. Measured against the patched build before the fix:

SET k v GET GET GET GET GET               -> $1\r\nv\r\n           (routed to the option parser, correct)
SET k v GET GET GET GET GET GET           -> -ERR wrong number of arguments for 'SET' command
SET k v GET GET GET GET GET GET GET EX 100 -> -ERR wrong number of arguments for 'SET' command

Redis accepts both of the rejected forms. Worse, the second one was silently dropping its EX and replying +OK on unpatched main in Release. NetworkSET now hands anything longer than two arguments to NetworkSETEXNX, which already parses the full option set; the check is Count < 2 for the genuine arity error. This also makes the handler agree with the arity that NetworkSKIP reads from RespCommandsInfo on the MULTI path — the two disagreed on the same input before.

2. GETWITHETAG and GETIFNOTMATCH still carried the bare assert. BasicEtagCommands.cs has Debug.Assert(parseState.Count == N) in those two handlers while every other handler in that same file validates at runtime. GETWITHETAG with no arguments aborts the process in Debug and reads an uninitialised parse-state slot in Release — the same defect this PR exists to remove, two doors down. Both are guarded now.

New assertions cover all of it, and I verified each one fails on the unpatched build before adding it. RespTests (353) and the ETag suite (86) green locally.

One deliberate divergence worth stating: the emitted names are uppercase ('SET', 'SUBSTR', 'GETWITHETAG') where Redis uses lowercase. That is Garnet's existing convention across the server and matches the RespCommandsInfo-driven MULTI-path error, so I kept it rather than making these seven handlers the odd ones out.