* Fix exception on invalid offset. (#1023)

* Always check token is available before checking it.
* Separate BITFIELD and BITFIELD_RO parsing, simplifying both.
* Fix return type when not found.
* Add test.

Co-authored-by: prvyk <github@privatemail.fastmailbox.net>

libs/server/Resp/Bitmap/BitmapCommands.cs

@@ -1,7 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-using System;
 using System.Collections.Generic;
 using System.Runtime.InteropServices;
 using System.Text;
@@ -10,6 +9,8 @@
 
 namespace Garnet.server
 {
+    using SecondaryCommandList = List<(RespCommand, ArgSlice[])>;
+
     ///  (1) , (2) , (3) 
     /// overflow check, ptr protection, and status not found implemented for below
     /// GETBIT, SETBIT, BITCOUNT, BITPOS (1),(2)
@@ -359,7 +360,7 @@ private bool NetworkStringBitOperation<TGarnetApi>(BitmapOperation bitOp, ref TG
         /// <summary>
         /// Performs arbitrary bitfield integer operations on strings.
         /// </summary>
-        private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi, bool readOnly = false)
+        private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi)
             where TGarnetApi : IGarnetApi
         {
             if (parseState.Count < 1)
@@ -371,93 +372,68 @@ private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi, bool readOnly
             // Extract Key
             var sbKey = parseState.GetArgSliceByRef(0).SpanByte;
 
-            var currTokenIdx = 1;
-
             var isOverflowTypeSet = false;
             ArgSlice overflowTypeSlice = default;
-            var secondaryCommandArgs = new List<(RespCommand, ArgSlice[])>();
+            var secondaryCommandArgs = new SecondaryCommandList();
 
+            var currTokenIdx = 1;
             while (currTokenIdx < parseState.Count)
             {
                 // Get subcommand
                 var commandSlice = parseState.GetArgSliceByRef(currTokenIdx++);
                 var command = commandSlice.ReadOnlySpan;
 
                 // Process overflow command
-                if (!readOnly && command.EqualsUpperCaseSpanIgnoringCase("OVERFLOW"u8))
+                if (command.EqualsUpperCaseSpanIgnoringCase("OVERFLOW"u8))
                 {
-                    // Get overflow parameter
-                    overflowTypeSlice = parseState.GetArgSliceByRef(currTokenIdx);
-                    isOverflowTypeSet = true;
-
                     // Validate overflow type
-                    if (!parseState.TryGetBitFieldOverflow(currTokenIdx, out _))
+                    if (currTokenIdx >= parseState.Count || !parseState.TryGetBitFieldOverflow(currTokenIdx, out _))
                     {
-                        while (!RespWriteUtils.TryWriteError(
-                                   $"ERR Overflow type {parseState.GetString(currTokenIdx)} not supported",
-                                   ref dcurr, dend))
+                        while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_INVALID_OVERFLOW_TYPE, ref dcurr, dend))
                             SendAndReset();
                         return true;
                     }
 
-                    currTokenIdx++;
+                    // Get overflow parameter
+                    overflowTypeSlice = parseState.GetArgSliceByRef(currTokenIdx++);
+                    isOverflowTypeSet = true;
 
                     continue;
                 }
 
                 // [GET <encoding> <offset>] [SET <encoding> <offset> <value>] [INCRBY <encoding> <offset> <increment>]
                 // Process encoding argument
-                var encodingSlice = parseState.GetArgSliceByRef(currTokenIdx);
-                var offsetSlice = parseState.GetArgSliceByRef(currTokenIdx + 1);
-                var encodingArg = parseState.GetString(currTokenIdx);
-                var offsetArg = parseState.GetString(currTokenIdx + 1);
-                currTokenIdx += 2;
-
-                // Validate encoding
-                if (encodingArg.Length < 2 ||
-                    (encodingArg[0] != 'i' && encodingArg[0] != 'u') ||
-                    !int.TryParse(encodingArg.AsSpan(1), out var bitCount) ||
-                    bitCount > 64 ||
-                    (bitCount == 64 && encodingArg[0] == 'u'))
+                if ((currTokenIdx >= parseState.Count) || !parseState.TryGetBitfieldEncoding(currTokenIdx, out _, out _))
                 {
                     while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_INVALID_BITFIELD_TYPE, ref dcurr,
                                dend))
                         SendAndReset();
                     return true;
                 }
+                var encodingSlice = parseState.GetArgSliceByRef(currTokenIdx++);
 
