[asterisk-dev] [Code Review] 3055: pbx.c: put copy of ast_exten.data on stack to prevent memory corruption

[Scott Griepentrog]

> On Dec. 7, 2013, 9:48 a.m., Matt Jordan wrote:
> > /branches/1.8/main/pbx.c, lines 4477-4491
> > <https://reviewboard.asterisk.org/r/3055/diff/4/?file=49180#file49180line4477>
> >
> >     I don't think we need the extra lock here.
> >     
> >     The purpose of the contexts lock is to prevent changes to the dialplan while some portion of it is being used. In this case, pbx_substitute_variables will extract data from the ast_exten object e when it is called.
> >     
> >     Interestingly enough, variable substitution does not actually need the ast_exten object. pbx_substitute_variables acts as a wrapper for pbx_substitute_variables_helper, which is what actually does the work. pbx_substitute_variables_helper takes in char *.
> >     
> >     The bug here seems to be that we unlock the context lock before we should. Either pbx_substitute_variables should be removed and the values extracted in this function, or pbx_substitute_variables should be responsible for unlocking the contexts lock before calling the variable substiution function.
> 
> Scott Griepentrog wrote:
>     The pbx_substitute_* functions *can* refer to the ast_exten object's data member, as that's where the expression to evaluate is stored.  This is the very nature of the fault that valgrind is exposing - e->data has been free'd because of a reload operation on another thread before a reference to it occurring during pbx_substitute_variables and functions it calls.
>     
>     This lock accurately and reliably (per testing) prevents this failure.
>     
>     An option to eliminate the need for a lock would be to rewrite pbx_substitute_variables() so that it is passed e->data instead of e, which is the only element it references (and then passes into pbx_substitute_variables_helper()).  Then a copy of e->data could be put locally on stack or other allocation prior to releaseing the global contexts mutex, and then pass it to pbx_substitute_variables().  That could be more performance impacting than the lock, depending on the speed of the lock.  I could run a test to determine which is the higher speed approach.
>
> 
> Matt Jordan wrote:
>     Some of the pbx_substitute_* functions pass pointers from an ast_exten object to pbx_substitute_variables_helper_full. However, if you look at pbx_substitute_variables_helper_full - which is what actually does the work of substituting variables - it doesn't use ast_exten objects. It uses char * to set the variable on the channel (or global variable). All other functions are just wrappers on getting to that point.
>     
>     My point is that wrappers around pbx_substitute_variables_helper_full should not be passing pointers to data on an ast_exten object to this function. You're right that if you do that, you have to have something that protects usage of the ast_exten objects while elements stored on them are being manipulated by pbx_substitute_variables_helper_full - but if you first copy the data onto some other storage location - such as a variable on the stack - then you no longer need to hold the contexts lock during this function.
>     
>     My point still stands: you don't need this mutex. It is duplicating an existing lock, and by copying elements off of ast_exten and then calling a variant of pbx_substitute_variables_helper_full that takes in char *, you solve the problem without introducing more complicated logic.

We're agreed except where I was presuming that a read lock would be less overhead than allocating storage and copying the string.  But I believe you're right, the lock isn't a valid option here.  Reworking to use a stack allocation instead. 


- Scott


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


On Dec. 10, 2013, 1:39 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3055/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2013, 1:39 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: AST-1179 and AST-1246
>     https://issues.asterisk.org/jira/browse/AST-1179
>     https://issues.asterisk.org/jira/browse/AST-1246
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> During dialplan execution in pbx_extension_helper(), the contexts global read lock is used prevent changes to the dialplan.  This patch puts a copy of exten->data on the stack so that can be referenced safely during variable substitution (expression evaluation) even if another thread is reloading dialplan and has deleted ast_exten.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/pbx.c 403615 
> 
> Diff: https://reviewboard.asterisk.org/r/3055/diff/
> 
> 
> Testing
> -------
> 
> Testsuite test 'dialplan_stress' (https://reviewboard.asterisk.org/r/3056/) created to isolate this problem and test this fix.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131210/ceed976f/attachment-0001.html>