Validate numkeys in SINTERCARD, ZINTERSTORE and LMPOP/BLMPOP

[hexonal (hexonal)]

Symptom

Against a server built from main (8b329e3), three commands close the client connection instead of replying:

$ redis-cli -p 3278
127.0.0.1:3278> SINTERCARD -1 key1
Error: Server closed the connection
127.0.0.1:3278> ZINTERSTORE dest -1 zset1
Error: Server closed the connection
127.0.0.1:3278> LMPOP -1 a b
Error: Server closed the connection

Each throws out of its command handler, so the session is torn down and nothing at all is written to the socket. A pipelining client does not see an error, it sees its remaining replies shift by one. Expected in all three cases is an error reply on a connection that stays open.

Three more inputs do not throw but answer something other than an error:

command	on main	expected
SINTERCARD 0 LIMIT 0	:0	-ERR numkeys should be greater than 0
LMPOP 0 LEFT COUNT 5	*-1	-ERR numkeys should be greater than 0
ZINTERSTORE dest 0 zset1	-ERR syntax error	-ERR at least 1 input key is needed for 'ZINTERSTORE' command

And ZINTERSTORE dest 2 zset1 — a numkeys larger than the number of keys actually sent — also drops the connection, where ZUNIONSTORE dest 2 zset1 already replies -ERR syntax error.

Root cause

Each handler parses numkeys and uses it directly as a span or array length with no lower bound:

• libs/server/Resp/Objects/SetCommands.cs:178 — parseState.Parameters.Slice(1, nKeys)
• libs/server/Resp/Objects/SortedSetCommands.cs:1248 — parseState.Parameters.Slice(2, nKeys)
• libs/server/Resp/Objects/ListCommands.cs:208 — new PinnedSpanByte[numKeys]
• libs/server/Resp/Objects/ListCommands.cs:878 — new byte[numKeys][]

The arity checks above those lines compare against numkeys itself and pass vacuously for negative values: SINTERCARD's (parseState.Count - 1) < nKeys is false for any negative nKeys, and LMPOP's parseState.Count != numKeys + 2 && parseState.Count != numKeys + 4 happens to match on the shapes above (LMPOP -1 a b has Count == 3 == numKeys + 4). Slice then throws ArgumentOutOfRangeException, new T[negative] throws OverflowException.

ZINTERSTORE additionally has no check that the argument count covers numkeys, which is what lets ZINTERSTORE dest 2 zset1 slice past the end of the parse state.

Fix

Add the missing numkeys < 1 guard to each handler, reusing the error string that handler already emits:

• SINTERCARD — ERR numkeys should be greater than 0, the same string as the (parseState.Count - 1) < nKeys check five lines below it, and the same string Redis's sinterCardCommand emits.
• ZINTERSTORE — GenericErrAtLeastOneKey, plus the argument-count check its SortedSetUnionStore sibling carries at libs/server/Resp/Objects/SortedSetCommands.cs:1464-1471. The count check is written as parseState.Count - 2 < nKeys rather than parseState.Count < nKeys + 2: the two are equivalent for every value that does not overflow, but the addition form wraps negative for numkeys >= 2147483646 and lets it through. parseState.Count is at least 3 here, so the subtraction cannot underflow.
• LMPOP / BLMPOP — folded into the existing TryGetInt condition, the same shape used for the COUNT guard in these two handlers, so each keeps the wording it already uses (LMPOP ERR numkeys should be greater than 0, BMPOP ERR Parameter `numkeys` should be greater than 0). No existing message is reworded.

Happy paths are unchanged: SINTERCARD 1 s, SINTERCARD 1 s LIMIT 2, ZINTERSTORE d 1 z and LMPOP 1 L LEFT behave as before, and every existing CanDoZInterStore case — including ZINTERSTORE dest 3 zset1 zset2 nx — still passes both new ZINTERSTORE guards.

Tests

Eleven new assertions, all of which fail on main today. None is a green-state regression guard.

CanDoSinterCardThowsErrors (RespSetTest.cs)

• SINTERCARD -1 key1 — connection dropped on main
• SINTERCARD 0 LIMIT 0 — replies :0 on main

CanDoRejectBadLMPOPCommand (RespListTests.cs)

• LMPOP -1 a b — connection dropped on main
• LMPOP 0 LEFT COUNT 5 — replies *-1 on main
• BLMPOP 0 -1 a b — connection dropped on main

CanDoZInterStoreWithBadNumKeysLC (RespSortedSetTests.cs, new)

• ZINTERSTORE dest -1 zset1 — connection dropped on main
• ZINTERSTORE dest 0 zset1 — replies -ERR syntax error on main
• ZINTERSTORE dest 2 zset1 — connection dropped on main
• ZINTERSTORE dest 3 zset1 zset2 — connection dropped on main
• ZINTERSTORE dest 2147483647 zset1 — connection dropped on main
• ZINTERSTORE dest 2147483646 zset1 — connection dropped on main

The last two are the overflow cases, added after review feedback on this PR: with the addition form of
the count check they slipped past the guard and still reached the slice. 2147483645 is the largest
numkeys that does not overflow and is handled correctly either way.

