Additional validation around enum parsing (#622)

* Enum parsing is subtle, numeric values (even those that don't correspond to anything) will be accepted.  Default values should also be excluded, that's a leaked implementation detail.  Fixing ExpireOption handling in (P)EXPIRE

* Fix another set of enum parsing issues, this time for FAILOVER

* Add Enum constraint to enum parsing methods

* fix validation of FailoverOption in CLUSTER|FAILOVER

* fix enum validation (and some other incidental parsing bugs) around CLUSTER|SETSLOT

* fix enum validation around ACL rules

* all tests passing

* typo

libs/cluster/CmdStrings.cs

@@ -52,6 +52,7 @@ static class CmdStrings
         public static ReadOnlySpan<byte> RESP_ERR_INVALID_SLOT => "ERR Invalid or out of range slot"u8;
         public static ReadOnlySpan<byte> RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER => "ERR value is not an integer or out of range."u8;
         public static ReadOnlySpan<byte> RESP_ERR_GENERIC_VALUE_IS_NOT_BOOLEAN => "ERR value is not a boolean."u8;
+        public static ReadOnlySpan<byte> RESP_SYNTAX_ERROR => "ERR syntax error"u8;
 
         /// <summary>
         /// Generic error response strings for <c>MIGRATE</c> command

libs/cluster/Server/HashSlot.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+using System;
 using System.Runtime.InteropServices;
 
 namespace Garnet.cluster
@@ -40,6 +41,23 @@ public enum SlotState : byte
         INVALID,
     }
 
+    /// <summary>
+    /// Extension methods for <see cref="SlotState"/>.
+    /// </summary>
+    public static class SlotStateExtensions
+    {
+        /// <summary>
+        /// Validate that the given <see cref="SlotState"/> is legal, and _could_ have come from the given <see cref="ReadOnlySpan{T}"/>.
+        /// 
+        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
+        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
+        /// </summary>
+        public static bool IsValid(this SlotState type, ReadOnlySpan<byte> fromSpan)
+        {
+            return type != SlotState.INVALID && type != SlotState.OFFLINE && Enum.IsDefined(type) && !fromSpan.ContainsAnyInRange((byte)'0', (byte)'9');
+        }
+    }
+
     /// <summary>
     /// Hashslot info
     /// </summary>

libs/cluster/Session/FailoverCommand.cs

@@ -21,11 +21,15 @@ private bool TryFAILOVER()
             var currTokenIdx = 0;
             while (currTokenIdx < parseState.Count)
             {
-                if (!parseState.TryGetEnum(currTokenIdx++, true, out FailoverOption failoverOption))
-                    failoverOption = FailoverOption.INVALID;
+                if (!parseState.TryGetEnum(currTokenIdx, true, out FailoverOption failoverOption) || !failoverOption.IsValid(parseState.GetArgSliceByRef(currTokenIdx).ReadOnlySpan))
+                {
+                    while (!RespWriteUtils.WriteError(CmdStrings.RESP_SYNTAX_ERROR, ref dcurr, dend))
+                        SendAndReset();
+
+                    return true;
+                }
 
-                if (failoverOption == FailoverOption.INVALID)
-                    continue;
+                currTokenIdx++;
 
                 switch (failoverOption)
                 {

libs/cluster/Session/RespClusterFailoverCommands.cs

@@ -29,15 +29,16 @@ private bool NetworkClusterFailover(out bool invalidParameters)
             TimeSpan failoverTimeout = default;
             if (parseState.Count > 0)
             {
-                var failoverOptionStr = parseState.GetString(0);
-
                 // Try to parse failover option
-                if (!Enum.TryParse(failoverOptionStr, ignoreCase: true, out failoverOption))
+                if (!parseState.TryGetEnum(0, ignoreCase: true, out failoverOption) || !failoverOption.IsValid(parseState.GetArgSliceByRef(0).Span))
                 {
+                    var failoverOptionStr = parseState.GetString(0);
+
                     // On failure set the invalid flag, write error and continue parsing to drain rest of parameters if any
                     while (!RespWriteUtils.WriteError($"ERR Failover option ({failoverOptionStr}) not supported", ref dcurr, dend))
                         SendAndReset();
-                    failoverOption = FailoverOption.INVALID;
+
+                    return true;
                 }
 
                 if (parseState.Count > 1)
@@ -52,10 +53,6 @@ private bool NetworkClusterFailover(out bool invalidParameters)
                 }
             }
 
-            // If option provided is invalid return early
-            if (failoverOption == FailoverOption.INVALID)
-                return true;
-
             if (clusterProvider.serverOptions.EnableAOF)
             {
                 if (failoverOption == FailoverOption.ABORT)

libs/cluster/Session/RespClusterSlotManagementCommands.cs

@@ -439,17 +439,30 @@ private bool NetworkClusterSetSlot(out bool invalidParameters)
                 return true;
             }
 
-            var subcommand = parseState.GetString(1);
+            if (!parseState.TryGetEnum<SlotState>(1, ignoreCase: true, out var slotState) || !slotState.IsValid(parseState.GetArgSliceByRef(1).ReadOnlySpan))
+            {
+                var slotStateStr = parseState.GetString(1);
+                while (!RespWriteUtils.WriteError($"ERR Slot state {slotStateStr} not supported.", ref dcurr, dend))
+                    SendAndReset();
 
-            if (!Enum.TryParse(subcommand, ignoreCase: true, out SlotState slotState))
-                slotState = SlotState.INVALID;
+                return true;
+            }
 
             string nodeId = null;
             if (parseState.Count > 2)
             {
                 nodeId = parseState.GetString(2);
             }
 
+            // Check that node id is only provided for options other than STABLE
+            if ((slotState == SlotState.STABLE && nodeId is not null) || (slotState != SlotState.STABLE && nodeId is null))
+            {
+                while (!RespWriteUtils.WriteError(CmdStrings.RESP_SYNTAX_ERROR, ref dcurr, dend))
+                    SendAndReset();
+
+                return true;
+            }
+
             if (!ClusterConfig.OutOfRange(slot))
             {
                 // Try to set slot state
@@ -471,9 +484,7 @@ private bool NetworkClusterSetSlot(out bool invalidParameters)
                         setSlotsSucceeded = clusterProvider.clusterManager.TryPrepareSlotForOwnershipChange(slot, nodeId, out errorMessage);
                         break;
                     default:
-                        setSlotsSucceeded = false;
-                        errorMessage = Encoding.ASCII.GetBytes($"ERR Slot state {subcommand} not supported.");
-                        break;
+                        throw new InvalidOperationException($"Unexpected {nameof(SlotState)}: {slotState}");
                 }
 
                 if (setSlotsSucceeded)

libs/common/FailoverOption.cs

@@ -59,4 +59,21 @@ public static class FailoverUtils
         public static byte[] GetRespFormattedFailoverOption(FailoverOption failoverOption)
             => infoSections[(int)failoverOption];
     }
+
+    /// <summary>
+    /// Extension methods for <see cref="FailoverOption"/>.
+    /// </summary>
+    public static class FailoverOptionExtensions
+    {
+        /// <summary>
+        /// Validate that the given <see cref="FailoverOption"/> is legal, and _could_ have come from the given <see cref="ReadOnlySpan{T}"/>.
+        /// 
+        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
+        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
+        /// </summary>
+        public static bool IsValid(this FailoverOption type, ReadOnlySpan<byte> fromSpan)
+        {
+            return type != FailoverOption.DEFAULT && type != FailoverOption.INVALID && Enum.IsDefined(type) && !fromSpan.ContainsAnyInRange((byte)'0', (byte)'9');
+        }
+    }
 }

