redisClusterAsyncRetryCallback crash after command timeout about hiredis-cluster HOT 6 CLOSED

@bjosv could please take a look at this and share some inputs / comments?

from hiredis-cluster.

hiredis_cluster does not officially support multi-threads since hiredis doesn't support it, but it might work.
Is the cluster contexts also mutex protected together with hiredis?

I can't see any obvious issue when looking around at redisClusterAsyncRetryCallback + 0x263, but your output is most likely different than mine.

Could you find out what line of code that is triggering the crash?
You should be able to check this using the start address of redisClusterAsyncRetryCallback presented by objdump and add 0x263 to it.
Use something like:
objdump -S -d libhiredis_cluster.so.0.8

The -S i.e. source annotation of the object dump requires the build flag -g which hopefully is used.

from hiredis-cluster.

Thank you.

We are trying to reproduce the issue with debug symbols enabled.

from hiredis-cluster.

We have been able to reproduce the crash with debug symbols enabled. Please find the backtrace below.
This is with Release 0.8.1:

0 libhiredis_cluster.so.0.8!redisClusterAsyncRetryCallback [hircluster.c : 3907 + 0x0]
1 libhiredis.so.1.0.0!__redisAsyncFree [async.c : 287 + 0x9]
2 libhiredis.so.1.0.0!redisProcessCallbacks [async.c : 577 + 0x8]
3 redisLibmyeventHandler(int, unsigned int, base::SystemInfo) [hiredis_libmyevent.h : 89 + 0x5]

from hiredis-cluster.

Some input from one of the team members below for another crash which may be related. Please see if this seems plausible:

Backtrace:

0 libhiredis_cluster.so.0.8!redisClusterAsyncRetryCallback + 0x263
1 libhiredis.so.1.0.0!__redisAsyncFree + 0x69
2 libhiredis_cluster.so.0.8!cluster_node_deinit.part.0 + 0x55
3 libhiredis_cluster.so.0.8!dictClusterNodeDestructor + 0x11
4 libhiredis_cluster.so.0.8!dictRelease + 0x76
5 libhiredis_cluster.so.0.8!cluster_update_route + 0x7e4
6 libhiredis_cluster.so.0.8!redisClusterAsyncRetryCallback + 0x3d5
7 libhiredis.so.1.0.0!redisProcessCallbacks + 0x16c
8 redisLibmyeventHandler(int, unsigned int, base::SystemInfo) + 0x1d1

"It seems this is simply that

• when cluster_node_deinit calls redisAsyncFree, it first sets ac->data to NULL
• __redisAsyncFree calls all of the pending callbacks
• redisClusterAsyncRetryCallback assumes that ac->data is non-NULL
• so if one of those callbacks is redisClusterAsyncRetryCallback, we hit this crash.

So why don't we hit this crash all the time?

I suspect that normally, cluster_node_deinit is called from the redisClusterAsyncRetryCallback, invoked on its own context. As a result, the redisClusterAsyncRetryCallback has already been invoked by the time redisAsyncFree is hit, and so isn't pending.

In this case, clusterNode_deinit is called from the redisClusterAsyncRetryCallback invoked on a different context, i.e. an event occured on the connection to Redis node 1 that caused us to decide that Redis node 2 should be de-initialized. We know this is true because redisAsyncFree would not call __redisAsyncFree synchronously if it was invoked on the same context.

I suspect the fix is simply for redisClusterAsyncCallback to check if ac->data is non-NULL (and adding an extensive comment to explain how this could happen). The alternative could be to unregister these callbacks in cluster_node_deinit before calling redisAsyncFree."

from hiredis-cluster.

Great investigation. I managed to reproduce the problem via a testcase, without any extra threads.
Prepared the PR #112. Review comments are always welcomed.

from hiredis-cluster.