From 163d3b05d43edfa22f82c45d7a5fa4b8ec81d957 Mon Sep 17 00:00:00 2001 From: Kinsey Moore Date: Thu, 5 Jul 2012 19:36:21 +0000 Subject: [PATCH] AST-2012-011: Resolve heap corruption issue with voicemail The heard and deleted arrays in the voicemail state structure were not handled properly following the memory leak fix in r354890 and a fix for an invalid free in r356797. This could result in accessing and writing into freed memory. The allocation for these arrays has been reworked to avoid the possibility of invalid frees, access of freed memory, and crashes that were occurring as a result of this. Locking around accesses and modifications of the voicemail state structure members dh_arraysize, heard, and deleted has been added to prevent simultaneous modification and access when IMAP storage is in use. If IMAP storage is not in use, this locking is not compiled in. Review: https://reviewboard.asterisk.org/r/1994/ (closes issue ASTERISK-19923) Reported by: Dan Delaney Tested by: Dan Delaney, Julian Yap Patches: vm_alloc_fix.diff uploaded by kmoore (license 6273) ........ Merged revisions 369652 from http://svn.asterisk.org/svn/asterisk/branches/1.8 ........ Merged revisions 369653 from http://svn.asterisk.org/svn/asterisk/branches/10 git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@369676 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- apps/app_voicemail.c | 90 +++++++++++++++++++++++++++++++++----------- 1 file changed, 68 insertions(+), 22 deletions(-) diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c index c9d2f7199b..6d0160325a 100644 --- a/apps/app_voicemail.c +++ b/apps/app_voicemail.c @@ -1915,25 +1915,28 @@ static void free_user(struct ast_vm_user *vmu) static int vm_allocate_dh(struct vm_state *vms, struct ast_vm_user *vmu, int count_msg) { int arraysize = (vmu->maxmsg > count_msg ? vmu->maxmsg : count_msg); - if (!vms->dh_arraysize) { - /* initial allocation */ + + /* remove old allocation */ + if (vms->deleted) { + ast_free(vms->deleted); + vms->deleted = NULL; + } + if (vms->heard) { + ast_free(vms->heard); + vms->heard = NULL; + } + vms->dh_arraysize = 0; + + if (arraysize > 0) { if (!(vms->deleted = ast_calloc(arraysize, sizeof(int)))) { return -1; } if (!(vms->heard = ast_calloc(arraysize, sizeof(int)))) { + ast_free(vms->deleted); + vms->deleted = NULL; return -1; } vms->dh_arraysize = arraysize; - } else if (vms->dh_arraysize < arraysize) { - if (!(vms->deleted = ast_realloc(vms->deleted, arraysize * sizeof(int)))) { - return -1; - } - if (!(vms->heard = ast_realloc(vms->heard, arraysize * sizeof(int)))) { - return -1; - } - memset(vms->deleted, 0, arraysize * sizeof(int)); - memset(vms->heard, 0, arraysize * sizeof(int)); - vms->dh_arraysize = arraysize; } return 0; @@ -7192,13 +7195,20 @@ static void adsi_message(struct ast_channel *chan, struct vm_state *vms) ast_callerid_parse(cid, &name, &num); if (!name) name = num; - } else + } else { name = "Unknown Caller"; + } /* If deleted, show "undeleted" */ - - if (vms->deleted[vms->curmsg]) +#ifdef IMAP_STORAGE + ast_mutex_lock(&vms->lock); +#endif + if (vms->deleted[vms->curmsg]) { keys[1] = ADSI_KEY_SKT | (ADSI_KEY_APPS + 11); + } +#ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); +#endif /* Except "Exit" */ keys[5] = ADSI_KEY_SKT | (ADSI_KEY_APPS + 5); @@ -7251,8 +7261,15 @@ static void adsi_delete(struct ast_channel *chan, struct vm_state *vms) } /* If deleted, show "undeleted" */ - if (vms->deleted[vms->curmsg]) +#ifdef IMAP_STORAGE + ast_mutex_lock(&vms->lock); +#endif + if (vms->deleted[vms->curmsg]) { keys[1] = ADSI_KEY_SKT | (ADSI_KEY_APPS + 11); + } +#ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); +#endif /* Except "Exit" */ keys[5] = ADSI_KEY_SKT | (ADSI_KEY_APPS + 5); @@ -8503,8 +8520,12 @@ static int play_message(struct ast_channel *chan, struct ast_vm_user *vmu, struc if (!res) { make_file(vms->fn, sizeof(vms->fn), vms->curdir, vms->curmsg); +#ifdef IMAP_STORAGE + ast_mutex_lock(&vms->lock); +#endif vms->heard[vms->curmsg] = 1; #ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); /*IMAP storage stores any prepended message from a forward * as a separate file from the rest of the message */ @@ -8719,6 +8740,7 @@ static int close_mailbox(struct vm_state *vms, struct ast_vm_user *vmu) } ast_unlock_path(vms->curdir); #else /* defined(IMAP_STORAGE) */ + ast_mutex_lock(&vms->lock); if (vms->deleted) { /* Since we now expunge after each delete, deleting in reverse order * ensures that no reordering occurs between each step. */ @@ -8733,12 +8755,18 @@ static int close_mailbox(struct vm_state *vms, struct ast_vm_user *vmu) #endif done: - if (vms->deleted && last_msg_idx) { + if (vms->deleted) { ast_free(vms->deleted); + vms->deleted = NULL; } - if (vms->heard && last_msg_idx) { + if (vms->heard) { ast_free(vms->heard); + vms->heard = NULL; } + vms->dh_arraysize = 0; +#ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); +#endif return 0; } @@ -9833,14 +9861,25 @@ static int vm_instructions_en(struct ast_channel *chan, struct ast_vm_user *vmu, res = ast_play_and_wait(chan, "vm-next"); } if (!res) { - if (!vms->deleted[vms->curmsg]) + int curmsg_deleted; +#ifdef IMAP_STORAGE + ast_mutex_lock(&vms->lock); +#endif + curmsg_deleted = vms->deleted[vms->curmsg]; +#ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); +#endif + if (!curmsg_deleted) { res = ast_play_and_wait(chan, "vm-delete"); - else + } else { res = ast_play_and_wait(chan, "vm-undelete"); - if (!res) + } + if (!res) { res = ast_play_and_wait(chan, "vm-toforward"); - if (!res) + } + if (!res) { res = ast_play_and_wait(chan, "vm-savemessage"); + } } } if (!res) { @@ -10591,7 +10630,13 @@ static int play_message_by_id_helper(struct ast_channel *chan, if ((wait_file(chan, vms, vms->fn)) < 0) { ast_log(AST_LOG_WARNING, "Playback of message %s failed\n", vms->fn); } else { +#ifdef IMAP_STORAGE + ast_mutex_lock(&vms->lock); +#endif vms->heard[vms->curmsg] = 1; +#ifdef IMAP_STORAGE + ast_mutex_unlock(&vms->lock); +#endif } return 0; @@ -11012,6 +11057,7 @@ static int vm_execmain(struct ast_channel *chan, const char *data) } vms.starting = 1; + vms.curmsg = 0; break; case '3': /* Advanced options */ ast_test_suite_event_notify("ADVOPTIONS", "Message: entering advanced options menu");