<div dir="ltr"><div>I'm not sure this should have been committed.<br><br></div>I hadn't seen that this had already been put up for review, otherwise I would have commented on the review - however, I did note the following on the issue:<br>
<br>You can't delay a masquerade. Failing to complete the fix up here
will result in a masqueraded (zombie) channel with an active session.
Anything using that session will be pointing to the Zombie channel, and -
assuming they don't blow up in some spectacular fashion - have a high
likelihood of simply failing.
<p>Even if we assume that the task can be put at the front of the
taskprocessor queue, any operation currently in flight will be operating
on an invalid channel.</p>
<p>I only see two possibilities here:</p>
<ol><li>Fix the locking inversion between the task processor and the entity
pushing the synchronous task. That is, you can't hold a channel lock
when pushing a synchronous task.</li><li>Go with a modified version of the patch, but assume that the session object has to be protected as well.</li></ol><p><br></p><p>How does this patch address these concerns?</p><p><br>
</p><div><div><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">SVN commits to the Digium repositories</b> <span dir="ltr"><<a href="mailto:svn-commits@lists.digium.com">svn-commits@lists.digium.com</a>></span><br>
Date: Mon, Dec 9, 2013 at 4:59 PM<br>Subject: [svn-commits] jrose: trunk r403588 - in /trunk: ./ channels/chan_pjsip.c<br>To: <a href="mailto:asterisk-commits@lists.digium.com">asterisk-commits@lists.digium.com</a>, <a href="mailto:svn-commits@lists.digium.com">svn-commits@lists.digium.com</a><br>
<br><br>Author: jrose<br>
Date: Mon Dec 9 16:59:14 2013<br>
New Revision: 403588<br>
<br>
URL: <a href="http://svnview.digium.com/svn/asterisk?view=rev&rev=403588" target="_blank">http://svnview.digium.com/svn/asterisk?view=rev&rev=403588</a><br>
Log:<br>
chan_pjsip: Fix a sticking channel lock caused by channel masquerades<br>
<br>
(closes issue ASTERISK-22936)<br>
Reported by: Jonathan Rose<br>
Review: <a href="https://reviewboard.asterisk.org/r/3042/" target="_blank">https://reviewboard.asterisk.org/r/3042/</a><br>
........<br>
<br>
Merged revisions 403587 from <a href="http://svn.asterisk.org/svn/asterisk/branches/12" target="_blank">http://svn.asterisk.org/svn/asterisk/branches/12</a><br>
<br>
Modified:<br>
trunk/ (props changed)<br>
trunk/channels/chan_pjsip.c<br>
<br>
Propchange: trunk/<br>
------------------------------------------------------------------------------<br>
--- branch-12-merged (original)<br>
+++ branch-12-merged Mon Dec 9 16:59:14 2013<br>
@@ -1,1 +1,1 @@<br>
-/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559<br>
+/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559,403587<br>
<br>
Modified: trunk/channels/chan_pjsip.c<br>
URL: <a href="http://svnview.digium.com/svn/asterisk/trunk/channels/chan_pjsip.c?view=diff&rev=403588&r1=403587&r2=403588" target="_blank">http://svnview.digium.com/svn/asterisk/trunk/channels/chan_pjsip.c?view=diff&rev=403588&r1=403587&r2=403588</a><br>
==============================================================================<br>
--- trunk/channels/chan_pjsip.c (original)<br>
+++ trunk/channels/chan_pjsip.c Mon Dec 9 16:59:14 2013<br>
@@ -841,7 +841,16 @@<br>
struct fixup_data {<br>
struct ast_sip_session *session;<br>
struct ast_channel *chan;<br>
+ struct ast_channel *oldchan;<br>
};<br>
+<br>
+static void fixup_data_destroy(struct fixup_data *fix_data)<br>
+{<br>
+ ao2_cleanup(fix_data->session);<br>
+ ast_channel_cleanup(fix_data->chan);<br>
+ ast_channel_cleanup(fix_data->oldchan);<br>
+ ast_free(fix_data);<br>
+}<br>
<br>
static int fixup(void *data)<br>
{<br>
@@ -849,6 +858,11 @@<br>
struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(fix_data->chan);<br>
struct chan_pjsip_pvt *pvt = channel->pvt;<br>
<br>
+ if (channel->session->channel != fix_data->oldchan) {<br>
+ fixup_data_destroy(fix_data);<br>
+ return -1;<br>
+ }<br>
+<br>
channel->session->channel = fix_data->chan;<br>
if (pvt->media[SIP_MEDIA_AUDIO] && pvt->media[SIP_MEDIA_AUDIO]->rtp) {<br>
ast_rtp_instance_set_channel_id(pvt->media[SIP_MEDIA_AUDIO]->rtp, ast_channel_uniqueid(fix_data->chan));<br>
@@ -857,6 +871,8 @@<br>
ast_rtp_instance_set_channel_id(pvt->media[SIP_MEDIA_VIDEO]->rtp, ast_channel_uniqueid(fix_data->chan));<br>
}<br>
<br>
+ fixup_data_destroy(fix_data);<br>
+<br>
return 0;<br>
}<br>
<br>
@@ -864,16 +880,23 @@<br>
static int chan_pjsip_fixup(struct ast_channel *oldchan, struct ast_channel *newchan)<br>
{<br>
struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(newchan);<br>
- struct fixup_data fix_data;<br>
-<br>
- fix_data.session = channel->session;<br>
- fix_data.chan = newchan;<br>
-<br>
- if (channel->session->channel != oldchan) {<br>
+ struct fixup_data *fix_data = ast_calloc(1, sizeof(*fix_data));<br>
+<br>
+ if (!fix_data) {<br>
return -1;<br>
}<br>
<br>
- if (ast_sip_push_task_synchronous(channel->session->serializer, fixup, &fix_data)) {<br>
+ fix_data->session = channel->session;<br>
+ ao2_ref(fix_data->session, +1);<br>
+<br>
+ fix_data->chan = newchan;<br>
+ ast_channel_ref(fix_data->chan);<br>
+<br>
+ fix_data->oldchan = oldchan;<br>
+ ast_channel_ref(fix_data->oldchan);<br>
+<br>
+ if (ast_sip_push_task(channel->session->serializer, fixup, fix_data)) {<br>
+ fixup_data_destroy(fix_data);<br>
ast_log(LOG_WARNING, "Unable to perform channel fixup\n");<br>
return -1;<br>
}<br>
<span class=""><font color="#888888"><br>
<br>
--<br>
_____________________________________________________________________<br>
-- Bandwidth and Colocation Provided by <a href="http://www.api-digital.com" target="_blank">http://www.api-digital.com</a> --<br>
<br>
svn-commits mailing list<br>
To UNSUBSCRIBE or update options visit:<br>
<a href="http://lists.digium.com/mailman/listinfo/svn-commits" target="_blank">http://lists.digium.com/mailman/listinfo/svn-commits</a><br>
</font></span></div><br><br clear="all"><br>-- <br><div dir="ltr"><div>Matthew Jordan<br></div><div>Digium, Inc. | Engineering Manager</div><div>445 Jan Davis Drive NW - Huntsville, AL 35806 - USA</div><div>Check us out at: <a href="http://digium.com" target="_blank">http://digium.com</a> & <a href="http://asterisk.org" target="_blank">http://asterisk.org</a></div>
</div>
</div></div></div>