f-list / fserv

Repository for the F-Chat server implementation.
BSD 2-Clause "Simplified" License
10 stars 5 forks source link

Lacking NULL pointer check in redis.cpp leads to segfault #28

Closed ghost closed 10 years ago

ghost commented 10 years ago

The chat server crashed just now because it incorrectly handled a hiredis error. The offending lines are here.

The backtrace makes pretty clear what happened.

Program terminated with signal 11, Segmentation fault.
#0  freeReplyObject (reply=0x0) at hiredis.c:76
76          switch(r->type) {
(gdb) bt
#0  freeReplyObject (reply=0x0) at hiredis.c:76
#1  0x000000000042cd50 in Redis::processRequest (req=0x6f42ba0) at redis.cpp:89
#2  0x000000000042d645 in Redis::timeoutCallback (loop=<value optimized out>, w=<value optimized out>, revents=<value optimized out>) at redis.cpp:185
#3  0x00007f9412d4e4d1 in ev_invoke_pending () from /usr/lib/libev.so.3
#4  0x00007f9412d5308d in ev_loop () from /usr/lib/libev.so.3
#5  0x000000000042c83b in Redis::runThread (param=<value optimized out>) at redis.cpp:154
#6  0x00007f94131678ca in start_thread () from /lib/libpthread.so.0
#7  0x00007f941020a86d in clone () from /lib/libc.so.6
#8  0x0000000000000000 in ?? ()

According to the hiredis docs:

The return value of redisCommand holds a reply when the command was successfully executed. When an error occurs, the return value is NULL and the err field in the context will be set (see section on Errors). Once an error is returned the context cannot be reused and you should set up a new connection. [...] Replies should be freed using the freeReplyObject() function.

Unfortunately, freeReplyObject() doesn't do a NULL check, which is why fserv crashed when a command failed.

kiranoot commented 10 years ago

Needs review.

ghost commented 10 years ago

Looks okay to me.