Deadlock in channel masquerade handling

Channels are stored in an ao2_container.  When accessing an item within
an ao2_container the proper locking order is to first lock the container,
and then the items within it.

In ast_do_masquerade both the clone and original channel must be locked
for the entire duration of the function.  The problem with this is that
it attemptes to unlink and link these channels back into the ao2_container
when one of the channel's name changes.  This is invalid locking order as
the process of unlinking and linking will lock the ao2_container while
the channels are locked!!! Now, both the channels in do_masquerade are
unlinked from the ao2_container and then locked for the entire function.
At the end of the function both channels are unlocked and linked back
into the container with their new names as hash values.

This new method of requiring all channels and tech pvts to be unlocked
before ast_do_masquerade() or ast_change_name() required several
changes throughout the code base.

(closes issue #15911)
Reported by: russell
Patches:
      masq_deadlock_trunk.diff uploaded by dvossel (license 671)
Tested by: dvossel, atis

(closes issue #15618)
Reported by: lmsteffan
Patches:
      deadlock_local_attended_transfers_trunk.diff uploaded by dvossel (license 671)
Tested by: lmsteffan, dvossel

Review: https://reviewboard.asterisk.org/r/387/



git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@222761 65c4cc65-6c06-0410-ace0-fbb531ad65f3

1 files changed, 7 insertions, 1 deletions

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 0f414e7a9..cab334685 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -1054,12 +1054,17 @@ int ast_queue_control_data(struct ast_channel *chan, enum ast_control_frame_type
 /*!
  * \brief Change channel name
  *
- * \pre The channel must be locked before calling this function.
+ * \pre Absolutely all channels _MUST_ be unlocked before calling this function.
  *
  * \param chan the channel to change the name of
  * \param newname the name to change to
  *
  * \return nothing
+ *
+ * \note this function must _NEVER_ be used when any channels are locked
+ * regardless if it is the channel who's name is being changed or not because
+ * it invalidates our channel container locking order... lock container first,
+ * then the individual channels, never the other way around.
  */
 void ast_change_name(struct ast_channel *chan, const char *newname);
 
@@ -1904,6 +1909,7 @@ 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