Skip to content

Commit 644c7f2

Browse files
committed
Fix memory corruption in sharded pubsub unsubscribe
This commit fixes two issues in pubsubUnsubscribeChannel that could lead to memory corruption: 1. The 'found' variable was not initialized to NULL, causing the serverAssert to potentially pass incorrectly when the hashtable lookup failed, leading to memory corruption in subsequent operations. 2. When calculating the slot for a channel, we were using getKeySlot() which might use the current_client's slot if available. This is problematic when a client kills another client (e.g., via CLIENT KILL command) as the slot won't match the channel's actual slot. The fix: - Initialize 'found' to NULL - Calculate the slot directly from the channel name using keyHashSlot() instead of relying on the current client's slot Added a test case that reproduces the issue by having one client kill another client that is subscribed to a sharded pubsub channel during a transaction. Signed-off-by: Uri Yagelnik <[email protected]>
1 parent 053bf9e commit 644c7f2

File tree

2 files changed

+35
-2
lines changed

2 files changed

+35
-2
lines changed

src/pubsub.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,9 +341,9 @@ int pubsubUnsubscribeChannel(client *c, robj *channel, int notify, pubsubtype ty
341341
retval = 1;
342342
/* Remove the client from the channel -> clients list hash table */
343343
if (server.cluster_enabled && type.shard) {
344-
slot = getKeySlot(channel->ptr);
344+
slot = keyHashSlot(channel->ptr, (int)sdslen(channel->ptr));
345345
}
346-
void *found;
346+
void *found = NULL;
347347
kvstoreHashtableFind(*type.serverPubSubChannels, slot, channel, &found);
348348
serverAssertWithInfo(c, NULL, found);
349349
clients = found;

tests/unit/cluster/sharded-pubsub.tcl

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,37 @@ start_cluster 1 1 {tags {external:skip cluster}} {
5353
catch {[$replica EXEC]} err
5454
assert_match {EXECABORT*} $err
5555
}
56+
57+
test "SSUBSCRIBE client killed during transaction" {
58+
# Create two clients
59+
set rd1 [valkey_deferring_client $primary_id]
60+
set rd2 [valkey_deferring_client $primary_id]
61+
62+
# Get client 1 ID
63+
$rd1 client id
64+
set rd1_id [$rd1 read]
65+
puts "rd1_id $rd1_id"
66+
# Client1 subscribes to a shard channel
67+
$rd1 ssubscribe channel0
68+
69+
# Wait for the subscription to be acknowledged
70+
assert_equal {ssubscribe channel0 1} [$rd1 read]
71+
# Client2 starts a transaction, sets a key
72+
$rd2 multi
73+
set multi_result [$rd2 read]
74+
assert_equal {OK} $multi_result
75+
76+
$rd2 set k v
77+
set result [$rd2 read]
78+
assert_equal {QUEUED} $result
79+
# Kill client1 inside client2's transaction
80+
$rd2 client kill id $rd1_id
81+
set result [$rd2 read]
82+
assert_equal {QUEUED} $result
83+
84+
# Execute the transaction
85+
$rd2 exec
86+
set exec_result [$rd2 read]
87+
assert_equal {OK 1} $exec_result "Transaction execution should return OK and kill count"
88+
}
5689
}

0 commit comments

Comments
 (0)