We have seen several segfaults of this class already.

This PR targets single-thread free UAFs: https://github.com/valkey-io/valkey/pull/4212 We also hit the same pattern in multithread RDMA crashes and Valkey Over RDMA server UAF in tests, so we are trying to address it at the architecture level.

Raise one corner, and infer the other three — or stop.

Broader direction (non-blocking): Part of the root cause is that many flows free clients synchronously (CLIENT KILL, evictClients(), COB-limit handling, …), and we keep tripping over iterators/cached pointers that outlive those frees. This snapshot fix is a good local solution, but it may be worth a broader discussion on whether these paths should prefer freeClientAsync() so client teardown always happens at a safe point (beforeSleep) rather than mid-iteration. Raising it here since it’s a recurring class of bug, not to block this PR.

Risk checklist

#

Core criterion:

listIter 缓存后继 ≠ 只删当前
+ 中途同步 freeClient / module 回调可能拆掉后继

High priority

#

Module-free multi-BLPOP scenario

#

1. What is BLPOP?

#

Ordinary LPOP mylist: if the list is empty, return (nil) immediately.

BLPOP mylist 0 is Blocking LPOP:

• List has elements → pop immediately, like LPOP
• List empty → do not reply yet; this TCP connection enters a blocked state on the server and waits for someone to LPUSH/RPUSH

From the network stack:

客户端 socket ──write──▶ 服务端 query buffer
                              │
                         解析出 BLPOP
                              │
                    list 空？──yes──▶ 标记 CLIENT_BLOCKED
                              │         可读 handler 仍在（epoll 仍监听）
                              │         但不再 parse/execute 后续命令
                              │         （数据可进 query buf，先不执行）
                              │
客户端 read() 阻塞等 RESP  ◀── 暂时没有任何 reply 写出

So BLPOP is: one long-lived connection + a server state machine that turns client-side polling into server-side suspension. Timeout 0 means wait forever.

Multiple clients BLPOP on the same key → multiple waiters in a queue.

2. How does the server track waiters?

#

Each DB has db->blocking_keys:

key "mylist" ──▶ list:  [ client A ] → [ client B ] → ...

On block (blockForKeys), append to the list tail:

        listAddNodeTail(l, c);
        dictSetVal(c->bstate->keys, client_blocked_entry, listLast(l));

In memory:

list 头
  │
  ▼
┌─────────────┐   next   ┌─────────────┐
│ listNode A  │ ───────▶ │ listNode B  │
│ value = &A  │          │ value = &B  │
└─────────────┘          └─────────────┘
       ▲                        ▲
       │                        │
   client A 还把自己的           client B 同理
   bstate->keys["mylist"]
   指回这个 listNode*
   （方便 O(1) 摘链，而不是扫链表）

Point: listNode is a separate heap object; the client also holds a pointer back to it. Whoever tears down the client typically listUnlink + zfree(listNode).

3. What happens after LPUSH?

#

LPUSH mylist v1 ends with signalKeyAsReady, enqueueing the key into server.ready_keys.

