[Home]

Summary:ASTERISK-14035: [patch] Invalid SDP connection information (c=) parsing leading to one way audio
Reporter:frawd (frawd)Labels:
Date Opened:2009-04-29 10:33:14Date Closed:2009-12-07 14:20:21.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) example_invite1.txt
( 1) sdp_c_parse.patch
( 2) sdp_parsing_asterisk14_rev220097.diff
( 3) sdp_parsing_new_asterisk_1.4.26.2.diff
( 4) sdp_parsing_new_asterisk14_rev221083.diff
( 5) sdp_parsing_new_asterisk14_rev221961.diff
( 6) sdp_parsing.patch
( 7) temp.c
Description:I have a one-way audio problem when a media-gateway was (re)inviting with audio and video on two different IPs, with an SDP formatted like so:

m=audio
c=IN IP4 <ip1>
[...]
m=video
c=IN IP4 <ip2>
[...]

Looking at the code it appears that asterisk has the following algorithm for parsing "c=" lines:

1. Look for the first "c=" line and initialize audio and video address structure
=> in my case, <ip1> is then initialized for audio and video
2. For each "m=" line found:
2.1. Look for the next "c=" line STARTING AFTER THE LAST ONE (step 1.)
=> in my case, <ip2> will be found
2.2. Initialize audio or video structure depending on what was in "m="
=> in my case, <ip2> is initialized for the audio part

As a result I have <ip2> for audio and <ip1> for video instead of the contrary!!!

Could someone please confirm my analysis? I could be totally wrong..

****** ADDITIONAL INFORMATION ******

I haven't looked at the code of 1.6 or trunk branch, only the latest 1.4 branch, and have not made any patch as I'm not sure how this should really be done.

A capture shows that RTP is sent to <ip2> with the correct port.
Comments:By: frawd (frawd) 2009-04-29 11:07:15

Looking more into the process_sdp function of chan_sip, there are more potential problems, as the "a=" parsing is also not done per media stream (per "m=").

I actually just care about the "c=" problem, as it is more than a potential issue for me (my clients are screaming), so just forget about this note! :-)
By: frawd (frawd) 2009-04-29 11:14:18

Just uploaded a simple fix for my problem (not tested) against 1.4 branch revision 191087.
By: frawd (frawd) 2009-05-07 14:38:38

I think this process_sdp function was first designed for audio streams, and then some dirty people made some ugly add-ons when support for video and image (T.38) was added.

I will try to upload a patch to reorganize that a bit, it shouldn't be huge.

Any testers (at least for regressions) willing to help me when I'll have this done?
By: frawd (frawd) 2009-05-07 15:00:37

Patch is uploaded (against 1.4 svn), works well so far with a few different (soft)phones so I think there are no regressions AFAIK.

I will check next monday if it resolves the bug.

Can someone please review or comment?

Thanks!
By: Leif Madsen (lmadsen) 2009-05-12 14:02:20

frawd: it's monday! :)  How went your testing?
By: Leif Madsen (lmadsen) 2009-05-12 14:02:46

Actually I lied... it's Tuesday... ouch :)
By: frawd (frawd) 2009-05-14 05:12:54

Sorry, I couldn't test so far in that production machine.

Could you help me out testing in normal cases, to know if this patch doesn't break the current functionality?

I'll report as soon as I verify this in my special case!
By: Leif Madsen (lmadsen) 2009-06-10 13:07:04

I don't have the infrastructure to test this issue. Please setup a development/testing environment that mirrors your configuration to test this patch prior to deployment. Thanks!
By: frawd (frawd) 2009-06-22 03:59:09

The infrastructure needed to test if that patch was breaking everything was just a computer, softphone and maybe a SIP account with a cheap provider. :-)

Anyways, I've been risking and testing in several production servers, with several SIP phones and softphones and providers, and so far so good!
By: Matthew Nicholson (mnicholson) 2009-09-22 15:50:58

This patch looks good. I do have a few comments. First, the patch needs to be updated to apply cleanly to the current 1.4 branch. The parsing code is an improvement, but your code needs to be updated to parse session level parameters such as 'a=' in addition to the current parsing of media level parameters. Other than that, it looks good.

One additional note: before updating your patch for the current 1.4 branch, review the latest coding guidelines for changes in the way scanf formatting should be done.
By: frawd (frawd) 2009-09-22 17:12:02

will do very soon, thanks looking at it!
By: frawd (frawd) 2009-09-24 08:30:32

Attached new patch against current 1.4 branch.

I reviewed coding guidelines for scanf formatting and found nothing wrong, tell me if something specific was in your mind.

Also by "session level parameters", do you mean those global parameters happening before the first "m="?

It's true they are not parsed, if you think it's ok, I will move the parsing of audio/video/image specific parameters into small functions so that:
1. It's clearer and easier to read/modify
2. I can call them several times (for global params and for media specific params).

I've been using the current patch since about May-June 2009, with no problems at all, and a small improvement in simultaneous new sessions per seconds while load-testing against non-patch version (I guess because it parses a bit better).
By: Matthew Nicholson (mnicholson) 2009-09-24 08:47:41

The section of the coding guidelines I am referring to is as follows:

"Also, to avoid a potential
libc bug, always specify a maximum width for each conversion specifier,
including integers and floats.  A good length for both integers and floats is
30, as this is more than generous, even if you're using doubles or long
integers."

