I'm not sure this should have been committed. 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:
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. 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. I only see two possibilities here: 1. 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. 2. Go with a modified version of the patch, but assume that the session object has to be protected as well. How does this patch address these concerns? ---------- Forwarded message ---------- From: SVN commits to the Digium repositories <svn-comm...@lists.digium.com> Date: Mon, Dec 9, 2013 at 4:59 PM Subject: [svn-commits] jrose: trunk r403588 - in /trunk: ./ channels/chan_pjsip.c To: asterisk-comm...@lists.digium.com, svn-comm...@lists.digium.com Author: jrose Date: Mon Dec 9 16:59:14 2013 New Revision: 403588 URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=403588 Log: chan_pjsip: Fix a sticking channel lock caused by channel masquerades (closes issue ASTERISK-22936) Reported by: Jonathan Rose Review: https://reviewboard.asterisk.org/r/3042/ ........ Merged revisions 403587 from http://svn.asterisk.org/svn/asterisk/branches/12 Modified: trunk/ (props changed) trunk/channels/chan_pjsip.c Propchange: trunk/ ------------------------------------------------------------------------------ --- branch-12-merged (original) +++ branch-12-merged Mon Dec 9 16:59:14 2013 @@ -1,1 +1,1 @@ -/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559 +/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559,403587 Modified: trunk/channels/chan_pjsip.c URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_pjsip.c?view=diff&rev=403588&r1=403587&r2=403588 ============================================================================== --- trunk/channels/chan_pjsip.c (original) +++ trunk/channels/chan_pjsip.c Mon Dec 9 16:59:14 2013 @@ -841,7 +841,16 @@ struct fixup_data { struct ast_sip_session *session; struct ast_channel *chan; + struct ast_channel *oldchan; }; + +static void fixup_data_destroy(struct fixup_data *fix_data) +{ + ao2_cleanup(fix_data->session); + ast_channel_cleanup(fix_data->chan); + ast_channel_cleanup(fix_data->oldchan); + ast_free(fix_data); +} static int fixup(void *data) { @@ -849,6 +858,11 @@ struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(fix_data->chan); struct chan_pjsip_pvt *pvt = channel->pvt; + if (channel->session->channel != fix_data->oldchan) { + fixup_data_destroy(fix_data); + return -1; + } + channel->session->channel = fix_data->chan; if (pvt->media[SIP_MEDIA_AUDIO] && pvt->media[SIP_MEDIA_AUDIO]->rtp) { ast_rtp_instance_set_channel_id(pvt->media[SIP_MEDIA_AUDIO]->rtp, ast_channel_uniqueid(fix_data->chan)); @@ -857,6 +871,8 @@ ast_rtp_instance_set_channel_id(pvt->media[SIP_MEDIA_VIDEO]->rtp, ast_channel_uniqueid(fix_data->chan)); } + fixup_data_destroy(fix_data); + return 0; } @@ -864,16 +880,23 @@ static int chan_pjsip_fixup(struct ast_channel *oldchan, struct ast_channel *newchan) { struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(newchan); - struct fixup_data fix_data; - - fix_data.session = channel->session; - fix_data.chan = newchan; - - if (channel->session->channel != oldchan) { + struct fixup_data *fix_data = ast_calloc(1, sizeof(*fix_data)); + + if (!fix_data) { return -1; } - if (ast_sip_push_task_synchronous(channel->session->serializer, fixup, &fix_data)) { + fix_data->session = channel->session; + ao2_ref(fix_data->session, +1); + + fix_data->chan = newchan; + ast_channel_ref(fix_data->chan); + + fix_data->oldchan = oldchan; + ast_channel_ref(fix_data->oldchan); + + if (ast_sip_push_task(channel->session->serializer, fixup, fix_data)) { + fixup_data_destroy(fix_data); ast_log(LOG_WARNING, "Unable to perform channel fixup\n"); return -1; } -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- svn-commits mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/svn-commits -- Matthew Jordan Digium, Inc. | Engineering Manager 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com & http://asterisk.org
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
