redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.27k stars 1.82k forks source link

redisCommand reply segfault in some cases #709

Closed stevemqeen closed 5 years ago

stevemqeen commented 5 years ago

Hello, in newer version 0.14 I have some issues with redisCommand reply. Lets assume i have this code:

if (reply != NULL)
    {
        if (reply->type == REDIS_REPLY_ERROR)
        {
            goto out;
        }
        else if (reply->type == REDIS_REPLY_ARRAY)
        {
            for (int i = 0; i < reply->elements; i++)
            {
                if (reply->element[i]->str == NULL && reply->element[i]->len == 0)
                    continue;
            }
        }
    }

so the reply type will be REDIS_REPLY_ARRAY, but when there is records in the set, element count is abnormally high usually it's a random value, also elements is not allocated. On older version 0.13 it was working OK, I used before this commit "a65537a672de845f3f4530050d1e7bd88d51ac67", after pulling newer version SMEMBERS results in segfault.

gdb output:

$1 = "MYSETLIST\000", <incomplete sequence \333>
(gdb) print (char*)fldKey
$2 = 0x7fffff8aabe0 "MYSETLIST"
(gdb) print reply
$3 = (redisReply *) 0x74d5c0
(gdb) print reply->type
$4 = 2
(gdb) print reply->elements
$5 = 7656848
(gdb) print reply->element
$6 = (redisReply **) 0x0
michael-grunder commented 5 years ago

Hi thanks for the issue report,

This will be much easier to fix if we are able to replicate segfault. A self contained program that causes it would be best (perhaps with a dump.rdb that triggers the error).

Also, just to clarify are you saying that 967027c881c8cf7c94d6f0223d079b9466c16cc0 crashes but 33a36dc25b34bdc2abf02b5dbe21ec72712297b4 is OK or do you mean something else?

There are commits around then having to do with multi-bulk so it certainly could be a bug the trick is going to be replicating it.

Edit: Other useful information we could use:

1) The exact OS you're using (and if it's 32 or 64 bit). 2) Exact commit SHA for hiredis that does crash and one that doesn't. 3) A core dump (for stack trace). 4) A dump.rdb with the set itself so we can try the command ourselves.

stevemqeen commented 5 years ago

Here is the example program pastebin, unfortunately it's not segfaulting, but all results it returns is empty. I used the latest commit from master. Using older version 0.13 of hiredis lib works fine with this app.

Also, i didn't test other commits, only latest (on which I have problems) and commit with id 'a65537a672de845f3f4530050d1e7bd88d51ac67' from 31-may-2018.

  1. CentOS 7.6 (64bit)
  2. Works - a65537a672de845f3f4530050d1e7bd88d51ac67 Errors - 1ac8fca35de6d9ecc1b6b94cbd17aa7499cb8821
  3. will add it later
  4. I tested on two different redis-server instances with random data, and had either a segfault or empty results (even if data exists in redis, and redis-cli returns correct results).
michael-grunder commented 5 years ago

I ran your program against generated data myself but was not able to replicate any error.

Just to be sure I both built both by linking dynamically to hiredis and by building everything together statically. I also ran the program under Valgrind on both Debian and a CentOS 7.6 container.

Perhaps there is a memory corruption happening elsewhere in the code?

michael-grunder commented 5 years ago

Hi @stevemqeen were you able to solve your problem?

I'm going to close the issue but feel free to reopen if you're able to post a small example that replicates the segfault.

stevemqeen commented 5 years ago

Hi @michael-grunder I think, I have found the root of the issue, I was using hiredis-vip (cluster) and hiredis both in my project, and it seems that when they linked both I got segmentation fault and error's on returning results. Seems that those libs are conflicting which is strange as hiredis-vip is built on top of the hiredis.

RohanMajumdar69 commented 4 years ago

Hello @michael-grunder I am facing the exact same problem. Simple datatypes can be retrieved but when we retrieve an array from the server. The same problem appears.

michael-grunder commented 4 years ago

Can you provide a code sample that demonstrates the problem?

If that would be difficult I'd suggest running your program under Valgrind to see if it reports any invalid memory accesses.

stevemqeen commented 4 years ago

Hello @michael-grunder I am facing the exact same problem. Simple datatypes can be retrieved but when we retrieve an array from the server. The same problem appears.

Hi, are you using any other lib with hiredis? My issue was based on that i was using hiredis-vip library also which was causing the issues.

RohanMajumdar69 commented 4 years ago

I have four other files that I am linking with the executable. But I am calling none of their functions. In the main I am just firing a command and trying to get a reply. Please find attached the valgrind log. valgrind.log reply = (redisReply *)redisCommand(c,"SMEMBERS amniscient-nvr-54C3A3"); printf("Number of elements in the list %d\n", reply->elements); \Code results into an inordinately large number here if (reply->type == REDIS_REPLY_ARRAY) {
for (j = 0; j < reply->elements; j++) {
printf("%u) %s\n", j, reply->element[j]->str); \Here it is throwing a segfault } }

My compilation command is cc example.c kissdb.c checkHostName.c -o hiredis-test -lhiredis And sorry I cant find a way to place after each line of code. I could not insert line breaks in this commenting space.

RohanMajumdar69 commented 4 years ago

And yes @stevemqeen Even when I compiled a simple program without any other modules it was also failing at that time. Attaching that module for your reference. example.txt

michael-grunder commented 4 years ago

What happens if you get your key with my program:

https://github.com/michael-grunder/hiredis-sigsegv-test

It's just your example program but I'm building hiredis directly into the project (not linking to anything external).

RohanMajumdar69 commented 4 years ago

Yes it is working ! Many thanks. What was the problem If I may ask. Also adds to my knowledge. I am an embedded guy please go into details. I wont mind !!

michael-grunder commented 4 years ago

Given that it works when you compile all of hiredis directly into your example, my guess is that there is a version mismatch going on when you link to an external hiredis.

From your valgrind log:

  6 ==11317==    at 0x5159154: write (write.c:27)
  5 ==11317==    by 0x4E41092: redisBufferWrite (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.13)
  4 ==11317==    by 0x4E41212: redisGetReply (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.13)
  3 ==11317==    by 0x4E41274: ??? (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.13)
  2 ==11317==    by 0x4E41613: redisCommand (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.13)

Your system thinks you have hiredis v0.13.0. Is it possible that you're compiling your program with the newer hiredis headers and then the system is calling into the old shared object?

Honestly, this is all wild speculation on my part but you could try purging your system of any remnant of hiredis and then reinstalling everything to make sure you don't have a version mismatch.