Bug: vsnprintf causes segfault during 'make test' (with patch)

[GoogleCodeExporter]

What steps will reproduce the problem?

1. svn checkout http://redis.googlecode.com/svn/trunk/ redis-read-only  
2. make
3. ./redis-server in one terminal, 'make test' in the other

What is the expected output? What do you see instead?

Expected: Tests to pass =)

Get instead:

server segfaults with stack trace:

Thread 1 (process 12396):
#0  0x00007f4fc73ea7b0 in strlen () from /lib/libc.so.6
No symbol table info available.
#1  0x00007f4fc73b347e in vfprintf () from /lib/libc.so.6
No symbol table info available.
#2  0x00007f4fc7464d18 in __vsnprintf_chk () from /lib/libc.so.6
No symbol table info available.
#3  0x0000000000408920 in sdscatprintf (s=0x17d4390 "", fmt=0x40a523
"%d\r\n%s\r\n") at /usr/include/bits/stdio2.h:78
    ap = {{gp_offset = 48, fp_offset = 48, overflow_arg_area = 0x7fffcf8f86d0,
reg_save_area = 0x7fffcf8f85e0}}
    t = <value optimized out>
    buflen = 64
#4  0x00000000004059c8 in lindexCommand (c=0x1640820) at redis.c:1741
    o = <value optimized out>
    de = <value optimized out>
#5  0x0000000000404323 in processCommand (c=0x1640820) at redis.c:848
    cmd = (struct redisCommand *) 0x60e480
#6  0x0000000000406de4 in readQueryFromClient (el=<value optimized out>,
fd=<value optimized out>, privdata=0x1640820, mask=<value optimized out>)
    at redis.c:922
    argv = (sds *) 0x17bd0a0
    argc = 3

client says:

LINDEX against non-list value error                                   
argument to math function didn't have numeric value
    while executing
"expr {abs($count)}"
    (procedure "redis_bulk_read" line 4)
    invoked from within
"redis_bulk_read $fd"
    (procedure "redis_lindex" line 3)
    invoked from within
...
} else {
    main [lindex $argv 0] [lindex $argv 1]
}"
    (file "test-redis.tcl" line 643)
make: *** [test] Error 1

What version of the product are you using? On what operating system?

URL: http://redis.googlecode.com/svn/trunk
Revision: 46

Linux bboxl 2.6.27-11-generic #1 SMP Thu Jan 29 19:28:32 UTC 2009 x86_64
GNU/Linux

Please provide any additional information below.

Patch attached for sds.c

man vsnprintf
"These functions do not call the va_end macro. Consequently, the value of
ap is undefined after the call. The application should call va_end(ap)
itself afterwards."

Original issue reported on code.google.com by brettben...@gmail.com on 3 Mar 2009 at 6:02

Attachments:

• sds.c.diff

[GoogleCodeExporter]

Hello! Probably you are the first one trying redis on a 64-bit system... First 
of all
thank you for the patch! I'll release beta-5 immediately with the only change 
of this
fix and make beta-4 deprecated. Please, could you run the redis server under
'valgrind' in your system and run make test against to check if there are subtle
errors running Redis in a 64bit system? Thanks again.

I'll close the issue once beta-5 will be online.

Original comment by anti...@gmail.com on 3 Mar 2009 at 6:42

[GoogleCodeExporter]

valgrind shows no errors, with one "possibly lost" leak

==12996== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 8 from 1)
==12996== malloc/free: in use at exit: 661,072 bytes in 22,061 blocks.
==12996== malloc/free: 452,606 allocs, 430,545 frees, 25,289,432 bytes 
allocated.
==12996== For counts of detected errors, rerun with: -v
==12996== searching for pointers to 22,061 not-freed blocks.
==12996== checked 553,752 bytes.
==12996== 
==12996== 256 bytes in 12 blocks are possibly lost in loss record 7 of 10
==12996==    at 0x4C265AE: malloc (vg_replace_malloc.c:207)
==12996==    by 0x408A16: sdsnewlen (sds.c:43)
==12996==    by 0x4066ED: main (redis.c:537)
==12996== 
==12996== LEAK SUMMARY:
==12996==    definitely lost: 0 bytes in 0 blocks.
==12996==      possibly lost: 256 bytes in 12 blocks.
==12996==    still reachable: 660,816 bytes in 22,049 blocks.
==12996==         suppressed: 0 bytes in 0 blocks.
==12996== Reachable blocks (those to which a pointer was found) are not shown.

Original comment by brettben...@gmail.com on 4 Mar 2009 at 9:06

[GoogleCodeExporter]

Thanks brettbender! Actually this 'possibly lost' bytes are ok.
This is a problem due to the sds library, that is my own dynamically allocated 
string
library. In my design I opted to have something like this in the allocation 
structure
of every string:

+----------+--------...
| metadata | string data
+----------+--------...

The returned pointer is at the start of string data, so Sds strings even if are
able to do all the stuff a dynamic string library is able to do, they look like
a plain C string, and you can pass an Sds string to every plain C function
accepting a C string.

What's wrong with this design is that the actual address returned by malloc()
is not stored in the pointer, I just take pointers to the address +
sizeof(sdsheader). And valgrind think this bytes may be lost.

Thank you very much!
antirez

Original comment by anti...@gmail.com on 4 Mar 2009 at 9:18

• Changed state: Verified