From aee4db1ba70b35d2b6a95d4d525b23d931b4197b Mon Sep 17 00:00:00 2001 From: Lukas Maas Date: Fri, 22 Mar 2024 18:56:08 -0700 Subject: [PATCH 1/2] Fixes #argument error handling and ranges for object store commands --- libs/server/Resp/Objects/HashCommands.cs | 25 +++---- libs/server/Resp/Objects/ListCommands.cs | 42 ++--------- libs/server/Resp/Objects/ObjectStoreUtils.cs | 36 ++++++---- libs/server/Resp/Objects/SetCommands.cs | 6 +- libs/server/Resp/Objects/SortedSetCommands.cs | 69 +++++-------------- .../Resp/Objects/SortedSetGeoCommands.cs | 12 +--- 6 files changed, 62 insertions(+), 128 deletions(-) diff --git a/libs/server/Resp/Objects/HashCommands.cs b/libs/server/Resp/Objects/HashCommands.cs index a114db4300..57fde56ac8 100644 --- a/libs/server/Resp/Objects/HashCommands.cs +++ b/libs/server/Resp/Objects/HashCommands.cs @@ -119,9 +119,7 @@ private unsafe bool HashGet(int count, byte* ptr, HashOperation op, (op == HashOperation.HGET && count != 3) || (op == HashOperation.HMGET && count < 3)) { - // Send error to output - WriteErrorTokenNumberInCommand(op.ToString()); - ReadLeftToken(count - 1, ref ptr); + return AbortWithWrongNumberOfArguments(op.ToString(), count); } else { @@ -221,11 +219,10 @@ private unsafe bool HashLength(int count, byte* ptr, ref TGarnetApi { ptr += 10; - if (count - 2 < 0) + if (count != 2) { hashItemsDoneCount = hashOpsCount = 0; - // Send error to output - WriteErrorTokenNumberInCommand("HLEN"); + return AbortWithWrongNumberOfArguments("HLEN", count); } else { @@ -293,11 +290,10 @@ private unsafe bool HashDelete(int count, byte* ptr, ref TGarnetApi { ptr += 10; - if (count - 2 <= 0) + if (count < 2) { hashItemsDoneCount = hashOpsCount = 0; - // Send error to output - WriteErrorTokenNumberInCommand("HDEL"); + return AbortWithWrongNumberOfArguments("DEL", count); } else { @@ -375,11 +371,10 @@ private unsafe bool HashExists(int count, byte* ptr, ref TGarnetApi { ptr += 13; - if (count < 3) + if (count != 3) { hashItemsDoneCount = hashOpsCount = 0; - // Send error to output - WriteErrorTokenNumberInCommand("HEXISTS"); + return AbortWithWrongNumberOfArguments("HEXISTS", count); } else { @@ -534,11 +529,9 @@ private unsafe bool HashIncrement(int count, byte* ptr, HashOperatio ptr += op == HashOperation.HINCRBY ? 13 : 19; // Check if parameters number is right - if (count < 4) + if (count != 4) { - // Send error to output - WriteErrorTokenNumberInCommand(op == HashOperation.HINCRBY ? "HINCRBY" : "HINCRBYFLOAT"); - ReadLeftToken(count - 1, ref ptr); + return AbortWithWrongNumberOfArguments(op == HashOperation.HINCRBY ? "HINCRBY" : "HINCRBYFLOAT", count); } else { diff --git a/libs/server/Resp/Objects/ListCommands.cs b/libs/server/Resp/Objects/ListCommands.cs index 66d04c35d4..4de0ecf5bb 100644 --- a/libs/server/Resp/Objects/ListCommands.cs +++ b/libs/server/Resp/Objects/ListCommands.cs @@ -268,11 +268,7 @@ private unsafe bool ListTrim(int count, byte* ptr, ref TGarnetApi st if (count != 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LTRIM"); + return AbortWithWrongNumberOfArguments("LTRIM", count); } else { @@ -342,11 +338,7 @@ private unsafe bool ListRange(int count, byte* ptr, ref TGarnetApi s if (count != 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LRANGE"); + return AbortWithWrongNumberOfArguments("LRANGE", count); } else { @@ -424,11 +416,7 @@ private unsafe bool ListIndex(int count, byte* ptr, ref TGarnetApi s if (count != 3) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LINDEX"); + return AbortWithWrongNumberOfArguments("LINDEX", count); } else { @@ -511,11 +499,7 @@ private unsafe bool ListInsert(int count, byte* ptr, ref TGarnetApi if (count != 5) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LINSERT"); + return AbortWithWrongNumberOfArguments("LINSERT", count); } else { @@ -599,11 +583,7 @@ private unsafe bool ListRemove(int count, byte* ptr, ref TGarnetApi // if params are missing return error if (count != 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LREM"); + return AbortWithWrongNumberOfArguments("LREM", count); } else { @@ -684,11 +664,7 @@ private unsafe bool ListMove(int count, byte* ptr, ref TGarnetApi st if (count != 5) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("LMOVE"); + return AbortWithWrongNumberOfArguments("LMOVE", count); } else { @@ -742,11 +718,7 @@ private unsafe bool ListRightPopLeftPush(int count, byte* ptr, ref T if (count != 3) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("RPOPLPUSH"); + return AbortWithWrongNumberOfArguments("RPOPLPUSH", count); } else { diff --git a/libs/server/Resp/Objects/ObjectStoreUtils.cs b/libs/server/Resp/Objects/ObjectStoreUtils.cs index 45aba5b4b2..1fb32237cd 100644 --- a/libs/server/Resp/Objects/ObjectStoreUtils.cs +++ b/libs/server/Resp/Objects/ObjectStoreUtils.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +using System; using System.Text; using Garnet.common; @@ -11,18 +12,6 @@ namespace Garnet.server /// internal sealed unsafe partial class RespServerSession : ServerSessionBase { - /// - /// Send the error of missing arguments for any command - /// - /// - //todo move this method to write utils ?? - private void WriteErrorTokenNumberInCommand(string cmdName) - { - var errorMessage = Encoding.ASCII.GetBytes($"-ERR wrong number of arguments for {cmdName} command.\r\n"); - while (!RespWriteUtils.WriteResponse(errorMessage, ref dcurr, dend)) - SendAndReset(); - } - /// /// Reads the n tokens from the current buffer, and returns /// total tokens read @@ -41,5 +30,28 @@ private int ReadLeftToken(int count, ref byte* ptr) return totalTokens; } + + /// + /// Aborts the execution of the current object store command and outputs + /// an error message to indicate a wrong number of arguments for the given command. + /// + /// Name of the command that caused the error message + /// Number of remaining tokens belonging to this command on the receive buffer + /// true if the command was completely consumed, false if the input on the receive buffer was incomplete + private bool AbortWithWrongNumberOfArguments(string cmdName, int count) + { + // Abort command and discard any remaining tokens on the input buffer. + var bufSpan = new ReadOnlySpan(recvBufferPtr, bytesRead); + + if (!DrainCommands(bufSpan, count)) + return false; + + // Print error message to result stream + var errorMessage = Encoding.ASCII.GetBytes($"-ERR wrong number of arguments for {cmdName} command.\r\n"); + while (!RespWriteUtils.WriteResponse(errorMessage, ref dcurr, dend)) + SendAndReset(); + + return true; + } } } \ No newline at end of file diff --git a/libs/server/Resp/Objects/SetCommands.cs b/libs/server/Resp/Objects/SetCommands.cs index a7c1ec256a..f476c43b7a 100644 --- a/libs/server/Resp/Objects/SetCommands.cs +++ b/libs/server/Resp/Objects/SetCommands.cs @@ -177,11 +177,11 @@ private unsafe bool SetLength(int count, byte* ptr, ref TGarnetApi s { ptr += 11; - if (count - 2 < 0) + if (count != 2) { setItemsDoneCount = setOpsCount = 0; - // Send error to output - WriteErrorTokenNumberInCommand("SCARD"); + return AbortWithWrongNumberOfArguments("SCARD", count); + } else { diff --git a/libs/server/Resp/Objects/SortedSetCommands.cs b/libs/server/Resp/Objects/SortedSetCommands.cs index 40d6752d23..5e91186498 100644 --- a/libs/server/Resp/Objects/SortedSetCommands.cs +++ b/libs/server/Resp/Objects/SortedSetCommands.cs @@ -103,11 +103,10 @@ private unsafe bool SortedSetRemove(int count, byte* ptr, ref TGarne { ptr += 10; - if (count - 2 <= 0) + if (count < 3) { zaddDoneCount = zaddAddCount = 0; - // send error to output and forward pointers to the end - WriteErrorTokenNumberInCommand("ZREM"); + return AbortWithWrongNumberOfArguments("ZREM", count); } else { @@ -191,11 +190,10 @@ private unsafe bool SortedSetLength(int count, byte* ptr, ref TGarne { ptr += 11; - if (count - 2 < 0) + if (count != 2) { zaddDoneCount = zaddAddCount = 0; - // send error to output - WriteErrorTokenNumberInCommand("ZCARD"); + return AbortWithWrongNumberOfArguments("ZCARD", count); } else { @@ -386,11 +384,9 @@ private unsafe bool SortedSetScore(int count, byte* ptr, ref TGarnet ptr += 12; //validation if minimum args - if (count - 3 != 0) + if (count != 3) { - // send error to output - WriteErrorTokenNumberInCommand("ZSCORE"); - ReadLeftToken(count - 1, ref ptr); + return AbortWithWrongNumberOfArguments("ZSCORE", count); } else { @@ -467,11 +463,9 @@ private unsafe bool SortedSetPop(int count, byte* ptr, SortedSetOper where TGarnetApi : IGarnetApi { ptr += 13; - if (count - 1 > 2 || count - 1 == 0) + if (count < 2 || count > 3) { - // send error to output - WriteErrorTokenNumberInCommand(op == SortedSetOperation.ZPOPMAX ? "ZPOPMAX" : "ZPOPMIN"); - ReadLeftToken(count - 1, ref ptr); + return AbortWithWrongNumberOfArguments(op == SortedSetOperation.ZPOPMAX ? "ZPOPMAX" : "ZPOPMIN", count); } else { @@ -559,12 +553,7 @@ private unsafe bool SortedSetCount(int count, byte* ptr, ref TGarnet if (count != 4) { - zaddDoneCount = zaddAddCount = 0; - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("ZCOUNT"); + return AbortWithWrongNumberOfArguments("ZCOUNT", count); } else { @@ -655,11 +644,7 @@ private unsafe bool SortedSetLengthByValue(int count, byte* ptr, Sor if (count != 4) { zaddDoneCount = zaddAddCount = 0; - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand(op == SortedSetOperation.ZLEXCOUNT ? "ZLEXCOUNT" : "ZREMRANGEBYLEX"); + return AbortWithWrongNumberOfArguments(op == SortedSetOperation.ZLEXCOUNT ? "ZLEXCOUNT" : "ZREMRANGEBYLEX", count); } else { @@ -749,11 +734,7 @@ private unsafe bool SortedSetIncrement(int count, byte* ptr, ref TGa //validation of required args if (count != 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("ZINCRBY"); + return AbortWithWrongNumberOfArguments("ZINCRBY", count); } else { @@ -850,11 +831,7 @@ private unsafe bool SortedSetRank(int count, byte* ptr, SortedSetOpe //validation of required args if (count < 3 || count > 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand(op == SortedSetOperation.ZRANK ? "ZRANK" : "ZREVRANK"); + return AbortWithWrongNumberOfArguments(op == SortedSetOperation.ZRANK ? "ZRANK" : "ZREVRANK", count); } else { @@ -935,11 +912,7 @@ private unsafe bool SortedSetRemoveRange(int count, byte* ptr, Sorte if (count != 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand(op == SortedSetOperation.ZREMRANGEBYRANK ? "ZREMRANGEBYRANK" : "ZREMRANGEBYSCORE"); + return AbortWithWrongNumberOfArguments(op == SortedSetOperation.ZREMRANGEBYRANK ? "ZREMRANGEBYRANK" : "ZREMRANGEBYSCORE", count); } else { @@ -1021,13 +994,9 @@ private unsafe bool SortedSetRandomMember(int count, byte* ptr, ref { ptr += 18; - if (count - 2 < 0 || count > 4) + if (count < 2 || count > 4) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("ZRANDMEMBER"); + return AbortWithWrongNumberOfArguments("ZRANDMEMBER", count); } else { @@ -1126,13 +1095,9 @@ private unsafe bool SortedSetDifference(int count, byte* ptr, ref TG where TGarnetApi : IGarnetApi { ptr += 11; - if (count - 3 < 0) + if (count < 3) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("ZDIFF"); + return AbortWithWrongNumberOfArguments("ZDIFF", count); } else { diff --git a/libs/server/Resp/Objects/SortedSetGeoCommands.cs b/libs/server/Resp/Objects/SortedSetGeoCommands.cs index 5b718143be..feaa74a52f 100644 --- a/libs/server/Resp/Objects/SortedSetGeoCommands.cs +++ b/libs/server/Resp/Objects/SortedSetGeoCommands.cs @@ -26,11 +26,7 @@ private unsafe bool GeoAdd(int count, byte* ptr, ref TGarnetApi stor // validate the number of parameters if (count < 5) { - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand("GEOADD"); + return AbortWithWrongNumberOfArguments("GEOADD", count); } else { @@ -135,11 +131,7 @@ private unsafe bool GeoCommands(int count, byte* ptr, SortedSetOpera if (count < paramsRequiredInCommand) { zaddDoneCount = zaddAddCount = 0; - var tokens = ReadLeftToken(count - 1, ref ptr); - if (tokens < count - 1) - return false; - // send error to output - WriteErrorTokenNumberInCommand(cmd); + return AbortWithWrongNumberOfArguments(cmd, count); } else { From 86d6b7e497a6c7a5abd63f0b4006f338af32297a Mon Sep 17 00:00:00 2001 From: Lukas Maas Date: Sat, 23 Mar 2024 19:18:41 -0700 Subject: [PATCH 2/2] Fixes a typo in HDEL error handling --- libs/server/Resp/Objects/HashCommands.cs | 2 +- libs/server/Resp/Objects/ObjectStoreUtils.cs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/server/Resp/Objects/HashCommands.cs b/libs/server/Resp/Objects/HashCommands.cs index 57fde56ac8..8842575640 100644 --- a/libs/server/Resp/Objects/HashCommands.cs +++ b/libs/server/Resp/Objects/HashCommands.cs @@ -293,7 +293,7 @@ private unsafe bool HashDelete(int count, byte* ptr, ref TGarnetApi if (count < 2) { hashItemsDoneCount = hashOpsCount = 0; - return AbortWithWrongNumberOfArguments("DEL", count); + return AbortWithWrongNumberOfArguments("HDEL", count); } else { diff --git a/libs/server/Resp/Objects/ObjectStoreUtils.cs b/libs/server/Resp/Objects/ObjectStoreUtils.cs index 1fb32237cd..eef4d659bd 100644 --- a/libs/server/Resp/Objects/ObjectStoreUtils.cs +++ b/libs/server/Resp/Objects/ObjectStoreUtils.cs @@ -35,12 +35,12 @@ private int ReadLeftToken(int count, ref byte* ptr) /// Aborts the execution of the current object store command and outputs /// an error message to indicate a wrong number of arguments for the given command. /// - /// Name of the command that caused the error message - /// Number of remaining tokens belonging to this command on the receive buffer - /// true if the command was completely consumed, false if the input on the receive buffer was incomplete + /// Name of the command that caused the error message. + /// Number of remaining tokens belonging to this command on the receive buffer. + /// true if the command was completely consumed, false if the input on the receive buffer was incomplete. private bool AbortWithWrongNumberOfArguments(string cmdName, int count) { - // Abort command and discard any remaining tokens on the input buffer. + // Abort command and discard any remaining tokens on the input buffer var bufSpan = new ReadOnlySpan(recvBufferPtr, bytesRead); if (!DrainCommands(bufSpan, count))