Hiredis: freeReplyObject usage

[ShePastAway0]

1) why do i actually have to use freeReplyObject? what this function actually do? for example this code works even without freeReplyObject:
redReply = (redisReply*)redisCommand(c, "PING"); // invoke PING command & return pointer to redisReply structure
printf("PING: %sn", redReply->str);
//freeReplyObject(redReply); // free reply structure

redReply = (redisReply*)redisCommand(c, "GET testkey");
printf("testkey: %s\n", redReply->str);

2) is this information still relevant?

Important: the current version of hiredis (0.10.0) frees replies when the asynchronous API is used. This means you should not call freeReplyObject when you use this API. The reply is cleaned up by hiredis after the callback returns. This behavior will probably change in future releases, so make sure to keep an eye on the changelog when upgrading (see issue #39).

[michael-grunder]

Well you don't _have_ to call freeReplyObject but your program will leak memory. 😄

#include <stdio.h>
#include <hiredis/hiredis.h>

int main(int argc, const char **argv) {
    redisContext *c = redisConnect("127.0.0.1", 6379);

    for (int i = 0; i < 100; i++) {
        redisReply *r = redisCommand(c, "PING %d", i);

        // Only free the replies if passed an argument
        if (argc > 1) {
            if (r) freeReplyObject(r);
        }
    }

    redisFree(c);
}

If I run this program under valgrind without passing an argument, we detect memory leaks. Each iteration of the loop hiredis is allocating memory via redisCommand but then we're not returning it to the operating system. In a long running program this would eventually use all of the memory on the system and crash the program.

$ valgrind --leak-check=full ./hrleak # No arguments, meaning we don't free replies
==32719== Memcheck, a memory error detector
==32719== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==32719== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==32719== Command: ./hrleak
==32719==
==32719==
==32719== HEAP SUMMARY:
==32719==     in use at exit: 5,090 bytes in 200 blocks
==32719==   total heap usage: 1,213 allocs, 1,013 frees, 20,656 bytes allocated
==32719==
==32719== 5,090 (4,800 direct, 290 indirect) bytes in 100 blocks are definitely lost in loss record 2 of 2
==32719==    at 0x483AB35: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==32719==    by 0x4874512: createReplyObject (hiredis.c:65)
==32719==    by 0x4874512: createStringObject (hiredis.c:105)
==32719==    by 0x487C77A: processBulkItem (read.c:339)
==32719==    by 0x487C77A: processItem (read.c:481)
==32719==    by 0x487C77A: redisReaderGetReply (read.c:576)
==32719==    by 0x4876633: redisGetReplyFromReader (hiredis.c:838)
==32719==    by 0x487671A: redisGetReply (hiredis.c:865)
==32719==    by 0x4876A14: __redisBlockForReply (hiredis.c:970)
==32719==    by 0x4876A14: redisvCommand (hiredis.c:980)
==32719==    by 0x4876AE3: redisCommand (hiredis.c:986)
==32719==    by 0x1090BE: main (hrleak.c:8)
==32719==
==32719== LEAK SUMMARY:
==32719==    definitely lost: 4,800 bytes in 100 blocks
==32719==    indirectly lost: 290 bytes in 100 blocks
==32719==      possibly lost: 0 bytes in 0 blocks
==32719==    still reachable: 0 bytes in 0 blocks
==32719==         suppressed: 0 bytes in 0 blocks
==32719==
==32719== For counts of detected and suppressed errors, rerun with: -v
==32719== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Running the program with an argument (meaning we will free replies) and there are no longer any leaks detected.

$ valgrind --leak-check=full ./hrleak an_argument # Now we will call freeReplyObject
==695== Memcheck, a memory error detector
==695== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==695== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==695== Command: ./hrleak an_argument
==695==
==695==
==695== HEAP SUMMARY:
==695==     in use at exit: 0 bytes in 0 blocks
==695==   total heap usage: 1,213 allocs, 1,213 frees, 20,656 bytes allocated
==695==
==695== All heap blocks were freed -- no leaks are possible
==695==
==695== For counts of detected and suppressed errors, rerun with: -v
==695== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

As for your second question yes, hiredis still frees replies in asynchronous calls. I didn't write that part of the system so I'm not totally sure why we thought it was going to change in the future.

Does that make sense?

[ShePastAway0]

@michael-grunder , thank you for answer! but if it will be like this:

#include <stdio.h>
#include <hiredis/hiredis.h>

int main(int argc, const char **argv) {
    redisContext *c = redisConnect("127.0.0.1", 6379);
    redisReply *r;

    for (int i = 0; i < 100; i++)
        redisReply *r = (redisReply*)redisCommand(c, "PING %d", i);

    freeReplyObject(r);
    redisFree(c);
}

is it ok? i dont have to manually "zeromemory" of redisReply structure to safely recieve data?

As for your second question yes, hiredis still frees replies in asynchronous calls. I didn't write that part of the system so I'm not totally sure why we thought it was going to change in the future.

so it works only in async calls and if i use redisConnect i need to freeReplyObject manually?

[michael-grunder]

is it ok? i dont have to manually "zeromemory" of redisReply structure to safely recieve data?

Correct, you don't need to zero memory in the structure even if you call redisCommand in a loop. Hiredis will take care of allocating and setting the structure.

#include <stdio.h>
#include <hiredis/hiredis.h>

int main(int argc, const char **argv) {
    redisContext *c = redisConnect("127.0.0.1", 6379);
    redisReply *r;
    for (int i = 0; i < 10; i++) {
        r = (redisReply*)redisCommand(c, "PING %d", i);
        if (r) {
            if (r->type == REDIS_REPLY_STRING) {
                printf("redisReply[%p]->str[%p] = '%s'\n", r, r->str, r->str);
            }
            freeReplyObject(r);
        }
    }

    redisFree(c);
}

If you run this program you'll see the pointers changing in the loop, since hiredis is doing all of the allocation for you.

so it works only in async calls and if i use redisConnect i need to freeReplyObject manually?

Right, redisAsync* commands still free the memory internally but the synchronous commands do not, and you'll want to free replies.

If you're new to C it's important to point out that if you want to store a reply from redis somewhere else, you will need to duplicate that memory. Otherwise the memory will be freed by freeReplyObject and then the data you stored will contain garbage.

I've created a small program showing what I mean

[michael-grunder]

I'm going to close this issue because it's not a bug in hiredis but good luck with your program.! 😄

[michael-grunder]

@h3xp1017 Hey, I just happened to look at one of your questions again and wanted to point something out.

This program will leak memory:

int main(int argc, const char **argv) {
    redisContext *c = redisConnect("127.0.0.1", 6379);
    redisReply *r;

    for (int i = 0; i < 100; i++)
        redisReply *r = (redisReply*)redisCommand(c, "PING %d", i);

    freeReplyObject(r);
    redisFree(c);
}

You need to call freeReplyObject each time you execute redisCommand, so in this program you would do something like:

#include <stdio.h>
#include <hiredis/hiredis.h>

int main(int argc, const char **argv) {
    redisContext *c = redisConnect("127.0.0.1", 6379);
    redisReply *r;

    for (int i = 0; i < 100; i++) {
        redisReply *r = (redisReply*)redisCommand(c, "PING %d", i);
        if (r) freeReplyObject(r);
    }

    redisFree(c);
}

[ShePastAway0]

You need to call freeReplyObject each time you execute redisCommand

well this is what i wanted to know, thank you =)

