[asterisk-dev] [Code Review] 3055: pbx.c: add lock around ast_exten use to prevent memory corruption

[Matt Jordan]

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



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/3055/#comment19726>

    So, as Mark alluded to, I would just remove these functions and use the actual calls to ast_rwlock*.
    
    The reason there is a wrapper around the contexts lock functions here in pbx.c is historical. At one point, there was an attempt to use read-write locks for the contexts lock. However, due to the recursive nature of some of the function calls, the locks could be obtained multiple times, resulting in invalid locking with r/w locks. A wrapper was provided in the (probably vain) hope that someday we'd be able to unravel the recursive calls and switch to r/w locks which - since dialplan isn't modified terribly often - provide some performance improvement.
    
    So, just use the direct calls :-)



/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/3055/#comment19727>

    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.


- Matt Jordan


On Dec. 6, 2013, 10:44 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3055/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2013, 10:44 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 adds a lock between that point and until after the extension previously found is no longer being referenced -- after variable substitution, but before app execution.  This new lock prevents the extension's allocation from being released by module reload when it is still being used in an expression evaluation.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/pbx.c 403430 
> 
> 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/20131207/ca2c5b23/attachment-0001.html>