shinberg / cpp-hiredis-cluster

c++ cluster wrapper for hiredis with async and unix sockets features
BSD 3-Clause "New" or "Revised" License
65 stars 26 forks source link

Double free memory error #19

Closed PCMan closed 7 years ago

PCMan commented 7 years ago

After calling Async command for more than 20000000 times, I got memory corruption (double free error). The gdb backtrace looks like this:

Program received signal SIGABRT, Aborted.
0x00007ffff6c2d428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb)
(gdb)
(gdb)
(gdb) bt
#0  0x00007ffff6c2d428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6c2f02a in __GI_abort () at abort.c:89
#2  0x00007ffff6c6f7ea in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff6d882e0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff6c77e0a in malloc_printerr (ar_ptr=<optimized out>, ptr=<optimized out>, str=0x7ffff6d883a8 "double free or corruption (fasttop)", action=3) at malloc.c:5004
#4  _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3865
#5  0x00007ffff6c7b98c in __GI___libc_free (mem=<optimized out>) at malloc.c:2966
#6  0x000000000041c2f3 in RedisCluster::ClusterException::ClusterException (this=0x1096fc90, reply=0x117df4e0, text="cluster is going down")
    at /home/appier-user/pcman/user_browse_history/logger/../cpp-hiredis-cluster/include/clusterexception.h:44
#7  0x000000000041c3a5 in RedisCluster::CriticalException::CriticalException (this=0x1096fc90, reply=0x117df4e0, text="cluster is going down")
    at /home/appier-user/pcman/user_browse_history/logger/../cpp-hiredis-cluster/include/clusterexception.h:52
#8  0x000000000041c9f0 in RedisCluster::ClusterDownException::ClusterDownException (this=0x1096fc90, reply=0x117df4e0)
    at /home/appier-user/pcman/user_browse_history/logger/../cpp-hiredis-cluster/include/clusterexception.h:105
#9  0x000000000041d189 in RedisCluster::HiredisProcess::checkCritical (reply=0x117df4e0, errorcritical=false, error="", con=0x0)
    at /home/appier-user/pcman/user_browse_history/logger/../cpp-hiredis-cluster/include/hiredisprocess.h:110
#10 0x00000000004216fc in RedisCluster::AsyncHiredisCommand<RedisCluster::Cluster<redisAsyncContext, RedisCluster::DefaultContainer<redisAsyncContext> > >::processCommandReply (con=0x657a00, r=0x117df4e0,
    data=0x2a5a9a0) at /home/appier-user/pcman/user_browse_history/logger/../cpp-hiredis-cluster/include/asynchirediscommand.h:290
#11 0x00007ffff7bd1e1d in redisProcessCallbacks () from /usr/lib/x86_64-linux-gnu/libhiredis.so.0.13
#12 0x000000000041a6c3 in redisLibuvPoll (handle=0x657b68, status=0, events=1) at /usr/include/hiredis/adapters/libuv.h:24
#13 0x00007ffff79bf055 in ?? () from /usr/lib/x86_64-linux-gnu/libuv.so.1
#14 0x00007ffff79b0efc in uv_run () from /usr/lib/x86_64-linux-gnu/libuv.so.1
#15 0x000000000041bc99 in Server::run (this=0x7fffffffdd30) at /home/appier-user/pcman/user_browse_history/logger/server.cpp:305
#16 0x000000000041a510 in main (argc=5, argv=0x7fffffffdeb8) at /home/appier-user/pcman/user_browse_history/logger/main.cpp:55

This seems to be caused by the double free of redisReply object. In AsyncCommand::processCommandReply():

HiredisProcess::checkCritical( reply, false );

This line throws exceptions like this, with "reply" object passed into the exception.

        static void checkCritical(redisReply *reply, bool errorcritical, string error = "",
                                  redisContext *con = nullptr) {
...
            if (reply->type == REDIS_REPLY_ERROR) {
                if (errorcritical) {
                    throw LogicError(reply, error);
                } else if (string(reply->str).find("CLUSTERDOWN") != string::npos) {
                    throw ClusterDownException(reply);
                }
            }
        }

Then, in the constructor of ClusterException, the reply object is freed.

        ClusterException(redisReply *reply, const std::string &text) : runtime_error(text) {
            if (reply)
                freeReplyObject(reply);
        }

However, hiredis actually tries to free the object after the callback returns. FYI: https://github.com/redis/hiredis/blob/master/async.c#L470

void redisProcessCallbacks(redisAsyncContext *ac) {
...
c->reader->fn->freeObject(reply);
...
}

This seems to be the cause of the double free.

PCMan commented 7 years ago

@shinberg I'll try to see if I can provide a patch for this. later.

PCMan commented 7 years ago

PR created for fixing this. https://github.com/shinberg/cpp-hiredis-cluster/pull/20

shinberg commented 7 years ago

thank you