libs/server/ACL/ACLParser.cs

@@ -250,7 +250,7 @@ static bool TryParseCommandForAcl(string commandName, out RespCommand command)
 
                 string effectiveName = isSubCommand ? commandName[..subCommandSepIx] + "_" + commandName[(subCommandSepIx + 1)..] : commandName;
 
-                if (!Enum.TryParse(effectiveName, ignoreCase: true, out command))
+                if (!Enum.TryParse(effectiveName, ignoreCase: true, out command) || !IsValidParse(command, effectiveName))
                 {
                     // We handle these commands specially because blind replacements would cause
                     // us to be too accepting of different values
@@ -285,6 +285,15 @@ static bool TryParseCommandForAcl(string commandName, out RespCommand command)
                 return !IsInvalidCommandToAcl(command);
             }
 
+            // Returns true if the parsed value could possibly result in this command
+            //
+            // Used to handle the weirdness in Enum.TryParse - long term we probably
+            // shift to something like IUtf8SpanParsable.
+            static bool IsValidParse(RespCommand command, ReadOnlySpan<char> fromStr)
+            {
+                return command != RespCommand.NONE && command != RespCommand.INVALID && !fromStr.ContainsAnyInRange('0', '9');
+            }
+
             // Some commands aren't really commands, so ACLs shouldn't accept their names
             static bool IsInvalidCommandToAcl(RespCommand command)
             => command == RespCommand.INVALID || command == RespCommand.NONE || command.NormalizeForACLs() != command;

