Fix crash in ZPOPMIN

[chakaz]

Crash was due to an attempt to access nullptr[-1], which is a bad idea :)

Fixes #949

[ashotland]

Please also uncomment https://github.com/dragonflydb/dragonfly/blob/85a1bf84943d3ff36bfe504aca8f4b74f4ad9d08/tests/dragonfly/utility.py#L202

[romange]

Hi @chakaz this is the fix. the bug was in zset_family code.

While searching for a root-cause, I compared our t_zset.c with redis 7.0 code, so I imported another bug fix from their code.

diff --git a/src/redis/t_zset.c b/src/redis/t_zset.c
index 2f4a0e1..9240288 100644
--- a/src/redis/t_zset.c
+++ b/src/redis/t_zset.c
@@ -1157,9 +1157,10 @@ void zsetConvert(robj *zobj, int encoding) {
         zs->zsl = zslCreate();
 
         eptr = lpSeek(zl,0);
-        serverAssertWithInfo(NULL,zobj,eptr != NULL);
-        sptr = lpNext(zl,eptr);
-        serverAssertWithInfo(NULL,zobj,sptr != NULL);
+        if (eptr != NULL) {
+            sptr = lpNext(zl,eptr);
+            serverAssertWithInfo(NULL,zobj,sptr != NULL);
+        }
 
         while (eptr != NULL) {
             score = zzlGetScore(sptr);
diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc
index b079185..e51527b 100644
--- a/src/server/zset_family.cc
+++ b/src/server/zset_family.cc
@@ -576,7 +576,7 @@ void IntervalVisitor::PopSkipList(ZSetFamily::TopNScored sc) {
   if (params_.reverse) {
     ln = zsl->tail;
   } else {
-    ln = zsl->header;
+    ln = zsl->header->level[0].forward;
   }
 
   while (ln && sc--) {
diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc
index e565648..941eef5 100644
--- a/src/server/zset_family_test.cc
+++ b/src/server/zset_family_test.cc
@@ -485,6 +485,6 @@ TEST_F(ZSetFamilyTest, ZAddPopCrash) {
   }
 
   auto resp = Run({"zpopmin", "key"});
-  EXPECT_THAT(resp, IntArg(1));
+  EXPECT_EQ(resp, "element:0");
 }
 }  // namespace dfly

[chakaz]

Nice fix! It would have taken me a long while to find it, as I have very little idea re/ the zsl API :)

[romange]

I do not know zsl API as well :)

I looked at Redis' corresponding code because if zsetConvert is the same for us and them, it means they probably handle it when reading the data. https://github.com/redis/redis/blob/9e15b42fda8fa622edf6a1e15cba0fc066a35407/src/t_zset.c#L3937