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

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
  */

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:
  *

main/channel.c

@@ -2468,19 +2468,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}",

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];