Nordix / hiredis-cluster

C client library for Valkey/Redis Cluster. This project is used and sponsored by Ericsson. It is a fork of the now unmaintained hiredis-vip.
BSD 3-Clause "New" or "Revised" License
86 stars 42 forks source link

SIGSEGV in clusterNodesReplyCallback while calling redisClusterAsyncFree #194

Closed 9chu closed 10 months ago

9chu commented 10 months ago

image

it seems there should be a free barrier in clusterNodesReplyCallback.

a patch can by applied to solve this issue:

diff --git a/hircluster.c b/hircluster.c
index 7ea0c82..d934537 100644
--- a/hircluster.c
+++ b/hircluster.c
@@ -1494,6 +1494,8 @@ void redisClusterFree(redisClusterContext *cc) {
     if (cc == NULL)
         return;

+    cc->flags |= REDIS_FREEING;
+
     if (cc->event_callback) {
         cc->event_callback(cc, HIRCLUSTER_EVENT_FREE_CONTEXT,
                            cc->event_privdata);
@@ -3866,6 +3868,9 @@ static redisClusterNode *selectNode(dict *nodes) {
  * arbitrary connected node is selected. */
 static int updateSlotMapAsync(redisClusterAsyncContext *acc,
                               redisAsyncContext *ac) {
+    if (acc->cc->flags & REDIS_FREEING) {
+        return REDIS_ERR;
+    }
     if (acc->lastSlotmapUpdateAttempt == SLOTMAP_UPDATE_ONGOING) {
         /* Don't allow concurrent slot map updates. */
         return REDIS_ERR;
zuiderkwast commented 10 months ago

Thanks for your report and analysis! We will look at this within the next days.

zuiderkwast commented 10 months ago

REDIS_FREEING is a flag for redisContext (hiredis), not the cluster context, so it is a bit confusing to use this flag here. I think #195 is a better way to solve it.

9chu commented 10 months ago

REDIS_FREEING is a flag for redisContext (hiredis), not the cluster context, so it is a bit confusing to use this flag here. I think #195 is a better way to solve it.

You're right, my patch here is just a temporary fix. I'll take a look at the PR, thank you.