-                // Validate offset
-                var isOffsetValid = offsetArg[0] == '#'
-                    ? long.TryParse(offsetArg.AsSpan(1), out _)
-                    : long.TryParse(offsetArg, out _);
-
-                if (!isOffsetValid)
+                // Process offset argument
+                if ((currTokenIdx >= parseState.Count) || !parseState.TryGetBitfieldOffset(currTokenIdx, out _, out _))
                 {
                     while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_GENERIC_BITOFFSET_IS_NOT_INTEGER, ref dcurr,
                                dend))
                         SendAndReset();
                     return true;
                 }
+                var offsetSlice = parseState.GetArgSliceByRef(currTokenIdx++);
 
-                // Subcommand takes 2 args, encoding and offset
-                if (command.EqualsUpperCaseSpanIgnoringCase("GET"u8))
+                // GET Subcommand takes 2 args, encoding and offset
+                if (command.EqualsUpperCaseSpanIgnoringCase(CmdStrings.GET))
                 {
                     secondaryCommandArgs.Add((RespCommand.GET, [commandSlice, encodingSlice, offsetSlice]));
                 }
                 else
                 {
-                    if (readOnly)
-                    {
-                        while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_SYNTAX_ERROR, ref dcurr, dend))
-                            SendAndReset();
-                        return true;
-                    }
-
                     RespCommand op;
                     // SET and INCRBY take 3 args, encoding, offset, and valueArg
-                    if (command.EqualsUpperCaseSpanIgnoringCase("SET"u8))
+                    if (command.EqualsUpperCaseSpanIgnoringCase(CmdStrings.SET))
                         op = RespCommand.SET;
-                    else if (command.EqualsUpperCaseSpanIgnoringCase("INCRBY"u8))
+                    else if (command.EqualsUpperCaseSpanIgnoringCase(CmdStrings.INCRBY))
                         op = RespCommand.INCRBY;
                     else
                     {
@@ -469,24 +445,96 @@ private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi, bool readOnly
                     }
 
                     // Validate value
-                    var valueSlice = parseState.GetArgSliceByRef(currTokenIdx);
-                    if (!parseState.TryGetLong(currTokenIdx, out _))
+                    if (currTokenIdx >= parseState.Count || !parseState.TryGetLong(currTokenIdx, out _))
                     {
                         while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_GENERIC_VALUE_IS_NOT_INTEGER, ref dcurr,
                                    dend))
                             SendAndReset();
                         return true;
                     }
+                    var valueSlice = parseState.GetArgSliceByRef(currTokenIdx);
                     currTokenIdx++;
 
                     secondaryCommandArgs.Add((op, [commandSlice, encodingSlice, offsetSlice, valueSlice]));
                 }
             }
 
