BUG: adjust order of memory release

[wasade]

On some long running code, we observed sporadic segmentation faults. After monitoring webdis in gdb, we observed the following traceback:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff4b22700 (LWP 53559)]
0x00007ffff76e8a97 in __strncasecmp_l_avx () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install glibc-2.17-196.el7_4.2.x86_64 openssl-libs-1.0.2k-8.el7.x86_64 zlib-1.2.7-17.el7.x86_64
(gdb) bt
#0  0x00007ffff76e8a97 in __strncasecmp_l_avx () from /lib64/libc.so.6
#1  0x0000000000402e0b in cmd_is_subscribe (cmd=0x7fffe4d249e0) at cmd.c:374
#2  0x0000000000402031 in cmd_free (c=0x7fffe4d249e0) at cmd.c:55
#3  0x000000000040b844 in format_send_reply (cmd=0x7fffe4d249e0, p=0x7fffe401b470 "{\"LPUSH\":2}", sz=11, content_type=0x420cbc "application/json") at formats/common.c:123
#4  0x000000000040a32c in json_reply (c=0x7fffe4001270, r=0x7fffe40045f0, privdata=0x7fffe4d249e0) at formats/json.c:40
#5  0x00000000004127b6 in __redisRunCallback (ac=0x7fffe4001270, cb=0x7ffff4b21c60, reply=0x7fffe40045f0) at hiredis/async.c:269
#6  0x0000000000412f1f in redisProcessCallbacks (ac=0x7fffe4001270) at hiredis/async.c:468
#7  0x0000000000413114 in redisAsyncHandleRead (ac=0x7fffe4001270) at hiredis/async.c:532
#8  0x0000000000408dc0 in redisLibeventReadEvent (fd=60, event=2, arg=0x7fffe40013d0) at ./hiredis/adapters/libevent.h:45
#9  0x00007ffff7baa24b in event_process_active_single_queue (base=0x7fffe4000900, activeq=0x7fffe4000d50, max_to_process=2147483647, endtime=0x0) at event.c:1646
#10 0x00007ffff7baa9cf in event_process_active (base=0x7fffe4000900) at event.c:1738
#11 event_base_loop (base=0x7fffe4000900, flags=<optimized out>) at event.c:1961
#12 0x000000000040331a in worker_main (p=0x6299d0) at worker.c:158
#13 0x00007ffff7974e25 in start_thread () from /lib64/libpthread.so.0
#14 0x00007ffff76a234d in clone () from /lib64/libc.so.6
(gdb) print cmd
$1 = 1701081711
(gdb) frame
#0  0x00007ffff76e8a97 in __strncasecmp_l_avx () from /lib64/libc.so.6
(gdb) up 1
#1  0x0000000000402e0b in cmd_is_subscribe (cmd=0x7fffe4d249e0) at cmd.c:374
374                     (strncasecmp(cmd->argv[0], "SUBSCRIBE", cmd->argv_len[0]) == 0 ||
(gdb) print cmd
$2 = (struct cmd *) 0x7fffe4d249e0
(gdb) print *cmd
$3 = {fd = 143, count = 4, argv = 0x7fffe40088c0, argv_len = 0x7fffe401b490, mime = 0x0, mime_free = 0, filename = 0x0, if_none_match = 0x0, jsonp = 0x0, separator = 0x0, keep_alive = 1, started_responding = 0,
  is_websocket = 0, http_version = 1, database = 0, pub_sub_client = 0x0, ac = 0x7fffe4001270, w = 0x6299d0}
(gdb) print *cmd->argv[0]
Cannot access memory at address 0x7fffd24c33f0
(gdb) print *cmd->argv_len
$4 = 5
(gdb) print *cmd->argv
$5 = 0x7fffd24c33f0 <Address 0x7fffd24c33f0 out of bounds> 

This lead us to examine cmd_free, where we observed that the order of deallocation lead to a potential to access memory that had already been freed. Specifically, the for loop freeing the argv entries was performed prior to subsequent use of argv by cmd_is_subscribe.

This pull request makes a minor reordering of the deallocations in cmd_free, and appears to resolve our segmentation faults.

[nicolasff]

Thanks a lot for the fix! Much appreciated. I merged it and released version 0.1.4

[wasade]

Thanks! And thank you for the rapid review and merge!

On Fri, Aug 10, 2018, 10:35 PM Nicolas Favre-Felix notifications@github.com wrote:

Thanks a lot for the fix! Much appreciated. I merged it and released version 0.1.4

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nicolasff/webdis/pull/148#issuecomment-412250189, or mute the thread https://github.com/notifications/unsubscribe-auth/AAc8slx2EctG2HuU0J20qaTXk2aGpQs_ks5uPl8VgaJpZM4V3mDa .