Fix crash due to to reuse iterator entry after list deletion in module (#11383)

In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
`quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
a freed memory address if the quicklist node is deleted. 

This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.

This PR also optimizes the release code of the original list iterator.

Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>

src/module.c

@@ -4147,10 +4147,7 @@ int RM_ListPush(RedisModuleKey *key, int where, RedisModuleString *ele) {
 
     if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR;
     if (key->value && key->value->type != OBJ_LIST) return REDISMODULE_ERR;
-    if (key->iter) {
+    if (key->iter) moduleFreeKeyIterator(key);
-        listTypeReleaseIterator(key->iter);
-        key->iter = NULL;
-    }
     if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_LIST);
     listTypePush(key->value, ele,
         (where == REDISMODULE_LIST_HEAD) ? LIST_HEAD : LIST_TAIL);
@@ -4180,10 +4177,7 @@ RedisModuleString *RM_ListPop(RedisModuleKey *key, int where) {
         errno = EBADF;
         return NULL;
     }
-    if (key->iter) {
+    if (key->iter) moduleFreeKeyIterator(key);
-        listTypeReleaseIterator(key->iter);
-        key->iter = NULL;
-    }
     robj *ele = listTypePop(key->value,
         (where == REDISMODULE_LIST_HEAD) ? LIST_HEAD : LIST_TAIL);
     robj *decoded = getDecodedObject(ele);
@@ -4246,8 +4240,7 @@ int RM_ListSet(RedisModuleKey *key, long index, RedisModuleString *value) {
         listTypeReplace(&key->u.list.entry, value);
         /* A note in quicklist.c forbids use of iterator after insert, so
          * probably also after replace. */
-        listTypeReleaseIterator(key->iter);
+        moduleFreeKeyIterator(key);
-        key->iter = NULL;
         return REDISMODULE_OK;
     } else {
         return REDISMODULE_ERR;
@@ -4292,8 +4285,7 @@ int RM_ListInsert(RedisModuleKey *key, long index, RedisModuleString *value) {
         int where = index < 0 ? LIST_TAIL : LIST_HEAD;
         listTypeInsert(&key->u.list.entry, value, where);
         /* A note in quicklist.c forbids use of iterator after insert. */
-        listTypeReleaseIterator(key->iter);
+        moduleFreeKeyIterator(key);
-        key->iter = NULL;
         return REDISMODULE_OK;
     } else {
         return REDISMODULE_ERR;
@@ -4314,7 +4306,24 @@ int RM_ListInsert(RedisModuleKey *key, long index, RedisModuleString *value) {
 int RM_ListDelete(RedisModuleKey *key, long index) {
     if (moduleListIteratorSeek(key, index, REDISMODULE_WRITE)) {
         listTypeDelete(key->iter, &key->u.list.entry);
-        moduleDelKeyIfEmpty(key);
+        if (moduleDelKeyIfEmpty(key)) return REDISMODULE_OK;
+        if (listTypeNext(key->iter, &key->u.list.entry)) {
+            /* After delete entry at position 'index', we need to update
+             * 'key->u.list.index' according to the following cases:
+             * 1) [1, 2, 3] => dir: forward, index: 0  => [2, 3] => index: still 0
+             * 2) [1, 2, 3] => dir: forward, index: -3 => [2, 3] => index: -2
+             * 3) [1, 2, 3] => dir: reverse, index: 2  => [1, 2] => index: 1
+             * 4) [1, 2, 3] => dir: reverse, index: -1 => [1, 2] => index: still -1 */
+            listTypeIterator *li = key->iter;
+            int reverse = li->direction == LIST_HEAD;
+            if (key->u.list.index < 0)
+                key->u.list.index += reverse ? 0 : 1;
+            else
+                key->u.list.index += reverse ? -1 : 0;
+        } else {
+            /* Reset list iterator if the next entry doesn't exist. */
+            moduleFreeKeyIterator(key);
+        }
         return REDISMODULE_OK;
     } else {
         return REDISMODULE_ERR;

tests/modules/list.c

@@ -50,7 +50,8 @@ int list_getall(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
  * The number of occurrences of "i" and "r" in cmdstr) should correspond to the
  * number of args after cmdstr.
  *
- * The reply is the number of edits (inserts + replaces + deletes) performed.
+ * Reply with a RESP3 Map, containing the number of edits (inserts, replaces, deletes)
+ * performed, as well as the last index and the entry it points to.
  */
 int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
     if (argc < 3) return RedisModule_WrongArity(ctx);
@@ -92,7 +93,7 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
     }
 
     /* Iterate over the chars in cmdstr (edit instructions) */
-    long long num_edits = 0;
+    long long num_inserts = 0, num_deletes = 0, num_replaces = 0;
     long index = reverse ? -1 : 0;
     RedisModuleString *value;
 
@@ -102,17 +103,17 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
             value = argv[argpos++];
             assert(RedisModule_ListInsert(key, index, value) == REDISMODULE_OK);
             index += reverse ? -1 : 1;
-            num_edits++;
+            num_inserts++;
             break;
         case 'd': /* delete */
             assert(RedisModule_ListDelete(key, index) == REDISMODULE_OK);
-            num_edits++;
+            num_deletes++;
             break;
         case 'r': /* replace */
             value = argv[argpos++];
             assert(RedisModule_ListSet(key, index, value) == REDISMODULE_OK);
             index += reverse ? -1 : 1;
-            num_edits++;
+            num_replaces++;
             break;
         case 'k': /* keep */
             index += reverse ? -1 : 1;
@@ -120,7 +121,22 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
         }
     }
 
-    RedisModule_ReplyWithLongLong(ctx, num_edits);
+    RedisModuleString *v = RedisModule_ListGet(key, index);
+    RedisModule_ReplyWithMap(ctx, v ? 5 : 4);
+    RedisModule_ReplyWithCString(ctx, "i");
+    RedisModule_ReplyWithLongLong(ctx, num_inserts);
+    RedisModule_ReplyWithCString(ctx, "d");
+    RedisModule_ReplyWithLongLong(ctx, num_deletes);
+    RedisModule_ReplyWithCString(ctx, "r");
+    RedisModule_ReplyWithLongLong(ctx, num_replaces);
+    RedisModule_ReplyWithCString(ctx, "index");
+    RedisModule_ReplyWithLongLong(ctx, index);
+    if (v) {
+        RedisModule_ReplyWithCString(ctx, "entry");
+        RedisModule_ReplyWithString(ctx, v);
+        RedisModule_FreeString(ctx, v);
+    } 
+
     RedisModule_CloseKey(key);
     return REDISMODULE_OK;
 }

tests/unit/moduleapi/list.tcl

@@ -1,5 +1,17 @@
 set testmodule [file normalize tests/modules/list.so]
 
+# The following arguments can be passed to args:
+#   i -- the number of inserts
+#   d -- the number of deletes
+#   r -- the number of replaces
+#   index -- the last index
+#   entry -- The entry pointed to by index
+proc verify_list_edit_reply {reply argv} {
+    foreach {k v} $argv {
+        assert_equal [dict get $reply $k] $v
+    }
+}
+
 start_server {tags {"modules"}} {
     r module load $testmodule
 
@@ -39,31 +51,73 @@ start_server {tags {"modules"}} {
     test {Module list insert & delete} {
         r del k
         r rpush k x y z
-        r list.edit k ikikdi foo bar baz
+        verify_list_edit_reply [r list.edit k ikikdi foo bar baz] {i 3 index 5}
         r list.getall k
     } {foo x bar y baz}
 
     test {Module list insert & delete, neg index} {
         r del k
         r rpush k x y z
-        r list.edit k REVERSE ikikdi foo bar baz
+        verify_list_edit_reply [r list.edit k REVERSE ikikdi foo bar baz] {i 3 index -6}
         r list.getall k
     } {baz y bar z foo}
 
     test {Module list set while iterating} {
         r del k
         r rpush k x y z
-        r list.edit k rkr foo bar
+        verify_list_edit_reply [r list.edit k rkr foo bar] {r 2 index 3}
         r list.getall k
     } {foo y bar}
 
     test {Module list set while iterating, neg index} {
         r del k
         r rpush k x y z
-        r list.edit k reverse rkr foo bar
+        verify_list_edit_reply [r list.edit k reverse rkr foo bar] {r 2 index -4}
         r list.getall k
     } {bar y foo}
 
+    test {Module list - list entry and index should be updated when deletion} {
+        set original_config [config_get_set list-max-listpack-size 1]
+
+        # delete from start (index 0)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l dd] {d 2 index 0 entry z}
+        assert_equal [r list.getall l] {z}
+
+        # delete from start (index -3)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l reverse kkd] {d 1 index -3}
+        assert_equal [r list.getall l] {y z}
+
+        # # delete from tail (index 2)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l kkd] {d 1 index 2}
+        assert_equal [r list.getall l] {x y}
+
+        # # delete from tail (index -1)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l reverse dd] {d 2 index -1 entry x}
+        assert_equal [r list.getall l] {x}
+
+        # # delete from middle (index 1)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l kdd] {d 2 index 1}
+        assert_equal [r list.getall l] {x}
+
+        # # delete from middle (index -2)
+        r del l
+        r rpush l x y z
+        verify_list_edit_reply [r list.edit l reverse kdd] {d 2 index -2}
+        assert_equal [r list.getall l] {z}
+
+        config_set list-max-listpack-size $original_config
+    }
+
     test "Unload the module - list" {
         assert_equal {OK} [r module unload list]
     }