[Home]

Summary:ASTERISK-15573: [patch] T.38 negotiation fails with Patton SN2400
Reporter:Raivis Rengelis (raivisr)Labels:
Date Opened:2010-02-03 16:00:00.000-0600Date Closed:2010-02-09 11:49:54.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/T.38
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) faulty_sdp_debug.txt
( 1) t38-sdp-newline-fix1.diff
( 2) t38-sdp-newline-fix2.diff
Description:Patton SN2400 refuses to negotiate T.38 session because of extra newline in SDP.

[Feb  3 22:31:46] DEBUG[31017] chan_sip.c:    Body  5 [ 22]: m=image 4326 udptl t38
[Feb  3 22:31:46] DEBUG[31017] chan_sip.c:    Body  6 [  0]:
[Feb  3 22:31:46] DEBUG[31017] chan_sip.c:    Body  7 [ 17]: a=T38FaxVersion:0

proposed fix:
--- asterisk.orig/channels/chan_sip.c 2010-02-03 23:52:03.000000000 +0200
+++ asterisk/channels/chan_sip.c 2010-02-03 23:58:41.000000000 +0200
@@ -9191,8 +9191,6 @@ static enum sip_result add_sdp(struct si
  ast_str_append(&m_video, 0, "\r\n");
  if (needtext)
  ast_str_append(&m_text, 0, "\r\n");
- if (add_t38)
- ast_str_append(&m_modem, 0, "\r\n");

  len = strlen(version) + strlen(subject) + strlen(owner) +
strlen(connection) + strlen(session_time);
Comments:By: Leif Madsen (lmadsen) 2010-02-08 10:29:49.000-0600

Information about where and why the change was added:

lmadsen@asterisk-releaser:~/WORKING/asterisk/trunk/channels$ svn annotate -r100000:HEAD chan_sip.c | grep "m_modem" | more
184948       file struct ast_str *m_modem = ast_str_alloca(256);  /* Media declaration line for modem */
243860    russell ast_str_append(&m_modem, 0, "m=image %d udptl t38\r\n", ntohs(udptldest.sin_port));
225089       file ast_str_append(&m_modem, 0, "c=IN IP4 %s\r\n", ast_inet_ntoa(udptldest.sin_addr));
184948       file ast_str_append(&m_modem, 0, "\r\n");
184948       file len += m_modem->used + a_modem->used;
184948       file add_line(resp, m_modem->str);
lmadsen@asterisk-releaser:~/WORKING/asterisk/trunk/channels$ svn log -r184948 http://svn.asterisk.org/svn/asterisk/trunk
------------------------------------------------------------------------
r184948 | file | 2009-03-30 09:37:47 -0500 (Mon, 30 Mar 2009) | 21 lines

Merged revisions 184947 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r184947 | file | 2009-03-30 11:35:47 -0300 (Mon, 30 Mar 2009) | 14 lines
 
 Improve our handling of T38 in the initial INVITE from a device.
 
 We now answer with matching media streams to what is requested. If an INVITE
 is received with both a T38 and RTP media stream this means we answer with both.
 For any outgoing calls created as a result of this inbound one no T38 is requested
 in the initial INVITE. Instead if we start receiving udptl packets we trigger a
 reinvite on the outbound side.
 
 (closes issue ASTERISK-11843)
 Reported by: marsosa
 Tested by: pinga-fogo, okrief, file, afu
 
 Review: http://reviewboard.digium.com/r/208/
........

------------------------------------------------------------------------
By: Leif Madsen (lmadsen) 2010-02-08 10:31:20.000-0600

The issue related here is what caused this regression.
By: Leif Madsen (lmadsen) 2010-02-08 10:35:19.000-0600

After further discussion, the issue I reference is not likely to be the actual cause of this issue, as it would seem improbable to only have this issue reported nearly a year after it was originally committed.

At this point will leave it to a capable developer to do some additional debugging and searching.
By: Raivis Rengelis (raivisr) 2010-02-08 11:50:29.000-0600

looking at the source it seems that somebody just ran across piece of code:
if (needaudio)
ast_str_append(&m_audio, 0, "\r\n");
if (needvideo)
ast_str_append(&m_video, 0, "\r\n");
if (needtext)
ast_str_append(&m_text, 0, "\r\n");

and went on adding the same for if (add_t38) ignoring the fact that m_modem is built a little different and has trailing \r\n added already. that's it :)
By: Matthew Nicholson (mnicholson) 2010-02-08 15:07:31.000-0600

Do you have an example of the faulty SDP packet?
By: Matthew Nicholson (mnicholson) 2010-02-08 15:18:41.000-0600

I think I have found the problem, test with the patch I uploaded (it is identical to your suggested patch).
By: Matthew Nicholson (mnicholson) 2010-02-08 15:22:54.000-0600

I have also uploaded an alternative patch that should fix the bug.  Both patches should work equally well.
By: Raivis Rengelis (raivisr) 2010-02-08 15:23:03.000-0600

i already tested before reporting the bug :) and yes, it fixes it. (just uploaded full sip/sdp debug log that was failing)
By: Raivis Rengelis (raivisr) 2010-02-08 15:26:09.000-0600

yes, the second one should work as well. I just did not dwell into why original author went one way for audio/video/text and different way for t38. anyways both of those patches fix the problem with pattons and possibly other gateways.
By: Raivis Rengelis (raivisr) 2010-02-08 15:36:17.000-0600

btw, redundant \r\n was introduced in revision 243860, if that matters.
By: Digium Subversion (svnbot) 2010-02-09 11:47:51.000-0600

Repository: asterisk
Revision: 245727

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r245727 | mnicholson | 2010-02-09 11:40:04 -0600 (Tue, 09 Feb 2010) | 8 lines

This commit removes an extra newline in T.38 generated SDP packets. This bug was caused by the fix introduced in r243860.

(closes issue ASTERISK-15573)
Reported by: raivisr
Patches:
     t38-sdp-newline-fix1.diff uploaded by mnicholson (license 96)
Tested by: raivisr

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=245727
By: Digium Subversion (svnbot) 2010-02-09 11:49:54.000-0600

Repository: asterisk
Revision: 245728

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r245728 | mnicholson | 2010-02-09 11:43:41 -0600 (Tue, 09 Feb 2010) | 15 lines

Merged revisions 245727 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r245727 | mnicholson | 2010-02-09 11:40:04 -0600 (Tue, 09 Feb 2010) | 2 lines
 
 This commit removes an extra newline in T.38 generated SDP packets. This bug was caused by the fix introduced in r243860.

 (closes issue ASTERISK-15573)
 Reported by: raivisr
 Patches:
       t38-sdp-newline-fix1.diff uploaded by mnicholson (license 96)
 Tested by: raivisr
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=245728