Fix/ready key blocked client uaf 4198

src/blocked.c

@@ -618,6 +618,30 @@ void signalDeletedKeyAsReady(serverDb *db, robj *key, int type) {
     signalKeyAsReadyLogic(db, key, type, 1);
 }
 
+/* Return 1 if client c is still present in the live blocked-clients list for
+ * rl->key. Safe to call with a possibly stale pointer: unlink happens before
+ * free, so a torn-down client cannot appear in the live list. */
+static int clientStillInBlockingKeysList(readyList *rl, client *c) {
+    dictEntry *de = dictFind(rl->db->blocking_keys, rl->key);
+    listNode *ln;
+    listIter li;
+
+    if (de == NULL) return 0;
+    listRewind(dictGetVal(de), &li);
+    while ((ln = listNext(&li)) != NULL) {
+        if (listNodeValue(ln) == c) return 1;
+    }
+    return 0;
+}
+
+/* Return 1 if a live client is still blocked waiting on rl->key. */
+static int clientStillBlockedOnReadyKey(client *c, readyList *rl) {
+    if (c == NULL || !c->flag.blocked) return 0;
+    if (c->bstate == NULL || c->bstate->keys == NULL) return 0;
+    if (c->db != rl->db) return 0;
+    return dictFind(c->bstate->keys, rl->key) != NULL;
+}
+
 /* Helper function for handleClientsBlockedOnKeys(). This function is called
  * whenever a key is ready. we iterate over all the clients blocked on this key
  * and try to re-execute the command (in case the key is still available). */
@@ -630,14 +654,43 @@ static void handleClientsBlockedOnKey(readyList *rl) {
         list *clients = dictGetVal(de);
         listNode *ln;
         listIter li;
+        long count = listLength(clients);
+        long snapshot_len = 0;
+        struct {
+            client *c;
+            uint64_t id;
+        } *snapshot;
+        long i;
+
+        /* Snapshot client* + id (not listNode*): serving one client may re-enter
+         * and free another still queued on this key (CLIENT KILL / evictClients),
+         * invalidating a listIter's cached successor. Prefer O(1) re-check via
+         * lookupClientByID(); fall back to a live-list scan for RM_Call fake
+         * clients that are absent from clients_index. */
+        snapshot = zmalloc(sizeof(*snapshot) * count);
         listRewind(clients, &li);
+        while ((ln = listNext(&li)) != NULL && snapshot_len < count) {
+            client *c = listNodeValue(ln);
+            snapshot[snapshot_len].c = c;
+            snapshot[snapshot_len].id = c->id;
+            snapshot_len++;
+        }
 
         /* Avoid processing more than the initial count so that we're not stuck
          * in an endless loop in case the reprocessing of the command blocks again. */
-        long count = listLength(clients);
-        while ((ln = listNext(&li)) && count--) {
-            client *receiver = listNodeValue(ln);
-            robj *o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NOEFFECTS);
+        for (i = 0; i < snapshot_len; i++) {
+            client *receiver = snapshot[i].c;
+            robj *o;
+
+            /* The client may have been killed/freed or may no longer be blocked
+             * on this key by the time we reach it. */
+            if (lookupClientByID(snapshot[i].id) == receiver) {
+                if (!clientStillBlockedOnReadyKey(receiver, rl)) continue;
+            } else if (!clientStillInBlockingKeysList(rl, receiver)) {
+                continue;
+            }
+
+            o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NOEFFECTS);
             /* 1. In case new key was added/touched we need to verify it satisfy the
              *    blocked type, since we might process the wrong key type.
              * 2. We want to serve clients blocked on module keys
@@ -653,6 +706,7 @@ static void handleClientsBlockedOnKey(readyList *rl) {
                     moduleUnblockClientOnKey(receiver, rl->key);
             }
         }
+        zfree(snapshot);
     }
 }
 

tests/modules/blockonkeys.c

@@ -600,6 +600,69 @@ int blockonkeys_blpopn(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc
     return VALKEYMODULE_OK;
 }
 
+/* --- Regression for ready-key iteration UAF when reply kills a successor --- */
+
+static unsigned long long blockonkeys_kill_successor_id = 0;
+
+static int blockonkeys_kill_successor_timeout(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
+    VALKEYMODULE_NOT_USED(argv);
+    VALKEYMODULE_NOT_USED(argc);
+    return ValkeyModule_ReplyWithSimpleString(ctx, "TIMEOUT");
+}
+
+static void blockonkeys_kill_successor_free(ValkeyModuleCtx *ctx, void *privdata) {
+    VALKEYMODULE_NOT_USED(ctx);
+    ValkeyModule_Free(privdata);
+}
+
+static int blockonkeys_kill_successor_reply(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
+    VALKEYMODULE_NOT_USED(argv);
+    VALKEYMODULE_NOT_USED(argc);
+    int *role = ValkeyModule_GetBlockedClientPrivateData(ctx);
+    if (role && *role == 1 && blockonkeys_kill_successor_id) {
+        ValkeyModuleCallReply *r = ValkeyModule_Call(ctx, "CLIENT", "ccl", "KILL", "ID",
+                                                     (long long)blockonkeys_kill_successor_id);
+        if (r) ValkeyModule_FreeCallReply(r);
+    }
+    return ValkeyModule_ReplyWithSimpleString(ctx, role && *role == 1 ? "FIRST" : "SECOND");
+}
+
+/* BLOCKONKEYS.BLOCK_KILL_SUCCESSOR key first|second
+ *
+ * Blocks on key. The first blocked client's reply callback kills the second
+ * blocked client via CLIENT KILL. Used to verify ready-key iteration survives
+ * reentrant teardown of a successor list node. */
+static int blockonkeys_block_kill_successor(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
+    if (argc != 3) return ValkeyModule_WrongArity(ctx);
+
+    size_t len = 0;
+    const char *role_str = ValkeyModule_StringPtrLen(argv[2], &len);
+    int *role = ValkeyModule_Alloc(sizeof(*role));
+    if (len == 5 && !memcmp(role_str, "first", 5)) {
+        *role = 1;
+    } else if (len == 6 && !memcmp(role_str, "second", 6)) {
+        *role = 2;
+        blockonkeys_kill_successor_id = ValkeyModule_GetClientId(ctx);
+    } else {
+        ValkeyModule_Free(role);
+        return ValkeyModule_ReplyWithError(ctx, "ERR role must be first|second");
+    }
+
+    ValkeyModule_BlockClientOnKeys(ctx, blockonkeys_kill_successor_reply,
+                                  blockonkeys_kill_successor_timeout,
+                                  blockonkeys_kill_successor_free, 0, &argv[1], 1, role);
+    return VALKEYMODULE_OK;
+}
+
+/* BLOCKONKEYS.SIGNAL_READY key - SET key and signal it ready for blocked modules. */
+static int blockonkeys_signal_ready(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
+    if (argc != 2) return ValkeyModule_WrongArity(ctx);
+    ValkeyModuleCallReply *r = ValkeyModule_Call(ctx, "SET", "sc", argv[1], "x");
+    if (r) ValkeyModule_FreeCallReply(r);
+    ValkeyModule_SignalKeyAsReady(ctx, argv[1]);
+    return ValkeyModule_ReplyWithSimpleString(ctx, "OK");
+}
+
 int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
     VALKEYMODULE_NOT_USED(argv);
     VALKEYMODULE_NOT_USED(argc);
@@ -662,5 +725,13 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg
                                       "write", 1, 1, 1) == VALKEYMODULE_ERR)
         return VALKEYMODULE_ERR;
 
