From 2a823011bd69e067f652c34bc5939320923bb220 Mon Sep 17 00:00:00 2001 From: Patrick Ribbsaeter Date: Thu, 1 Oct 2026 16:13:42 +0200 Subject: [PATCH] Resolve slot from key for module and arbitrary-key commands (#4424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cached-slot optimization in `getCachedKeySlot()` reuses the slot cached on `server.current_client` during command execution under the assumption that all keys accessed belong to the executing command's declared slot. However, module commands (`CMD_MODULE`) and arbitrary key-touching commands (`CMD_TOUCHES_ARBITRARY_KEYS`) are permitted to open and manipulate undeclared keys across any slot via `VM_OpenKey()`. When a module command accesses an undeclared key in a different slot: - In debug builds: `debugServerAssertWithInfo` aborts the server because `keyHashSlot(key) != server.current_client->slot`. - In non-debug builds: DB lookups search the wrong slot's kvstore, returning false `NULL` misses on read or inserting new keys into the wrong slot's kvstore on write. By excluding `CMD_MODULE` and `CMD_TOUCHES_ARBITRARY_KEYS` from the cached-slot shortcut in `getCachedKeySlot()`, module commands derive the correct slot directly from the key via `keyHashSlot()`, fixing all read, write, update, and expiry paths (`VM_OpenKey`, `VM_StringSet`, `moduleCreateEmptyKey`, `VM_SetExpire`) without impacting the hot path of built-in engine commands. Fixes #4398. --------- Signed-off-by: Patrick Ribbsaeter Signed-off-by: Viktor Söderqvist --- src/db.c | 4 +- tests/modules/cluster.c | 53 ++++++++++++++++++++++ tests/unit/moduleapi/cluster.tcl | 77 ++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/src/db.c b/src/db.c index f8cc7d9..e2831c7 100644 --- a/src/db.c +++ b/src/db.c @@ -266,7 +266,9 @@ int getCachedKeySlot(sds key) { * so we must always recompute the slot for commands coming from the primary or AOF. */ if (server.current_client && server.current_client->slot >= 0 && server.current_client->flag.executing_command && - !mustObeyClient(server.current_client)) { + !mustObeyClient(server.current_client) && + server.current_client->cmd && + !(server.current_client->cmd->flags & (CMD_MODULE | CMD_TOUCHES_ARBITRARY_KEYS))) { debugServerAssertWithInfo(server.current_client, NULL, (int)keyHashSlot(key, (int)sdslen(key)) == server.current_client->slot); return server.current_client->slot; diff --git a/tests/modules/cluster.c b/tests/modules/cluster.c index 5029496..7185b4e 100644 --- a/tests/modules/cluster.c +++ b/tests/modules/cluster.c @@ -115,6 +115,57 @@ int test_send_msg_uaf(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) return ValkeyModule_ReplyWithSimpleString(ctx, "OK"); } + +int test_openkey_cross_slot(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { + if (argc < 4) return ValkeyModule_WrongArity(ctx); + + size_t op_len; + const char *op = ValkeyModule_StringPtrLen(argv[3], &op_len); + + if (!strcmp(op, "read")) { + ValkeyModuleKey *key = ValkeyModule_OpenKey(ctx, argv[2], VALKEYMODULE_READ); + if (!key) { + return ValkeyModule_ReplyWithNull(ctx); + } + size_t len; + char *s = ValkeyModule_StringDMA(key, &len, VALKEYMODULE_READ); + if (!s) { + ValkeyModule_CloseKey(key); + return ValkeyModule_ReplyWithError(ctx, VALKEYMODULE_ERRORMSG_WRONGTYPE); + } + ValkeyModule_ReplyWithStringBuffer(ctx, s, len); + ValkeyModule_CloseKey(key); + return VALKEYMODULE_OK; + } else if (!strcmp(op, "write")) { + if (argc < 5) return ValkeyModule_WrongArity(ctx); + ValkeyModuleKey *key = ValkeyModule_OpenKey(ctx, argv[2], VALKEYMODULE_WRITE); + if (!key) { + return ValkeyModule_ReplyWithError(ctx, "ERR OpenKey failed"); + } + ValkeyModule_StringSet(key, argv[4]); + ValkeyModule_CloseKey(key); + return ValkeyModule_ReplyWithSimpleString(ctx, "OK"); + } else if (!strcmp(op, "expire")) { + if (argc < 5) return ValkeyModule_WrongArity(ctx); + long long expire_ms; + if (ValkeyModule_StringToLongLong(argv[4], &expire_ms) != VALKEYMODULE_OK) { + return ValkeyModule_ReplyWithError(ctx, "ERR invalid expire"); + } + ValkeyModuleKey *key = ValkeyModule_OpenKey(ctx, argv[2], VALKEYMODULE_WRITE); + if (!key) { + return ValkeyModule_ReplyWithError(ctx, "ERR OpenKey failed"); + } + if (ValkeyModule_SetExpire(key, expire_ms) != VALKEYMODULE_OK) { + ValkeyModule_CloseKey(key); + return ValkeyModule_ReplyWithError(ctx, "ERR SetExpire failed"); + } + ValkeyModule_CloseKey(key); + return ValkeyModule_ReplyWithSimpleString(ctx, "OK"); + } + + return ValkeyModule_ReplyWithError(ctx, "ERR unknown op"); +} + int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) { VALKEYMODULE_NOT_USED(argv); VALKEYMODULE_NOT_USED(argc); @@ -141,6 +192,8 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg return VALKEYMODULE_ERR; if (ValkeyModule_CreateCommand(ctx, "test.send_msg_uaf", test_send_msg_uaf, "", 0, 0, 0) == VALKEYMODULE_ERR) return VALKEYMODULE_ERR; + if (ValkeyModule_CreateCommand(ctx, "test.openkey_cross_slot", test_openkey_cross_slot, "write", 1, 1, 1) == VALKEYMODULE_ERR) + return VALKEYMODULE_ERR; /* Register our handlers for different message types. */ ValkeyModule_RegisterClusterMessageReceiver(ctx, MSGTYPE_DING, DingReceiver); diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index cd26402..e24e800 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -414,6 +414,83 @@ start_cluster 2 0 {overrides {cluster-node-timeout 1000}} { assert_equal {OK} [R 0 MODULE LOAD $testmodule] assert_equal {OK} [R 0 MODULE UNLOAD test] } + + test "Module VM_OpenKey cross-slot undeclared key access" { + set clustermodule [file normalize tests/modules/cluster.so] + assert_equal {OK} [R 0 MODULE LOAD $clustermodule] + + # Find keys with distinct hash slots that all belong to node 0 (slots 0-5460): + # {bar} = 5061, {baz} = 4813, {alpha} = 865, {k2} = 449, {k3} = 4576 + set key_declared "{bar}declared" + set key_target_read "{baz}target_read" + set key_target_write "{alpha}target_write" + set key_target_expire "{k2}target_expire" + set key_target_wrongtype "{k3}target_wrongtype" + + set slot_declared [R 0 CLUSTER KEYSLOT $key_declared] + set slot_target_read [R 0 CLUSTER KEYSLOT $key_target_read] + set slot_target_write [R 0 CLUSTER KEYSLOT $key_target_write] + set slot_target_expire [R 0 CLUSTER KEYSLOT $key_target_expire] + set slot_target_wrongtype [R 0 CLUSTER KEYSLOT $key_target_wrongtype] + + # Explicitly verify all hash slots are genuinely distinct + assert {$slot_declared != $slot_target_read} + assert {$slot_declared != $slot_target_write} + assert {$slot_declared != $slot_target_expire} + assert {$slot_declared != $slot_target_wrongtype} + assert {$slot_target_read != $slot_target_write} + + # Derive actual local slot range owned by node 0 dynamically + set node0_id [R 0 CLUSTER MYID] + set local_start -1 + set local_end -1 + foreach slot_range [R 0 cluster slots] { + set primary_info [lindex $slot_range 2] + set primary_id [lindex $primary_info 2] + if {$primary_id eq $node0_id} { + set local_start [lindex $slot_range 0] + set local_end [lindex $slot_range 1] + break + } + } + assert {$local_start >= 0 && $local_end >= $local_start} + + # Explicitly verify all hash slots belong to node 0's actual slot range + assert {$slot_declared >= $local_start && $slot_declared <= $local_end} + assert {$slot_target_read >= $local_start && $slot_target_read <= $local_end} + assert {$slot_target_write >= $local_start && $slot_target_write <= $local_end} + assert {$slot_target_expire >= $local_start && $slot_target_expire <= $local_end} + assert {$slot_target_wrongtype >= $local_start && $slot_target_wrongtype <= $local_end} + + # 1. READ: Declared key in slot 5061, undeclared target in slot 4813 + R 0 SET $key_target_read "read_val" + assert_equal "read_val" [R 0 test.openkey_cross_slot $key_declared $key_target_read read] + + # 2. WRITE: Command declared key in slot 5061, writes undeclared key in slot 865 + assert_equal "OK" [R 0 test.openkey_cross_slot $key_declared $key_target_write write "written_val"] + assert_equal "written_val" [R 0 GET $key_target_write] + + # 3. EXPIRY: Command declared key in slot 5061, sets TTL on undeclared key in slot 449 + R 0 SET $key_target_expire "expiring_val" + assert_equal "OK" [R 0 test.openkey_cross_slot $key_declared $key_target_expire expire 100000] + set ttl [R 0 TTL $key_target_expire] + assert {$ttl > 0 && $ttl <= 100} + + # 4. EXPIRY ERROR HANDLING: missing key and invalid TTL (< -1) + assert_error "*ERR SetExpire failed*" {R 0 test.openkey_cross_slot $key_declared "{k2}nonexistent" expire 100000} + assert_error "*ERR SetExpire failed*" {R 0 test.openkey_cross_slot $key_declared $key_target_expire expire -10} + + # 5. EXPIRY CANCELLATION: VALKEYMODULE_NO_EXPIRE (-1) cancels expiration (PERSIST) + assert_equal "OK" [R 0 test.openkey_cross_slot $key_declared $key_target_expire expire -1] + assert_equal -1 [R 0 TTL $key_target_expire] + + # 6. WRONG-TYPE READ: string DMA on a list key returns WRONGTYPE + R 0 LPUSH $key_target_wrongtype "list_elem" + assert_error "*WRONGTYPE*" {R 0 test.openkey_cross_slot $key_declared $key_target_wrongtype read} + + assert_equal {OK} [R 0 MODULE UNLOAD cluster] + } + } } ;# end tag -- 2.55.0.windows.3