[svn-commits] wdoekes: branch 11 r426692 - in /branches/11: ./ apps/app_voicemail.c

[SVN commits to the Digium repositories]

Author: wdoekes
Date: Thu Oct 30 04:16:47 2014
New Revision: 426692

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=426692
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

Modified:
    branches/11/   (props changed)
    branches/11/apps/app_voicemail.c

Propchange: branches/11/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.

Modified: branches/11/apps/app_voicemail.c
URL: http://svnview.digium.com/svn/asterisk/branches/11/apps/app_voicemail.c?view=diff&rev=426692&r1=426691&r2=426692
==============================================================================
--- branches/11/apps/app_voicemail.c (original)
+++ branches/11/apps/app_voicemail.c Thu Oct 30 04:16:47 2014
@@ -814,7 +814,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 */
@@ -2352,6 +2353,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);
@@ -2981,6 +2983,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;
 }
 
@@ -3258,6 +3271,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));
@@ -3372,6 +3386,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 */
@@ -3430,10 +3445,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)
@@ -3455,11 +3474,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);
 }