From: George Joseph <gjoseph@sangoma.com>
Date: Tue, 5 May 2026 16:41:15 +0000 (-0600)
Subject: channel.c: Don't lock the channel in ast_softhangup while setting rtp instance vars
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e52d75281b98a74e30d31abc2746fa14ee5dca0;p=thirdparty%2Fasterisk.git

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 90a7dc293d..c24f8ed6e4 100644
--- 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 0caca86120..eab55ec33a 100644
--- 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 ff2e38a343..146e2115ee 100644
--- 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 184f39b604..07b560184f 100644
--- 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];