Summary:	ASTERISK-09110: [patch] Valid RTP stream can be treated as invalid
Reporter:	Peter Kocsis (doodlehu)	Labels:
Date Opened:	2007-03-27 05:18:01	Date Closed:	2007-06-19 12:08:47
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/RTP
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) rtp.c.diff
Description:	The CC field of the RTP header is not interpreted at all (see RFC 3550, chapter 5.1), which means that RTP streams containing CSRC fields will be misinterpreted, and the optional CSRC header (following the fixed RTP header) will be interpreted as part of the audio payload. This generates warnings in the Asterisk log, and makes clicky and wrong audio, depending on the audio codec being used. ****** ADDITIONAL INFORMATION ****** I've tried to find the problem in the Asterisk code, and found it in rtp.c, please see the attached diff for it. I think that the changes for ast_rtp_read() are clean, the only thing which might require some explanation is the change in bridge_p2p_rtp_write(). The code there seemed to be buggy, because it took the Padding, Mark and Ext bits, but only used masking, so the bits stayed at their original places. Later, when it reconstructed the packet header, it assumed that the Mark bit is either 0 or 1, and shifted it up, practically shifting it out from 32 bits. This means that the Mark bit was never really forwarded. Another problem with the old approach was that the packet header was reconstructed without keeping an eye on the Ext and CC fields, so if the original packet contained an optional CSRC list or an extended header after the fix RTP header, the forwarded packet did not have the info that they are there, and those became part of the payload.
Comments:	By: Serge Vecher (serge-v) 2007-03-27 08:53:24 thanks for contributing -> can you please get a disclaimer on file and mention here when done? By: Peter Kocsis (doodlehu) 2007-03-27 09:24:13 Sorry, I'm new to this, and left the "Disclaimer on File" field on "No". So I disclaim all rights for the patch. By: Serge Vecher (serge-v) 2007-03-27 09:32:46 can you please go to the bottom of the following page (http://bugs.digium.com/main_page.php) and fax one of the signed disclaimer forms to Digium? By: Peter Kocsis (doodlehu) 2007-03-28 05:03:46 Signed disclaimer was faxed today. By: Daniel Floyd (dmfloyd) 2007-04-20 11:01:19 I can verify that packets containing RTP extension headers do indeed get forwarded without the ext header bit set as described above under Additional Information.  The ext headers bytes remain in the forwarded packet but become part of the payload (which royally upsets GSM, btw).  I tested against SVN revision 61689.  The above patch did not apply cleanly but after applying it by hand, it fixed at least the RTP ext issue.  I was contemplating submitting a bug for the RTP extension issue, but found this instead. By: Joshua C. Colp (jcolp) 2007-06-19 12:08:46 Fixed in 1.2 as of revision 69992, 1.4 as of revision 70003, and trunk as of revision 70006. Thanks!