At the end of that command’s processCommand, handleClientsBlockedOnKeys → handleClientsBlockedOnKey:

        listRewind(clients, &li);
        ...
        while ((ln = listNext(&li)) && count--) {
            client *receiver = listNodeValue(ln);
            ...
                    unblockClientOnKey(receiver, rl->key);

For list commands, wake-up is not “just stuff a reply”:

1. Unlink the client from blocking_keys, clear CLIENT_BLOCKED
2. Re-execute the original BLPOP (processCommand again)
3. Now the list has elements; BLPOP pops and addReply

That is the “reexec / pending_command” path.

4. Root cause: listIter caches the successor early

#

adlist’s iterator contract is clear: during iteration you may only delete the current node, not others.

listNode *listNext(listIter *iter) {
    listNode *current = iter->next;

    if (current != NULL) {
        if (iter->direction == AL_START_HEAD)
            iter->next = current->next;   /* 已经把“下一个”指针存进 iter */
        else
            iter->next = current->prev;
    }
    return current;
}

Serving A:

listNext(&li) 得到 nodeA
同时 li.next = nodeB   ← 后继指针已经钉死在栈上的 listIter 里

Then unblockClientOnKey(A). If before that returns someone unlinks and frees nodeB, the outer listNext later reads a freed listNode → heap UAF.

adlist only guarantees: deleting current leaves the cached next valid. It does not guarantee deleting the successor inside a callback.

5. Who kills successor B in the callback? (module-free path)

#

Call stack (from ASan):

handleClientsBlockedOnKey          // 正在 for 链表，li.next == nodeB
  └─ unblockClientOnKey(A)
       └─ processCommand(A)        // 重跑 BLPOP
            └─ evictClients()      // processCommand 开头：客户端内存超限就踢人
                 └─ freeClient(B)  // B 输出缓冲很大，被选中
                      └─ unblockClient / unblockClientWaitingData
                           └─ zfree(nodeB)   // 正是 li.next 指向的那块
  … 回到 while
  └─ listNext(&li)                 // 读已 free 的 nodeB → UAF

Systems view:

Not a “multithread race” — single-thread reentrancy: outer frame holds structure, inner same-thread tears it down.

Difference vs module path #4198 is only who triggers freeClient(successor):

• Module: CLIENT KILL in a reply callback
• This path: processCommand → evictClients

Same list + same dangerous listIter usage.

6. Why MULTI with CONFIG SET + LPUSH?

#

If you just:

CONFIG SET maxmemory-clients 很小
LPUSH mylist v1

then LPUSH’s own processCommand on entry runs evictClients, and B may be killed before handleClientsBlockedOnKey — you miss “free successor mid-iteration”.

MULTI/EXEC puts the lowered limit and LPUSH in one EXEC:

1. processCommand(EXEC) entry: limit still high → do not kick B
2. In-txn CONFIG SET lowers the limit (nested call(), not a full outer processCommand)
3. In-txn LPUSH marks the key ready
4. Only after EXEC does handleClientsBlockedOnKeys run
5. Serving A nested processCommand(A) kicks B under the low limit → hits the iteration window

Artificially creates “sync free mid-iteration on the wake path”. Uncommon in day-to-day scripts, but proves the ordinary BLPOP path can hit the hole.

7. One-line takeaway

#

BLPOP = when the list is empty, hang the connection on the blocking_keys[key] waiter list.
Bug = wake-up scans that list with listIter, but the sync callback for the current waiter (reexec → evictClients) frees the successor’s listNode, then outer listNext reads a dangling pointer.

Fix direction: do not rely on a bare iterator that caches successor listNode* across callbacks that may sync-freeClient; snapshot IDs/pointers or collect-then-process so a torn-down successor is never dereferenced.

Core stack:

# use:  listNext ← handleClientsBlockedOnKey
# free: unblockClientWaitingData ← freeClient ← evictClients
#         ← processCommand ← unblockClientOnKey ← handleClientsBlockedOnKey
# alloc: listAddNodeTail ← blockForKeys ← blpopCommand

Memory layout: adlist’s listIter caches the successor listNode* in li.next; nested processCommand → evictClients sync-freeClients the successor waiter and zfrees that blocking_keys list node; outer listNext → UAF.

H1 scenario

#

evictClients:
  listNext → 当前 fat1，iter->next = fat2 的 listNode*
  freeClient(fat1)
    ├─ CLIENT_CHANGE_DISCONNECTED          ← 此时 fat1 还在 bucket 上
    │    └─ module: CLIENT KILL fat2
    │         └─ freeClient(fat2)
    │              └─ listDelNode → zfree(fat2 的 listNode)   ← 后继堆块已释放
    └─ … 稍后才 listDelNode(fat1)
  内存仍超限（fat3 还在）→ 再 listNext → 读已释放的 fat2 节点 → UAF

This class of bug shows up here.

H2 scenario

#

Conclusion: crash on the outer clientKillCommand’s second listNext — reading a successor listNode already freed by an inner KILL.

1. Entry (network → command)

#

Test client sends CLIENT KILL ID <id1>:

readQueryFromClient → processInputBuffer → processCommand → clientKillCommand

At that moment server.clients looks like:

... → [node_r] → [node_A / id1] → [node_B / id2] → ...
                      ↑ 当前要杀的          ↑ listIter 会缓存的后继

Each listNode is ~24 bytes on the heap (prev/next/value); client->client_list_node points at its own node.

2. Outer loop: “see” A and cache B early

#

listNode *listNext(listIter *iter) {
    listNode *current = iter->next;

    if (current != NULL) {
        if (iter->direction == AL_START_HEAD)
            iter->next = current->next;   /* 关键：把后继指针抄进 iter */
        ...
    }
    return current;
}

When at A:

Then freeClient(A).

3. Critical window: CLIENT_CHANGE before unlinkClient

#

    if (c->conn) {
        moduleFireServerEvent(..., CLIENT_CHANGE_DISCONNECTED, c);
    }

unlinkClient runs later:

    unlinkClient(c);

So inside the callback:

• A’s client* / conn still live
• A and B’s listNodes still on server.clients
• Outer li.next still points at node_B

4. Module sync-kills B (nested same path)

#

h2uaf callback: ValkeyModule_Call(CLIENT KILL ID id2)

Call stack:

clientKillCommand          ← 外层（杀 A，li.next = node_B）
  freeClient(A)
    moduleFireServerEvent
      clientChangeCallback
        VM_Call → CLIENT KILL ID B
          clientKillCommand ← 内层
            freeClient(B)
              unlinkClient(B)
                listDelNode(server.clients, node_B)  ← zfree(node_B)

listDelNode zfrees the node directly:

void listDelNode(list *list, listNode *node) {
    listUnlinkNode(list, node);
    ...
    zfree(node);   /* 堆块进 freelist / ASan 标成 fd */
}

Memory state:

栈上 li.next  ──────────┐
                        ▼
                   [已释放的 node_B]   ← ASan shadow = fd
server.clients:  ... → node_A → (原 B 的 next) → ...

(A may not be unlinked yet; irrelevant — UAF object is B’s node.)

5. The crashing read

#

Inner KILL returns → freeClient(A) finishes (unlink A, etc.) → outer while calls listNext again:

#0 listNext          adlist.c:267   ← READ iter->next->next 之类
#1 clientKillCommand networking.c:5386

First thing listNext does:

listNode *current = iter->next;   /* = 已 free 的 node_B */
iter->next = current->next;       /* 读 freed heap → UAF */

ASan report:

• READ: listNext / outer clientKillCommand:5386
• freed by: listDelNode ← unlinkClient ← inner freeClient(B) ← module VM_Call ← outer freeClient(A) disconnect hook

Same shape as #4198: iterator caches successor listNode*; sync freeClient tears that node down and zfrees it; iterator dereferences again. Only the list changed from blocking_keys to server.clients.

6. Why “kill current is safe, kill successor crashes”

#

• Kill current: adlist allows deleting current; listNext already copied next, so deleting current does not hurt iter->next.
• Kill successor: that is exactly the heap block iter->next points at; after listDelNode, the next listNext is UAF.

PoC uses adjacent id1/id2 so A’s successor is exactly B — hits the window.

H3 scenario

#

1. What subscription means in Valkey

#

Pub/Sub is not a key-space data structure; it is: a TCP connection (client) enters a special mode.

Client sends:

SUBSCRIBE h3chan

Protocol path: subscribeCommand → pubsubSubscribeChannel(). Effects:

1. Client is marked pubsub (mostly cannot run ordinary commands afterward — only receive / unsubscribe).
2. Bidirectional index is built:

client.pubsub_data->pubsub_channels
        "h3chan" ──► (存在即可)

server.pubsub_channels
        "h3chan" ──► hashtable{ clientA, clientB, ... } # 所以应该在这里记录了一个客户端的链表(还是list?)

Corresponding code:

int pubsubSubscribeChannel(client *c, robj *channel, pubsubtype type) {
    ...
    /* client → channels */
    hashtableInsertAtPosition(type.clientPubSubChannels(c), channel, &position);
    ...
    /* channel → clients */
    serverAssert(hashtableAdd(clients, c));
    ...
    addReplyPubsubSubscribed(c, channel, type);
}

From the network stack:

• The connection is still an ordinary accept() fd on the ae event loop.
• Difference: this fd’s client has pubsub_data, and PUBLISH finds subscribers by channel, writes into each reply buffer, then the write event write()s out.
• There is no separate “subscribe socket”; subscription state = heap client + pointers in the global channel table.

There is also a completely separate list:

server.clients ──► listNode ──► listNode ──► ...
                     │            │
                     ▼            ▼
                  client*      client*

All connected clients (admin, subscribers, replicas, …) hang here. ACL kicks scan this list, not server.pubsub_channels.

Note: ACL means Access Control List — permission control, not “a linked list of access”.

2. How ACL and subscriptions intertwine

#

ACL can restrict which channels a user may touch (&foo, resetchannels, etc.).

Subscribe checks permission; more important: if rules tighten later, what about already-subscribed connections?

Valkey’s policy: kick the connection, not merely UNSUBSCRIBE.

ACL SETUSER h3user resetchannels (or ACL LOAD with tighter rules) ends in:

static void ACLKillPubsubClientsIfNeeded(user *new, user *original) {
    ...
    listRewind(server.clients, &li);
    while ((ln = listNext(&li)) != NULL) {
        client *c = listNodeValue(ln);
        if (c->user != original) continue;
        if (ACLShouldKillPubsubClient(c, channels)) {
            freeClientOrCloseLater(c, 0);  /* 同步 free（非 current_client） */
        }
    }
}

ACLShouldKillPubsubClient checks: if the client is pubsub and its channels/patterns are outside the new allow-set, kill it.

H3 business semantics:

两个 TCP 连接 AUTH 成同一 ACL 用户
  → 各自 SUBSCRIBE h3chan
  → 管理员收紧该用户的 channel ACL
  → 服务器遍历 server.clients，把这两个订阅者 free 掉

Kick path: freeClient → freeClientPubSubData → remove self from channel→clients, then unlinkClient closes the fd and unlinks from server.clients.

3. Why “just kick two subscribers” does not crash

#

adlist iterator contract:

listNode *listNext(listIter *iter) {
    listNode *current = iter->next;
    if (current != NULL) {
        if (iter->direction == AL_START_HEAD)
            iter->next = current->next;  /* 提前缓存后继指针 */
        ...
    }
    return current;
}

In memory:

listIter.li.next ──────────┐
                           ▼
[node_admin] → [node_sub1] → [node_sub2] → NULL
                  │              │
                  ▼              ▼
               clientA        clientB

When processing node_sub1, listNext has already set li.next = node_sub2.

If you only freeClient(A):

• unlinkClient(A) only listDelNode(node_sub1)
• node_sub2 remains; li.next is still valid
  → next listNext gets B, then freeClient(B) — fine.

That is why the control test passes: sync-free of current is safe for adlist.

4. Why a module callback crashes (the real H3 hole)

#

The blast is not “subscription itself” but freeClient reentrancy timing.

    if (c->conn) {
        moduleFireServerEvent(..., CLIENT_CHANGE_DISCONNECTED, c);
    }
    ...
    /* 很后面才 */
    unlinkClient(c);   /* 约 2221 行 */

Timeline:

ACL 循环: listNext → 拿到 sub1
  li.next 已经缓存 = node_sub2   ← 关键！

  freeClientOrCloseLater(sub1)
    freeClient(sub1)
      ① module: DISCONNECTED(sub1)     ← 此时 sub1 还在 server.clients 上
           h3uaf: CLIENT KILL ID sub2
             freeClient(sub2)
               unlinkClient(sub2)
               listDelNode(node_sub2)  ← 24 字节 listNode 被 zfree
               zfree(clientB)
      ② unlinkClient(sub1)             ← 才摘自己

  ACL 循环: listNext(&li)
      读 li.next (= 已释放的 node_sub2)  → ASan: heap-use-after-free

Stack you see:

listNext
  ← ACLKillPubsubClientsIfNeeded / ACLLoadFromFile
freed by:
  listDelNode ← unlinkClient ← freeClient(sub2)
    ← CLIENT KILL ← module CLIENT_CHANGE ← freeClient(sub1)

Memory layout:

listNode 是独立堆块（约 24B：prev/next/value）
client 结构是另一块堆

li.next 存的是 listNode*，不是 client*

杀后继时：
  clientB 和 node_sub2 都被释放
  但栈上/寄存器里的 listIter 仍握着毒指针 node_sub2
下次 listNext 一解引用 → UAF（读 next/value）

Same class as #4198 (blocked list on ready-key):

Single-threaded: listIter caches successor listNode*; a sync call chain mid-loop unlinks and frees that successor on the same list.

Subscription only supplies a batch sync-freeClient entry (ACL tightening channels); detonation needs free current then free successor (PoC uses a module disconnect hook).

5. End-to-end path (vs PoC)

#

[TCP] rd1, rd2 accept → linkClient → 挂上 server.clients
[AUTH] 绑定同一 ACL user
[SUBSCRIBE] 写入 client↔channel 双向表，client 变 pubsub

[ACL SETUSER resetchannels]   // 或 ACL LOAD 写严规则
  └─ ACLKillPubsubClientsIfNeeded
       扫 server.clients
       发现 rd1 订了不再允许的 channel → freeClient(rd1)
            └─ DISCONNECTED hook → CLIENT KILL rd2
                 └─ 拆掉 li 已缓存的 node_rd2
       listNext → 炸

若没有 hook：free(rd1) 只拆 node_rd1，再 free(rd2)，安全。

6. One-line essence

#

• Subscribe: connection mode + bidirectional hash indexes; messages write into subscriber reply buffers.
• ACL tighten: walk server.clients and sync-kick illegal subscribers.
• Crash reason: not SUBSCRIBE corrupting tables — the kick loop’s listIter cached a successor listNode*, and freeClient fires module events before unlink so the successor is also sync-torn-down — cached pointer becomes dangling.

Next step (optional): compare with H2 (CLIENT KILL walking server.clients) and draw “two entries onto the same clients list” — clarifies why governance should unify (snapshot / async free / ban sync-killing other clients inside DISCONNECTED).

Note: this scenario is deliberately manufactured with a module; rare in production. We only prove the class exists.

Medium priority

#

Next: check whether medium-priority paths can hit the same class.