Author: wdoekes
Date: Thu Oct 30 04:21:42 2014
New Revision: 426706

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=426706
Log:
app_voicemail: Fix unchecked bounds of myArray in IMAP_STORAGE.

In update_messages_by_imapuser(), messages were appended to a finite
array which resulted in a crash when an IMAP mailbox contained more
than 256 entries. This memory is now dynamically increased as needed.

Observe that this patch adds a bunch of XXX's to questionable code. See
the review (url below) for more information.

ASTERISK-24190 #close
Reported by: Nick Adams
Tested by: Nick Adams

Review: https://reviewboard.asterisk.org/r/4126/
........

Merged revisions 426691 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 426692 from http://svn.asterisk.org/svn/asterisk/branches/11
........

Merged revisions 426696 from http://svn.asterisk.org/svn/asterisk/branches/12
........

Merged revisions 426702 from http://svn.asterisk.org/svn/asterisk/branches/13

Modified:
    trunk/   (props changed)
    trunk/apps/app_voicemail.c

Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-13-merged' - no diff available.

Modified: trunk/apps/app_voicemail.c
URL: 
http://svnview.digium.com/svn/asterisk/trunk/apps/app_voicemail.c?view=diff&rev=426706&r1=426705&r2=426706
==============================================================================
--- trunk/apps/app_voicemail.c (original)
+++ trunk/apps/app_voicemail.c Thu Oct 30 04:21:42 2014
@@ -861,7 +861,8 @@
 #ifdef IMAP_STORAGE
        ast_mutex_t lock;
        int updated;                         /*!< decremented on each mail 
check until 1 -allows delay */
-       long msgArray[VMSTATE_MAX_MSG_ARRAY];
+       long *msgArray;
+       unsigned msg_array_max;
        MAILSTREAM *mailstream;
        int vmArrayIndex;
        char imapuser[80];                   /*!< IMAP server login */
@@ -2440,6 +2441,7 @@
                free_user(vmu);
                return -1;
        }
+       ast_assert(msgnum < vms->msg_array_max);
 
        /* check if someone is accessing this box right now... */
        vms_p = get_vm_state_by_imapuser(vmu->imapuser, 1);
@@ -3081,6 +3083,17 @@
        }
 
        ast_debug(3, "saving mailbox message number %lu as message %d. 
Interactive set to %d\n", number, vms->vmArrayIndex, vms->interactive);
+
+       /* Ensure we have room for the next message. */
+       if (vms->vmArrayIndex >= vms->msg_array_max) {
+               long *new_mem = ast_realloc(vms->msgArray, 2 * 
vms->msg_array_max * sizeof(long));
+               if (!new_mem) {
+                       return;
+               }
+               vms->msgArray = new_mem;
+               vms->msg_array_max *= 2;
+       }
+
        vms->msgArray[vms->vmArrayIndex++] = number;
 }
 
@@ -3358,6 +3371,7 @@
                return vms_p;
        }
        ast_debug(5, "Adding new vmstate for %s\n", vmu->imapuser);
+       /* XXX: Is this correctly freed always? */
        if (!(vms_p = ast_calloc(1, sizeof(*vms_p))))
                return NULL;
        ast_copy_string(vms_p->imapuser, vmu->imapuser, 
sizeof(vms_p->imapuser));
@@ -3472,6 +3486,7 @@
                        vms->newmessages = altvms->newmessages;
                        vms->oldmessages = altvms->oldmessages;
                        vms->vmArrayIndex = altvms->vmArrayIndex;
+                       /* XXX: no msgArray copying? */
                        vms->lastmsg = altvms->lastmsg;
                        vms->curmsg = altvms->curmsg;
                        /* get a pointer to the persistent store */
@@ -3530,10 +3545,14 @@
        
        if (vc) {
                ast_mutex_destroy(&vc->vms->lock);
+               ast_free(vc->vms->msgArray);
+               vc->vms->msgArray = NULL;
+               vc->vms->msg_array_max = 0;
+               /* XXX: is no one supposed to free vms itself? */
                ast_free(vc);
-       }
-       else
+       } else {
                ast_log(AST_LOG_ERROR, "No vmstate found for user:%s, mailbox 
%s\n", vms->imapuser, vms->username);
+       }
 }
 
 static void set_update(MAILSTREAM * stream)
@@ -3555,11 +3574,13 @@
 
 static void init_vm_state(struct vm_state *vms)
 {
-       int x;
+       vms->msg_array_max = VMSTATE_MAX_MSG_ARRAY;
+       vms->msgArray = ast_calloc(vms->msg_array_max, sizeof(long));
+       if (!vms->msgArray) {
+               /* Out of mem? This can't be good. */
+               vms->msg_array_max = 0;
+       }
        vms->vmArrayIndex = 0;
-       for (x = 0; x < VMSTATE_MAX_MSG_ARRAY; x++) {
-               vms->msgArray[x] = 0;
-       }
        ast_mutex_init(&vms->lock);
 }
 


-- 
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

svn-commits mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/svn-commits

Reply via email to