[asterisk-commits] mmichelson: trunk r254362 - /trunk/main/pbx.c

[SVN commits to the Asterisk project]

Author: mmichelson
Date: Wed Mar 24 16:10:38 2010
New Revision: 254362

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=254362
Log:
Fix potential invalid reads that could occur in pbx.c

Here is a cut and paste of my review request for this change:
This past weekend, Russell ran our current suite of unit tests for Asterisk
under valgrind. The PBX pattern match test caused valgrind to spew forth two
invalid read errors. This patch contains two changes that shut valgrind up and
do not cause any new memory leaks.

Change 1: In ast_context_remove_extension_callerid2, valgrind reported an
invalid read in the for loop close to the function's end. Specifically, one of
the the strcmp calls in the loop control was reading invalid memory. This was
because the caller of ast_context_remove_extension_callerid2 (__ast_context
destroy in this case) passed as a parameter a shallow copy of an ast_exten's
exten field. This same ast_exten was what was destroyed inside the for loop,
thus any iterations of the for loop beyond the destruction of the ast_exten
would result in invalid reads. My fix for this is to make a copy of the
ast_exten's exten field and pass the copy to
ast_context_remove_extension_callerid2. In addition, I have also acted
similarly with the ast_exten's matchcid field. Since in this case a NULL is
handled quite differently than an empty string, I needed to be a bit more
careful with its handling.

Change 2: In __ast_context_destroy, we iterated over a hashtab and called
ast_context_remove_extension_callerid2 on each item. Specifically, the hashtab
over which we were iterating was an ast_exten's peer_table. Inside of
ast_context_remove_extension_callerid2, we could possibly destroy this
ast_exten, which also caused the hashtab to be freed. Attempting to call
ast_hashtab_end_traversal on the hashtab iterator caused an invalid read to
occur when trying to read the iterator->tab->do_locking field since
iterator->tab had already been freed. My handling of this problem is a bit less
straightforward. With each iteration over the hashtab's contents, we set a
variable called "end_traversal" based on the return of
ast_context_remove_extension_callerid2. If 0 is ever returned, then we know
that the extension was found and destroyed. Because of this, we cannot call
ast_hashtab_end_traversal because we will be guaranteeing a read of invalid
memory. In such a case, we forego calling ast_hashtab_end_traversal and instead
call ast_free on the hashtab iterator.

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


Modified:
    trunk/main/pbx.c

Modified: trunk/main/pbx.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/pbx.c?view=diff&rev=254362&r1=254361&r2=254362
==============================================================================
--- trunk/main/pbx.c (original)
+++ trunk/main/pbx.c Wed Mar 24 16:10:38 2010
@@ -8693,17 +8693,36 @@
 			if (tmp->root_table) { /* it is entirely possible that the context is EMPTY */
 				exten_iter = ast_hashtab_start_traversal(tmp->root_table);
 				while ((exten_item=ast_hashtab_next(exten_iter))) {
+					int end_traversal = 1;
 					prio_iter = ast_hashtab_start_traversal(exten_item->peer_table);
 					while ((prio_item=ast_hashtab_next(prio_iter))) {
+						char extension[AST_MAX_EXTENSION];
+						char cidmatch[AST_MAX_EXTENSION];
 						if (!prio_item->registrar || strcmp(prio_item->registrar, registrar) != 0) {
 							continue;
 						}
 						ast_verb(3, "Remove %s/%s/%d, registrar=%s; con=%s(%p); con->root=%p\n",
 								 tmp->name, prio_item->exten, prio_item->priority, registrar, con? con->name : "<nil>", con, con? con->root_table: NULL);
 						/* set matchcid to 1 to insure we get a direct match, and NULL registrar to make sure no wildcarding is done */
-						ast_context_remove_extension_callerid2(tmp, prio_item->exten, prio_item->priority, prio_item->cidmatch, 1, NULL, 1);
+						ast_copy_string(extension, prio_item->exten, sizeof(extension));
+						if (prio_item->cidmatch) {
+							ast_copy_string(cidmatch, prio_item->cidmatch, sizeof(cidmatch));
+						}
+						end_traversal &= ast_context_remove_extension_callerid2(tmp, extension, prio_item->priority, prio_item->cidmatch ? cidmatch : NULL, 1, NULL, 1);
 					}
-					ast_hashtab_end_traversal(prio_iter);
+					/* Explanation:
+					 * ast_context_remove_extension_callerid2 will destroy the extension that it comes across. This
+					 * destruction includes destroying the exten's peer_table, which we are currently traversing. If
+					 * ast_context_remove_extension_callerid2 ever should return '0' then this means we have destroyed
+					 * the hashtable which we are traversing, and thus calling ast_hashtab_end_traversal will result
+					 * in reading invalid memory. Thus, if we detect that we destroyed the hashtable, then we will simply
+					 * free the iterator
+					 */
+					if (end_traversal) {
+						ast_hashtab_end_traversal(prio_iter);
+					} else {
+						ast_free(prio_iter);
+					}
 				}
 				ast_hashtab_end_traversal(exten_iter);
 			}