[asterisk-dev] [Code Review] Fix invalid reads in main/pbx.c

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/585/#review1743
-----------------------------------------------------------

Ship it!


The pbx.c changes look fine to me. The dialplan functions changes look irrelevant, so I'd prefer you didn't include them in the commit :-)

- Kevin


On 2010-03-24 15:11:23, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/585/
> -----------------------------------------------------------
> 
> (Updated 2010-03-24 15:11:23)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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.
> 
> I could use some opinions to determine if my approaches to both fixes are ideal or could be improved. Thanks!
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/sip/dialplan_functions.c 254276 
>   /trunk/main/pbx.c 254276 
> 
> Diff: https://reviewboard.asterisk.org/r/585/diff
> 
> 
> Testing
> -------
> 
> I have rerun the pattern match test under valgrind to ensure that no invalid memory access occurs. In addition, I also used the "memory show summary" command after running the test to ensure that running the test causes no memory leaks to occur in test_pbx.c and pbx.c
> 
> 
> Thanks,
> 
> Mark
> 
>