There may be examples of proper sscanf usage in chan_sip, and they can certainly be found in other asterisk modules.

Your understanding of "session level parameters" is correct.  I am not sure if I am using the proper term for that.

Your purposed solution sounds reasonable.
By: frawd (frawd) 2009-09-25 04:11:27

Sorry about not reading well the coding guidelines, I thought you were referring to the %i => %d guideline. This will be fixed soon.

About the "session level parameters", I will then do that solution. As the change compared to the current patch is not so important, it shouldn't last more than a few days to implement and test it well in production environment.
The current patch was tested in all versions from 1.4.24 to 1.4.26.2 in about 50 production servers against many different devices.

Thanks a lot for making that move.
By: frawd (frawd) 2009-09-25 12:32:56

mnicholson, could you please take a look at my current work (temp.c)?

More or less, this is my idea of how SDP parsing will work. I haven't tested including that in chan_sip.c nor compiling/testing it yet (next week), but to give you a preview of the general idea. Next week I will finish and test that until it works in all production machines I have (shouldn't take more than a few days).

I would appreciate any positive or negative comments, I think this new parsing will really be great for maintenance or to easily add new features (think trunk). Be kind, it's a work in progress. :-)

I will also resolve a bug I saw with some softphones adding capability for media specific (audio/video) sendonly/inactive/sendrecv feature. Asterisk is currently stopping all audio/video when an a=inactive option was found under an m=video.
By: Matthew Nicholson (mnicholson) 2009-09-25 13:46:06

I like that approach.  It is a bit cleaner than the way things are currently done.  There are some bugs in that code, but I am guessing that is because you haven't attempted to compile or run it.

Once you get something working, post it on review board, and I will take a look at it.
By: frawd (frawd) 2009-09-30 05:43:35

New patches for branch 1.4 and 1.4.26.2.

Tested in production 1.4.26.2 and it works well at least for audio and video with several different SIP devices.

I would need help for more testing, but it's mainly moving code around so there shouldn't be (m)any introduced bugs.
By: frawd (frawd) 2009-09-30 06:07:57

mnicholson: posted on review board as well.
By: Leif Madsen (lmadsen) 2009-09-30 10:42:45

What is the link to the reviewboard post for historical purposes?
By: frawd (frawd) 2009-09-30 12:47:34

https://reviewboard.asterisk.org/r/385/
By: frawd (frawd) 2009-10-02 07:18:13

uploaded latest version changing a couple of if/else to switch, see diff_r2 from reviewboard:

https://reviewboard.asterisk.org/r/385/
By: frawd (frawd) 2009-10-14 03:42:01

Uploaded an example invite packet (from sip debug) where it would fail with the current version, for having media specific connection info (c=) for audio and video.

This packet also shows a media specific a=recvonly parameter that should be applied to video only.
By: Digium Subversion (svnbot) 2009-11-04 14:01:12.000-0600

Repository: asterisk
Revision: 227758

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r227758 | mnicholson | 2009-11-04 14:01:11 -0600 (Wed, 04 Nov 2009) | 10 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd
Tested by: frawd, mnicholson, file

Review: https://reviewboard.asterisk.org/r/385/

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

http://svn.digium.com/view/asterisk?view=rev&revision=227758
By: Digium Subversion (svnbot) 2009-11-04 14:19:17.000-0600

Repository: asterisk
Revision: 227759

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r227759 | mnicholson | 2009-11-04 14:19:17 -0600 (Wed, 04 Nov 2009) | 10 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd
Tested by: frawd, mnicholson, file

Review: https://reviewboard.asterisk.org/r/385/

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

http://svn.digium.com/view/asterisk?view=rev&revision=227759
By: Digium Subversion (svnbot) 2009-11-04 14:22:01.000-0600

Repository: asterisk
Revision: 227759

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r227759 | mnicholson | 2009-11-04 14:13:50 -0600 (Wed, 04 Nov 2009) | 10 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd
Tested by: frawd, mnicholson, file

Review: https://reviewboard.asterisk.org/r/414/

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

http://svn.digium.com/view/asterisk?view=rev&revision=227759
By: Digium Subversion (svnbot) 2009-11-04 14:22:33.000-0600

Repository: asterisk
Revision: 227760

U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r227760 | mnicholson | 2009-11-04 14:22:33 -0600 (Wed, 04 Nov 2009) | 7 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd

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

http://svn.digium.com/view/asterisk?view=rev&revision=227760
By: Digium Subversion (svnbot) 2009-11-04 14:22:52.000-0600

Repository: asterisk
Revision: 227761

U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r227761 | mnicholson | 2009-11-04 14:22:51 -0600 (Wed, 04 Nov 2009) | 7 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd

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

http://svn.digium.com/view/asterisk?view=rev&revision=227761
By: Digium Subversion (svnbot) 2009-11-04 14:23:08.000-0600

Repository: asterisk
Revision: 227763

U   branches/1.6.0/channels/chan_sip.c

------------------------------------------------------------------------
r227763 | mnicholson | 2009-11-04 14:23:07 -0600 (Wed, 04 Nov 2009) | 7 lines

Modify the SDP parsing code to parse session and media level items separately.

With the new code, media level proprieties should no longer be confused with session level proprieties. This change also reorganizes some of the SDP parsing code which should make it easier to manage in the future.

(closes issue ASTERISK-14035)
Reported by: frawd

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

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