fix: add value range check for SETBIT command (#3750)

src/facade/cmd_arg_parser.h

@@ -15,6 +15,22 @@
 
 namespace facade {
 
+// Helper class for numerical range restriction during parsing
+template <auto min, auto max> struct FInt {
+  decltype(min) value = {};
+  operator decltype(min)() {
+    return value;
+  }
+
+  static_assert(std::is_same_v<decltype(min), decltype(max)>, "inconsistent types");
+  static constexpr auto kMin = min;
+  static constexpr auto kMax = max;
+};
+
+template <class T> constexpr bool is_fint = false;
+
+template <auto min, auto max> constexpr bool is_fint<FInt<min, max>> = true;
+
 // Utility class for easily parsing command options from argument lists.
 struct CmdArgParser {
   enum ErrorType { OUT_OF_BOUNDS, SHORT_OPT_TAIL, INVALID_INT, INVALID_CASES, INVALID_NEXT };
@@ -170,13 +186,25 @@ struct CmdArgParser {
   }
 
   template <class T> T Convert(size_t idx) {
-    static_assert(std::is_arithmetic_v<T> || std::is_constructible_v<T, std::string_view>,
-                  "incorrect type");
+    static_assert(
+        std::is_arithmetic_v<T> || std::is_constructible_v<T, std::string_view> || is_fint<T>,
+        "incorrect type");
     if constexpr (std::is_arithmetic_v<T>) {
       return Num<T>(idx);
     } else if constexpr (std::is_constructible_v<T, std::string_view>) {
       return static_cast<T>(SafeSV(idx));
+    } else if constexpr (is_fint<T>) {
+      return {ConvertFInt<T::kMin, T::kMax>(idx)};
+    }
+  }
+
+  template <auto min, auto max> FInt<min, max> ConvertFInt(size_t idx) {
+    auto res = Num<decltype(min)>(idx);
+    if (res < min || res > max) {
+      Report(INVALID_INT, idx);
+      return {};
     }
+    return {res};
   }
 
   std::string_view SafeSV(size_t i) const {

src/facade/cmd_arg_parser_test.cc

@@ -143,4 +143,29 @@ TEST_F(CmdArgParserTest, IgnoreCase) {
   EXPECT_EQ(absl::implicit_cast<string_view>(parser.Next()), "world"sv);
 }
 
+TEST_F(CmdArgParserTest, FixedRangeInt) {
+  {
+    auto parser = Make({"10", "-10", "12"});
+
+    EXPECT_EQ((parser.Next<FInt<-11, 11>>().value), 10);
+    EXPECT_EQ((parser.Next<FInt<-11, 11>>().value), -10);
+    EXPECT_EQ((parser.Next<FInt<-11, 11>>().value), 0);
+
+    auto err = parser.Error();
+    EXPECT_TRUE(err);
+    EXPECT_EQ(err->type, CmdArgParser::INVALID_INT);
+    EXPECT_EQ(err->index, 2);
+  }
+
+  {
+    auto parser = Make({"-12"});
+    EXPECT_EQ((parser.Next<FInt<-11, 11>>().value), 0);
+
+    auto err = parser.Error();
+    EXPECT_TRUE(err);
+    EXPECT_EQ(err->type, CmdArgParser::INVALID_INT);
+    EXPECT_EQ(err->index, 0);
+  }
+}
+
 }  // namespace facade

src/server/bitops_family.cc

@@ -1209,12 +1209,11 @@ void SetBit(CmdArgList args, ConnectionContext* cntx) {
   // Support for the command "SETBIT key offset new_value"
   // see https://redis.io/commands/setbit/
 
-  uint32_t offset{0};
-  int32_t value{0};
-  std::string_view key = ArgS(args, 0);
+  CmdArgParser parser(args);
+  auto [key, offset, value] = parser.Next<string_view, uint32_t, FInt<0, 1>>();
 
-  if (!absl::SimpleAtoi(ArgS(args, 1), &offset) || !absl::SimpleAtoi(ArgS(args, 2), &value)) {
-    return cntx->SendError(kInvalidIntErr);
+  if (auto err = parser.Error(); err) {
+    return cntx->SendError(err->MakeReply());
   }
 
   auto cb = [&](Transaction* t, EngineShard* shard) {

src/server/bitops_family_test.cc

@@ -235,6 +235,23 @@ TEST_F(BitOpsFamilyTest, SetBitMissingKey) {
   }
 }
 
+TEST_F(BitOpsFamilyTest, SetBitIncorrectValues) {
+  EXPECT_EQ(0, CheckedInt({"setbit", "foo", "0", "1"}));
+  EXPECT_THAT(Run({"setbit", "foo", "1", "-1"}),
+              ErrArg("ERR value is not an integer or out of range"));
+  EXPECT_THAT(Run({"setbit", "foo", "2", "11"}),
+              ErrArg("ERR value is not an integer or out of range"));
+  EXPECT_THAT(Run({"setbit", "foo", "3", "a"}),
+              ErrArg("ERR value is not an integer or out of range"));
+  EXPECT_THAT(Run({"setbit", "foo", "4", "O"}),
+              ErrArg("ERR value is not an integer or out of range"));
+  EXPECT_EQ(1, CheckedInt({"getbit", "foo", "0"}));
+  EXPECT_EQ(0, CheckedInt({"getbit", "foo", "1"}));
+  EXPECT_EQ(0, CheckedInt({"getbit", "foo", "2"}));
+  EXPECT_EQ(0, CheckedInt({"getbit", "foo", "3"}));
+  EXPECT_EQ(0, CheckedInt({"getbit", "foo", "4"}));
+}
+
 const int32_t EXPECTED_VALUES_BYTES_BIT_COUNT[] = {  // got this from redis 0 as start index
     4, 7, 11, 14, 17, 21, 21, 21, 21};