[asterisk-commits] rmudgett: branch 1.8 r368405 - /branches/1.8/main/channel.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Mon Jun 4 13:41:22 CDT 2012
Author: rmudgett
Date: Mon Jun 4 13:41:18 2012
New Revision: 368405
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=368405
Log:
Fix potential deadlock between masquerade and chan_local.
* Restructure ast_do_masquerade() to not hold channel locks while it calls
ast_indicate().
* Simplify many calls to ast_do_masquerade() since it will never return a
failure now. If it does fail internally because a channel driver callback
operation failed, the only thing ast_do_masquerade() can do is generate a
warning message about strange things may happen and press on.
* Fixed the call to ast_bridged_channel() in ast_do_masquerade(). This
change fixes half of the deadlock reported in ASTERISK-19801 between
masquerades and chan_iax.
(closes issue ASTERISK-19537)
Reported by: rmudgett
Tested by: rmudgett
Review: https://reviewboard.asterisk.org/r/1915/
Modified:
branches/1.8/main/channel.c
Modified: branches/1.8/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/channel.c?view=diff&rev=368405&r1=368404&r2=368405
==============================================================================
--- branches/1.8/main/channel.c (original)
+++ branches/1.8/main/channel.c Mon Jun 4 13:41:18 2012
@@ -2770,13 +2770,7 @@
*/
while (chan->masq) {
ast_channel_unlock(chan);
- if (ast_do_masquerade(chan)) {
- ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-
- /* Abort the loop or we might never leave. */
- ast_channel_lock(chan);
- break;
- }
+ ast_do_masquerade(chan);
ast_channel_lock(chan);
}
@@ -2792,6 +2786,7 @@
return 0;
}
+ /* Mark as a zombie so a masquerade cannot be setup on this channel. */
if (!(was_zombie = ast_test_flag(chan, AST_FLAG_ZOMBIE))) {
ast_set_flag(chan, AST_FLAG_ZOMBIE);
}
@@ -3155,10 +3150,8 @@
/* Perform any pending masquerades */
for (x = 0; x < n; x++) {
- if (c[x]->masq && ast_do_masquerade(c[x])) {
- ast_log(LOG_WARNING, "Masquerade failed\n");
- *ms = -1;
- return NULL;
+ while (c[x]->masq) {
+ ast_do_masquerade(c[x]);
}
ast_channel_lock(c[x]);
@@ -3289,10 +3282,8 @@
/* See if this channel needs to be masqueraded */
- if (chan->masq && ast_do_masquerade(chan)) {
- ast_log(LOG_WARNING, "Failed to perform masquerade on %s\n", chan->name);
- *ms = -1;
- return NULL;
+ while (chan->masq) {
+ ast_do_masquerade(chan);
}
ast_channel_lock(chan);
@@ -3371,10 +3362,8 @@
struct ast_channel *winner = NULL;
for (i = 0; i < n; i++) {
- if (c[i]->masq && ast_do_masquerade(c[i])) {
- ast_log(LOG_WARNING, "Masquerade failed\n");
- *ms = -1;
- return NULL;
+ while (c[i]->masq) {
+ ast_do_masquerade(c[i]);
}
ast_channel_lock(c[i]);
@@ -3753,11 +3742,8 @@
*/
if (chan->masq) {
- if (ast_do_masquerade(chan))
- ast_log(LOG_WARNING, "Failed to perform masquerade\n");
- else
- f = &ast_null_frame;
- return f;
+ ast_do_masquerade(chan);
+ return &ast_null_frame;
}
/* if here, no masq has happened, lock the channel and proceed */
@@ -4815,12 +4801,9 @@
goto done;
/* Handle any pending masquerades */
- if (chan->masq) {
+ while (chan->masq) {
ast_channel_unlock(chan);
- if (ast_do_masquerade(chan)) {
- ast_log(LOG_WARNING, "Failed to perform masquerade\n");
- return res; /* no need to goto done: chan is already unlocked for masq */
- }
+ ast_do_masquerade(chan);
ast_channel_lock(chan);
}
if (chan->masqr) {
@@ -6428,11 +6411,11 @@
*/
int ast_do_masquerade(struct ast_channel *original)
{
- format_t x;
+ int x;
int i;
- int res=0;
int origstate;
int visible_indication;
+ int clone_was_zombie = 0;/*!< TRUE if the clonechan was a zombie before the masquerade. */
struct ast_frame *current;
const struct ast_channel_tech *t;
void *t_pvt;
@@ -6447,8 +6430,9 @@
struct ast_cdr *cdr;
struct ast_datastore *xfer_ds;
struct xfer_masquerade_ds *xfer_colp;
- format_t rformat = original->readformat;
- format_t wformat = original->writeformat;
+ format_t rformat;
+ format_t wformat;
+ format_t tmp_format;
char newn[AST_CHANNEL_NAME];
char orig[AST_CHANNEL_NAME];
char masqn[AST_CHANNEL_NAME];
@@ -6459,52 +6443,59 @@
* original channel's backend. While the features are nice, which is the
* reason we're keeping it, it's still awesomely weird. XXX */
- /* The reasoning for the channels ao2_container lock here is complex.
- *
- * In order to check for a race condition, the original channel must
- * be locked. If it is determined that the masquerade should proceed
- * the original channel can absolutely not be unlocked until the end
- * of the function. Since after determining the masquerade should
- * continue requires the channels to be unlinked from the ao2_container,
- * the container lock must be held first to achieve proper locking order.
+ /*
+ * The reasoning for the channels ao2_container lock here is
+ * complex.
+ *
+ * There is a race condition that exists for this function.
+ * Since all pvt and channel locks must be let go before calling
+ * ast_do_masquerade, it is possible that it could be called
+ * multiple times for the same channel. In order to prevent the
+ * race condition with competing threads to do the masquerade
+ * and new masquerade attempts, the channels container must be
+ * locked for the entire masquerade. The original and clonechan
+ * need to be unlocked earlier to avoid potential deadlocks with
+ * the chan_local deadlock avoidance method.
+ *
+ * The container lock blocks competing masquerade attempts from
+ * starting as well as being necessary for proper locking order
+ * because the channels must to be unlinked to change their
+ * names.
+ *
+ * The original and clonechan locks must be held while the
+ * channel contents are shuffled around for the masquerade.
+ *
+ * The masq and masqr pointers need to be left alone until the
+ * masquerade has restabilized the channels to prevent another
+ * masquerade request until the AST_FLAG_ZOMBIE can be set on
+ * the clonechan.
*/
ao2_lock(channels);
- /* lock the original channel to determine if the masquerade is required or not */
+ /*
+ * Lock the original channel to determine if the masquerade is
+ * still required.
+ */
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.
- */
- while ((clonechan = original->masq) && ast_channel_trylock(clonechan)) {
+ clonechan = original->masq;
+ if (!clonechan) {
/*
- * A masq is needed but we could not get the clonechan lock
- * immediately. Since this function already holds the global
- * container lock, unlocking original for deadlock avoidance
- * will not result in any sort of masquerade race condition. If
- * masq is called by a different thread while this happens, it
- * will be stuck waiting until we unlock the container.
+ * The masq is already completed by another thread or never
+ * needed to be done to begin with.
*/
- CHANNEL_DEADLOCK_AVOIDANCE(original);
- }
-
- /*
- * A final masq check must be done after deadlock avoidance for
- * clonechan above or we could get a double masq. This is
- * posible with ast_hangup at least.
- */
- if (!clonechan) {
- /* masq already completed by another thread, or never needed to be done to begin with */
ast_channel_unlock(original);
ao2_unlock(channels);
return 0;
}
+
+ /* Bump the refs to ensure that they won't dissapear on us. */
+ ast_channel_ref(original);
+ ast_channel_ref(clonechan);
+
+ /* unlink from channels container as name (which is the hash value) will change */
+ ao2_unlink(channels, original);
+ ao2_unlink(channels, clonechan);
/* Get any transfer masquerade connected line exchange data. */
xfer_ds = ast_channel_datastore_find(original, &xfer_ds_info, NULL);
@@ -6516,30 +6507,26 @@
}
/*
- * Release any hold on the transferee channel before proceeding
- * with the masquerade.
+ * Stop any visible indication on the original channel so we can
+ * transfer it to the clonechan taking the original's place.
+ */
+ visible_indication = original->visible_indication;
+ ast_channel_unlock(original);
+ ast_indicate(original, -1);
+
+ /*
+ * Release any hold on the transferee channel before going any
+ * further with the masquerade.
*/
if (xfer_colp && xfer_colp->transferee_held) {
ast_indicate(clonechan, AST_CONTROL_UNHOLD);
}
- /* 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);
+ /* Start the masquerade channel contents rearangement. */
+ 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);
-
- /*
- * Stop any visible indiction on the original channel so we can
- * transfer it to the clonechan taking the original's place.
- */
- visible_indication = original->visible_indication;
- ast_indicate(original, -1);
chans[0] = clonechan;
chans[1] = original;
@@ -6550,8 +6537,12 @@
"OriginalState: %s\r\n",
clonechan->name, ast_state2str(clonechan->_state), original->name, ast_state2str(original->_state));
- /* Having remembered the original read/write formats, we turn off any translation on either
- one */
+ /*
+ * Remember the original read/write formats. We turn off any
+ * translation on either one
+ */
+ rformat = original->readformat;
+ wformat = original->writeformat;
free_translation(clonechan);
free_translation(original);
@@ -6576,14 +6567,14 @@
original->tech = clonechan->tech;
clonechan->tech = t;
+ t_pvt = original->tech_pvt;
+ original->tech_pvt = clonechan->tech_pvt;
+ clonechan->tech_pvt = t_pvt;
+
/* Swap the cdrs */
cdr = original->cdr;
original->cdr = clonechan->cdr;
clonechan->cdr = cdr;
-
- t_pvt = original->tech_pvt;
- original->tech_pvt = clonechan->tech_pvt;
- clonechan->tech_pvt = t_pvt;
/* Swap the alertpipes */
for (i = 0; i < 2; i++) {
@@ -6592,7 +6583,7 @@
clonechan->alertpipe[i] = x;
}
- /*
+ /*
* Swap the readq's. The end result should be this:
*
* 1) All frames should be on the new (original) channel.
@@ -6605,8 +6596,8 @@
*/
{
AST_LIST_HEAD_NOLOCK(, ast_frame) tmp_readq;
- AST_LIST_HEAD_SET_NOLOCK(&tmp_readq, NULL);
-
+
+ AST_LIST_HEAD_INIT_NOLOCK(&tmp_readq);
AST_LIST_APPEND_LIST(&tmp_readq, &original->readq, frame_list);
AST_LIST_APPEND_LIST(&original->readq, &clonechan->readq, frame_list);
@@ -6623,12 +6614,13 @@
}
/* Swap the raw formats */
- x = original->rawreadformat;
+ tmp_format = original->rawreadformat;
original->rawreadformat = clonechan->rawreadformat;
- clonechan->rawreadformat = x;
- x = original->rawwriteformat;
+ clonechan->rawreadformat = tmp_format;
+
+ tmp_format = original->rawwriteformat;
original->rawwriteformat = clonechan->rawwriteformat;
- clonechan->rawwriteformat = x;
+ clonechan->rawwriteformat = tmp_format;
clonechan->_softhangup = AST_SOFTHANGUP_DEV;
@@ -6639,23 +6631,6 @@
origstate = original->_state;
original->_state = clonechan->_state;
clonechan->_state = origstate;
-
- if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
- ast_log(LOG_WARNING, "Fixup failed on channel %s, strange things may happen.\n", clonechan->name);
- }
-
- /* Start by disconnecting the original's physical side */
- if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
- ast_log(LOG_WARNING, "Hangup failed! Strange things may happen!\n");
- res = -1;
- goto done;
- }
-
- /*
- * We just hung up the physical side of the channel. Set the
- * new zombie to use the kill channel driver for safety.
- */
- clonechan->tech = &ast_kill_tech;
/* Mangle the name of the clone channel */
snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig); /* quick, hide the brains! */
@@ -6757,37 +6732,108 @@
ast_debug(1, "Putting channel %s in %s/%s formats\n", original->name,
ast_getformatname(wformat), ast_getformatname(rformat));
- /* Okay. Last thing is to let the channel driver know about all this mess, so he
- can fix up everything as best as possible */
- if (original->tech->fixup) {
- if (original->tech->fixup(clonechan, original)) {
- ast_log(LOG_WARNING, "Channel for type '%s' could not fixup channel %s\n",
- original->tech->type, original->name);
- res = -1;
- goto done;
- }
- } else
- ast_log(LOG_WARNING, "Channel type '%s' does not have a fixup routine (for %s)! Bad things may happen.\n",
+ /* Fixup the original clonechan's physical side */
+ if (original->tech->fixup && original->tech->fixup(clonechan, original)) {
+ ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (clonechan)\n",
original->tech->type, original->name);
-
- /*
- * If an indication is currently playing, maintain it on the channel
- * that is taking the place of original
+ }
+
+ /* Fixup the original original's physical side */
+ if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
+ ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (original)\n",
+ clonechan->tech->type, clonechan->name);
+ }
+
+ /*
+ * Now, at this point, the "clone" channel is totally F'd up.
+ * We mark it as a zombie so nothing tries to touch it. If it's
+ * already been marked as a zombie, then we must free it (since
+ * it already is considered invalid).
*
- * This is needed because the masquerade is swapping out in the internals
- * of this channel, and the new channel private data needs to be made
- * aware of the current visible indication (RINGING, CONGESTION, etc.)
+ * This must be done before we unlock clonechan to prevent
+ * setting up another masquerade on the clonechan.
+ */
+ if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+ clone_was_zombie = 1;
+ } else {
+ ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
+ ast_queue_frame(clonechan, &ast_null_frame);
+ }
+
+ /* clear the masquerade channels */
+ original->masq = NULL;
+ clonechan->masqr = NULL;
+
+ /*
+ * When we unlock original here, it can be immediately setup to
+ * masquerade again or hungup. The new masquerade or hangup
+ * will not actually happen until we release the channels
+ * container lock.
+ */
+ ast_channel_unlock(original);
+
+ /* Disconnect the original original's physical side */
+ if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
+ ast_log(LOG_WARNING, "Hangup failed! Strange things may happen!\n");
+ } else {
+ /*
+ * We just hung up the original original's physical side of the
+ * channel. Set the new zombie to use the kill channel driver
+ * for safety.
+ */
+ clonechan->tech = &ast_kill_tech;
+ }
+
+ ast_channel_unlock(clonechan);
+
+ /*
+ * If an indication is currently playing, maintain it on the
+ * channel that is taking the place of original.
+ *
+ * This is needed because the masquerade is swapping out the
+ * internals of the channel, and the new channel private data
+ * needs to be made aware of the current visible indication
+ * (RINGING, CONGESTION, etc.)
*/
if (visible_indication) {
ast_indicate(original, visible_indication);
}
- /* Now, at this point, the "clone" channel is totally F'd up. We mark it as
- a zombie so nothing tries to touch it. If it's already been marked as a
- zombie, then free it now (since it already is considered invalid). */
- if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+ ast_channel_lock(original);
+
+ /* Signal any blocker */
+ if (ast_test_flag(original, AST_FLAG_BLOCKING)) {
+ pthread_kill(original->blocker, SIGURG);
+ }
+
+ ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
+
+ if ((bridged = ast_bridged_channel(original))) {
+ ast_channel_ref(bridged);
+ ast_channel_unlock(original);
+ ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
+ ast_channel_unref(bridged);
+ } else {
+ ast_channel_unlock(original);
+ }
+ ast_indicate(original, AST_CONTROL_SRCCHANGE);
+
+ if (xfer_colp) {
+ /*
+ * After the masquerade, the original channel pointer actually
+ * points to the new transferee channel and the bridged channel
+ * is still the intended transfer target party.
+ */
+ masquerade_colp_transfer(original, xfer_colp);
+ }
+
+ if (xfer_ds) {
+ ast_datastore_free(xfer_ds);
+ }
+
+ if (clone_was_zombie) {
+ ast_channel_lock(clonechan);
ast_debug(1, "Destroying channel clone '%s'\n", clonechan->name);
- ast_channel_unlock(clonechan);
ast_manager_event(clonechan, EVENT_FLAG_CALL, "Hangup",
"Channel: %s\r\n"
"Uniqueid: %s\r\n"
@@ -6798,52 +6844,25 @@
clonechan->hangupcause,
ast_cause2str(clonechan->hangupcause)
);
- clonechan = ast_channel_release(clonechan);
+ ast_channel_unlock(clonechan);
+
+ /*
+ * Drop the system reference to destroy the channel since it is
+ * already unlinked.
+ */
+ ast_channel_unref(clonechan);
} else {
- ast_debug(1, "Released clone lock on '%s'\n", clonechan->name);
- ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
- ast_queue_frame(clonechan, &ast_null_frame);
- }
-
- /* Signal any blocker */
- if (ast_test_flag(original, AST_FLAG_BLOCKING))
- pthread_kill(original->blocker, SIGURG);
- ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
-
- if ((bridged = ast_bridged_channel(original))) {
- ast_channel_lock(bridged);
- ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
- ast_channel_unlock(bridged);
- }
- ast_indicate(original, AST_CONTROL_SRCCHANGE);
-
- if (xfer_colp) {
- /*
- * After the masquerade, the original channel pointer actually
- * points to the new transferee channel and the bridged channel
- * is still the intended transfer target party.
- */
- masquerade_colp_transfer(original, xfer_colp);
- }
-
-done:
- if (xfer_ds) {
- ast_datastore_free(xfer_ds);
- }
- /* it is possible for the clone channel to disappear during this */
- if (clonechan) {
- ast_channel_unlock(original);
- ast_channel_unlock(clonechan);
ao2_link(channels, clonechan);
- ao2_link(channels, original);
- } else {
- ast_channel_unlock(original);
- ao2_link(channels, original);
- }
-
+ }
+
+ ao2_link(channels, original);
ao2_unlock(channels);
- return res;
+ /* Release our held safety references. */
+ ast_channel_unref(original);
+ ast_channel_unref(clonechan);
+
+ return 0;
}
void ast_set_callerid(struct ast_channel *chan, const char *cid_num, const char *cid_name, const char *cid_ani)
More information about the asterisk-commits
mailing list