redis freeReplyObject not freeing memory

Question

Here is the code:

redisReply * reply;
char * stats = get_key(key, reply);
freeReplyObject( reply );

get_key is a function in a separate header file:

char * get_key(const char* key, redisReply * reply)
{
    reply = redisCommand(rc, "GET %s", key);
    if (reply->type != REDIS_REPLY_ERROR) {
        return reply->str;
    } else {
        return "ERROR";
    }
}

and Valgrind says this about it:

==24846== 2,592 bytes in 54 blocks are definitely lost in loss record 63 of 85
==24846==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24846==    by 0x4E3E342: createReplyObject (hiredis.c:64)
==24846==    by 0x4E3E342: createNilObject (hiredis.c:179)
==24846==    by 0x4E469FE: processBulkItem (read.c:267)
==24846==    by 0x4E469FE: processItem (read.c:407)
==24846==    by 0x4E469FE: redisReaderGetReply (read.c:503)
==24846==    by 0x4E406E3: redisGetReplyFromReader (hiredis.c:863)
==24846==    by 0x4E407CA: redisGetReply (hiredis.c:890)
==24846==    by 0x409DEE: RedisCluster::HiredisCommand<RedisCluster::Cluster<redisContext, RedisCluster::DefaultContainer<redisContext> > >::processHiredisCommand(redisContext*) (in /bucket)
==24846==    by 0x408E3A: RedisCluster::HiredisCommand<RedisCluster::Cluster<redisContext, RedisCluster::DefaultContainer<redisContext> > >::process() (in /bucket)
==24846==    by 0x408818: RedisCluster::HiredisCommand<RedisCluster::Cluster<redisContext, RedisCluster::DefaultContainer<redisContext> > >::Command(RedisCluster::Cluster<redisContext, RedisCluster::DefaultContainer<redisContext> >*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, char const*, ...) (in /bucket)
==24846==    by 0x405104: get_key(char const*, redisReply*) (in /bucket)
==24846==    by 0x406013: main (in /bucket)

I'm guessing it has something to do with the way I pass the reply pointer around, but I can't really tell what I'm doing wrong.

Pass reply by address (pointer to pointer) to get_key. Free it on the caller side. Or better still, pass a properly-sized key buffer to the get_key function and populate it, returning a success/failure state. That way you can just free the reply all within get_key. Right now reply back in the caller is unchanged, and you're code is invoking undefined behavior. — WhozCraig
@WhozCraig ended up using a buffer, like I should have all along! Thanks for the idea. — Christopher Reid

Paul Floyd Paul Floyd · Accepted Answer · 2017-09-21T08:25:14

As said in the first comment, you have a problem of indirection.

If I rewrite your code but inline get_key and rename the formal argument of get_key from reply to replyLocal and just concentrate on the reply variable we get

redisReply * reply;
//char * stats = get_key(key, reply);
{
// this is what happens in the function call, reply is copied in to replyLocal
char* replyLocal = reply;
// and this is in the body of the function
replyLocal = redisCommand(rc, "GET %s", key);
// if and return ignored
}

I hope that it is clear that the assignment to replyLocal does not modify reply. This means that when you try to free the memory it does not work as hoped.

If you change the signature of get_key to

char * get_key(const char* key, redisReply ** reply)

and adapt the body to the extra level of indirection, starting with

*reply = redisCommand(rc, "GET %s", key);

and the call then becomes

char * stats = get_key(key, &reply);

Applying the same sort of inlining as above we have

redisReply * reply;
//char * stats = get_key(key, &reply);
{
// this is what happens in the function call,
// ** the address of ** reply is copied in to replyLocal
char** replyLocal = &reply;
// and this is in the body of the function
*replyLocal = redisCommand(rc, "GET %s", key);
// if and return ignored
}

This now modifies what replyLocal points to so it also modifies reply

redis freeReplyObject not freeing memory

2 Answers