Fix binary safety of cluster key lookup functions

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.

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
 }

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));
 }

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 "$<TARGET_FILE:ut_slotmap_update>")
 
+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 "$<TARGET_FILE:ut_slot_by_key>")
+
 # Cluster tests
 add_executable(ct_commands ct_commands.c test_utils.c)
 target_link_libraries(ct_commands valkey ${TLS_LIBRARY})

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);

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;
@@ -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);

tests/ut_slot_by_key.c

@@ -0,0 +1,41 @@
+/* Unit tests for valkeyClusterGetSlotByKey. */
+
+#include "cluster.h"
+#include "test_utils.h"
+
+#include <assert.h>
+
+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;
+}