Validate argument count in SET-family string commands

SET, GETSET, SETEX/PSETEX, SETRANGE, APPEND and GETRANGE/SUBSTR only
Debug.Assert(parseState.Count == N) their argument count (or consumed a
missing EX/PX option value) instead of validating it at runtime. Malformed
input such as 'SET k', 'SETEX k 10', 'SETRANGE k', 'APPEND k', 'GETRANGE k'
or 'SET k v EX' therefore aborted the process via the assert in a Debug/CI
build, and read an out-of-bounds parse-state slot in Release, instead of
returning an error - dropping the connection and every command pipelined
after it.

Validate the argument count in each handler (and, for SET's EX/PX option,
that a value token follows before consuming it), matching what the
object-store handlers and NetworkSETWITHETAG already do. Well-formed
commands are unchanged; the wire errors match Redis.

Author: hexonal

libs/server/Resp/BasicCommands.cs

@@ -351,7 +351,9 @@ bool NetworkGET_SG<TGarnetApi>(ref TGarnetApi storageApi)
         private bool NetworkSET<TGarnetApi>(ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
-            Debug.Assert(parseState.Count == 2);
+            if (parseState.Count != 2)
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.SET));
+
             var key = parseState.GetArgSliceByRef(0);
             var value = parseState.GetArgSliceByRef(1);
 
@@ -379,7 +381,9 @@ private bool NetworkSET<TGarnetApi>(ref TGarnetApi storageApi)
         private bool NetworkGETSET<TGarnetApi>(ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
-            Debug.Assert(parseState.Count == 2);
+            if (parseState.Count != 2)
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.GETSET));
+
             var key = parseState.GetArgSliceByRef(0);
 
             return NetworkSET_Conditional(RespCommand.SET, 0, key, getValue: true, highPrecision: false,
@@ -392,6 +396,9 @@ private bool NetworkGETSET<TGarnetApi>(ref TGarnetApi storageApi)
         private bool NetworkSetRange<TGarnetApi>(ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
+            if (parseState.Count != 3)
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.SETRANGE));
+
             var key = parseState.GetArgSliceByRef(0);
 
             // Validate offset
@@ -433,9 +440,14 @@ private bool NetworkSetRange<TGarnetApi>(ref TGarnetApi storageApi)
             return true;
         }
 
