fix: debug crash inside parsing of ZRANGE (#3611)

Also, fix error msg for EXEC command and finally tune more fakeredis tests. Signed-off-by: Roman Gershman <[email protected]>
dragonflydb · Aug 31, 2024 · 5c48320 · 5c48320
1 parent 06f6dca
commit 5c48320
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 866 deletions.
diff --git a/src/server/command_registry.cc b/src/server/command_registry.cc
@@ -75,7 +75,10 @@ uint64_t CommandId::Invoke(CmdArgList args, ConnectionContext* cntx) const {
 optional<facade::ErrorReply> CommandId::Validate(CmdArgList tail_args) const {
   if ((arity() > 0 && tail_args.size() + 1 != size_t(arity())) ||
       (arity() < 0 && tail_args.size() + 1 < size_t(-arity()))) {
-    return facade::ErrorReply{facade::WrongNumArgsError(name()), kSyntaxErrType};
+    string prefix;
+    if (name() == "EXEC")
+      prefix = "-EXECABORT Transaction discarded because of: ";
+    return facade::ErrorReply{prefix + facade::WrongNumArgsError(name()), kSyntaxErrType};
   }
 
   if ((opt_mask() & CO::INTERLEAVED_KEYS)) {

diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc
@@ -2133,12 +2133,19 @@ void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) {
 
 void ZSetFamily::ZRangeGeneric(CmdArgList args, ConnectionContext* cntx, RangeParams range_params) {
   facade::CmdArgParser parser{args.subspan(3)};
-  while (parser.HasNext()) {
+  while (true) {
+    if (auto err = parser.Error(); err)
+      return cntx->SendError(err->MakeReply());
+
+    if (!parser.HasNext())
+      break;
+
     if (parser.Check("BYSCORE")) {
       if (exchange(range_params.interval_type, RangeParams::SCORE) == RangeParams::LEX)
         return cntx->SendError("BYSCORE and BYLEX options are not compatible");
       continue;
     }
+
     if (parser.Check("BYLEX")) {
       if (exchange(range_params.interval_type, RangeParams::LEX) == RangeParams::SCORE)
         return cntx->SendError("BYSCORE and BYLEX options are not compatible");
@@ -2152,7 +2159,8 @@ void ZSetFamily::ZRangeGeneric(CmdArgList args, ConnectionContext* cntx, RangePa
       range_params.with_scores = true;
       continue;
     }
-    if (parser.Check("LIMIT").ExpectTail(2)) {
+
+    if (parser.Check("LIMIT")) {
       int64_t limit;
       tie(range_params.offset, limit) = parser.Next<uint32_t, int64_t>();
       range_params.limit = limit < 0 ? UINT32_MAX : static_cast<uint32_t>(limit);
@@ -2162,9 +2170,6 @@ void ZSetFamily::ZRangeGeneric(CmdArgList args, ConnectionContext* cntx, RangePa
     return cntx->SendError(absl::StrCat("unsupported option ", parser.Peek()));
   }
 
-  if (auto err = parser.Error(); err)
-    return cntx->SendError(err->MakeReply());
-
   ZRangeInternal(args.subspan(0, 3), range_params, cntx);
 }
 

diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc
@@ -1135,4 +1135,14 @@ TEST_F(ZSetFamilyTest, GeoRadiusByMember) {
                            "WITHHASH and WITHCOORDS options"));
 }
 
+TEST_F(ZSetFamilyTest, RangeLimit) {
+  auto resp = Run({"ZRANGEBYSCORE", "", "0.0", "0.0", "limit", "0"});
+  EXPECT_THAT(resp, ErrArg("syntax error"));
+  resp = Run({"ZRANGEBYSCORE", "", "0.0", "0.0", "limit", "0", "0"});
+  EXPECT_THAT(resp, ArrLen(0));
+
+  resp = Run({"ZRANGEBYSCORE", "", "0.0", "0.0", "foo"});
+  EXPECT_THAT(resp, ErrArg("unsupported option"));
+}
+
 }  // namespace dfly
diff --git a/tests/fakeredis/test/test_stack/test_bloomfilter.py b/tests/fakeredis/test/test_stack/test_bloomfilter.py
@@ -61,6 +61,7 @@ def test_bf_mexists(r: redis.Redis):
         r.bf().add("key1", "v")
 
 
+@pytest.mark.unsupported_server_types("dragonfly")
 def test_bf_reserve(r: redis.Redis):
     assert r.bf().reserve("bloom", 0.01, 1000)
     assert r.bf().reserve("bloom_ns", 0.01, 1000, noScale=True)