Fix binary safety of cluster key lookup functions

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 <bjorn.a.svensson@est.tech>

Author: bjosv

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