libs/server/ExpireOption.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+using System;
+
 namespace Garnet.server
 {
     /// <summary>
@@ -29,4 +31,21 @@ public enum ExpireOption : byte
         /// </summary>
         LT
     }
+
+    /// <summary>
+    /// Extension methods for <see cref="ExpireOption"/>.
+    /// </summary>
+    public static class ExpireOptionExtensions
+    {
+        /// <summary>
+        /// Validate that the given <see cref="ExpireOption"/> is legal, and _could_ have come from the given <see cref="ArgSlice"/>.
+        /// 
+        /// TODO: Long term we can kill this and use <see cref="IUtf8SpanParsable{ClientType}"/> instead of <see cref="Enum.TryParse{TEnum}(string?, bool, out TEnum)"/>
+        /// and avoid extra validation.  See: https://github.com/dotnet/runtime/issues/81500 .
+        /// </summary>
+        public static bool IsValid(this ExpireOption type, ref ArgSlice fromSlice)
+        {
+            return type != ExpireOption.None && Enum.IsDefined(type) && !fromSlice.ReadOnlySpan.ContainsAnyInRange((byte)'0', (byte)'9');
+        }
+    }
 }

libs/server/Resp/KeyAdminCommands.cs

@@ -134,23 +134,18 @@ private bool NetworkEXPIRE<TGarnetApi>(RespCommand command, ref TGarnetApi stora
                 ? TimeSpan.FromSeconds(expiryValue)
                 : TimeSpan.FromMilliseconds(expiryValue);
 
-            var invalidOption = false;
             var expireOption = ExpireOption.None;
-            var optionStr = "";
 
             if (parseState.Count > 2)
             {
-                if (!parseState.TryGetEnum(2, true, out expireOption))
+                if (!parseState.TryGetEnum(2, true, out expireOption) || !expireOption.IsValid(ref parseState.GetArgSliceByRef(2)))
                 {
-                    invalidOption = true;
-                }
-            }
+                    var optionStr = parseState.GetString(2);
 
-            if (invalidOption)
-            {
-                while (!RespWriteUtils.WriteError($"ERR Unsupported option {optionStr}", ref dcurr, dend))
-                    SendAndReset();
-                return true;
+                    while (!RespWriteUtils.WriteError($"ERR Unsupported option {optionStr}", ref dcurr, dend))
+                        SendAndReset();
+                    return true;
+                }
             }
 
             var status = command == RespCommand.EXPIRE ?

libs/server/Resp/Parser/SessionParseState.cs

@@ -211,7 +211,7 @@ public string GetString(int i)
         /// </summary>
         /// <returns>True if enum parsed successfully</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public T GetEnum<T>(int i, bool ignoreCase) where T : struct
+        public T GetEnum<T>(int i, bool ignoreCase) where T : struct, Enum
         {
             Debug.Assert(i < Count);
             return Enum.Parse<T>(GetString(i), ignoreCase);
@@ -222,7 +222,7 @@ public T GetEnum<T>(int i, bool ignoreCase) where T : struct
         /// </summary>
         /// <returns>True if integer parsed successfully</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public bool TryGetEnum<T>(int i, bool ignoreCase, out T value) where T : struct
+        public bool TryGetEnum<T>(int i, bool ignoreCase, out T value) where T : struct, Enum
         {
             Debug.Assert(i < Count);
             return Enum.TryParse(GetString(i), ignoreCase, out value);