+            return StringBitFieldAction(ref storageApi, ref sbKey, RespCommand.BITFIELD,
+                                     secondaryCommandArgs, isOverflowTypeSet, overflowTypeSlice);
+        }
+
+        /// <summary>
+        /// Performs arbitrary read-only bitfield integer operations
+        /// </summary>
+        private bool StringBitFieldReadOnly<TGarnetApi>(ref TGarnetApi storageApi)
+            where TGarnetApi : IGarnetApi
+        {
+            if (parseState.Count < 1)
+            {
+                return AbortWithWrongNumberOfArguments(nameof(RespCommand.BITFIELD_RO));
+            }
+
+            // BITFIELD_RO key [GET encoding offset [GET encoding offset] ... ]
+            // Extract Key
+            var sbKey = parseState.GetArgSliceByRef(0).SpanByte;
+
+            var secondaryCommandArgs = new SecondaryCommandList();
+
+            var currTokenIdx = 1;
+            while (currTokenIdx < parseState.Count)
+            {
+                // Get subcommand
+                var commandSlice = parseState.GetArgSliceByRef(currTokenIdx++);
+                var command = commandSlice.ReadOnlySpan;
+
+                // Read-only variant supports only GET subcommand
+                if (!command.EqualsUpperCaseSpanIgnoringCase(CmdStrings.GET))
+                {
+                    while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_SYNTAX_ERROR, ref dcurr, dend))
+                        SendAndReset();
+                    return true;
+                }
+
+                // GET Subcommand takes 2 args, encoding and offset
+
+                // Process encoding argument
+                if ((currTokenIdx >= parseState.Count) || !parseState.TryGetBitfieldEncoding(currTokenIdx, out _, out _))
+                {
+                    while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_INVALID_BITFIELD_TYPE, ref dcurr,
+                               dend))
+                        SendAndReset();
+                    return true;
+                }
+                var encodingSlice = parseState.GetArgSliceByRef(currTokenIdx++);
+
+                // Process offset argument
+                if ((currTokenIdx >= parseState.Count) || !parseState.TryGetBitfieldOffset(currTokenIdx, out _, out _))
+                {
+                    while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_GENERIC_BITOFFSET_IS_NOT_INTEGER, ref dcurr,
+                               dend))
+                        SendAndReset();
+                    return true;
+                }
+                var offsetSlice = parseState.GetArgSliceByRef(currTokenIdx++);
+
+                secondaryCommandArgs.Add((RespCommand.GET, [commandSlice, encodingSlice, offsetSlice]));
+            }
+
+            return StringBitFieldAction(ref storageApi, ref sbKey, RespCommand.BITFIELD_RO, secondaryCommandArgs);
+        }
+
+        private bool StringBitFieldAction<TGarnetApi>(ref TGarnetApi storageApi,
+                                                      ref SpanByte sbKey,
+                                                      RespCommand cmd,
+                                                      SecondaryCommandList secondaryCommandArgs,
+                                                      bool isOverflowTypeSet = false,
+                                                      ArgSlice overflowTypeSlice = default)
+            where TGarnetApi : IGarnetApi
+        {
             while (!RespWriteUtils.TryWriteArrayLength(secondaryCommandArgs.Count, ref dcurr, dend))
                 SendAndReset();
 
-            var input = new RawStringInput(RespCommand.BITFIELD);
+            var input = new RawStringInput(cmd);
 
             for (var i = 0; i < secondaryCommandArgs.Count; i++)
             {
@@ -512,7 +560,7 @@ private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi, bool readOnly
 
                 if (status == GarnetStatus.NOTFOUND && opCode == RespCommand.GET)
                 {
-                    while (!RespWriteUtils.TryWriteArrayItem(0, ref dcurr, dend))
+                    while (!RespWriteUtils.TryWriteInt32(0, ref dcurr, dend))
                         SendAndReset();
                 }
                 else
@@ -526,15 +574,5 @@ private bool StringBitField<TGarnetApi>(ref TGarnetApi storageApi, bool readOnly
 
             return true;
         }
-
-        /// <summary>
-        /// Performs arbitrary read-only bitfield integer operations
-        /// </summary>
-        private bool StringBitFieldReadOnly<TGarnetApi>(ref TGarnetApi storageApi)
-            where TGarnetApi : IGarnetApi
-        {
-            // BITFIELD_RO key [GET encoding offset [GET encoding offset] ... ]
-            return StringBitField(ref storageApi, true);
-        }
     }
 }

libs/server/Resp/Bitmap/BitmapManagerBitfield.cs

@@ -441,5 +441,24 @@ public static (long, bool) BitFieldExecute(BitFieldCmdArgs args, byte* value, in
                 _ => throw new GarnetException("BITFIELD secondary op not supported"),
             };
         }
+
+        /// <summary>
+        /// Execute readonly bitfield operation described at input on bitmap stored within value.
+        /// </summary>
+        /// <param name="args"></param>
+        /// <param name="value"></param>
+        /// <param name="valLen"></param>
+        /// <returns></returns>
+        public static long BitFieldExecute_RO(BitFieldCmdArgs args, byte* value, int valLen)
+        {
+            var bitCount = (byte)(args.typeInfo & 0x7F);
+            var signed = (args.typeInfo & (byte)BitFieldSign.SIGNED) > 0;
+
+            return args.secondaryCommand switch
+            {
+                RespCommand.GET => GetBitfield(value, valLen, args.offset, bitCount, signed),
+                _ => throw new GarnetException("BITFIELD secondary op not supported"),
+            };
+        }
     }
 }

libs/server/Resp/CmdStrings.cs

@@ -148,6 +148,7 @@ static partial class CmdStrings
         public static ReadOnlySpan<byte> FIELDS => "FIELDS"u8;
         public static ReadOnlySpan<byte> TIMEOUT => "TIMEOUT"u8;
         public static ReadOnlySpan<byte> ERROR => "ERROR"u8;
+        public static ReadOnlySpan<byte> INCRBY => "INCRBY"u8;
         public static ReadOnlySpan<byte> NOGET => "NOGET"u8;
 
         /// <summary>
