[asterisk-dev] [Code Review]: SIP INFO DTMF non-numeric codes treated as '1'

[Matt Jordan]

> On Feb. 8, 2012, 9:46 p.m., rmudgett wrote:
> >
> 
> rmudgett wrote:
>     This fix will also need to be done in v1.8.

Yes.  The issue was reported against the 10.1.0, hence why the patch was initially done in the 10 branch.  I've already backported this to a 1.8 branch as well - since the DTMF handling in handle_request_info is similar between the two branches, this patch applies to both branches without issue.


> On Feb. 8, 2012, 9:46 p.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, lines 19342-19343
> > <https://reviewboard.asterisk.org/r/1722/diff/1/?file=23956#file23956line19342>
> >
> >     The code starting from here to the end of the Content-Type application/dtmf-relay block is common with the appliction/dtmf block and probably should be factored out.

I've combined the two blocks into one.  The only difference between the two is the parsing of the message body, and since neither needs to do a whole lot to parse the message body, it seemed relatively clean to have them both be handled within the same block of code.


> On Feb. 8, 2012, 9:46 p.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, line 19360
> > <https://reviewboard.asterisk.org/r/1722/diff/1/?file=23956#file23956line19360>
> >
> >     Use %30u instead of %30ud.

Done.


> On Feb. 8, 2012, 9:46 p.m., rmudgett wrote:
> > /branches/10/channels/chan_sip.c, lines 19354-19355
> > <https://reviewboard.asterisk.org/r/1722/diff/1/?file=23956#file23956line19354>
> >
> >     I don't see why this needed to be moved.

I moved it because in the handling of the DTMF codes, we now check for:
a) All special characters (*, #, !)
b) Alphabetic characters
c) Numeric characters

as opposed to:
a) Numeric character
b) Some special characters (*, #)
c) Alphabetic characters
d) Some more special characters (!)

Having them grouped together seems cleaner to me.  Note that it was moving the numeric value checking above the other processing that caused the original regression, so having it flow from explicit cases to more general ranges of values seems to be safer.


- Matt


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


On Feb. 8, 2012, 5:43 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1722/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Summary
> -------
> 
> In ASTERISK-18924, SIP INFO DTMF handling was changed to account for both lowercase alphabetic DTMF events, as well as uppercase alphabetic DTMF events.  When this occurred, the comparison of the character buffer containing the event code was changed such that the buffer was first compared against '0' and '9' to determine if it was numeric.  Unfortunately, since the first character in the buffer will typically be '1' in the case of event codes 10, 11, 16, and (if non-alphabetic) 12-15 (for 'A' - 'D'), this caused those codes to be converted to a DTMF event of '1'.
> 
> Pressing '#' in voicemail resulted in hilarity.
> 
> This patch changes this to attempt to parse the DTMF codes first as '*', '#', '!', and 'A'/'a' - 'D'/'d'.  If those fail, it then converts the character buffer to an integer value; if this is invalid a 200 OK is sent and no DTMF tone is generated.
> 
> I didn't change the sending of a 200 OK for an invalid DTMF event sent from a UA, as that was the previous behavior.
> 
> 
> This addresses bug ASTERISK-19290.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19290
> 
> 
> Diffs
> -----
> 
>   /branches/10/channels/chan_sip.c 354428 
> 
> Diff: https://reviewboard.asterisk.org/r/1722/diff
> 
> 
> Testing
> -------
> 
> Test written for the Asterisk Test Suite (see review https://reviewboard.asterisk.org/r/1723/).  Without this patch the test fails, with it - great success.
> 
> 
> Thanks,
> 
> Matt
> 
>

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