[asterisk-dev] Fwd: [svn-commits] jrose: trunk r403588 - in /trunk: ./ channels/chan_pjsip.c

[Matthew Jordan]

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-commits at 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-commits at lists.digium.com, svn-commits at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131210/c11e91a2/attachment-0001.html>