[asterisk-dev] [Code Review] 2620: bridge features: Make one touch Monitor and MixMonitor into bridge features

[Mark Michelson]

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



/trunk/bridges/bridge_builtin_features.c
<https://reviewboard.asterisk.org/r/2620/#comment17677>

    This idiom, in tandem with how set_touch_variables() is used, may cause a memory leak.
    
    *touch_format is guaranteed to allocate some memory, either copying c_touch_format or copying an empty string. The other touch variables follow the same pattern. If all three touch variables copy an empty string, then it means that set_touch_variables() will return non-zero.
    
    In the places where set_touch_variables() is used, if the return is non-zero, then set_touch_variables() is called a second time with the same pointers passed in. This will result in the original allocations of empty strings being overwritten. The second allocation will be properly freed since the pointers are RAII_VARs.
    
    There are a few possible fixes:
    
    1) Change the ast_strdup() to be
    if (!ast_strlen_zero(c_touch_format)) {
        *touch_format = ast_strdup(c_touch_format);
        res = 0;
    }
    This way, there is a direct correlation between duplicating a valid string and returning 0.
    
    2) Have distinct sets of pointers to pass into set_touch_variables() so that you don't overwrite allocations
    
    3) Explicitly free the pointers after the first call to set_touch_variables() before calling it a second time.


- Mark Michelson


On June 26, 2013, 8:50 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2620/
> -----------------------------------------------------------
> 
> (Updated June 26, 2013, 8:50 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: asterisk-21553
>     https://issues.asterisk.org/jira/browse/asterisk-21553
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds Monitor and MixMonitor as bridge features.  In order to avoid using PBX execution and autoservice, I added some additional API for MixMonitor provided through loadable function callbacks provided by app_mixmonitor.
> 
> In order to make One Touch MixMonitor behave more like its Monitor equivalent, I went ahead and added a couple channel variables for playing start and stop recording sounds (TOUCH_MIXMONITOR_MESSAGE_START and TOUCH_MIXMONITOR_MESSAGE_STOP)
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 392970 
>   /trunk/apps/app_mixmonitor.c 392970 
>   /trunk/bridges/bridge_builtin_features.c 392970 
>   /trunk/configs/features.conf.sample 392970 
>   /trunk/include/asterisk/features_config.h 392970 
>   /trunk/include/asterisk/mixmonitor.h PRE-CREATION 
>   /trunk/main/features_config.c 392970 
>   /trunk/main/mixmonitor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2620/diff/
> 
> 
> Testing
> -------
> 
> Tried both Monitor and MixMonitor with various channel variables set for filename/format and start/stop sounds. They appear to be working as one would expect.
> Also checked the behavior of MixMonitor with app_mixmonitor loaded and unloaded. As expected, it is rejected smoothly when MixMonitor is not loaded.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130627/86ad5d8f/attachment-0001.htm>