[asterisk-dev] [Code Review] Enable gosub use in connected line, redirecting, and ccss

[Mark Michelson]

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


Besides what I've mentioned here, I think you should also go through the affected areas where macros are either configured or executed and add some deprecation warning messages.


trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1760/#comment10204>

    This pattern is used throughout this code. This will result in both the macro and gosub being run if both are configured. Seems like the logic used here should be to call one or the other, preferring the gosub version over the macro.



trunk/include/asterisk/app.h
<https://reviewboard.asterisk.org/r/1760/#comment10205>

    Bunch o red blobs.



trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1760/#comment10206>

    Red blobs.



trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1760/#comment10207>

    Red blobs.



trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1760/#comment10208>

    red blobs



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10212>

    Put some spacing into this.
    
    char *app_type = use_gosub ? "GoSub" : "Macro";



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10213>

    Name this something else since the variable declaration shadows a previous one.



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10214>

    What was the compiler error?



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10216>

    Why?



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10211>

    s/too/took/



trunk/main/app.c
<https://reviewboard.asterisk.org/r/1760/#comment10215>

    There is a potential for some buffer overflow here.
    
    The reason is that offset is set to the return value of a previous snprintf() call. snprintf() returns the number of bytes written. If the number of bytes to write is greater than the size specified in the second argument to snprintf(), then it will return the number of bytes that *would have been written* had the size limitation not arisen. This means that if someone tries to write 2048 bytes to your 1024 byte buffer, the offset returned from snprintf() would be 2048 even though snprintf() safely made sure to only print 1023 (1024th is a null byte) to the buffer.
    
    If offset should end up being larger than the size of the buffer, then this snprintf call would try to write past the end of the buffer because buf + offset is past the end, and sizeof(buf) - offset is interpreted as an incredibly large unsigned value.


- Mark


On Feb. 21, 2012, 7:47 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1760/
> -----------------------------------------------------------
> 
> (Updated Feb. 21, 2012, 7:47 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> To help along the deprecation of app_macro, this enables the use of app_stack (aka GoSub) in relation to connected line, redirecting, and call completion supplementary services.  The only major changes between the two implementations involve dropping the distinction between caller and callee for connected line and redirecting since this distinction never really made sense in the first place.
> 
> 
> This addresses bug SWP-4254.
>     https://issues.asterisk.org/jira/browse/SWP-4254
> 
> 
> Diffs
> -----
> 
>   trunk/UPGRADE.txt 356213 
>   trunk/apps/app_dial.c 356213 
>   trunk/apps/app_followme.c 356213 
>   trunk/apps/app_queue.c 356213 
>   trunk/configs/ccss.conf.sample 356213 
>   trunk/include/asterisk/app.h 356213 
>   trunk/include/asterisk/ccss.h 356213 
>   trunk/include/asterisk/channel.h 356213 
>   trunk/main/app.c 356213 
>   trunk/main/ccss.c 356213 
>   trunk/main/channel.c 356213 
>   trunk/main/dial.c 356213 
>   trunk/main/features.c 356213 
>   trunk/main/rtp_engine.c 356213 
> 
> Diff: https://reviewboard.asterisk.org/r/1760/diff
> 
> 
> Testing
> -------
> 
> Tested with the new tests to go with these changes here: https://reviewboard.asterisk.org/r/1761/
> 
> 
> Thanks,
> 
> opticron
> 
>

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