These use LightClientRequest rather than StackExchange.Redis on purpose: the pre-fix failure is an absent reply on a dropped socket, which a StackExchange.Redis assertion cannot tell apart from a normal error. Each command is pipelined with a following PING so the assertion also proves the session survives and the reply stream stays aligned.

Out of scope

The additive form of this argument-count check predates this PR and appears in other handlers in the
same family on main. This PR only changes the check it introduces; auditing the rest belongs in a
separate change, which I am happy to prepare. SINTERCARD and LMPOP/BLMPOP are unaffected either way —
their bound checks do not add to numkeys.

[Copilot]

Pull request overview

This PR hardens RESP object command handlers against invalid numkeys inputs that previously caused unhandled exceptions and connection teardown (no error reply written), bringing behavior in line with Redis by returning explicit error replies while keeping the session alive.

Changes:

• Add numkeys < 1 validation to SINTERCARD, ZINTERSTORE, and LMPOP/BLMPOP handlers to prevent negative/zero values from reaching slicing/allocation paths.
• Add an argument-count bound check for ZINTERSTORE to prevent slicing beyond provided keys.
• Add LightClient-based regression tests that assert (a) correct error replies and (b) the connection remains usable (reply stream stays aligned via a pipelined PING).

Reviewed changes

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

Show a summary per file

File	Description
test/standalone/Garnet.test.collections/RespSortedSetTests.cs	Adds LightClient test coverage for invalid ZINTERSTORE numkeys scenarios and ensures the session stays open.
test/standalone/Garnet.test.collections/RespSetTest.cs	Adds LightClient assertions for SINTERCARD invalid numkeys cases that previously dropped the connection or returned non-error replies.
test/standalone/Garnet.test.collections/RespListTests.cs	Adds LightClient assertions for LMPOP/BLMPOP invalid numkeys cases and validates session survivability.
libs/server/Resp/Objects/SortedSetCommands.cs	Adds ZINTERSTORE numkeys lower-bound and argument-count validation to avoid handler exceptions and improve error behavior.
libs/server/Resp/Objects/SetCommands.cs	Adds SINTERCARD numkeys < 1 guard to avoid out-of-range slicing and connection teardown.
libs/server/Resp/Objects/ListCommands.cs	Adds numkeys < 1 validation in LMPOP and BLMPOP handlers to avoid negative-length allocations and align error replies with Redis.

[hexonal (hexonal)]

The single red check here is Garnet Standalone (ubuntu-latest, net8.0, Debug, Garnet.test.vectorset) (job). I don't think it's caused by this PR. Evidence rather than an assertion:

The same job fails on main without this diff.

• main @ c962980e ("Bump version to 2.1.2 (Bump Garnet version to 2.1.2 #2024)", whose entire diff is Version.props +1/-1) — run 31051128856 — failed this exact job on VectorSetOverwriteTests(False,True).SETAsync, the same test and the same fixture parameters as here, with RedisTimeoutException : Timeout awaiting response (... 30616ms elapsed, timeout is 30000ms), command=SAVE. This PR's log has that same line at 30086ms.
• main @ 8b329e30, this PR's base commit — run 31065050333 — also failed this job, on two different vectorset tests (VADDErrors, VADDLowMemoryStorageTierForcesDiskSpill).
• MultiLog replication: drift-barrier throttling, inline tail-witness pulses, and AOF shipping backpressure #2003 currently has the same red job, failing both of the tests that fail here plus two more.

Both failures in this run are transport-level, not assertion diffs. RedisConnectionException : SocketClosed in InterruptedVectorSetDelete_BeforeMark (a deliberate fault-injection test), and the command=SAVE timeout above, over budget by 86 ms on 30 s. The job log is complete — it ends with the Total tests: 389 / Passed: 387 / Failed: 2 summary and the runner's cleanup line — and over that complete log Expected:, But was:, Assert.That and ClassicAssert each occur zero times. A numkeys-validation regression would surface as a wrong error string, i.e. an assertion diff.

Both failing tests pass in sibling fixtures in this same run. InterruptedVectorSetDelete_BeforeMark passes under (0) [34 ms] and fails under (1000). SETAsync passes under (False,False) [83 ms], (True,False) [118 ms] and (True,True) [55 s], and fails only under (False,True). (True,True) exercises the same EvictToDisk foreground-SAVE path and passed at 55 s, so that path is sitting close to the client's 30 s per-command deadline either way.

The diff can't reach these tests. It only adds lower-bound numkeys guards to SINTERCARD, ZINTERSTORE and LMPOP/BLMPOP. The one test in the 389-test vectorset suite that issues any of those is VectorSetWrongTypeTests.ZINTERSTOREAsync, which sends numkeys=1 (unaffected by a < 1 guard) and passed [42 ms] in this very job.

What I did not prove. I could not reproduce the CI failure locally. This machine has no net8.0 runtime (10.0.x only) and the red job is net8.0/Debug/ubuntu, so nothing I ran locally clears that job; the vectorset suite doesn't run here at all, on main or on this branch. I also have no root cause for the underlying instability beyond the timings above.

I don't have permission to re-run the job. A re-run of just that one job should settle it. I'd rather not push an empty commit, since that re-triggers the whole matrix and the flake looks frequent enough that it could recur anyway. Happy to open a separate issue for the two flaky tests if that's useful.