From bd33d7a13fa2cb65d37e5bbf5abdbb62e9b10610 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Mon, 24 Jul 2017 17:26:44 +0200 Subject: [PATCH] vss: Avoid use after free in vss_send(). 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vss.c b/vss.c index 5484db9d..4d73a95c 100644 --- 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 */ -- 2.39.5