channel.c: Don't lock the channel in ast_softhangup while setting rtp instance vars

ast_softhangup() was locking the channel before calling ast_rtp_instance_set_stats_vars()
which, if the channel was in a bridge, then locked the bridge peer channel.  If another
thread attempted to set bridge variables on the peer, it would lock that channel first,
then this channel causing a lock inversion.  ast_softhangup() now holds the channel lock
while retrieving the rtp instance, then unlocks it before calling
ast_rtp_instance_set_stats_vars(), then locks it again after it returns.

Resolves: #1907

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 90a7dc293d64036d820fa0bcd5f089c643f9e792..c24f8ed6e41980746727b433a9e3eb8f698b3812 100644 (file)
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -1654,7 +1654,14 @@ void ast_softhangup_all(void);
  * (use this if you are trying to
  * safely hangup a channel managed by another thread.
  *
- * \note The channel passed to this function does not need to be locked.
+ * \warning The channel passed to this function must NOT be locked.
+ * ast_softhangup() calls ast_rtp_instance_set_stats_vars() to set RTP QOS variables.
+ * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
+ * attempt to lock the bridge peer as well as this channel.  This can cause
+ * a lock inversion if we already have this channel locked and another
+ * thread tries to set bridge variables on the peer because it will have
+ * locked the peer first, then this channel.  For this reason, we must
+ * NOT have the channel locked when we call ast_softhangup().
  *
  * \return Returns 0 regardless
  */

diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h
index 0caca86120e93c93fd6439adf749025eea50460f..eab55ec33ae8f04396e795d258595491565d4e38 100644 (file)
--- a/include/asterisk/rtp_engine.h
+++ b/include/asterisk/rtp_engine.h
@@ -2447,7 +2447,13 @@ int ast_rtp_instance_get_stats(struct ast_rtp_instance *instance, struct ast_rtp
  * \param chan Channel to set the statistics on
  * \param instance The RTP instance that statistics will be retrieved from
  *
- * \note Absolutely _NO_ channel locks should be held before calling this function.
+ * \warning Absolutely _NO_ channel locks should be held before calling this function.
+ * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
+ * attempt to lock the bridge peer as well as this channel.  This can cause
+ * a lock inversion if we already have this channel locked and another
+ * thread tries to set bridge variables on the peer because it will have
+ * locked the peer first, then this channel.  For this reason, we must
+ * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars().
  *
  * Example usage:
  *

diff --git a/main/channel.c b/main/channel.c
index ff2e38a343171aa7890891068875f76aeac31092..146e2115ee2268e69e2053f8f82aaab77a2c7aef 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -2465,19 +2465,41 @@ int ast_softhangup(struct ast_channel *chan, int cause)
        int res;
        int tech_cause = 0;
        struct ast_rtp_glue *glue;
-       RAII_VAR(struct ast_rtp_instance *, rtp, NULL, ao2_cleanup);
+       struct ast_rtp_instance *rtp = NULL;
        const struct ast_channel_tech *tech;
 
+       /*
+        * Only hold the channel lock long enough to get the rtp instance.
+        * glue->get_rtp_info() will bump the refcount on it.
+        */
        ast_channel_lock(chan);
-
        tech = ast_channel_tech(chan);
        glue = ast_rtp_instance_get_glue(tech->type);
        if (glue) {
                glue->get_rtp_info(chan, &rtp);
-               if (rtp) {
-                       ast_rtp_instance_set_stats_vars(chan, rtp);
-               }
        }
+       ast_channel_unlock(chan);
+
+       /*
+        * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will
+        * attempt to lock the bridge peer as well as this channel.  This can cause
+        * a lock inversion if we already have this channel locked and another
+        * thread tries to set bridge variables on the peer because it will have
+        * locked the peer first, then this channel.  For this reason, we must
+        * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars().
+        * This should be safe since glue->get_rtp_info() will have bumped the
+        * refcount on the rtp instance so it can't go away while the channel
+        * is unlocked.
+        */
+       if (rtp) {
+               ast_rtp_instance_set_stats_vars(chan, rtp);
+               ao2_ref(rtp, -1);
+       }
+
+       /*
+        * Now it's safe to lock the channel again.
+        */
+       ast_channel_lock(chan);
 
        res = ast_softhangup_nolock(chan, cause);
        blob = ast_json_pack("{s: i, s: b}",

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 184f39b604198ffe8aa65542deeddb313fd5104e..07b560184f25bbf95b51d7708c217413324dc147 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -2744,6 +2744,17 @@ char *ast_rtp_instance_get_quality(struct ast_rtp_instance *instance, enum ast_r
        } \
 })
 
+/*!
+ * \internal
+ *
+ * \warning Absolutely _NO_ channel locks should be held before calling this function.
+ * If the channel is in a bridge, ast_rtp_instance_set_stats_vars() will
+ * attempt to lock the bridge peer as well as this channel.  This can cause
+ * a lock inversion if we already have this channel locked and another
+ * thread tries to set bridge variables on the peer because it will have
+ * locked the peer first, then this channel.  For this reason, we must
+ * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars().
+ */
 void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_instance *instance)
 {
        char quality_buf[AST_MAX_USER_FIELD];