-        private bool NetworkGetRange<TGarnetApi>(ref TGarnetApi storageApi)
+        private bool NetworkGetRange<TGarnetApi>(RespCommand cmd, ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
+            Debug.Assert(cmd is RespCommand.GETRANGE or RespCommand.SUBSTR);
+
+            if (parseState.Count != 3)
+                return AbortWithWrongNumberOfArguments(cmd.ToString());
+
             var key = parseState.GetArgSliceByRef(0);
 
             // Validate range
@@ -476,6 +488,9 @@ private bool NetworkGetRange<TGarnetApi>(ref TGarnetApi storageApi)
         private bool NetworkSETEX<TGarnetApi>(bool highPrecision, ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
+            if (parseState.Count != 3)
+                return AbortWithWrongNumberOfArguments(highPrecision ? nameof(RespCommand.PSETEX) : nameof(RespCommand.SETEX));
+
             var key = parseState.GetArgSliceByRef(0);
 
             // Validate expiry
@@ -579,7 +594,17 @@ private bool NetworkSETEXNX<TGarnetApi>(ref TGarnetApi storageApi)
                     // based on above check if it is not KEEPTTL, it has to be either EX or PX
                     if (expOption != ExpirationOption.KEEPTTL)
                     {
-                        // EX and PX optionare followed by an expiry argument; account for the expiry argument by moving past the tokenIdx
+                        // EX and PX are followed by an expiry argument. Reject the option
+                        // appearing as the final token before consuming it, otherwise the
+                        // read runs past the parse state (a Debug.Assert failure that aborts
+                        // the process, an out-of-bounds read in Release).
+                        if (tokenIdx >= parseState.Count)
+                        {
+                            errorMessage = CmdStrings.RESP_ERR_GENERIC_SYNTAX_ERROR;
+                            break;
+                        }
+
+                        // account for the expiry argument by moving past the tokenIdx
                         if (!parseState.TryGetInt(tokenIdx++, out expiry))
                         {
                             errorMessage = CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER;
@@ -889,6 +914,9 @@ private bool NetworkIncrementByFloat<TGarnetApi>(ref TGarnetApi storageApi)
         private bool NetworkAppend<TGarnetApi>(ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
+            if (parseState.Count != 2)
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.APPEND));
+
             var sbKey = parseState.GetArgSliceByRef(0);
 
             var input = new StringInput(RespCommand.APPEND, ref parseState, startIdx: 1);

libs/server/Resp/RespServerSession.cs

@@ -832,8 +832,8 @@ private bool ProcessBasicCommands<TGarnetApi>(RespCommand cmd, ref TGarnetApi st
                 RespCommand.EXPIRETIME => NetworkEXPIRETIME(RespCommand.EXPIRETIME, ref storageApi),
                 RespCommand.PEXPIRETIME => NetworkEXPIRETIME(RespCommand.PEXPIRETIME, ref storageApi),
                 RespCommand.PERSIST => NetworkPERSIST(ref storageApi),
-                RespCommand.GETRANGE => NetworkGetRange(ref storageApi),
-                RespCommand.SUBSTR => NetworkGetRange(ref storageApi),
+                RespCommand.GETRANGE => NetworkGetRange(RespCommand.GETRANGE, ref storageApi),
+                RespCommand.SUBSTR => NetworkGetRange(RespCommand.SUBSTR, ref storageApi),
                 RespCommand.TTL => NetworkTTL(RespCommand.TTL, ref storageApi),
                 RespCommand.PTTL => NetworkTTL(RespCommand.PTTL, ref storageApi),
                 RespCommand.SETRANGE => NetworkSetRange(ref storageApi),

test/standalone/Garnet.test/RespTests.cs

@@ -600,6 +600,62 @@ public void MultiSetNX()
             TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
         }
 
+        [Test]
+        public void StringCommandsWrongArityReturnErrorAndKeepSessionAlive()
+        {
+            using var lightClientRequest = TestUtils.CreateRequest();
+
+            // Each malformed command is followed by PING. On unpatched main these
+            // reach a handler that only Debug.Assert()s the argument count (or reads
+            // the missing argument past the parse state), aborting the process in a
+            // Debug build and reading out of bounds in Release, instead of replying
+            // with an error. The trailing PONG proves the session stayed alive.
+            void ExpectError(string cmd, string expectedError)
+            {
+                var response = lightClientRequest.SendCommands(cmd, "PING");
+                TestUtils.AssertEqualUpToExpectedLength($"{expectedError}\r\n+PONG\r\n", response);
+            }
+
+            ExpectError("SET k", "-ERR wrong number of arguments for 'SET' command");
+            ExpectError("SET", "-ERR wrong number of arguments for 'SET' command");
+            ExpectError("SET k v EX", "-ERR syntax error");
+            ExpectError("SET k v PX", "-ERR syntax error");
+            ExpectError("GETSET k", "-ERR wrong number of arguments for 'GETSET' command");
+            ExpectError("GETSET k v extra", "-ERR wrong number of arguments for 'GETSET' command");
+            ExpectError("SETEX k", "-ERR wrong number of arguments for 'SETEX' command");
+            ExpectError("SETEX k 10", "-ERR wrong number of arguments for 'SETEX' command");
+            ExpectError("PSETEX k 10", "-ERR wrong number of arguments for 'PSETEX' command");
+            ExpectError("SETRANGE k", "-ERR wrong number of arguments for 'SETRANGE' command");
+            ExpectError("SETRANGE k 0", "-ERR wrong number of arguments for 'SETRANGE' command");
+            ExpectError("APPEND k", "-ERR wrong number of arguments for 'APPEND' command");
+            ExpectError("GETRANGE k", "-ERR wrong number of arguments for 'GETRANGE' command");
+            ExpectError("GETRANGE k 0", "-ERR wrong number of arguments for 'GETRANGE' command");
+            ExpectError("SUBSTR k", "-ERR wrong number of arguments for 'SUBSTR' command");
+            ExpectError("SUBSTR k 0", "-ERR wrong number of arguments for 'SUBSTR' command");
+
+            // Well-formed forms are unchanged.
+            void ExpectOk(string cmd)
+            {
+                var response = lightClientRequest.SendCommands(cmd, "PING");
+                TestUtils.AssertEqualUpToExpectedLength("+OK\r\n+PONG\r\n", response);
+            }
+
+            ExpectOk("SET a 1");
+            ExpectOk("SETEX b 100 v");
+            ExpectOk("PSETEX c 5000 v");
+
+            var response = lightClientRequest.SendCommands("SETRANGE a 1 XY", "PING");
+            TestUtils.AssertEqualUpToExpectedLength(":3\r\n+PONG\r\n", response);
+            response = lightClientRequest.SendCommands("APPEND a Z", "PING");
+            TestUtils.AssertEqualUpToExpectedLength(":4\r\n+PONG\r\n", response);
+            response = lightClientRequest.SendCommands("GETSET a fresh", "PING");
+            TestUtils.AssertEqualUpToExpectedLength("$4\r\n1XYZ\r\n+PONG\r\n", response);
+            response = lightClientRequest.SendCommands("GETRANGE a 0 2", "PING");
+            TestUtils.AssertEqualUpToExpectedLength("$3\r\nfre\r\n+PONG\r\n", response);
+            response = lightClientRequest.SendCommands("SUBSTR a 0 2", "PING");
+            TestUtils.AssertEqualUpToExpectedLength("$3\r\nfre\r\n+PONG\r\n", response);
+        }
+
         [Test]
         public void LargeSetGet()
         {