[asterisk-commits] dvossel: branch dvossel/masq_locking_order_trunk r222150 - in /team/dvossel/m...
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Mon Oct 5 16:44:48 CDT 2009
Author: dvossel
Date: Mon Oct 5 16:44:45 2009
New Revision: 222150
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=222150
Log:
Fixes a race condition where do_masquerade could be called twice
for the same channel.
Modified:
team/dvossel/masq_locking_order_trunk/channels/chan_sip.c
team/dvossel/masq_locking_order_trunk/main/channel.c
Modified: team/dvossel/masq_locking_order_trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/team/dvossel/masq_locking_order_trunk/channels/chan_sip.c?view=diff&rev=222150&r1=222149&r2=222150
==============================================================================
--- team/dvossel/masq_locking_order_trunk/channels/chan_sip.c (original)
+++ team/dvossel/masq_locking_order_trunk/channels/chan_sip.c Mon Oct 5 16:44:45 2009
@@ -19696,8 +19696,6 @@
struct ast_channel *replacecall = p->refer->refer_call->owner; /* The channel we're about to take over */
struct ast_channel *targetcall; /* The bridge to the take-over target */
- struct ast_channel *test;
-
/* Check if we're in ring state */
if (replacecall->_state == AST_STATE_RING)
earlyreplace = 1;
@@ -19724,8 +19722,8 @@
call we are replacing exists, so an accepted
can't harm */
transmit_response_with_sdp(p, "200 OK", req, XMIT_RELIABLE, FALSE, FALSE);
- /* Do something more clever here */
ast_channel_unlock(c);
+ *nounlock = 1;
ast_channel_unlock(replacecall);
sip_pvt_unlock(p->refer->refer_call);
return 1;
@@ -19760,9 +19758,9 @@
/* Answer the incoming call and set channel to UP state */
transmit_response_with_sdp(p, "200 OK", req, XMIT_RELIABLE, FALSE, FALSE);
-
+
ast_setstate(c, AST_STATE_UP);
-
+
/* Stop music on hold and other generators */
ast_quiet_chan(replacecall);
ast_quiet_chan(targetcall);
@@ -19778,46 +19776,32 @@
else
ast_debug(4, "Invite/Replaces: Going to masquerade %s into %s\n", c->name, replacecall->name);
- /* C should now be in place of replacecall. all channel locks should be removed before issuing the masq */
- ast_channel_unlock(replacecall);
- ast_channel_unlock(c);
- if (ast_do_masquerade(replacecall)) {
- ast_log(LOG_WARNING, "Failed to perform masquerade with INVITE replaces\n");
- }
- ast_channel_lock_both(replacecall, c);
-
- if (earlyreplace || oneleggedreplace ) {
- c->hangupcause = AST_CAUSE_SWITCH_CONGESTION;
- }
-
- ast_setstate(c, AST_STATE_DOWN);
- ast_debug(4, "After transfer:----------------------------\n");
- ast_debug(4, " -- C: %s State %s\n", c->name, ast_state2str(c->_state));
- if (replacecall)
- ast_debug(4, " -- replacecall: %s State %s\n", replacecall->name, ast_state2str(replacecall->_state));
- if (p->owner) {
- ast_debug(4, " -- P->owner: %s State %s\n", p->owner->name, ast_state2str(p->owner->_state));
- test = ast_bridged_channel(p->owner);
- if (test)
- ast_debug(4, " -- Call bridged to P->owner: %s State %s\n", test->name, ast_state2str(test->_state));
- else
- ast_debug(4, " -- No call bridged to C->owner \n");
- } else
- ast_debug(4, " -- No channel yet \n");
- ast_debug(4, "End After transfer:----------------------------\n");
-
- /* unlock sip pvt and owner so hangup can do its thing */
+ /* C should now be in place of replacecall. all channel locks and pvt locks should be removed before issuing the masq */
ast_channel_unlock(replacecall);
ast_channel_unlock(c);
sip_pvt_unlock(p->refer->refer_call);
sip_pvt_unlock(p);
- *nounlock = 1;
+ if (ast_do_masquerade(replacecall)) {
+ ast_log(LOG_WARNING, "Failed to perform masquerade with INVITE replaces\n");
+ }
+
+ ast_channel_lock(c);
+ if (earlyreplace || oneleggedreplace ) {
+ c->hangupcause = AST_CAUSE_SWITCH_CONGESTION;
+ }
+ ast_setstate(c, AST_STATE_DOWN);
+ ast_channel_unlock(c);
/* The call should be down with no ast_channel, so hang it up */
c->tech_pvt = dialog_unref(c->tech_pvt, "unref dialog c->tech_pvt");
+
+ /* c and c's tech pvt must be unlocked at this point for ast_hangup */
ast_hangup(c);
- sip_pvt_lock(p); /* lock PVT structure again after hangup */
-
+
+ /* this indicates to handle_request_do that the owner channel has already been unlocked */
+ *nounlock = 1;
+ /* lock PVT structure again after hangup */
+ sip_pvt_lock(p);
return 0;
}
@@ -21025,6 +21009,9 @@
transmit_notify_with_sipfrag(transferer, seqno, "486 Busy Here", TRUE);
append_history(transferer, "Xfer", "Refer failed");
ast_clear_flag(&transferer->flags[0], SIP_DEFER_BYE_ON_TRANSFER);
+ /* if transfer failed, go ahead and unlock targetcall_pvt and it's owner channel */
+ sip_pvt_unlock(targetcall_pvt);
+ ast_channel_unlock(target.chan1);
} else {
/* Transfer succeeded! */
const char *xfersound = pbx_builtin_getvar_helper(target.chan1, "ATTENDED_TRANSFER_COMPLETE_SOUND");
@@ -21042,17 +21029,19 @@
/* By forcing the masquerade, we know that target.chan1 and target.chan2 are bridged. We then
* can queue connected line updates where they need to go.
+ *
+ * before a masquerade, all channel and pvt locks must be unlocked. Any recursive
+ * channel locks held before this function invalidates channel container locking order
*/
- if (target.chan1->masq) {
- /* before a masquerade, all channel locks must be unlocked. Any recursive
- * channel locks held before this function invalidates channel container locking order */
- ast_channel_unlock(target.chan1);
- ast_channel_unlock(current->chan1); /* current.chan1 is p->owner before the masq, it was locked by socket_read() */
- *nounlock = 1; /* we just unlocked the dialog's channel and have no plans of locking it again. */
- /* If the channel thread already did the masquerade, then we don't need to do anything */
- ast_do_masquerade(target.chan1);
- ast_channel_lock(target.chan1);
- }
+ ast_channel_unlock(target.chan1);
+ ast_channel_unlock(current->chan1); /* current.chan1 is p->owner before the masq, it was locked by socket_read()*/
+ *nounlock = 1; /* we just unlocked the dialog's channel and have no plans of locking it again. */
+ sip_pvt_unlock(targetcall_pvt);
+ sip_pvt_unlock(transferer);
+
+ ast_do_masquerade(target.chan1);
+
+ ast_channel_lock(transferer); /* the transferer pvt is expected to remain locked on return */
if (target.chan2) {
ast_channel_queue_connected_line_update(target.chan1, &connected_to_transferee);
@@ -21066,9 +21055,8 @@
ast_channel_update_connected_line(target.chan1, &connected_to_target);
}
}
- /* remove locks from targetcall_pvt and target.chan1 (targetcall_pvt's owner at the beginning of the function) */
- sip_pvt_unlock(targetcall_pvt);
- ast_channel_unlock(target.chan1);
+
+ /* at this point if the transfer is successful only the transferer pvt should be locked. */
ast_party_connected_line_free(&connected_to_target);
ast_party_connected_line_free(&connected_to_transferee);
if (targetcall_pvt)
Modified: team/dvossel/masq_locking_order_trunk/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/dvossel/masq_locking_order_trunk/main/channel.c?view=diff&rev=222150&r1=222149&r2=222150
==============================================================================
--- team/dvossel/masq_locking_order_trunk/main/channel.c (original)
+++ team/dvossel/masq_locking_order_trunk/main/channel.c Mon Oct 5 16:44:45 2009
@@ -3066,7 +3066,7 @@
struct ast_frame *f = NULL; /* the return value */
int blah;
int prestate;
- int count = 0, cause = 0;
+ int cause = 0;
/* this function is very long so make sure there is only one return
* point at the end (there are only two exceptions to this).
@@ -5078,7 +5078,7 @@
/*!
\brief Masquerade a channel
- \note Assumes _NO_ channels are locked. If a channel is locked while calling
+ \note Assumes _NO_ channels and _NO_ channel pvt's are locked. If a channel is locked while calling
this function, it invalidates our channel container locking order. All channels
must be unlocked before it is permissible to lock the channels' ao2 container.
*/
@@ -5095,7 +5095,7 @@
struct ast_party_connected_line connected;
struct ast_party_redirecting redirecting;
} exchange;
- struct ast_channel *clonechan = original->masq;
+ struct ast_channel *clonechan;
struct ast_cdr *cdr;
int rformat = original->readformat;
int wformat = original->writeformat;
@@ -5109,12 +5109,38 @@
* original channel's backend. While the features are nice, which is the
* reason we're keeping it, it's still awesomely weird. XXX */
+ ao2_lock(channels);
+
+ ast_channel_lock(original);
+
+ /* This checks to see if the masquerade has already happened or not. There is a
+ * race condition that exists for this function. Since all pvt and channel locks
+ * must be let go before calling do_masquerade, it is possible that it could be
+ * called multiple times for the same channel. This check verifies whether
+ * or not the masquerade has already been completed by another thread */
+ if (!original->masq) {
+ ast_channel_unlock(original);
+ ao2_unlock(channels);
+ return 0; /* masq already completed by another thread, or never needed to be done to begin with */
+ }
+
+ /* now that we have verified no race condition exists, set the clone channel */
+ clonechan = original->masq;
+
+ /* since this function already holds the global containers lock, unlocking original
+ * for deadlock avoidance will not result in a sort of masquerade race condition */
+ while (ast_channel_trylock(clonechan)) {
+ CHANNEL_DEADLOCK_AVOIDANCE(original);
+ }
+
+ /* clear the masquerade channels */
+ original->masq = NULL;
+ clonechan->masqr = NULL;
+
+ /* unlink from channels container as name (which is the hash value) will change */
ao2_unlink(channels, original);
ao2_unlink(channels, clonechan);
- /* Now that we have unlinked from the channels container, We need both
- * the original and clone channels locked */
- ast_channel_lock_both(original, clonechan);
ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
clonechan->name, clonechan->_state, original->name, original->_state);
@@ -5125,10 +5151,6 @@
one */
free_translation(clonechan);
free_translation(original);
-
- /* Unlink the masquerade */
- original->masq = NULL;
- clonechan->masqr = NULL;
/* Save the original name */
ast_copy_string(orig, original->name, sizeof(orig));
@@ -5372,7 +5394,6 @@
pthread_kill(original->blocker, SIGURG);
ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
-
done:
/* it is possible for the clone channel to disappear during this */
if (clonechan) {
@@ -5381,7 +5402,7 @@
}
ast_channel_unlock(original);
ao2_link(channels, original);
-
+ ao2_unlock(channels);
return 0;
}
More information about the asterisk-commits
mailing list