vss: Avoid use after free in vss_send().
authorAndre Noll <maan@tuebingen.mpg.de>
Mon, 24 Jul 2017 15:26:44 +0000 (17:26 +0200)
committerAndre Noll <maan@tuebingen.mpg.de>
Thu, 27 Jul 2017 23:12:52 +0000 (01:12 +0200)
In vss_send() we increment the current slice number for the fec
client after fc->send_fec() has sent a slice. This results in a use
after free in case of a write error because ->send_fec() frees the
fec client structure on write errors. Valgrind complains about this
with the splat below.

To avoid this, the fc pointer must not be dereferenced after
->send_fec() has been called. This patch increases the current slice
number *before* the call to ->send_fec(). This works because the fec
clients do not care about this number.

The bug was introduced eight years ago in commit 625c5cd (Add forward
error correction code to the udp sender/receiver).

==8615== Invalid read of size 1
==8615==    at 0x805022B: vss_send (vss.c:1051)
==8615==    by 0x805022B: vss_post_select (vss.c:1168)
==8615==    by 0x8061DC7: call_post_select (sched.c:84)
==8615==    by 0x8061DC7: sched_post_select (sched.c:110)
==8615==    by 0x8061DC7: schedule (sched.c:163)
==8615==    by 0x804CBFD: main (server.c:607)
==8615==  Address 0x4670168 is 80 bytes inside a block of size 116 free'd
==8615==    at 0x402D221: free (vg_replace_malloc.c:530)
==8615==    by 0x8062D7C: udp_delete_target (udp_send.c:80)
==8615==    by 0x80630DC: udp_send_fec (udp_send.c:305)
==8615==    by 0x805022A: vss_send (vss.c:1049)
==8615==    by 0x805022A: vss_post_select (vss.c:1168)
==8615==    by 0x8061DC7: call_post_select (sched.c:84)
==8615==    by 0x8061DC7: sched_post_select (sched.c:110)
==8615==    by 0x8061DC7: schedule (sched.c:163)
==8615==    by 0x804CBFD: main (server.c:607)
==8615==  Block was alloc'd at
==8615==    at 0x402C1F0: malloc (vg_replace_malloc.c:299)
==8615==    by 0x8052D7E: para_malloc (string.c:67)
==8615==    by 0x8052FBD: para_calloc (string.c:90)
==8615==    by 0x804F48F: vss_add_fec_client (vss.c:686)
==8615==    by 0x8063433: udp_com_add (udp_send.c:342)
==8615==    by 0x8063688: udp_init_target_list (udp_send.c:395)
==8615==    by 0x806371D: udp_send_init (udp_send.c:442)
==8615==    by 0x805062A: init_vss_task (vss.c:1195)
==8615==    by 0x804CA57: server_init (server.c:537)
==8615==    by 0x804CA57: main (server.c:605)

vss.c

diff --git a/vss.c b/vss.c
index 5484db9..4d73a95 100644 (file)
--- a/vss.c
+++ b/vss.c
@@ -1029,9 +1029,9 @@ static void vss_send(struct vss_task *vsst)
                        continue;
                PARA_DEBUG_LOG("sending %u:%u (%u bytes)\n", fc->group.num,
                        fc->current_slice_num, fc->group.slice_bytes);
+               fc->current_slice_num++;
                fc->fcp->send_fec(fc->sc, (char *)fc->enc_buf,
                        fc->group.slice_bytes + FEC_HEADER_SIZE);
-               fc->current_slice_num++;
                fec_active = 1;
        }
        if (mmd->current_chunk >= mmd->afd.afhi.chunks_total) { /* eof */