| Summary: | ASTERISK-20897: case sensitive match against T.38 params causes T38MaxBitRate to be negotiated at 2400 baud instead of 14400 | ||||||
| Reporter: | Eric Hill (erichill) | Labels: | |||||
| Date Opened: | 2013-01-04 15:05:56.000-0600 | Date Closed: | 2013-01-30 08:29:58.000-0600 | ||||
| Priority: | Major | Regression? | |||||
| Status: | Closed/Complete | Components: | Channels/chan_sip/T.38 | ||||
| Versions: | 11.1.2 | Frequency of Occurrence | Constant | ||||
| Related Issues: |
| ||||||
| Environment: | Linux x86/64, however unimportant to the issue. | Attachments: | ( 0) ASTERISK-20837-1.8.diff | ||||
| Description: | This problem started as an interoperability report with Level 3 communications T.38 fax service and an XMedius fax server with Asterisk handling the call supervision. Faxes were being negotiated at 2400 baud rather than 14400 baud. During testing, the XMedius fax server would indicate a=T38maxBitRate:14400, however Asterisk would relay a=T38MaxBitRate:2400 to Level 3.
I traced the problem to chan_sip line 11138: ...sscanf(a, "T38MaxBitRate:%30u"... sscanf is case sensitive with static strings, and as such, "T38MaxBitRate" does not match the offered "T38maxBitRate" from XMedius, and as such doesn't parse the 14400 into the rate field in the structure. The structure baudrate is defaulted to the lowest value (2400), so the lower speed is offered to Level 3. RFC5347 section 2.5.2 includes this statement: ... The attribute "T38MaxBitRate" was once incorrectly registered with IANA as "T38maxBitRate" (lower-case "m"). In accordance with T.38 examples and common implementation practice, the form "T38MaxBitRate" SHOULD be generated by implementations conforming to this package. In general, it is RECOMMENDED that implementations of this package accept lowercase, uppercase, and mixed upper/lowercase encodings of all the T.38 attributes. ... Asterisk would be better compatible with other T.38 systems by implementing a case-insensitive match against T.38 parameters since the specification was incorrectly registered in past times. By being case-sensitive, asterisk is compatible only at the slowest 2400 baud with equipment using the incorrect T38maxBitRate name. I have created this issue at Major since no workaround is possible without a code change. | ||||||
| Comments: | By: Rusty Newton (rnewton) 2013-01-11 09:59:06.601-0600 Thanks for reporting this. If you can provide a patch, that will always speed things up and be greatly appreciated. By: Eric Hill (erichill) 2013-01-11 14:39:03.942-0600 I have a patch for you, but I'm waiting on the license agreement to be approved. By: Matt Jordan (mjordan) 2013-01-13 16:35:32.561-0600 Sorry that the license approval is taking a bit - I've attached a patch as well that I think will fix this issue per the full recommendation of the RFC. Let me know if this approach works for you - if you're okay with it, I'll make sure you get proper attribution for providing a solution as well. By: Eric Hill (erichill) 2013-01-13 19:53:05.644-0600 Your changes look good to me. I was going to use the sscanf %*[tT]38%*[mM]... approach to avoid converting the input to lower case first, but your code is easier to read while trying to figure out its' purpose. I'll get your diff applied to a code base and test it in production and let you know how it works out. Thanks, Eric By: Matt Jordan (mjordan) 2013-01-13 19:57:33.016-0600 Hm... but the regular expression approach is probably more performant. I kind of like that approach better! Let's get the license approved and we can pick one. :-) By: Eric Hill (erichill) 2013-01-13 20:02:46.077-0600 Sounds like a plan to me. :) By: Eric Hill (erichill) 2013-01-14 08:19:42.025-0600 sscanf(a, "T38FaxMaxRate:%30u", &x) converted to: sscanf(a, "%*[tT]38%*[mM]%*[aA]%*[xX]%*[bB]%*[iI]%*[tT]%*[rR]%*[aA]%*[tT]%*[eE]:%30u", &x) Will match T38FaxMaxRate, T38faxMaxRate, t38faxmaxrate, T38fAxMaXrAtE, and every other combination of upper and lower case letters. It's harder to read than your solution, but doesn't have the side effect of converting the input to lower case. Let me know your thoughts. By: Walter Doekes (wdoekes) 2013-01-18 04:56:22.706-0600 scanf != regular expressions I like the tolower() because it has to be done exactly once. The various strcmp/scanf scanning later on will be more "performant". By: Walter Doekes (wdoekes) 2013-01-18 04:59:59.733-0600 (If you're worried about performance, then use a custom tolower() that lowers only A-Z and doesn't do charset lookups for non-C locales.) By: Matt Jordan (mjordan) 2013-01-29 08:24:35.187-0600 Writing a custom tolower() feels like a nice enhancement, but I'm not sure it's really needed here. We should only be parsing the attributes during SDP negotiation, and that shouldn't happen terribly often. If it turns out we need it, we can certainly add it here. | ||||||