+    if (ValkeyModule_CreateCommand(ctx, "blockonkeys.block_kill_successor",
+                                  blockonkeys_block_kill_successor, "", 1, 1, 1) == VALKEYMODULE_ERR)
+        return VALKEYMODULE_ERR;
+
+    if (ValkeyModule_CreateCommand(ctx, "blockonkeys.signal_ready",
+                                  blockonkeys_signal_ready, "write", 1, 1, 1) == VALKEYMODULE_ERR)
+        return VALKEYMODULE_ERR;
+
     return VALKEYMODULE_OK;
 }

tests/unit/moduleapi/blockonkeys.tcl

@@ -217,6 +217,26 @@ start_server {tags {"modules"}} {
         r client kill id $cid ;# try to smoke-out client-related memory leak
     }
 
+    test {Module blocked-on-keys reply killing successor must not UAF} {
+        # Regression for #4198: serving a BLOCKED_MODULE client may run a reply
+        # callback that CLIENT KILLs another client still on the same ready-key
+        # list. Iteration must not retain a stale listNode* across that teardown.
+        r del race
+        set rd1 [valkey_deferring_client]
+        set rd2 [valkey_deferring_client]
+
+        $rd1 blockonkeys.block_kill_successor race first
+        wait_for_blocked_clients_count 1
+        $rd2 blockonkeys.block_kill_successor race second
+        wait_for_blocked_clients_count 2
+
+        assert_equal {OK} [r blockonkeys.signal_ready race]
+        assert_equal {FIRST} [$rd1 read]
+        assert_equal {PONG} [r ping]
+        $rd1 close
+        catch {$rd2 close}
+    }
+
     test {Module client blocked on keys (with metadata): Blocked, CLIENT UNBLOCK TIMEOUT} {
         r del k
         r fsl.push k 32