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

[opticron]

> On Feb. 22, 2012, 11:50 a.m., Mark Michelson wrote:
> > trunk/main/app.c, line 268
> > <https://reviewboard.asterisk.org/r/1760/diff/1/?file=24462#file24462line268>
> >
> >     Name this something else since the variable declaration shadows a previous one.

Done.


> On Feb. 22, 2012, 11:50 a.m., Mark Michelson wrote:
> > trunk/main/app.c, line 298
> > <https://reviewboard.asterisk.org/r/1760/diff/1/?file=24462#file24462line298>
> >
> >     Why?

If passed in as-is, the priority will be decremented (usually to 0 in which case the extension will never begin execution).  This is documented in the code in a few places, but could be the result of a bug.  I'd have to dig in a bit deeper to find out for sure.


> On Feb. 22, 2012, 11:50 a.m., Mark Michelson wrote:
> > trunk/main/app.c, line 318
> > <https://reviewboard.asterisk.org/r/1760/diff/1/?file=24462#file24462line318>
> >
> >     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.

This should now be addressed properly.


> On Feb. 22, 2012, 11:50 a.m., Mark Michelson wrote:
> > trunk/apps/app_dial.c, lines 959-964
> > <https://reviewboard.asterisk.org/r/1760/diff/1/?file=24455#file24455line959>
> >
> >     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.
> 
> rmudgett wrote:
>     I would suggest this replacement code pattern:
>     if (ast_channel_redirecting_sub(...)
>         && ast_channel_redirecting_macro(...)) {
>         ast_channel_update_redirecting(...);
>     }
>     
>     The same would apply to the connected line updates as well of course.

Addressed in the new patch.


> On Feb. 22, 2012, 11:50 a.m., Mark Michelson wrote:
> > trunk/main/app.c, line 272
> > <https://reviewboard.asterisk.org/r/1760/diff/1/?file=24462#file24462line272>
> >
> >     What was the compiler error?

I don't have an issue using a regular initializer, but it seems this has been the case in the past (possibly with older versions of gcc) as documented in other portions of code that have probably been copied from the original.  Switched to an initializer.


- opticron


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


On Feb. 26, 2012, 6:58 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1760/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2012, 6:58 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/main/rtp_engine.c 356213 
>   trunk/main/dial.c 356213 
>   trunk/main/features.c 356213 
>   trunk/main/channel.c 356213 
>   trunk/main/ccss.c 356213 
>   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 
> 
> 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/20120227/db9c2a86/attachment-0001.htm>