[asterisk-commits] mmichelson: trunk r404368 - in /trunk: ./ include/asterisk/ main/ res/
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Thu Dec 19 11:45:26 CST 2013
Author: mmichelson
Date: Thu Dec 19 11:45:21 2013
New Revision: 404368
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=404368
Log:
Fix a deadlock that occurred due to a conflict of masquerades.
For the explanation, here is a copy-paste of the review board explanation:
Initially, it was discovered that performing an attended transfer of a
multiparty bridge with a PJSIP channel would cause a deadlock. A PBX thread
started a masquerade and reached the point where it was calling the fixup()
callback on the "original" channel. For chan_pjsip, this involves pushing a
synchronous task to the session's serializer. The problem was that a task ahead
of the fixup task was also attempting to perform a channel masquerade. However,
since masquerades are designed in a way to only allow for one to occur at a
time, the task ahead of the fixup could not continue until the masquerade
already in progress had completed. And of course, the masquerade in progress
could not complete until the task ahead of the fixup task had completed.
Deadlock.
The initial fix was to change the fixup task to be asynchronous. While this
prevented the deadlock from occurring, it had the frightful side effect of
potentially allowing for tasks in the session's serializer to operate on a
zombie channel.
Taking a step back from this particular deadlock, it became clear that the
problem was not really this one particular issue but that masquerades
themselves needed to be addressed. A PJSIP attended transfer operation calls
ast_channel_move(), which attempts to both set up and execute a masquerade. The
problem was that after it had set up the masquerade, the PBX thread had swooped
in and tried to actually perform the masquerade. Looking at changes that had
been made to Asterisk 12, it became clear that there never is any time now that
anyone ever wants to set up a masquerade and allow for the channel thread to
actually perform the masquerade. Everyone always is calling ast_channel_move(),
performs the masquerade itself before returning.
In this patch, I have removed all blocks of code from channel.c that will
attempt to perform a masquerade if ast_channel_masq() returns true. Now, there
is no distinction between setting up a masquerade and performing the
masquerade. It is one operation. The only remaining checks for
ast_channel_masq() and ast_channel_masqr() are in ast_hangup() since we do not
want to interrupt a masquerade by hanging up the channel. Instead, now
ast_hangup() will wait for a masquerade to complete before moving forward with
its operation.
The ast_channel_move() function has been modified to basically in-line the
logic that used to be in ast_channel_masquerade(). ast_channel_masquerade() has
been killed off for real. ast_channel_move() now has a lock associated with it
that is used to prevent any simultaneous moves from occurring at once. This
means there is no need to make sure that ast_channel_masq() or
ast_channel_masqr() are already set on a channel when ast_channel_move() is
called. It also means the channel container lock is not pulling double duty by
both keeping the container locked and preventing multiple masquerades from
occurring simultaneously.
The ast_do_masquerade() function has been renamed to do_channel_masquerade()
and is now internal to channel.c. The function now takes explicit arguments of
which channels are involved in the masquerade instead of a single channel.
While it probably is possible to do some further refactoring of this method, I
feel that I would be treading dangerously. Instead, all I did was change some
comments that no longer are true after this changeset.
The other more minor change introduced in this patch is to res_pjsip.c to make
ast_sip_push_task_synchronous() run the task in-place if we are already a SIP
servant thread. This is related to this patch because even when we isolate the
channel masquerade to only running in the SIP servant thread, we would still
deadlock when the fixup() callback is reached since we would essentially be
waiting forever for ourselves to finish before actually running the fixup. This
makes it so the fixup is run without having to push a task into a serializer at
all.
(closes issue ASTERISK-22936)
Reported by Jonathan Rose
Review: https://reviewboard.asterisk.org/r/3069
........
Merged revisions 404356 from http://svn.asterisk.org/svn/asterisk/branches/12
Modified:
trunk/ (props changed)
trunk/include/asterisk/autochan.h
trunk/include/asterisk/channel.h
trunk/main/channel.c
trunk/res/res_pjsip.c
Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-12-merged' - no diff available.
Modified: trunk/include/asterisk/autochan.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/autochan.h?view=diff&rev=404368&r1=404367&r2=404368
==============================================================================
--- trunk/include/asterisk/autochan.h (original)
+++ trunk/include/asterisk/autochan.h Thu Dec 19 11:45:21 2013
@@ -98,8 +98,9 @@
* \details
* Traverses the list of autochans. All autochans which point to
* old_chan will be updated to point to new_chan instead. Currently
- * this is only called from ast_do_masquerade in channel.c.
- *
+ * this is only called during an ast_channel_move() operation in
+ * channel.c.
+ *
* \pre Both channels must be locked before calling this function.
*
* \param old_chan The channel that autochans may currently point to
Modified: trunk/include/asterisk/channel.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/channel.h?view=diff&rev=404368&r1=404367&r2=404368
==============================================================================
--- trunk/include/asterisk/channel.h (original)
+++ trunk/include/asterisk/channel.h Thu Dec 19 11:45:21 2013
@@ -2014,26 +2014,6 @@
int ast_channel_early_bridge(struct ast_channel *c0, struct ast_channel *c1);
/*!
- * \brief Weird function made for call transfers
- *
- * \param original channel to make a copy of
- * \param clone copy of the original channel
- *
- * \details
- * This is a very strange and freaky function used primarily for transfer. Suppose that
- * "original" and "clone" are two channels in random situations. This function takes
- * the guts out of "clone" and puts them into the "original" channel, then alerts the
- * channel driver of the change, asking it to fixup any private information (like the
- * p->owner pointer) that is affected by the change. The physical layer of the original
- * channel is hung up.
- *
- * \note Neither channel passed here should be locked before
- * calling this function. This function performs deadlock
- * avoidance involving these two channels.
- */
-int ast_channel_masquerade(struct ast_channel *original, struct ast_channel *clone);
-
-/*!
* \brief Gives the string form of a given cause code.
*
* \param state cause to get the description of
@@ -2305,18 +2285,6 @@
* \param dest destination extension for transfer
*/
int ast_transfer(struct ast_channel *chan, char *dest);
-
-/*!
- * \brief Start masquerading a channel
- * \note absolutely _NO_ channel locks should be held before calling this function.
- * \details
- * XXX This is a seriously whacked out operation. We're essentially putting the guts of
- * the clone channel into the original channel. Start by killing off the original
- * channel's backend. I'm not sure we're going to keep this function, because
- * while the features are nice, the cost is very high in terms of pure nastiness. XXX
- * \param chan Channel to masquerade
- */
-void ast_do_masquerade(struct ast_channel *chan);
/*!
* \brief Inherits channel variable from parent to child channel
Modified: trunk/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/channel.c?view=diff&rev=404368&r1=404367&r2=404368
==============================================================================
--- trunk/main/channel.c (original)
+++ trunk/main/channel.c Thu Dec 19 11:45:21 2013
@@ -2682,30 +2682,8 @@
ast_channel_lock(chan);
- /*
- * Do the masquerade if someone is setup to masquerade into us.
- *
- * NOTE: We must hold the channel lock after testing for a
- * pending masquerade and setting the channel as a zombie to
- * prevent ast_channel_masquerade() from setting up a
- * masquerade with a dead channel.
- */
- while (ast_channel_masq(chan)) {
- ast_channel_unlock(chan);
- ast_do_masquerade(chan);
- ast_channel_lock(chan);
- }
-
- if (ast_channel_masqr(chan)) {
- /*
- * This channel is one which will be masqueraded into something.
- * Mark it as a zombie already so ast_do_masquerade() will know
- * to free it later.
- */
- ast_set_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE);
- destroy_hooks(chan);
- ast_channel_unlock(chan);
- return;
+ while (ast_channel_masq(chan) || ast_channel_masqr(chan)) {
+ CHANNEL_DEADLOCK_AVOIDANCE(chan);
}
/* Mark as a zombie so a masquerade cannot be setup on this channel. */
@@ -3087,12 +3065,7 @@
return NULL;
}
- /* Perform any pending masquerades */
for (x = 0; x < n; x++) {
- while (ast_channel_masq(c[x])) {
- ast_do_masquerade(c[x]);
- }
-
ast_channel_lock(c[x]);
if (!ast_tvzero(*ast_channel_whentohangup(c[x]))) {
if (ast_tvzero(whentohangup))
@@ -3232,12 +3205,6 @@
struct ast_channel *winner = NULL;
struct ast_epoll_data *aed = NULL;
-
- /* See if this channel needs to be masqueraded */
- while (ast_channel_masq(chan)) {
- ast_do_masquerade(chan);
- }
-
ast_channel_lock(chan);
/* Figure out their timeout */
if (!ast_tvzero(*ast_channel_whentohangup(chan))) {
@@ -3319,10 +3286,6 @@
struct ast_channel *winner = NULL;
for (i = 0; i < n; i++) {
- while (ast_channel_masq(c[i])) {
- ast_do_masquerade(c[i]);
- }
-
ast_channel_lock(c[i]);
if (!ast_tvzero(*ast_channel_whentohangup(c[i]))) {
if (whentohangup == 0) {
@@ -3767,13 +3730,6 @@
/* this function is very long so make sure there is only one return
* point at the end (there are only two exceptions to this).
*/
-
- if (ast_channel_masq(chan)) {
- ast_do_masquerade(chan);
- return &ast_null_frame;
- }
-
- /* if here, no masq has happened, lock the channel and proceed */
ast_channel_lock(chan);
/* Stop if we're a zombie or need a soft hangup */
@@ -4991,17 +4947,6 @@
if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE) || ast_check_hangup(chan))
goto done;
- /* Handle any pending masquerades */
- while (ast_channel_masq(chan)) {
- ast_channel_unlock(chan);
- ast_do_masquerade(chan);
- ast_channel_lock(chan);
- }
- if (ast_channel_masqr(chan)) {
- res = 0; /* XXX explain, why 0 ? */
- goto done;
- }
-
/* Perform the framehook write event here. After the frame enters the framehook list
* there is no telling what will happen, how awesome is that!!! */
if (!(fr = ast_framehook_list_write_event(ast_channel_framehooks(chan), fr))) {
@@ -6262,60 +6207,6 @@
return 0;
}
-int ast_channel_masquerade(struct ast_channel *original, struct ast_channel *clonechan)
-{
- int res = -1;
-
- if (original == clonechan) {
- ast_log(LOG_WARNING, "Can't masquerade channel '%s' into itself!\n",
- ast_channel_name(original));
- return -1;
- }
-
- ast_channel_lock_both(original, clonechan);
-
- if (ast_test_flag(ast_channel_flags(original), AST_FLAG_ZOMBIE)
- || ast_test_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE)) {
- /* Zombies! Run! */
- ast_log(LOG_WARNING,
- "Can't setup masquerade. One or both channels is dead. (%s <-- %s)\n",
- ast_channel_name(original), ast_channel_name(clonechan));
- ast_channel_unlock(clonechan);
- ast_channel_unlock(original);
- return -1;
- }
-
- ast_debug(1, "Planning to masquerade channel %s into the structure of %s\n",
- ast_channel_name(clonechan), ast_channel_name(original));
-
- if (!ast_channel_masqr(original) && !ast_channel_masq(original) && !ast_channel_masq(clonechan) && !ast_channel_masqr(clonechan)) {
- ast_channel_masq_set(original, clonechan);
- ast_channel_masqr_set(clonechan, original);
- ast_queue_frame(original, &ast_null_frame);
- ast_queue_frame(clonechan, &ast_null_frame);
- ast_debug(1, "Done planning to masquerade channel %s into the structure of %s\n", ast_channel_name(clonechan), ast_channel_name(original));
- res = 0;
- } else if (ast_channel_masq(original)) {
- ast_log(LOG_WARNING, "%s is already going to masquerade as %s\n",
- ast_channel_name(ast_channel_masq(original)), ast_channel_name(original));
- } else if (ast_channel_masqr(original)) {
- /* not yet as a previously planned masq hasn't yet happened */
- ast_log(LOG_WARNING, "%s is already going to masquerade as %s\n",
- ast_channel_name(original), ast_channel_name(ast_channel_masqr(original)));
- } else if (ast_channel_masq(clonechan)) {
- ast_log(LOG_WARNING, "%s is already going to masquerade as %s\n",
- ast_channel_name(ast_channel_masq(clonechan)), ast_channel_name(clonechan));
- } else { /* (clonechan->masqr) */
- ast_log(LOG_WARNING, "%s is already going to masquerade as %s\n",
- ast_channel_name(clonechan), ast_channel_name(ast_channel_masqr(clonechan)));
- }
-
- ast_channel_unlock(clonechan);
- ast_channel_unlock(original);
-
- return res;
-}
-
/*! \brief this function simply changes the name of the channel and issues a manager_event
* with out unlinking and linking the channel from the ao2_container. This should
* only be used when the channel has already been unlinked from the ao2_container.
@@ -6481,14 +6372,13 @@
* this function, it invalidates our channel container locking order. All channels
* must be unlocked before it is permissible to lock the channels' ao2 container.
*/
-void ast_do_masquerade(struct ast_channel *original)
+static void channel_do_masquerade(struct ast_channel *original, struct ast_channel *clonechan)
{
int x;
int origstate;
unsigned int orig_disablestatecache;
unsigned int clone_disablestatecache;
int visible_indication;
- int clone_was_zombie = 0;/*!< TRUE if the clonechan was a zombie before the masquerade. */
int clone_hold_state;
struct ast_frame *current;
const struct ast_channel_tech *t;
@@ -6500,7 +6390,6 @@
struct ast_party_connected_line connected;
struct ast_party_redirecting redirecting;
} exchange;
- struct ast_channel *clonechan;
struct ast_channel *bridged;
struct ast_format rformat;
struct ast_format wformat;
@@ -6516,50 +6405,18 @@
* reason we're keeping it, it's still awesomely weird. XXX */
/*
- * 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 unreal/local channel 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
+ * The container lock is necessary for proper locking order
+ * because the channels must 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.
+ * The masq and masqr pointers need to be left alone until the masquerade
+ * has restabilized the channels to hold off ast_hangup() and until
+ * AST_FLAG_ZOMBIE can be set on the clonechan.
*/
ao2_lock(channels);
-
- /*
- * Lock the original channel to determine if the masquerade is
- * still required.
- */
- ast_channel_lock(original);
-
- clonechan = ast_channel_masq(original);
- if (!clonechan) {
- /*
- * The masq is already completed by another thread or never
- * needed to be done to begin with.
- */
- ast_channel_unlock(original);
- ao2_unlock(channels);
- return;
- }
/* Bump the refs to ensure that they won't dissapear on us. */
ast_channel_ref(original);
@@ -6573,6 +6430,7 @@
* Stop any visible indication on the original channel so we can
* transfer it to the clonechan taking the original's place.
*/
+ ast_channel_lock(original);
visible_indication = ast_channel_visible_indication(original);
ast_channel_unlock(original);
ast_indicate(original, -1);
@@ -6813,30 +6671,14 @@
/*
* 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).
+ * We mark it as a zombie so nothing tries to touch it.
*
* This must be done before we unlock clonechan to prevent
* setting up another masquerade on the clonechan.
*/
- if (ast_test_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE)) {
- clone_was_zombie = 1;
- } else {
- ast_set_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE);
- ast_queue_frame(clonechan, &ast_null_frame);
- }
-
- /* clear the masquerade channels */
- ast_channel_masq_set(original, NULL);
- ast_channel_masqr_set(clonechan, 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_set_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE);
+ ast_queue_frame(clonechan, &ast_null_frame);
+
ast_channel_unlock(original);
ast_channel_unlock(clonechan);
@@ -6901,17 +6743,18 @@
}
ast_indicate(original, AST_CONTROL_SRCCHANGE);
- if (!clone_was_zombie) {
- ao2_link(channels, clonechan);
- }
+ /* Now that the operation is complete, we can clear the masq
+ * and masqr fields of both channels.
+ */
+ ast_channel_lock_both(original, clonechan);
+ ast_channel_masq_set(original, NULL);
+ ast_channel_masqr_set(clonechan, NULL);
+ ast_channel_unlock(original);
+ ast_channel_unlock(clonechan);
+
+ ao2_link(channels, clonechan);
ao2_link(channels, original);
ao2_unlock(channels);
-
- if (clone_was_zombie) {
- /* Restart the ast_hangup() that was deferred because of this masquerade. */
- ast_debug(1, "Destroying channel clone '%s'\n", ast_channel_name(clonechan));
- ast_hangup(clonechan);
- }
/* Release our held safety references. */
ast_channel_unref(original);
@@ -10365,13 +10208,48 @@
return yanked_chan;
}
+/*!
+ * Mutex that prevents multiple ast_channel_move() operations
+ * from occurring simultaneously. This is necessary since the
+ * involved channels have to be locked and unlocked throughout
+ * the move operation.
+ *
+ * The most important data being protected are the masq and masqr
+ * data on channels. We don't want them getting criss-crossed due
+ * to multiple moves mucking with them.
+ */
+AST_MUTEX_DEFINE_STATIC(channel_move_lock);
+
int ast_channel_move(struct ast_channel *dest, struct ast_channel *source)
{
- if (ast_channel_masquerade(dest, source)) {
+ SCOPED_MUTEX(lock, &channel_move_lock);
+
+ if (dest == source) {
+ ast_log(LOG_WARNING, "Can't move channel '%s' into itself!\n",
+ ast_channel_name(dest));
return -1;
}
- ast_do_masquerade(dest);
+ ast_channel_lock_both(dest, source);
+
+ if (ast_test_flag(ast_channel_flags(dest), AST_FLAG_ZOMBIE)
+ || ast_test_flag(ast_channel_flags(source), AST_FLAG_ZOMBIE)) {
+ /* Zombies! Run! */
+ ast_log(LOG_WARNING,
+ "Can't move channel. One or both is dead (%s <-- %s)\n",
+ ast_channel_name(dest), ast_channel_name(source));
+ ast_channel_unlock(source);
+ ast_channel_unlock(dest);
+ return -1;
+ }
+
+ ast_channel_masq_set(dest, source);
+ ast_channel_masqr_set(source, dest);
+
+ ast_channel_unlock(dest);
+ ast_channel_unlock(source);
+
+ channel_do_masquerade(dest, source);
return 0;
}
Modified: trunk/res/res_pjsip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_pjsip.c?view=diff&rev=404368&r1=404367&r2=404368
==============================================================================
--- trunk/res/res_pjsip.c (original)
+++ trunk/res/res_pjsip.c Thu Dec 19 11:45:21 2013
@@ -1870,6 +1870,11 @@
{
/* This method is an onion */
struct sync_task_data std;
+
+ if (ast_sip_thread_is_servant()) {
+ return sip_task(task_data);
+ }
+
ast_mutex_init(&std.lock);
ast_cond_init(&std.cond, NULL);
std.fail = std.complete = 0;
More information about the asterisk-commits
mailing list