Add OOM testing of replica parsing

Fix memory issues when the parsing of replicas fails. Signed-off-by: Björn Svensson <[email protected]>
valkey-io · Oct 11, 2024 · 3c517a5 · 3c517a5
1 parent 83e60c0
commit 3c517a5
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 7 deletions.
diff --git a/src/cluster.c b/src/cluster.c
@@ -628,12 +628,12 @@ static int cluster_master_slave_mapping_with_name(valkeyClusterContext *cc,
             }
 
             if (node_old->slaves != NULL) {
-                node_old->slaves->free = NULL;
                 while (listLength(node_old->slaves) > 0) {
                     lnode = listFirst(node_old->slaves);
                     if (listAddNodeHead(node->slaves, lnode->value) == NULL) {
                         goto oom;
                     }
+                    node_old->slaves->free = NULL;
                     listDelNode(node_old->slaves, lnode);
                 }
                 listRelease(node_old->slaves);
@@ -952,7 +952,6 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) {
                     ret = cluster_master_slave_mapping_with_name(
                         cc, &nodes_name, master, master->name);
                     if (ret != VALKEY_OK) {
-                        freeValkeyClusterNode(master);
                         goto error;
                     }
                 }
@@ -1042,8 +1041,19 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) {
 error:
     sdsfreesplitres(part, count_part);
     sdsfreesplitres(slot_start_end, count_slot_start_end);
+    if (nodes_name != NULL) {
+        /* Only free parsed replicas since the `nodes` dict owns primary nodes. */
+        dictIterator di;
+        dictInitIterator(&di, nodes_name);
+        dictEntry *de;
+        while ((de = dictNext(&di))) {
+            valkeyClusterNode *node = dictGetEntryVal(de);
+            if (node->role == VALKEY_ROLE_SLAVE)
+                freeValkeyClusterNode(node);
+        }
+        dictRelease(nodes_name);
+    }
     dictRelease(nodes);
-    dictRelease(nodes_name);
     return NULL;
 }
 

diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c
@@ -125,6 +125,7 @@ void test_alloc_failure_handling(void) {
         cc = valkeyClusterContextInit();
         assert(cc);
     }
+    cc->flags |= VALKEYCLUSTER_FLAG_ADD_SLAVE;
 
     // Add nodes
     {
@@ -170,14 +171,14 @@ void test_alloc_failure_handling(void) {
 
     // Connect
     {
-        for (int i = 0; i < 128; ++i) {
+        for (int i = 0; i < 148; ++i) {
             prepare_allocation_test(cc, i);
             result = valkeyClusterConnect2(cc);
             assert(result == VALKEY_ERR);
             ASSERT_STR_EQ(cc->errstr, "Out of memory");
         }
 
-        prepare_allocation_test(cc, 128);
+        prepare_allocation_test(cc, 148);
         result = valkeyClusterConnect2(cc);
         assert(result == VALKEY_OK);
     }
@@ -493,6 +494,7 @@ void test_alloc_failure_handling_async(void) {
         acc = valkeyClusterAsyncContextInit();
         assert(acc);
     }
+    acc->cc->flags |= VALKEYCLUSTER_FLAG_ADD_SLAVE;
 
     // Set callbacks
     {
@@ -519,14 +521,14 @@ void test_alloc_failure_handling_async(void) {
 
     // Connect
     {
-        for (int i = 0; i < 126; ++i) {
+        for (int i = 0; i < 146; ++i) {
             prepare_allocation_test(acc->cc, i);
             result = valkeyClusterConnect2(acc->cc);
             assert(result == VALKEY_ERR);
             ASSERT_STR_EQ(acc->cc->errstr, "Out of memory");
         }
 
-        prepare_allocation_test(acc->cc, 126);
+        prepare_allocation_test(acc->cc, 146);
         result = valkeyClusterConnect2(acc->cc);
         assert(result == VALKEY_OK);
     }