[asterisk-commits] kmoore: branch 10 r369653 - in /branches/10: ./ apps/app_voicemail.c

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Thu Jul 5 14:12:40 CDT 2012


Author: kmoore
Date: Thu Jul  5 14:12:33 2012
New Revision: 369653

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

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

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

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

Modified: branches/10/apps/app_voicemail.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/apps/app_voicemail.c?view=diff&rev=369653&r1=369652&r2=369653
==============================================================================
--- branches/10/apps/app_voicemail.c (original)
+++ branches/10/apps/app_voicemail.c Thu Jul  5 14:12:33 2012
@@ -1759,24 +1759,27 @@
 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;
 	}
 
@@ -6554,13 +6557,20 @@
 		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);
@@ -6613,8 +6623,15 @@
 	}
 
 	/* 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);
@@ -7795,8 +7812,12 @@
 
 	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
 		 */
@@ -8011,6 +8032,7 @@
 	}
 	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. */
@@ -8025,12 +8047,18 @@
 #endif
 
 done:
-	if (vms->deleted && last_msg_idx) {
+	if (vms->deleted) {
 		ast_free(vms->deleted);
-	}
-	if (vms->heard && last_msg_idx) {
+		vms->deleted = NULL;
+	}
+	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;
 }
@@ -9125,14 +9153,25 @@
 				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) {
@@ -10169,6 +10208,7 @@
 			}
 
 			vms.starting = 1;
+			vms.curmsg = 0;
 			break;
 		case '3': /* Advanced options */
 			ast_test_suite_event_notify("ADVOPTIONS", "Message: entering advanced options menu");




More information about the asterisk-commits mailing list