From eac3c607a5c5adf22babbfb71bbb3aaf3dde4588 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= Date: Mon, 23 Mar 2026 11:03:21 +0100 Subject: [PATCH 1/2] Fix binary safety of cluster key lookup functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit valkeyClusterGetSlotByKey and valkeyClusterGetNodeByKey used strlen() to determine the key length, which truncated keys containing embedded null bytes, resulting in incorrect slot calculations and node lookups. Add an explicit keylen parameter so callers provide the actual key length, consistent with how the rest of the library handles binary-safe keys. This is a breaking API change. Reference: https://valkey.io/topics/keyspace/ Signed-off-by: Björn Svensson --- docs/migration-guide.md | 10 +++++++++- include/valkey/cluster.h | 4 ++-- src/cluster.c | 8 ++++---- tests/ct_out_of_memory_handling.c | 10 +++++----- tests/ct_specific_nodes.c | 10 +++++----- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/docs/migration-guide.md b/docs/migration-guide.md index ed2abbbd..b7d55026 100644 --- a/docs/migration-guide.md +++ b/docs/migration-guide.md @@ -11,7 +11,7 @@ The general actions needed are: All `libvalkey` headers are now found under `include/valkey/`. * Update used build options, e.g. `USE_TLS` replaces `USE_SSL`. -## Migrating from `hiredis` v1.2.0 +## Migrating from `hiredis` v1.2.0 or v1.3.0 The type `sds` is removed from the public API. @@ -92,6 +92,14 @@ The type `sds` is removed from the public API. * `HIRCLUSTER_FLAG_ADD_SLAVE` removed, flag can be replaced with an option, see `VALKEY_OPT_USE_REPLICAS`. * `HIRCLUSTER_FLAG_ROUTE_USE_SLOTS` removed, the use of `CLUSTER SLOTS` is enabled by default. +### Changed API function signatures + +* `valkeyClusterGetSlotByKey` now requires a `keylen` parameter of type `size_t`. + Previously the key length was determined using `strlen()`, which gave incorrect + results for keys containing embedded null characters. +* `valkeyClusterGetNodeByKey` now requires a `keylen` parameter of type `size_t`, + for the same reason as above. + ### Removed support for splitting multi-key commands per slot Since old days (from `hiredis-vip`) there has been support for sending some commands with multiple keys that covers multiple slots. diff --git a/include/valkey/cluster.h b/include/valkey/cluster.h index 61d1e442..f88849f9 100644 --- a/include/valkey/cluster.h +++ b/include/valkey/cluster.h @@ -336,9 +336,9 @@ LIBVALKEY_API void valkeyClusterInitNodeIterator(valkeyClusterNodeIterator *iter LIBVALKEY_API valkeyClusterNode *valkeyClusterNodeNext(valkeyClusterNodeIterator *iter); /* Helper functions */ -LIBVALKEY_API unsigned int valkeyClusterGetSlotByKey(char *key); +LIBVALKEY_API unsigned int valkeyClusterGetSlotByKey(char *key, size_t keylen); LIBVALKEY_API valkeyClusterNode *valkeyClusterGetNodeByKey(valkeyClusterContext *cc, - char *key); + char *key, size_t keylen); #ifdef __cplusplus } diff --git a/src/cluster.c b/src/cluster.c index 7a20240a..ba13057a 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -3426,12 +3426,12 @@ valkeyClusterNode *valkeyClusterNodeNext(valkeyClusterNodeIterator *iter) { } /* Get hash slot for given key string, which can include hash tags */ -unsigned int valkeyClusterGetSlotByKey(char *key) { - return keyHashSlot(key, strlen(key)); +unsigned int valkeyClusterGetSlotByKey(char *key, size_t keylen) { + return keyHashSlot(key, (int)keylen); } /* Get node that handles given key string, which can include hash tags */ valkeyClusterNode *valkeyClusterGetNodeByKey(valkeyClusterContext *cc, - char *key) { - return node_get_by_table(cc, keyHashSlot(key, strlen(key))); + char *key, size_t keylen) { + return node_get_by_table(cc, keyHashSlot(key, (int)keylen)); } diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c index a7131819..1bd910cb 100644 --- a/tests/ct_out_of_memory_handling.c +++ b/tests/ct_out_of_memory_handling.c @@ -158,7 +158,7 @@ void test_alloc_failure_handling(void) { valkeyReply *reply; const char *cmd = "SET key value"; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"key"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"key", 3); assert(node); int n; @@ -217,7 +217,7 @@ void test_alloc_failure_handling(void) { valkeyReply *reply; const char *cmd = "SET foo one"; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); /* Discover allocations needed for a successful append to node. */ @@ -259,8 +259,8 @@ void test_alloc_failure_handling(void) { prepare_allocation_test(cc, 1000); /* Get the source information for the migration. */ - unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo"); - valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo", 3); + valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); int srcPort = srcNode->port; /* Get a destination node to migrate the slot to. */ @@ -318,7 +318,7 @@ void test_alloc_failure_handling(void) { * allowing a high number of allocations. */ prepare_allocation_test(cc, 1000); /* Fetch the nodes again, in case the slotmap has been reloaded. */ - srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); dstNode = getNodeByPort(cc, dstPort); reply = valkeyClusterCommandToNode( cc, srcNode, "CLUSTER SETSLOT %d NODE %s", slot, replyDstId->str); diff --git a/tests/ct_specific_nodes.c b/tests/ct_specific_nodes.c index 7eedcbb3..7561019b 100644 --- a/tests/ct_specific_nodes.c +++ b/tests/ct_specific_nodes.c @@ -43,7 +43,7 @@ void test_command_to_all_nodes(valkeyClusterContext *cc) { void test_transaction(valkeyClusterContext *cc) { - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); valkeyReply *reply; @@ -71,7 +71,7 @@ void test_streams(valkeyClusterContext *cc) { char *id; /* Get the node that handles given stream */ - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"mystream"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"mystream", 8); assert(node); /* Preparation: remove old stream/key */ @@ -80,7 +80,7 @@ void test_streams(valkeyClusterContext *cc) { freeReplyObject(reply); /* Query wrong node */ - valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, (char *)"otherstream"); + valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, (char *)"otherstream", 11); assert(node != wrongNode); reply = valkeyClusterCommandToNode(cc, wrongNode, "XLEN mystream"); CHECK_REPLY_ERROR(cc, reply, "MOVED"); @@ -230,7 +230,7 @@ void test_pipeline_transaction(valkeyClusterContext *cc) { int status; valkeyReply *reply; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); status = valkeyClusterAppendCommandToNode(cc, node, "MULTI"); @@ -466,7 +466,7 @@ void test_async_transaction(void) { valkeyClusterAsyncContext *acc = valkeyClusterAsyncConnectWithOptions(&options); ASSERT_MSG(acc && acc->err == 0, acc ? acc->errstr : "OOM"); - valkeyClusterNode *node = valkeyClusterGetNodeByKey(&acc->cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(&acc->cc, (char *)"foo", 3); assert(node); int status; From 6b829eacec777da19846662ab1601c431e245448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= Date: Mon, 23 Mar 2026 11:29:57 +0100 Subject: [PATCH 2/2] Add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Björn Svensson --- tests/CMakeLists.txt | 4 ++++ tests/ct_specific_nodes.c | 18 +++++++++++++++++ tests/ut_slot_by_key.c | 41 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 tests/ut_slot_by_key.c diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 15d82f57..a44c056c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -97,6 +97,10 @@ target_include_directories(ut_slotmap_update PRIVATE "${PROJECT_SOURCE_DIR}/src" target_link_libraries(ut_slotmap_update valkey_unittest) add_test(NAME ut_slotmap_update COMMAND "$") +add_executable(ut_slot_by_key ut_slot_by_key.c) +target_link_libraries(ut_slot_by_key valkey_unittest) +add_test(NAME ut_slot_by_key COMMAND "$") + # Cluster tests add_executable(ct_commands ct_commands.c test_utils.c) target_link_libraries(ct_commands valkey ${TLS_LIBRARY}) diff --git a/tests/ct_specific_nodes.c b/tests/ct_specific_nodes.c index 7561019b..11b4803c 100644 --- a/tests/ct_specific_nodes.c +++ b/tests/ct_specific_nodes.c @@ -498,6 +498,22 @@ void test_async_transaction(void) { event_base_free(base); } +void test_get_node_by_key(valkeyClusterContext *cc) { + /* A key with embedded null should use the full length for slot + * calculation, not stop at the null byte. */ + valkeyClusterNode *node_full = valkeyClusterGetNodeByKey(cc, (char *)"ke\0y", 4); + valkeyClusterNode *node_truncated = valkeyClusterGetNodeByKey(cc, (char *)"ke", 2); + assert(node_full != NULL); + assert(node_truncated != NULL); + assert(node_full != node_truncated); +} + +void test_get_node_by_key_empty(valkeyClusterContext *cc) { + /* An empty string is a valid key. */ + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"", 0); + assert(node != NULL); +} + int main(void) { valkeyClusterOptions options = {0}; options.initial_nodes = CLUSTER_NODE; @@ -512,6 +528,8 @@ int main(void) { test_command_to_all_nodes(cc); test_transaction(cc); test_streams(cc); + test_get_node_by_key(cc); + test_get_node_by_key_empty(cc); // Pipeline API test_pipeline_to_single_node(cc); diff --git a/tests/ut_slot_by_key.c b/tests/ut_slot_by_key.c new file mode 100644 index 00000000..08111906 --- /dev/null +++ b/tests/ut_slot_by_key.c @@ -0,0 +1,41 @@ +/* Unit tests for valkeyClusterGetSlotByKey. */ + +#include "cluster.h" +#include "test_utils.h" + +#include + +void test_slot_by_key_basic(void) { + /* A simple key should produce a consistent slot. */ + unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo", 3); + assert(slot == valkeyClusterGetSlotByKey((char *)"foo", 3)); +} + +void test_slot_by_key_with_hashtag(void) { + /* Keys with the same hash tag should map to the same slot. */ + unsigned int slot1 = valkeyClusterGetSlotByKey((char *)"{tag}key1", 9); + unsigned int slot2 = valkeyClusterGetSlotByKey((char *)"{tag}key2", 9); + assert(slot1 == slot2); +} + +void test_slot_by_key_binary_safe(void) { + /* Key with embedded null: "ke\0y" (length 4) should produce a + * different slot than "ke" (length 2, what strlen would give). */ + unsigned int slot_full = valkeyClusterGetSlotByKey((char *)"ke\0y", 4); + unsigned int slot_truncated = valkeyClusterGetSlotByKey((char *)"ke", 2); + assert(slot_full != slot_truncated); +} + +void test_slot_by_key_empty(void) { + /* An empty string is a valid key. */ + unsigned int slot = valkeyClusterGetSlotByKey((char *)"", 0); + assert(slot == 0); +} + +int main(void) { + test_slot_by_key_basic(); + test_slot_by_key_with_hashtag(); + test_slot_by_key_binary_safe(); + test_slot_by_key_empty(); + return 0; +}