Summary:	ASTERISK-16086: [patch] Computation of Content-Length is wrong under some circumstances
Reporter:	Richard Kenner (kenner)	Labels:
Date Opened:	2010-05-12 10:14:49	Date Closed:	2010-06-22 08:05:16
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/Interoperability
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) chan_sip.c.diff ( 1) chan_sip.c.diff.2 ( 2) sip.patch7.1.6.2.diff ( 3) sip-automatic-content-length162-8.diff ( 4) sip-automatic-content-length2.diff ( 5) sip-automatic-content-length7.diff ( 6) sip-automatic-content-length8.diff
Description:	add_sdp has a bug in the computation of Content-Length.  It first computes the length by adding together the sizes of a large number of things, then outputs the length, THEN the things.  That's error-prone if the computation doesn't agree with what's written.  And, in fact, it doesn't, in some cases. In each of the cases where it write a dummy media line, that line isn't included into the count.  This causes interoperability issues with the Polycom VSX 7000 since it checks the Content-Length.  I have the fairly obvious patch to account for the length of these lines, but it just makes the kludge of that part of the code worse.  My recommendation is to not use add_line, but instead to build up a single string and then take the size of that string for the header and then outut it.
Comments:	By: Leif Madsen (lmadsen) 2010-05-13 09:24:31 Thanks for the submission and bug report. A developer will look at this as soon as time and resources allow. Thanks! By: Matthew Nicholson (mnicholson) 2010-06-04 12:46:13 Test the patch I just uploaded to see if it fixes your issue. By: Richard Kenner (kenner) 2010-06-05 09:47:23 The patch you uploaded appears to be for 1.8.  I backported it to 1.6.2 (attached) and tested that and that indeed appears to work just fine.  Thanks. By: Matthew Nicholson (mnicholson) 2010-06-07 12:52:52 Please test again with v7 of the patch.  You will have to backport it again. By: Richard Kenner (kenner) 2010-06-07 20:55:35 This works too and I attached the backport I used. By: Richard Kenner (kenner) 2010-06-07 21:32:25 I found a problem with this version of the patch.  All occurrences of Content-Length=>0 need to be deleted from sip_notify.conf or else two copies of that header will be written. By: Matthew Nicholson (mnicholson) 2010-06-08 10:40:04 Just uploaded v8 of the patch that should fix the duplicate content-length header issue. By: Richard Kenner (kenner) 2010-06-08 21:00:03 This seems fine now (but the demo sip_notify.conf ought to be changed). By: Digium Subversion (svnbot) 2010-06-22 07:54:39 Repository: asterisk Revision: 271689 U   branches/1.4/channels/chan_sip.c U   branches/1.4/configs/sip_notify.conf.sample ------------------------------------------------------------------------ r271689 | mnicholson | 2010-06-22 07:52:27 -0500 (Tue, 22 Jun 2010) | 8 lines Modify chan_sip's packet generation api to automatically calculate the Content-Length.  This is done by storing packet content in a buffer until it is actually time to send the packet, at which time the size of the packet is calculated.  This change was made to ensure that the Content-Length is always correct. (closes issue ASTERISK-16086) Reported by: kenner Tested by: mnicholson, kenner Review: https://reviewboard.asterisk.org/r/693/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=271689 By: Digium Subversion (svnbot) 2010-06-22 07:58:28 Repository: asterisk Revision: 271690 _U  trunk/ U   trunk/channels/chan_sip.c U   trunk/channels/sip/include/sip.h U   trunk/include/asterisk/strings.h ------------------------------------------------------------------------ r271690 | mnicholson | 2010-06-22 07:58:27 -0500 (Tue, 22 Jun 2010) | 18 lines Merged revisions 271689 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........  r271689 | mnicholson | 2010-06-22 07:52:27 -0500 (Tue, 22 Jun 2010) | 8 lines    Modify chan_sip's packet generation api to automatically calculate the Content-Length.  This is done by storing packet content in a buffer until it is actually time to send the packet, at which time the size of the packet is calculated.  This change was made to ensure that the Content-Length is always correct.    (closes issue ASTERISK-16086)  Reported by: kenner  Tested by: mnicholson, kenner    Review: https://reviewboard.asterisk.org/r/693/ ........ This change also adds an ast_str_copy_string() function (similar to ast_copy_string), that copies one ast_str into another, properly handling embedded nulls. ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=271690 By: Digium Subversion (svnbot) 2010-06-22 08:05:16 Repository: asterisk Revision: 271691 _U  branches/1.6.2/ U   branches/1.6.2/channels/chan_sip.c U   branches/1.6.2/configs/sip_notify.conf.sample U   branches/1.6.2/include/asterisk/strings.h ------------------------------------------------------------------------ r271691 | mnicholson | 2010-06-22 08:05:15 -0500 (Tue, 22 Jun 2010) | 25 lines Merged revisions 271690 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................  r271690 | mnicholson | 2010-06-22 07:58:28 -0500 (Tue, 22 Jun 2010) | 18 lines    Merged revisions 271689 via svnmerge from  https://origsvn.digium.com/svn/asterisk/branches/1.4    ........    r271689 | mnicholson | 2010-06-22 07:52:27 -0500 (Tue, 22 Jun 2010) | 8 lines        Modify chan_sip's packet generation api to automatically calculate the Content-Length.  This is done by storing packet content in a buffer until it is actually time to send the packet, at which time the size of the packet is calculated.  This change was made to ensure that the Content-Length is always correct.        (closes issue ASTERISK-16086)    Reported by: kenner    Tested by: mnicholson, kenner        Review: https://reviewboard.asterisk.org/r/693/  ........      This change also adds an ast_str_copy_string() function (similar to ast_copy_string), that copies one ast_str into another, properly handling embedded nulls. ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=271691