@@ -244,6 +245,7 @@ static partial class CmdStrings
         public static ReadOnlySpan<byte> RESP_ERR_GT_LT_NX_NOT_COMPATIBLE => "ERR GT, LT, and/or NX options at the same time are not compatible"u8;
         public static ReadOnlySpan<byte> RESP_ERR_INCR_SUPPORTS_ONLY_SINGLE_PAIR => "ERR INCR option supports a single increment-element pair"u8;
         public static ReadOnlySpan<byte> RESP_ERR_INVALID_BITFIELD_TYPE => "ERR Invalid bitfield type. Use something like i16 u8. Note that u64 is not supported but i64 is"u8;
+        public static ReadOnlySpan<byte> RESP_ERR_INVALID_OVERFLOW_TYPE => "ERR Invalid OVERFLOW type specified"u8;
         public static ReadOnlySpan<byte> RESP_ERR_SCRIPT_FLUSH_OPTIONS => "ERR SCRIPT FLUSH only support SYNC|ASYNC option"u8;
         public static ReadOnlySpan<byte> RESP_ERR_BUSSYKEY => "BUSYKEY Target key name already exists."u8;
         public static ReadOnlySpan<byte> RESP_ERR_LENGTH_AND_INDEXES => "If you want both the length and indexes, please just use IDX."u8;

libs/server/SessionParseStateExtensions.cs

@@ -139,6 +139,84 @@ internal static bool TryGetBitFieldOverflow(this SessionParseState parseState, i
             return true;
         }
 
+        /// <summary>
+        /// Parse bit field ENCODING slice from parse state at specified index
+        /// </summary>
+        /// <param name="parseState">The parse state</param>
+        /// <param name="idx">The argument index</param>
+        /// <param name="bitCount">parsed bitcount</param>
+        /// <param name="isSigned">bitfield signtype</param>
+        /// <returns></returns>
+        internal static unsafe bool TryGetBitfieldEncoding(this SessionParseState parseState, int idx, out long bitCount, out bool isSigned)
+        {
+            bitCount = default;
+            isSigned = default;
+            var encodingSlice = parseState.GetArgSliceByRef(idx);
+
+            if (encodingSlice.length <= 1)
+            {
+                return false;
+            }
+
+            var ptr = encodingSlice.ptr + 1;
+
+            isSigned = *encodingSlice.ptr == 'i';
+
+            if (!isSigned && *encodingSlice.ptr != 'u')
+            {
+                return false;
+            }
+
+            return
+                RespReadUtils.TryReadInt64Safe(ref ptr, encodingSlice.ptr + encodingSlice.length,
+                                           out bitCount, out var bytesRead,
+                                           out _, out _, allowLeadingZeros: false) &&
+                ((int)bytesRead == encodingSlice.length - 1) && (bytesRead > 0L) &&
+                (bitCount > 0) &&
+                ((isSigned && bitCount <= 64) ||
+                 (!isSigned && bitCount < 64));
+        }
+
+        /// <summary>
+        /// Parse bit field OFFSET slice from parse state at specified index
+        /// </summary>
+        /// <param name="parseState">The parse state</param>
+        /// <param name="idx">The argument index</param>
+        /// <param name="bitFieldOffset">parsed value</param>
+        /// <param name="multiplyOffset">should value by multiplied by bitcount</param>
+        /// <returns></returns>
+        internal static unsafe bool TryGetBitfieldOffset(this SessionParseState parseState, int idx, out long bitFieldOffset, out bool multiplyOffset)
+        {
+            bitFieldOffset = default;
+            multiplyOffset = default;
+            var offsetSlice = parseState.GetArgSliceByRef(idx);
+
+            if (offsetSlice.Length <= 0)
+            {
+                return false;
+            }
+
+            var ptr = offsetSlice.ptr;
+            var len = offsetSlice.length;
+
+            if (*ptr == '#')
+            {
+                if (offsetSlice.length == 1)
+                    return false;
+
+                multiplyOffset = true;
+                ptr++;
+                len--;
+            }
+
+            return
+                RespReadUtils.TryReadInt64Safe(ref ptr, offsetSlice.ptr + offsetSlice.length,
+                                           out bitFieldOffset, out var bytesRead,
+                                           out _, out _, allowLeadingZeros: false) &&
+                ((int)bytesRead == len) && (bytesRead > 0L) &&
+                (bitFieldOffset >= 0);
+        }
+
         /// <summary>
         /// Parse manager type from parse state at specified index
         /// </summary>