Summary:	ASTERISK-14964: [patch] crash in ast_rtp_instance_early_bridge_make_compatible() when directmedia=yes
Reporter:	Elazar Broad (ebroad)	Labels:
Date Opened:	2009-10-08 16:50:13	Date Closed:	2013-01-14 14:40:56.000-0600
Priority:	Critical	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_skinny
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) bt.txt ( 1) chan_skinny-rtp-fixup.txt
Description:	Looks like glue1->get_codec() is null in ast_rtp_instance_early_bridge_make_compatible.
Comments:	By: dea (dea) 2010-07-28 15:00:48 I have a fix for this one.  Chan_skinny does not have the get_codec function, and the new rtp code blindly calls for it.  It is less than 10 lines of code to add to chan_skinny, so here is is. By: Damien Wedhorn (wedhorn) 2010-07-28 15:41:26 Dan, worth testing to see if the various things exist (and also, use l for line). I'd suggest, or equivalent: if (chan->techpvt && chan->tech_pvt->parent) {  return chan->tech_pvt->parent->capability; } PS we really need to rename sub->parent to sub->line. By: dea (dea) 2010-07-28 16:21:57 The code is lifted out of chan_mgcp and nearly identical to chan_sip.  I limited the changes to only identifying the correct channel structure. I assumed that the original author felt such tests to be extranious.  I just checked the other six channels with this function and they are basically identical to the proposed addition.  Of note, I also see three other channels that use rtp and lack this function, so are likely to segfault if used-  chan_h323, chan_unistim and chan_multicast_rtp I kind of like the sub->parent name, but I do not feel strongly about it.  That whould be a seperate project/cleanup task? By: Damien Wedhorn (wedhorn) 2010-07-28 16:35:53 Not sure about mgcp, but unlike skinny, SIP is a point to point protocol. Basically, SIP tech_pvt is extra data for chan. On skinny, tech_pvt is extra data for chan AND data for the device. As such, generally for SIP, if there's a chan, there is also a chan->tech_pvt. However, in skinny we can't rely on that. During dialing, there is a sub with no associated channel. At the end of a call, we break chan and sub apart for the cleanup. If there is a codec request during the hangup we could get a segfault with your code. However, on second thought it would likely be due to some other bug where asterisk is not aware the channel is going down. So maybe leave it as is. I'll ponder it some more. PS The change to sub->line would be a separate bug. By: dea (dea) 2010-07-30 11:20:33 Wedhorn is right that this is not complete.  The correct short term fix has already been merged to rtp_engine.c, so this change is not needed. We should develop a valid get_codec() for chan_skinny as I assume it was added to the core for a reason, but that is critical at this point. By: Matt Jordan (mjordan) 2013-01-14 14:40:21.708-0600 I've gone ahead and discarded the Review Board review for this (https://reviewboard.asterisk.org/r/816/). Since {{rtp_engine}} now checks for the presence of {{get_codec}}, the crash referenced in this issue should no longer occur. As such, I'm going to close this issue out, as an implementation of {{get_codec}} for {{chan_skinny}} would arguably be an improvement to the implementation, and doesn't need to explicitly be tracked in the issue tracker. By: Matt Jordan (mjordan) 2013-01-14 14:40:56.202-0600 Of course, @wedhorn, if you want to re-open this to track the implementation, feel free to do so :-)