Summary:	ASTERISK-20296: In certain scenarios, asterisk can send rtp in an unsupported payload type to an endpoint
Reporter:	NITESH BANSAL (nbansal)	Labels:
Date Opened:	2012-08-22 07:08:37	Date Closed:	2012-09-05 11:15:07
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Channels/chan_sip/Interoperability
Versions:	11.0.0-beta1	Frequency of Occurrence	Constant
Related Issues:	is related to ASTERISK-11199 in certain scenarios, asterisk can send rtp in an unsupported payload type to an endpoint
Environment:	OS: Debian squeeze distribution x86_64 architecture	Attachments:	( 0) CODEC_NEGOTIATION_SIPP_SCRIPTS_AND_SIP_CONF_2.tar.gz ( 1) CODEC_NEGOTIATION_SIPP_SCRIPTS_AND_SIP_CONF.tar.gz ( 2) codec_negotiation.patch ( 3) codec_negotiation.patch
Description:	SIP caller A supports alaw/ulaw Asterisk is configured to support alaw,ulaw,g729 and places all 3 in the offer to B SIP called B supports g729,alaw,ulaw putting all 3 in the 200 OK. Asterisk sees that there is one common codec between A and B, it sets up a packet to packet bridge If B sends audio in G.729, it gets forwarded to A without transcoding which is expecting alaw/ulaw. Asterisk does not check the incoming payload from the peer and forwards to another peer even if the payload received was different from the payload expected on the bridge.
Comments:	By: NITESH BANSAL (nbansal) 2012-08-22 07:28:08.252-0500 I have resolved this issue. Please find the patch file attached: I have added a new function into rtp_engine.c to check for a payload code in the ast_rtp_codecs structure and called this function from bridge_p2p_rtp_write() in res_rtp_asterisk.c, it serves the purpose of checking the incoming payload against the codecs stored on the bridge. By: NITESH BANSAL (nbansal) 2012-08-24 03:16:07.567-0500 I would like to add that this issue was found out in the regression testing of an old JIRA issue: ASTERISK-11199 By: NITESH BANSAL (nbansal) 2012-08-24 03:20:06.420-0500 Resubmitting the same patch with a license file. By: NITESH BANSAL (nbansal) 2012-08-29 10:40:11.805-0500 Hello, Please find the SIPP scripts and sip.conf which can be used to simulate this scenario. In this case A Party supports alaw and ulaw, but receives G729. By: Mark Michelson (mmichelson) 2012-08-31 16:11:18.307-0500 The attached scenarios helped to debug this a lot. Thank you very much for that. The proper fix is actually much simpler than the patch you have submitted. The if statement you have patched in bridge_p2p_rtp_write of res_rtp_asterisk.c is doing an incorrect check for failure on its first condition. It assumes that a zero return is a failure for ast_rtp_codecs_payload_code(), but in actuality, a failing result is -1. Changing the first condition to compare to -1 fixes the issue for me when I use your attached scenarios. The problem with your patch is that it removes the format comparison logic that was there before. For static payload types (such as G.729 and G.711 alaw) this does not pose a problem. My fear is that for dynamic payload types, this may cause bridges to break due to incompatibility concerns that are not true. I am going to commit the simpler patch and close the issue. If you find a reason for this to be incorrect, then you can re-open this one and explain why. By: NITESH BANSAL (nbansal) 2012-09-03 04:08:32.036-0500 Hi Mark, I had a look at your patch and it does not solve the problem. If you look at the function <<ast_rtp_codecs_payload_code>>, after searching for the incoming payload on the bridge codecs, it also searches the array <<static_RTP_PT>> which is not correct. I am attaching another scenario which breaks ASTERISK, in this case, A side supports alaw but receives ulaw. By: NITESH BANSAL (nbansal) 2012-09-03 04:32:32.558-0500 Hi Mark, I can not see any option here to reopen the issues, so could you please re-open the issue? By: Mark Michelson (mmichelson) 2012-09-04 10:09:17.384-0500 The issue is open again. I believe just by commenting on it, you reopened it. Thanks for the new scenario. I'll give it a test. By: Matt Jordan (mjordan) 2012-09-04 10:13:29.711-0500 Nope, actually Michael Young opened it again (thanks :-)) By: Mark Michelson (mmichelson) 2012-09-04 16:24:39.493-0500 I've had a look over, and I see how checking {{static_RTP_PT}} is incorrect. Between Asterisk 10 and 11, the internal storage for codec payloads changed from an array to an ao2 container. It appears that as a result of this conversion, the semantics of checking compatibility in bridge_p2p_rtp_write() changed for the worse. Here's what the Asterisk 10 code looked like: {code} if (!(ast_rtp_instance_get_codecs(instance1)->payloads[bridged_payload].rtp_code) &&    !(ast_rtp_instance_get_codecs(instance1)->payloads[bridged_payload].asterisk_format)) {        return -1; } {code} Your change takes into account the first condition of the if statement. I'm not sure that the second condition is satisfied though. I *think* that an easy way to satisfy both conditions is just to make a single call to {code} ast_rtp_codecs_get_payload_format(ast_rtp_instance_get_codecs(instance1), bridged_payload) {code} This will first search for the payload type using the same search algorithm that your patch provides. It then also checks that the payload type found is an asterisk_format. I believe this will give the same semantics that the code in Asterisk 10 provided. What do you think? By: NITESH BANSAL (nbansal) 2012-09-05 05:21:41.329-0500 Hi Mark, The fix you have provided is working fine for my regression testing, i have just one question regarding the second condition, is it necessary that all the payload types have asterisk_format flag set to 1? By: Mark Michelson (mmichelson) 2012-09-05 10:41:35.081-0500 After looking at the code, I wasn't sure so I asked on IRC. David Vossel, the writer of the code in Asterisk 10 said the reason he thinks that the check for {{asterisk_format}} is there is because of the ulaw format. {{ast_rtp_instance_get_codecs(instance1)->payloads[bridged_payload].rtp_code}} would legitimately be 0 when the payload is ulaw. The extra check for {{asterisk_format}} was to account for this. He agrees that your change to find the codec in the ao2_container (i.e. your original patch) would do the trick here and satisfy backwards compatibility. Kevin Fleming stated that there should be a restriction to only passing Asterisk formats though since if the p2p bridge breaks (e.g. for hold), then Asterisk would not be able to communicate directly with one of the endpoints. I'm not sure that would be an issue though since Asterisk could just communicate using one of the negotiated Asterisk formats instead of the non-Asterisk format currently being used by the endpoint. In all, I think your original patch is the way to go here. If it turns out to cause problems, we can re-visit the possibility of restricting p2p bridges to Asterisk formats, but I don't think that will happen. By: NITESH BANSAL (nbansal) 2012-09-05 10:51:58.806-0500 Thanks for the explanation Mark and i do hope that original patch does not break anything :)