will leak memory

why this happen? i mean i use only one redisReply structure which is overwritten every time in the loop, why it leaks memory if its doesnt creates new objects in heap/stack?

[michael-grunder]

why this happen? i mean i use only one redisReply structure which is overwritten every time in the loop, why it leaks memory if its doesnt creates new objects in heap/stack?

It does allocate memory on the heap. If you were to step into hiredis you'd see it executing a malloc for the redisReply itself, and then additional allocations depending on what the return type is.

Here's a short program that does something similar but removes all of the complexity of hiredis

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct exampleReply {
    char *str;
} exampleReply;

char *createString(int count) {
    char buf[1024], *str;
    size_t len;

    len = snprintf(buf, sizeof(buf), "Example reply %d", count);
    str = malloc(len + 1);
    memcpy(str, buf, len);
    str[len] = '\0';

    return str;
}

exampleReply *getReply(int count) {
    exampleReply *r = malloc(sizeof(exampleReply));
    r->str = createString(count);
    return r;
}

void freeReply(exampleReply *reply) {
    free(reply->str);
    free(reply);
}

int main(void) {
    for (int i = 0; i < 3; i++) {
        exampleReply *r = getReply(i);
        printf("exampleReply[%p]->str[%p] = '%s'\n", r, r->str, r->str);
        freeReply(r);
    }
}

Notice how getReply mallocs memory for the structure, and createString mallocs memory for the string? Hiredis is doing something similar to this internally (although the logic is substantially more complex).

I highly recommend using programs like valgrind or clang's asan when learning to help you find things like memory leaks or invalid memory reads. Even programmers who've been using C for decades use these tools.